[PATCH] D54550: Mark lambda decl as invalid if a captured variable has an invalid type.

2018-11-14 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe created this revision.
jgorbe added a reviewer: rsmith.

This causes the compiler to crash when trying to compute a layout for
the lambda closure type (see included test).


Repository:
  rC Clang

https://reviews.llvm.org/D54550

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/lambda-invalid-capture.cpp


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() { 
+  g child;
+  auto q = [child]{}; 
+  const int n = sizeof(q); 
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14941,6 +14941,13 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (const CXXRecordDecl *RD = FieldType->getAsCXXRecordDecl()) {
+if (RD->isInvalidDecl()) {
+  Lambda->setInvalidDecl();
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() { 
+  g child;
+  auto q = [child]{}; 
+  const int n = sizeof(q); 
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14941,6 +14941,13 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (const CXXRecordDecl *RD = FieldType->getAsCXXRecordDecl()) {
+if (RD->isInvalidDecl()) {
+  Lambda->setInvalidDecl();
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D54550: Mark lambda decl as invalid if a captured variable has an invalid type.

2018-11-14 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe updated this revision to Diff 174118.
jgorbe added a comment.

Fixed some issues pointed out in review comments:

- call to getBaseElementType before checking type validity.
- when the type is incomplete, mark not only the lambda closure type as invalid 
but also the field

Also, added a couple of checks to resemble more closely the code that checks 
user-declared fields in classes.


https://reviews.llvm.org/D54550

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/lambda-invalid-capture.cpp


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() {
+  g child;
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14941,6 +14941,22 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  QualType EltTy = S.Context.getBaseElementType(FieldType);
+  if (!EltTy->isDependentType()) {
+if (S.RequireCompleteType(Loc, EltTy, diag::err_field_incomplete)) {
+  Lambda->setInvalidDecl();
+  Field->setInvalidDecl();
+} else {
+  NamedDecl *Def;
+  EltTy->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() {
+  g child;
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14941,6 +14941,22 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  QualType EltTy = S.Context.getBaseElementType(FieldType);
+  if (!EltTy->isDependentType()) {
+if (S.RequireCompleteType(Loc, EltTy, diag::err_field_incomplete)) {
+  Lambda->setInvalidDecl();
+  Field->setInvalidDecl();
+} else {
+  NamedDecl *Def;
+  EltTy->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D54550: Mark lambda decl as invalid if a captured variable has an invalid type.

2018-11-14 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe marked 2 inline comments as done.
jgorbe added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:14946
+  // as invalid as well.
+  if (const CXXRecordDecl *RD = FieldType->getAsCXXRecordDecl()) {
+if (RD->isInvalidDecl()) {

rsmith wrote:
> You'll need to use `FieldType->getBaseElementTypeUnsafe()` or similar here to 
> handle the case where the field type is an array whose element type is a 
> class. (That can happen in a lambda if you capture a local array variable by 
> value.)
Done, I ended up using Context.getBaseElementType because the call to 
RequireCompleteType below required a QualType.



Comment at: clang/lib/Sema/SemaExpr.cpp:14948
+if (RD->isInvalidDecl()) {
+  Lambda->setInvalidDecl();
+}

rsmith wrote:
> For consistency with the class case, I think we should mark the field invalid 
> in this case too.
Done!


https://reviews.llvm.org/D54550



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


[PATCH] D54550: Mark lambda decl as invalid if a captured variable has an invalid type.

2018-11-20 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe updated this revision to Diff 174853.
jgorbe marked 2 inline comments as done.

https://reviews.llvm.org/D54550

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/lambda-invalid-array-capture.cpp
  clang/test/SemaCXX/lambda-invalid-capture.cpp


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() {
+  g child;
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/test/SemaCXX/lambda-invalid-array-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-array-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() {
+  g child[100];
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14967,6 +14967,21 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (!FieldType->isDependentType()) {
+if (S.RequireCompleteType(Loc, FieldType, diag::err_field_incomplete)) {
+  Lambda->setInvalidDecl();
+  Field->setInvalidDecl();
+} else {
+  NamedDecl *Def;
+  FieldType->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() {
+  g child;
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/test/SemaCXX/lambda-invalid-array-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-array-capture.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void h() {
+  g child[100];
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14967,6 +14967,21 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (!FieldType->isDependentType()) {
+if (S.RequireCompleteType(Loc, FieldType, diag::err_field_incomplete)) {
+  Lambda->setInvalidDecl();
+  Field->setInvalidDecl();
+} else {
+  NamedDecl *Def;
+  FieldType->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D54550: Mark lambda decl as invalid if a captured variable has an invalid type.

2018-11-20 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

Added a test for the "capturing an array of incomplete type" case. See also 
responses to inline comments below.




Comment at: clang/lib/Sema/SemaExpr.cpp:14946
+  // as invalid as well.
+  if (const CXXRecordDecl *RD = FieldType->getAsCXXRecordDecl()) {
+if (RD->isInvalidDecl()) {

rsmith wrote:
> jgorbe wrote:
> > rsmith wrote:
> > > You'll need to use `FieldType->getBaseElementTypeUnsafe()` or similar 
> > > here to handle the case where the field type is an array whose element 
> > > type is a class. (That can happen in a lambda if you capture a local 
> > > array variable by value.)
> > Done, I ended up using Context.getBaseElementType because the call to 
> > RequireCompleteType below required a QualType.
> Hah, OK, now you've switched to using `RequireCompleteType` this has become 
> redundant again (it'll walk to the base element type for you). =)
Removed call to `getBaseElementType()`, changed appearances of `EltTy` to 
`FieldType` instead.



Comment at: clang/lib/Sema/SemaExpr.cpp:14952-14958
+  NamedDecl *Def;
+  EltTy->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}

rsmith wrote:
> I believe the `else` case here is redundant: if `RequireCompleteType` returns 
> `false`, the type is complete and there's nothing more you need to do.
If I just remove the `else` block, the test case included with the patch keeps 
crashing. Is there any way a `Decl` can be invalid even after 
`RequireCompleteType` returns `false`?


https://reviews.llvm.org/D54550



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


[PATCH] D54550: Mark lambda decl as invalid if a captured variable has an invalid type.

2018-11-21 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe updated this revision to Diff 174944.
jgorbe added a comment.

Folded the two test cases (capturing an invalid type and capturing an invalid 
array type) into a single file.


https://reviews.llvm.org/D54550

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/lambda-invalid-capture.cpp


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void captures_invalid_type() {
+  g child;
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
+
+void captures_invalid_array_type() {
+  g child[100];
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14967,6 +14967,21 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (!FieldType->isDependentType()) {
+if (S.RequireCompleteType(Loc, FieldType, diag::err_field_incomplete)) {
+  Lambda->setInvalidDecl();
+  Field->setInvalidDecl();
+} else {
+  NamedDecl *Def;
+  FieldType->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);


Index: clang/test/SemaCXX/lambda-invalid-capture.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-invalid-capture.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Don't crash.
+
+struct g {
+  j; // expected-error {{C++ requires a type specifier for all declarations}}
+};
+
+void captures_invalid_type() {
+  g child;
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
+
+void captures_invalid_array_type() {
+  g child[100];
+  auto q = [child]{};
+  const int n = sizeof(q);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14967,6 +14967,21 @@
 = FieldDecl::Create(S.Context, Lambda, Loc, Loc, nullptr, FieldType,
 S.Context.getTrivialTypeSourceInfo(FieldType, Loc),
 nullptr, false, ICIS_NoInit);
+  // If the variable being captured has an invalid type, mark the lambda class
+  // as invalid as well.
+  if (!FieldType->isDependentType()) {
+if (S.RequireCompleteType(Loc, FieldType, diag::err_field_incomplete)) {
+  Lambda->setInvalidDecl();
+  Field->setInvalidDecl();
+} else {
+  NamedDecl *Def;
+  FieldType->isIncompleteType(&Def);
+  if (Def && Def->isInvalidDecl()) {
+Lambda->setInvalidDecl();
+Field->setInvalidDecl();
+  }
+}
+  }
   Field->setImplicit(true);
   Field->setAccess(AS_private);
   Lambda->addDecl(Field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

2022-03-31 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

Hi, this patch is causing floating point exceptions for us during profile 
generation. On a debug build I get this assertion failure (see stack trace 
below):

  clang: 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/BranchProbability.cpp:41:
 llvm::BranchProbability::BranchProbability(uint32_t, uint32_t): Assertion 
`Denominator > 0 && "Denominator cannot be 0!"' failed.

Printing some values around the problem I got

  TotalBranchWeight = 4294967296   
  LikelyBranchWeight = 2147483648   

   
  UnlikelyBranchWeight = 2147483648 

   
  NumUnlikelyTargets = 1

I see the `BranchProbability` constructor takes `uint32_t`s, so this looks like 
it's overflowing to 0 (4294967296 is exactly 2**32).

I'm going to revert the patch to unbreak our build. Please let me know if you 
need more details and I'll try to come up with a reproducer I can share. Here's 
the stack trace for the assertion.

   #0 0x0a7f992a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:565:11
   #1 0x0a7f9afb PrintStackTraceSignalHandler(void*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:632:1 
  
   #2 0x0a7f80bb llvm::sys::RunSignalHandlers() 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Signals.cpp:102:5  
   
   #3 0x0a7fa271 SignalHandler(int) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:407:1 

   
   #4 0x7ff57cfe2200 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x13200) 

 
   #5 0x7ff57ca678a1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1 

   
   #6 0x7ff57ca51546 abort ./stdlib/abort.c:81:7

   
   #7 0x7ff57ca5142f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8 

   
   #8 0x7ff57ca5142f _nl_load_domain ./intl/loadmsgcat.c:970:34 

   
   #9 0x7ff57ca60222 (/lib/x86_64-linux-gnu/libc.so.6+0x35222)  

   
  #10 0x0a6cb517 llvm::BranchProbability::BranchProbability(unsigned 
int, unsigned int) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/BranchProbability.cpp:0:3
 
  #11 0x0a937a4d llvm::misexpect::verifyMisExpect(llvm::Instruction&, 
llvm::ArrayRef, llvm::ArrayRef) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/Utils/MisExpect.cpp:190:54



  #12 0x0a937ef3 
llvm::misexpect::checkBackendInstrumentation(llvm::Instruction&, 
llvm::ArrayRef) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/Utils/MisExpect.cpp:217:1
  
  #13 0x0a93807b 
llvm::misexpect::checkExpectAnnotations(llvm::Instruction&, 
llvm::ArrayRef, bool) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/Utils/MisExpect.cpp:236:1
 
  #14 0x09e1339c (anonymous 
namespace)::SampleProfileLoader::generateMDProfMetadata(llvm::Function&) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:1755:19
 
  #15 0x09e10d94 (anonymous 
namespace)::SampleProfileLoader::emitAnnotations(llvm::Function&) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:0:5

  #16 0x09e1022b (anonymous 
namespace)::SampleProfileLoader::runOnFunction(llvm::Function&, 
llvm::AnalysisManager*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:2199:5
  

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

2022-03-31 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

By the way, the line number for `llvm::misexpect::verifyMisExpect` in the stack 
trace above includes the debug `printf`s I inserted to get the variable values 
so it won't match the exact line number in the repo. But I think that's the 
only call to `BranchProbability` in that function so I hope it's not very 
confusing anyway. Sorry about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907

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


[PATCH] D121450: [clang-format] Handle attributes before case label.

2022-03-21 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

Hi,

We've observed that this patch introduces infinite loops in some cases. Here's 
a reduced test case:

  export class Foo extends Bar {
get case(): Case {
  return (
  (this.Bar$has('case')) ? (this.Bar$get('case')) :
   (this.case = new Case()));
}
  }

Saving this as `/tmp/test.ts` and running `clang-format /tmp/test.ts` loops 
indefinitely with this patch (I stopped it after 1m27s) and finishes instantly 
(0.12s) after reverting the patch locally. I'm going to go ahead and push a 
revert. Please let me know if you need help reproducing the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121450

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


[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

2021-11-09 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

This change is causing asan errors when running the clang-tidy checks under 
ASan, most likely because of the reason akuegel pointed out in his comment. I'm 
going to revert the patch to unbreak HEAD until the problem can be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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


[PATCH] D51650: Implement target_clones multiversioning

2021-11-11 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

This breaks some existing code (we have found this issue building the JPEG XL 
library at github.com/libjxl). This is a very simplified version of the problem:

  #pragma clang attribute push(__attribute__((target("sse2,ssse3"))), apply_to 
= function)
  __attribute__((target("sse2"))) void f() {
  }
  #pragma clang attribute pop

This used to build before this patch, but now fails with

  $ clang -c a.cc
  a.cc:1:45: error: attribute 'target' cannot appear more than once on a 
declaration
  #pragma clang attribute push(__attribute__((target("sse2,ssse3"))), apply_to 
= function)
  ^
  a.cc:2:1: note: when applied to this declaration
  __attribute__((target("sse2"))) void f() {
  ^
  a.cc:2:16: note: conflicting attribute is here
  __attribute__((target("sse2"))) void f() {
 ^
  1 error generated.

Before this patch, the function-level attribute would win. Here's the relevant 
part of the generated IR for that example:

  ; Function Attrs: mustprogress noinline nounwind optnone uwtable
  define dso_local void @_Z1fv() #0 {
ret void
  }
  
  attributes #0 = { mustprogress noinline nounwind optnone uwtable 
"frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" 
"stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
"target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }

Note how there's `sse2` (which was in the function-level attribute) but no 
`ssse3` (which wasn't).

Was this semantic change intentional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D51650

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


[PATCH] D122215: [WebAssembly] Initial support for reference type externref in clang

2023-01-31 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:4012-4018
+  if (Target->getTriple().isWasm() && Target->hasFeature("reference-types")) {
+#define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS)  
\
+  if (BuiltinType::Id == BuiltinType::WasmExternRef)   
\
+return SingletonId;
+#include "clang/Basic/WebAssemblyReferenceTypes.def"
+  }
+  return QualType();

aaron.ballman wrote:
> pmatos wrote:
> > aaron.ballman wrote:
> > > Rather than returning a null type, should we assert we're in a wasm 
> > > triple? We shouldn't be trying to form the type outside of a wasm target, 
> > > right?
> > asserting false with message rather than llvm_unreachable is the right way, 
> > correct?
> I'd recommend something like:
> ```
> assert(Target->getTriple().isWasm() && Target->hasFeature("reference-types") 
> && "shouldn't try to generate type externref outsid of WebAssembly target");
> #define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS) 
>  \
>   if (BuiltinType::Id == BuiltinType::WasmExternRef)  
>  \
> return SingletonId;
> #include "clang/Basic/WebAssemblyReferenceTypes.def"
>   }
> llvm_unreachable("couldn't find the externref type?");
> ```
> so it's clear that you shouldn't call this outside of a wasm target and that 
> it's not an option to not find the type.
In a non-assert build, doing just `assert(false)` causes the build to fail if 
you use `-Werror -Wreturn-type`.
```
clang/lib/AST/ASTContext.cpp:4097:1: error: non-void function does not return a 
value in all control paths [-Werror,-Wreturn-type]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122215

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


[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

2019-11-18 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

Hi,

I found a clang crash that seems to be caused by this patch. Here's a reduced 
test case:

  template 
  class a {
   public:
~a();
void b();
  };
  
  template 
  a::~a() try {
b();
  } catch (...) {
  }
  
  class d {
   public:
d(const char *, int);
a e;
  }
  
  d("", 1);

Building it with `clang -c -frounding-math` results in the following crash:

  Stack dump:
  0.  Program arguments: 
/usr/local/google/home/jgorbe/code/llvm-build/bin/clang-10 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-fi
  le-name repro.cc -mrelocation-model static -mthread-model posix 
-mframe-pointer=all -fmath-errno -frounding-math -masm-verbose 
-mconstructor-aliases -munwind-tables -fu
  se-init-array -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb 
-resource-dir /usr/local/google/home/jgorbe/code/llvm-build/lib/clang/10.0.0 
-internal-isystem 
  /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem
   /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/backward -intern
  al-isystem /usr/local/include -internal-isystem 
/usr/local/google/home/jgorbe/code/llvm-build/lib/clang/10.0.0/include 
-internal-externc-isystem /usr/include/x86_64-lin
  ux-gnu -internal-externc-isystem /include -internal-externc-isystem 
/usr/include -fdeprecated-macro -fdebug-compilation-dir 
/usr/local/google/home/jgorbe/repro4 -ferror
  -limit 19 -fmessage-length 0 -fgnuc-version=4.2.1 -fobjc-runtime=gcc 
-fcxx-exceptions -fexceptions -fdiagnostics-show-option -fcolor-diagnostics 
-faddrsig -o /tmp/repro
  -c9cae0.o -x c++ repro.cc 
  1.   parser at end of file
  2.  Per-file LLVM IR generation
  3.  repro.cc:4:3: Generating code for declaration 'a::~a'
   #0 0x07fb2b27 llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:532:11
 
   #1 0x07fb2cc9 PrintStackTraceSignalHandler(void*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:593:1 

   #2 0x07fb160b llvm::sys::RunSignalHandlers() 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Signals.cpp:67:5   
 
   #3 0x07fb3415 SignalHandler(int) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:384:1 
   
   #4 0x7f6789ec03a0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x123a0) 

   #5 0x0739af45 
llvm::AttributeList::hasFnAttribute(llvm::Attribute::AttrKind) const 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/IR/Attributes.cpp:1315:10
   #6 0x051a1964 
llvm::Function::hasFnAttribute(llvm::Attribute::AttrKind) const 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/Function.h:324:5   
 
   #7 0x052421a1 llvm::IRBuilderBase::setConstrainedFPFunctionAttr() 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:262:9
   #8 0x05241b60 
llvm::IRBuilderBase::setConstrainedFPCallAttr(llvm::CallInst*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:271:3
   #9 0x084a516b llvm::IRBuilder::CreateCall(llvm::FunctionType*, 
llvm::Value*, llvm::ArrayRef, 
llvm::ArrayRef >, llvm::Twine const&, 
llvm::MDNode*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:2274:9 


  #10 0x084a4488 llvm::IRBuilder::CreateCall(llvm::FunctionCallee, 
llvm::ArrayRef, 
llvm::ArrayRef >, llvm::Twine const&, 
llvm::MDNode*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:2288:5
  #11 0x0849288c 
clang::CodeGen::CodeGenFunction::EmitRuntimeCall(llvm::FunctionCallee, 
llvm::ArrayRef, llvm::Twine const&) 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/CGCall.cpp:3726:26
  #12 0x0849278d 
clang::CodeGen::CodeGenFunction::EmitNounwindRuntimeCall(llvm::FunctionCallee, 
llvm::ArrayRef, llvm::Twine const&) 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/CGCall.cpp:3691:19


  #13 0x0897b3e4 (anonymous 
namespace)::ItaniumCXXABI::emitTerminateForUnexpectedException(clang::CodeGen::CodeGenFunction&,
 llvm::Value*) 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/ItaniumCXXABI.cpp:4364:5
   
  #14 0x0865698d clang::CodeGen::CodeGenFunction::getTerminateHandler() 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/

[PATCH] D100466: clang-format: [JS] merge import lines.

2021-04-21 Thread Jorge Gorbe Moya via Phabricator via cfe-commits
jgorbe added a comment.

Hi, one of our tests is failing because of this patch and it looks like an 
actual bug. clang-format now turns this file:

  import './a';
  import {bar} from './a';

into this:

  barimport './a';


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100466

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