[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D72242#1872827 , @smeenai wrote:

> CC @hans for the 10.0 branch.


Cherry-picked to 10.x as 808f8a632f8bc12830157c57461ae8f848c566a3 
. Please 
let me know if there are any follow-ups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa58017e5cae5: Fix type-dependency of bitfields in templates 
(authored by eandrews).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242

Files:
  clang/lib/AST/Expr.cpp
  clang/test/SemaTemplate/enum-argument.cpp
  clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template 
+class A {
+  int c : b;
+
+public:
+  void f() {
+if (c)
+  ;
+  }
+};
Index: clang/test/SemaTemplate/enum-argument.cpp
===
--- clang/test/SemaTemplate/enum-argument.cpp
+++ clang/test/SemaTemplate/enum-argument.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
 
 enum Enum { val = 1 };
 template  struct C {
@@ -30,7 +31,7 @@
 unsigned long long bitfield : e0;
 
 void f(int j) {
-  bitfield + j; // expected-warning {{expression result unused}}
+  bitfield + j;
 }
   };
 }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1680,6 +1680,11 @@
 CXXRecordDecl *RD = dyn_cast_or_null(DC);
 if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
   E->setTypeDependent(T->isDependentType());
+
+// Bitfield with value-dependent width is type-dependent.
+FieldDecl *FD = dyn_cast(MemberDecl);
+if (FD && FD->isBitField() && FD->getBitWidth()->isValueDependent())
+  E->setTypeDependent(true);
   }
 
   if (HasQualOrFound) {


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template 
+class A {
+  int c : b;
+
+public:
+  void f() {
+if (c)
+  ;
+  }
+};
Index: clang/test/SemaTemplate/enum-argument.cpp
===
--- clang/test/SemaTemplate/enum-argument.cpp
+++ clang/test/SemaTemplate/enum-argument.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
 
 enum Enum { val = 1 };
 template  struct C {
@@ -30,7 +31,7 @@
 unsigned long long bitfield : e0;
 
 void f(int j) {
-  bitfield + j; // expected-warning {{expression result unused}}
+  bitfield + j;
 }
   };
 }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1680,6 +1680,11 @@
 CXXRecordDecl *RD = dyn_cast_or_null(DC);
 if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
   E->setTypeDependent(T->isDependentType());
+
+// Bitfield with value-dependent width is type-dependent.
+FieldDecl *FD = dyn_cast(MemberDecl);
+if (FD && FD->isBitField() && FD->getBitWidth()->isValueDependent())
+  E->setTypeDependent(true);
   }
 
   if (HasQualOrFound) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

In D72242#1873017 , @smeenai wrote:

> I'm not sure why Phabricator is still showing Needs Review, but @rnk's 
> approval should make this count as accepted.


Ok. Thank you. I've pushed the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I'm not sure why Phabricator is still showing Needs Review, but @rnk's approval 
should make this count as accepted.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

Actually I just noticed it still says 'Needs review' above. I'm not sure what 
the protocol is. Can I push the patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

Thanks! I'll push it shortly. Just running a final ninja check-all after 
updating workspace.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a subscriber: hans.
smeenai added a comment.

CC @hans for the 10.0 branch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

lgtm

The fix looks like what Richard asked for, so I feel comfortable approving 
this. Thanks for the fix!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

This also fixes https://bugs.llvm.org/show_bug.cgi?id=44886


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 243049.
eandrews edited the summary of this revision.
eandrews added a comment.

Thanks for taking a look Richard. This patch adds the required value-dependency 
check and sets type-dependency accordingly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242

Files:
  clang/lib/AST/Expr.cpp
  clang/test/SemaTemplate/enum-argument.cpp
  clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template 
+class A {
+  int c : b;
+
+public:
+  void f() {
+if (c)
+  ;
+  }
+};
Index: clang/test/SemaTemplate/enum-argument.cpp
===
--- clang/test/SemaTemplate/enum-argument.cpp
+++ clang/test/SemaTemplate/enum-argument.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
 
 enum Enum { val = 1 };
 template  struct C {
@@ -30,7 +31,7 @@
 unsigned long long bitfield : e0;
 
 void f(int j) {
-  bitfield + j; // expected-warning {{expression result unused}}
+  bitfield + j;
 }
   };
 }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1685,6 +1685,11 @@
 CXXRecordDecl *RD = dyn_cast_or_null(DC);
 if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
   E->setTypeDependent(T->isDependentType());
+
+// Bitfield with value-dependent width is type-dependent.
+FieldDecl *FD = dyn_cast(MemberDecl);
+if (FD && FD->isBitField() && FD->getBitWidth()->isValueDependent())
+  E->setTypeDependent(true);
   }
 
   if (HasQualOrFound) {


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template 
+class A {
+  int c : b;
+
+public:
+  void f() {
+if (c)
+  ;
+  }
+};
Index: clang/test/SemaTemplate/enum-argument.cpp
===
--- clang/test/SemaTemplate/enum-argument.cpp
+++ clang/test/SemaTemplate/enum-argument.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
 
 enum Enum { val = 1 };
 template  struct C {
@@ -30,7 +31,7 @@
 unsigned long long bitfield : e0;
 
 void f(int j) {
-  bitfield + j; // expected-warning {{expression result unused}}
+  bitfield + j;
 }
   };
 }
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1685,6 +1685,11 @@
 CXXRecordDecl *RD = dyn_cast_or_null(DC);
 if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
   E->setTypeDependent(T->isDependentType());
+
+// Bitfield with value-dependent width is type-dependent.
+FieldDecl *FD = dyn_cast(MemberDecl);
+if (FD && FD->isBitField() && FD->getBitWidth()->isValueDependent())
+  E->setTypeDependent(true);
   }
 
   if (HasQualOrFound) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

This is not an appropriate fix; the code is locally correct. The right way to 
handle this is exactly what Clang currently does: if the expression is not 
type-dependent, then contextually convert to `bool`, and if it's not 
value-dependent, then make sure it's a constant expression.

The bug is that we should treat the name of a bit-field with a value-dependent 
width as being type-dependent -- the `c` expression in the testcase should be a 
type-dependent expression (even though its type is known to be `int`) because 
we don't know how to promote it due to the value-dependent bit-width. (This is 
ultimately a bug in the C++ standard, which fails to provide such a rule, but 
I've reported that to the committee and it should be fixed in the standard at 
some point. But the right fix seems obvious enough that we can just do it.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

In D72242#1820908 , @rnk wrote:

> I think I liked the first version of this patch better. I would say, if the 
> AST after instantiation remains the same as it was before D69950 
> , then the first one seems like the right 
> fix.


Thanks for taking a look Reid. The AST after instantiation 
(ClassTemplateSpecializationDecl) remains the same. I'll upload the old patch 
for review again after modifying test to add AST checks @erichkeane do you have 
any concerns?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think I liked the first version of this patch better. I would say, if the AST 
after instantiation remains the same as it was before D69950 
, then the first one seems like the right fix. 
The pattern AST will just be missing a node to represent the promotion from the 
bitfield to a full integer, and then again to a boolean.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews updated this revision to Diff 237132.
eandrews edited the summary of this revision.
eandrews added a comment.

Semantic analysis for value dependent conditions is now skipped as per Erich's 
comment.  Patch adds an argument to CheckBooleanCondition to still do the 
required analysis for noexcept expressions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -ast-dump | FileCheck %s 
--check-prefix=CHECK-AST
+// expected-no-diagnostics
+
+template 
+class A {
+  int c : b;
+
+public:
+  void f() {
+if (c)
+  ;
+  }
+};
+
+void foo() {
+
+  // CHECK-AST: ClassTemplateSpecializationDecl{{.*}}class A definition
+  // CHECK-AST: TemplateArgument integral 3
+  // CHECK-AST: FieldDecl{{.*}}c 'int'
+  // CHECK-AST-NEXT: ConstantExpr{{.*}}'int' Int: 3
+  // CHECK-AST-NEXT: SubstNonTypeTemplateParmExpr{{.*}}'int'
+  // CHECK-AST-NEXT: IntegerLiteral{{.*}}'int' 3
+  A<3> a;
+
+  // CHECK-AST: IfStmt
+  // CHECK-AST-NEXT: ImplicitCastExpr{{.*}}'bool' 
+  // CHECK-AST-NEXT: ImplicitCastExpr{{.*}}'int' 
+  // CHECK-AST-NEXT: MemberExpr{{.*}}'int' lvalue bitfield ->c
+  // CHECK-AST-NEXT: CXXThisExpr{{.*}}'A<3> *' implicit this
+  a.f();
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -17362,7 +17362,7 @@
 }
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
-   bool IsConstexpr) {
+   bool IsConstexpr, bool IsNoexceptExpr) {
   DiagnoseAssignmentAsCondition(E);
   if (ParenExpr *parenE = dyn_cast(E))
 DiagnoseEqualityWithExtraParens(parenE);
@@ -17372,8 +17372,11 @@
   E = result.get();
 
   if (!E->isTypeDependent()) {
-if (getLangOpts().CPlusPlus)
+if (getLangOpts().CPlusPlus) {
+  if (E->isValueDependent() && !IsNoexceptExpr)
+return E;
   return CheckCXXBooleanCondition(E, IsConstexpr); // C++ 6.4p4
+}
 
 ExprResult ERes = DefaultFunctionArrayLvalueConversion(E);
 if (ERes.isInvalid())
Index: clang/lib/Sema/SemaExceptionSpec.cpp
===
--- clang/lib/Sema/SemaExceptionSpec.cpp
+++ clang/lib/Sema/SemaExceptionSpec.cpp
@@ -80,7 +80,9 @@
Expr *NoexceptExpr,
ExceptionSpecificationType ) {
   // FIXME: This is bogus, a noexcept expression is not a condition.
-  ExprResult Converted = CheckBooleanCondition(NoexceptLoc, NoexceptExpr);
+  ExprResult Converted = CheckBooleanCondition(NoexceptLoc, NoexceptExpr,
+   /*IsConstexpr*/ false,
+   /*IsNoexceptExpr*/ true);
   if (Converted.isInvalid())
 return Converted;
 
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -10885,7 +10885,8 @@
   /// 'if' keyword.
   /// \return true iff there were any errors
   ExprResult CheckBooleanCondition(SourceLocation Loc, Expr *E,
-   bool IsConstexpr = false);
+   bool IsConstexpr = false,
+   bool IsNoexceptExpr = false);
 
   /// ActOnExplicitBoolSpecifier - Build an ExplicitSpecifier from an 
expression
   /// found in an explicit(bool) specifier.


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -ast-dump | FileCheck %s --check-prefix=CHECK-AST
+// expected-no-diagnostics
+
+template 
+class A {
+  int c : b;
+
+public:
+  void f() {
+if (c)
+  ;
+  }
+};
+
+void foo() {
+
+  // CHECK-AST: ClassTemplateSpecializationDecl{{.*}}class A definition
+  // CHECK-AST: TemplateArgument integral 3
+  // CHECK-AST: FieldDecl{{.*}}c 'int'
+  // CHECK-AST-NEXT: ConstantExpr{{.*}}'int' Int: 3
+  // CHECK-AST-NEXT: SubstNonTypeTemplateParmExpr{{.*}}'int'
+  // CHECK-AST-NEXT: IntegerLiteral{{.*}}'int' 3
+  A<3> a;
+
+  // CHECK-AST: IfStmt
+  // CHECK-AST-NEXT: ImplicitCastExpr{{.*}}'bool' 
+  // CHECK-AST-NEXT: ImplicitCastExpr{{.*}}'int' 
+  // CHECK-AST-NEXT: MemberExpr{{.*}}'int' lvalue bitfield ->c
+  // CHECK-AST-NEXT: 

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

Thanks for taking a look Erich!

> I'm concerned about just making this fallthrough to 'false'. These ARE 
> integral promotions, we just don't know the type size.

In this case, when integral promotion is skipped, boolean conversion (C++ 4.12) 
is done instead when parsing the template declaration.

> What happens if you instantiate this template? Will it still give the correct 
> answer/type/diagnosis?

I stepped through the code for an instantiated template and in this case, 
integral promotion is done since 'b' is no longer value dependent and is 
whatever we instantiated it with. I'm not sure what the correct behavior here 
is, so I compared current behavior to Clang prior to D69950 
 + this patch. The only difference  I could 
see in the AST is an additional implicit cast for if(c) in template declaration 
(expected since D69950  changed the type 
dependency of c and this is the only guard).  There is no difference in IR 
generated.

> In the case of the crash, I would suspect that the expression 'if (c)' should 
> just be dependent and skipped from the semantic analysis because of it.

It turns out skipping semantic analysis for value dependent condition 
expressions is not entirely straightforward. CheckBooleanCondition has a check 
to skip semantic analysis for type-dependent expressions. Adding a value 
dependency check here however causes crashes (lit tests) because apparently 
semantic analysis for noexcept calls this as well.

  ExprResult Sema::ActOnNoexceptSpec(SourceLocation NoexceptLoc,
 Expr *NoexceptExpr,
 ExceptionSpecificationType ) {
// FIXME: This is bogus, a noexcept expression is not a condition.
ExprResult Converted = CheckBooleanCondition(NoexceptLoc, NoexceptExpr);

I don't really know how noexcept is handled in Clang but it looks like it 
expects the conversion of value dependent expressions. I guess I could add a 
guard in ActOnCond instead and see what happens.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm concerned about just making this fallthrough to 'false'.  These ARE 
integral promotions, we just don't know the type size. What happens if you 
instantiate this template?  Will it still give the correct 
answer/type/diagnosis?

In the case of the crash, I would suspect that the expression 'if (c)' should 
just be dependent and skipped from the semantic analysis because of it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews created this revision.
eandrews added reviewers: rnk, erichkeane, alexfh, alexfh_.

This patch is a follow up to D69950 , to fix a 
new crash on CXX condition expressions in templates, for value dependent 
bitfields.  Clang currently crashes when integral promotion is attempted on the 
condition. This patch adds a guard to prevent the promotion for value dependent 
bitfields. Prior to D69950 , the guard was 
unnecessary because incorrect type dependency of bitfield prevented the 
compiler from reaching this code.


https://reviews.llvm.org/D72242

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template
+class a {
+  int c : b;
+  void f() {
+if (c)
+  ;
+  }
+};
+
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -2158,6 +2158,7 @@
 if (FieldDecl *MemberDecl = From->getSourceBitField()) {
   llvm::APSInt BitWidth;
   if (FromType->isIntegralType(Context) &&
+  !MemberDecl->getBitWidth()->isValueDependent() &&
   MemberDecl->getBitWidth()->isIntegerConstantExpr(BitWidth, Context)) 
{
 llvm::APSInt ToSize(BitWidth.getBitWidth(), BitWidth.isUnsigned());
 ToSize = Context.getTypeSize(ToType);


Index: clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template
+class a {
+  int c : b;
+  void f() {
+if (c)
+  ;
+  }
+};
+
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -2158,6 +2158,7 @@
 if (FieldDecl *MemberDecl = From->getSourceBitField()) {
   llvm::APSInt BitWidth;
   if (FromType->isIntegralType(Context) &&
+  !MemberDecl->getBitWidth()->isValueDependent() &&
   MemberDecl->getBitWidth()->isIntegerConstantExpr(BitWidth, Context)) {
 llvm::APSInt ToSize(BitWidth.getBitWidth(), BitWidth.isUnsigned());
 ToSize = Context.getTypeSize(ToType);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits