[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/AST/Interp/arrays.cpp:143
+
+};

cor3ntin wrote:
> tahonermann wrote:
> > cor3ntin wrote:
> > > tbaeder wrote:
> > > > cor3ntin wrote:
> > > > > tahonermann wrote:
> > > > > > As others already noted, additional testing of multicharacter 
> > > > > > literals and UCNs (including named universal characters like 
> > > > > > `\N{LATIN_CAPITAL_LETTER_E}` would be beneficial. Some tests of 
> > > > > > character escapes like `\t` wouldn't hurt either.
> > > > > > 
> > > > > > Clang does not yet support use of `-fexec-charset` to set the 
> > > > > > literal encoding (execution character set) to anything other than 
> > > > > > UTF-8 though work on that has been done (see D93031). If such work 
> > > > > > was completed, it would be useful to run some tests against a 
> > > > > > non-UTF-8 encoding. Maybe next year.
> > > > > Yes, wide **multicharacter** literals, that's was important 
> > > > > information missing, thanks for spotting that.
> > > > > 
> > > > > I have mixed feeling about adding tests for escape sequences.  Their 
> > > > > replacement doesn't happen during constant evaluation.
> > > > > We shouldn't replicate the lexing tests here.
> > > > > 
> > > > > but we should compare string literal with byte values. Testing a 
> > > > > string literal against another one doesn't ensure the code units are 
> > > > > correct if both are equally miss evaluated.
> > > > > 
> > > > > Also we could add explicit tests for null termination here as they 
> > > > > are added as part of evaluation in theory - but then again that's 
> > > > > also something clang does earlier.
> > > > > 
> > > > > If we want we could consider enabling the byte code interpreter on 
> > > > > the existing lexing test files, i actually think that's the better 
> > > > > way to deal with the escape sequences tests.
> > > > I changed the first test that inspects all characters of a string to 
> > > > comparing with integers instead. Do you have a suggestion for what 
> > > > lexing tests to enable the constant interpreter in?
> > > I think good candidates are
> > > 
> > > Lexer/char-escapes.c
> > > Lexer/char-escapes-delimited.c
> > > Lexer/char-literal.cpp
> > Of those, only `Lexer/char-escapes.c` does much validation of literal 
> > values. I prefer the approach Timm has already taken relative to those 
> > tests.
> > 
> > It looks like we don't have an existing set of Sema tests for character and 
> > string literals. How about we move this test under `clang/test/Sema`? That 
> > would be the appropriate place to exercise values relative to 
> > `-fexec-charset` support for non-UTF-8 encodings in the future. If that 
> > sounds amenable, then I'd like the test split to exercise character and 
> > string literals separately.
> > 
> > The character literal tests don't really belong in a test named 
> > `arrays.cpp` as is.
> > Of those, only Lexer/char-escapes.c does much validation of literal values. 
> > I prefer the approach Timm has already taken relative to those tests.
> 
> We can do both, I was not arguing against the test we have here, I'm happy 
> with those :)
> I'm opposed to duplicate tests for escape sequences here.  using the new 
> interpreter on tests that already exist (in addition of the existing run 
> commands) is pretty easy and cheap to do.
> 
> I don't have opinions how the new interpreter tests are organized.
> Ideally we would have a complete set of test that is equally suitable for 
> both constexpr engines, but maybe that's something that does not need to be 
> done as part of this PR 
I've added a new run line to `test/Lexer/char-escapes.c`, which works fine. To 
summarize, the plan is to add a new test to `clang/test/Sema`? Or split the 
char tests out from `arrays.cpp` and add some for multicharacter char sequences?


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

https://reviews.llvm.org/D135366

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

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@tarunprabhu Could you confirm that with the build command above 
"pass-plugin.f90" is failing for you? It is for me.

In order to fix that, you will have to add this 

 in Flang's test/CMakeLists.txt 
.
 After the change, `libBye.so` will be added as a dependency to Flang tests, 
but only when `FLANG_BUILD_EXAMPLES` is set. I would also add a note in 
"pass-plugin.f90" that it depends on the "Bye" pass from LLVM.


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

https://reviews.llvm.org/D129156

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

[PATCH] D128750: [C++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5511-5539
+  template ::value, bool> = true>
+  bool operator()(T1 *PS1, T2 *PS2) {
+return hasEqualTemplateArgumentList(
+PS1->getTemplateArgsAsWritten()->arguments(),
+PS2->getTemplateArgsAsWritten()->arguments());
+  }

I think you are not supposed to use the `TemplateArgsAsWritten` here.

The injected arguments are 'Converted' arguments, and the transformation above, 
by unpacking the arguments, is reversing just a tiny part of the conversion 
process.

It's not very meaningful to canonicalize the arguments as written to perform a 
semantic comparison, as that works well just for some kinds of template 
arguments, like types and templates, but not for other kinds in which the 
conversion process is not trivial.

For example, I think this may fail to compare the same integers written in 
different ways, like `2` vs `1 + 1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128750

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

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[Ting Wang via Phabricator via cfe-commits]

tingwang added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:327
+///   if the argument is smaller than a slot, set this flag will force
+///   right-adjust the argument in its slot irrespective of the type.
 static Address emitVoidPtrDirectVAArg(CodeGenFunction &CGF,

rjmccall wrote:
> Argh, Phabricator dropped one of my comments, and it's the one that explains 
> why I CC'ed Tim Northover.
> 
> I'm a little worried about the existing uses of this function because this 
> function is sensitive to the type produced by `ConvertTypeForMem`.  
> `ConvertTypeForMem` *mostly* only generates IR struct types for C structs and 
> unions, but there are a few places where it generates an IR struct for some 
> fundamental type that stores multiple values.  Most of those types are at 
> least as large as an argument slot (e.g. they contain pointers), unless 
> there's some weird target with huge slots.  However, some of them are not; I 
> think the most important example is `_Complex T`, which of course gets 
> translated into a struct containing two `T`s.  So if `T` is smaller than half 
> an argument slot, we're not going to right-align `_Complex T` on big-endian 
> targets other than PPC64, and I don't know if that's right.
> 
> That would affect `_Complex _Float16` on 64-bit targets; on 32-bit targets, I 
> think you'd need something obscure like `_Complex char` to exercise it.
> 
> Now, if Clang generates arguments for one of these types using a single value 
> that's also of IR struct type, and the backend considers that when deciding 
> whether to right-align arguments, then maybe those two decisions cancel out 
> and we've at least got call/va_arg compatibility, even if it's not 
> necessarily what's formally specified by the appropriate psABI.  But 
> `DirectTy` is definitely not necessarily the type that call-argument lowering 
> will use, so I'm a little worried.
Thank you!

I checked the `_Complex char` case on PPC64: complex element size smaller than 
argument slot is handled by `complexTempStructure()` 
(https://github.com/llvm/llvm-project/blob/51d33afcbe0a81bb8508d5685f38dc9fdb2b60c9/clang/lib/CodeGen/TargetInfo.cpp#L5451),
 and the right-adjustment is taken care by that logic. Both AIX and PPC64 use 
`complexTempStructure()` to produce variadic callee arguments in this case.

In case `_Complex char` is encapsulated inside structure, then the whole is 
considered as an aggregate, and is addressed by this fix. I will add a test 
case to illustrate.

Hope these addressed your concern.




Comment at: clang/lib/CodeGen/TargetInfo.cpp:5461
 
   // Otherwise, just use the general rule.
+  // TODO: a better approach may refer to SystemZABI use same logic for caller

rjmccall wrote:
> Please add this comment explaining the use of ForceRightAdjust:
> 
> > The PPC64 ABI passes some arguments in integer registers, even to variadic 
> > functions.  To allow `va_list` to use the simple "`void*`" representation, 
> > variadic calls allocate space in the argument area for the integer argument 
> > registers, and variadic functions spill their integer argument registers to 
> > this area in their prologues.  When aggregates smaller than a register are 
> > passed this way, they are passed in the least significant bits of the 
> > register, which means that after spilling on big-endian targets they will 
> > be right-aligned in their argument slot.  This is uncommon; for a variety 
> > of reasons, other big-endian targets don't end up right-aligning aggregate 
> > types this way, and so right-alignment only applies to fundamental types.  
> > So on PPC64, we must force the use of right-alignment even for aggregates.
> 
> I'm not sure what your TODO is hoping for.  You'd like to re-use logic 
> between the frontend's va_arg emission and the backend's variadic argument 
> emission?  That would be very tricky.
Sure, I will add the comment. Thank you.

Maybe I misunderstood some previous comment. I will drop the TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D18

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

[PATCH] D133488: [clang][PowerPC][NFC] Add base test case for PPC64 VAArg aggregate smaller than a slot

[Ting Wang via Phabricator via cfe-commits]

tingwang updated this revision to Diff 467040.
tingwang added a reviewer: rjmccall.
tingwang added a comment.

Add test case according to comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133488

Files:
  clang/test/CodeGen/PowerPC/ppc64-align-struct.c


Index: clang/test/CodeGen/PowerPC/ppc64-align-struct.c
===
--- clang/test/CodeGen/PowerPC/ppc64-align-struct.c
+++ clang/test/CodeGen/PowerPC/ppc64-align-struct.c
@@ -9,6 +9,8 @@
 struct test5 { int x[17]; };
 struct test6 { int x[17]; } __attribute__((aligned (16)));
 struct test7 { int x[17]; } __attribute__((aligned (32)));
+struct test8 { char x; };
+struct test9 { _Complex char x; };
 
 // CHECK: define{{.*}} void @test1(i32 noundef signext %x, i64 %y.coerce)
 void test1 (int x, struct test1 y)
@@ -48,6 +50,16 @@
 {
 }
 
+// CHECK: define{{.*}} void @test8(i32 noundef signext %x, i8 %y.coerce)
+void test8 (int x, struct test8 y)
+{
+}
+
+// CHECK: define{{.*}} void @test9(i32 noundef signext %x, i16 %y.coerce)
+void test9 (int x, struct test9 y)
+{
+}
+
 // CHECK: define{{.*}} void @test1va(ptr noalias sret(%struct.test1) align 4 
%[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
 // CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
 // CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
@@ -116,6 +128,38 @@
   return y;
 }
 
+// Error pattern will be fixed in https://reviews.llvm.org/D18
+// CHECK: define{{.*}} void @test8va(ptr noalias sret(%struct.test8) align 1 
%[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
+// CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
+// CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
+// CHECK: store ptr %[[NEXT]], ptr %ap
+// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr 
align 8 %[[CUR]], i64 1, i1 false)
+struct test8 test8va (int x, ...)
+{
+  struct test8 y;
+  va_list ap;
+  va_start(ap, x);
+  y = va_arg (ap, struct test8);
+  va_end(ap);
+  return y;
+}
+
+// Error pattern will be fixed in https://reviews.llvm.org/D18
+// CHECK: define{{.*}} void @test9va(ptr noalias sret(%struct.test9) align 1 
%[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
+// CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
+// CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
+// CHECK: store ptr %[[NEXT]], ptr %ap
+// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr 
align 8 %[[CUR]], i64 2, i1 false)
+struct test9 test9va (int x, ...)
+{
+  struct test9 y;
+  va_list ap;
+  va_start(ap, x);
+  y = va_arg (ap, struct test9);
+  va_end(ap);
+  return y;
+}
+
 // CHECK: define{{.*}} void @testva_longdouble(ptr noalias 
sret(%struct.test_longdouble) align 16 %[[AGG_RESULT:.*]], i32 noundef signext 
%x, ...)
 // CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
 // CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 16


Index: clang/test/CodeGen/PowerPC/ppc64-align-struct.c
===
--- clang/test/CodeGen/PowerPC/ppc64-align-struct.c
+++ clang/test/CodeGen/PowerPC/ppc64-align-struct.c
@@ -9,6 +9,8 @@
 struct test5 { int x[17]; };
 struct test6 { int x[17]; } __attribute__((aligned (16)));
 struct test7 { int x[17]; } __attribute__((aligned (32)));
+struct test8 { char x; };
+struct test9 { _Complex char x; };
 
 // CHECK: define{{.*}} void @test1(i32 noundef signext %x, i64 %y.coerce)
 void test1 (int x, struct test1 y)
@@ -48,6 +50,16 @@
 {
 }
 
+// CHECK: define{{.*}} void @test8(i32 noundef signext %x, i8 %y.coerce)
+void test8 (int x, struct test8 y)
+{
+}
+
+// CHECK: define{{.*}} void @test9(i32 noundef signext %x, i16 %y.coerce)
+void test9 (int x, struct test9 y)
+{
+}
+
 // CHECK: define{{.*}} void @test1va(ptr noalias sret(%struct.test1) align 4 %[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
 // CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
 // CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
@@ -116,6 +128,38 @@
   return y;
 }
 
+// Error pattern will be fixed in https://reviews.llvm.org/D18
+// CHECK: define{{.*}} void @test8va(ptr noalias sret(%struct.test8) align 1 %[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
+// CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
+// CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
+// CHECK: store ptr %[[NEXT]], ptr %ap
+// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr align 8 %[[CUR]], i64 1, i1 false)
+struct test8 test8va (int x, ...)
+{
+  struct test8 y;
+  va_list ap;
+  va_start(ap, x);
+  y = va_arg (ap, struct test8);
+  va_end(ap);
+  return y;
+}
+
+// Error pattern will be fixed in https://reviews.llvm.org/D18
+// CHECK: define{{.*}} void @test9va(ptr noalias sret(%struct.test9) align 1 %[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
+// CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
+// 

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

[Liming Liu via Phabricator via cfe-commits]

lime updated this revision to Diff 467039.
lime added a comment.

I updated the patch as I said, while kept the key of `NormalizationCache` 
relatively heavy. That means I:

- changed the parameter of `IsAtLeastAsConstrained` to `MutableArrayRef`,
- and let the new `CalculateTemplateDepthForConstraints` handle 
`BinaryOperator` and `ParenExpr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -912,11 +912,7 @@
 

 https://wg21.link/p0857r0";>P0857R0
-
-  Partial
-Constraining template template parameters is not yet supported.
-  
-
+Clang 16
   

 https://wg21.link/p1084r2";>P1084R2
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -59,11 +59,10 @@
 x.operator()(); // expected-error {{no matching member function}}
   }
 
-  // FIXME: This is valid under P0857R0.
   template concept C = true;
-  template requires C typename U> struct X {}; // expected-error {{requires 'class'}} expected-error 0+{{}}
+  template requires C typename U> struct X {};
   template requires C struct Y {};
-  X xy; // expected-error {{no template named 'X'}}
+  X xy;
 }
 
 namespace PR50306 {
Index: clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
===
--- clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
+++ clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp
@@ -1,22 +1,27 @@
 // RUN:  %clang_cc1 -std=c++2a -frelaxed-template-template-args -verify %s
 
-template concept C = T::f();
-// expected-note@-1{{similar constraint}}
+template concept C = T::f(); // #C
 template concept D = C && T::g();
-template concept F = T::f();
-// expected-note@-1{{similar constraint expressions not considered equivalent}}
-template class P> struct S1 { }; // expected-note 2{{'P' declared here}}
+template concept F = T::f(); // #F
+template class P> struct S1 { }; // #S1
 
 template struct X { };
-
-template struct Y { }; // expected-note{{'Y' declared here}}
+template struct Y { }; // #Y
 template struct Z { };
-template struct W { }; // expected-note{{'W' declared here}}
+template struct W { }; // #W
 
 S1 s11;
-S1 s12; // expected-error{{template template argument 'Y' is more constrained than template template parameter 'P'}}
+S1 s12;
+// expected-error@-1 {{template template argument 'Y' is more constrained than template template parameter 'P'}}
+// expected-note@#S1 {{'P' declared here}}
+// expected-note@#Y {{'Y' declared here}}
 S1 s13;
-S1 s14; // expected-error{{template template argument 'W' is more constrained than template template parameter 'P'}}
+S1 s14;
+// expected-error@-1 {{template template argument 'W' is more constrained than template template parameter 'P'}}
+// expected-note@#S1 {{'P' declared here}}
+// expected-note@#W {{'W' declared here}}
+// expected-note@#F 1-2{{similar constraint expressions not considered equivalent}}
+// expected-note@#C 1-2{{similar constraint}}
 
 template class P> struct S2 { };
 
@@ -32,3 +37,25 @@
 
 using s31 = S3;
 using s32 = S3;
+
+template requires C class P> struct S4 { }; // #S4
+
+S4 s41;
+S4 s42;
+// expected-error@-1 {{template template argument 'Y' is more constrained than template template parameter 'P'}}
+// expected-note@#S4 {{'P' declared here}}
+// expected-note@#Y {{'Y' declared here}}
+S4 s43;
+S4 s44;
+// expected-error@-1 {{template template argument 'W' is more constrained than template template parameter 'P'}}
+// expected-note@#S4 {{'P' declared here}}
+// expected-note@#W {{'W' declared here}}
+
+template requires C typename U> struct S5 {
+  template static U V;
+};
+
+struct Nothing {};
+
+// FIXME: Wait the standard to clarify the intent.
+template<> template<> Z S5::V;
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6429,7 +6429,7 @@
   NamedDecl *ND = Function;
   if (auto *SpecInfo = Function->getTemplateSpecializationInfo())
 ND = SpecInfo->getTemplate();
-  
+
   if (ND->getFormalLinkage() == Linkage::InternalLinkage) {
 Candidate.Viable = false;
 Candidate.FailureKind = ovl_fail_module_mismatched;
@@ -9700,7 +9700,7 @@
   

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[Ting Wang via Phabricator via cfe-commits]

tingwang updated this revision to Diff 467048.
tingwang added a comment.

Update according to comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D18

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/PowerPC/ppc64-align-struct.c

Index: clang/test/CodeGen/PowerPC/ppc64-align-struct.c
===
--- clang/test/CodeGen/PowerPC/ppc64-align-struct.c
+++ clang/test/CodeGen/PowerPC/ppc64-align-struct.c
@@ -128,12 +128,12 @@
   return y;
 }
 
-// Error pattern will be fixed in https://reviews.llvm.org/D18
 // CHECK: define{{.*}} void @test8va(ptr noalias sret(%struct.test8) align 1 %[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
 // CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
 // CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
 // CHECK: store ptr %[[NEXT]], ptr %ap
-// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr align 8 %[[CUR]], i64 1, i1 false)
+// CHECK: [[T0:%.*]] = getelementptr inbounds i8, ptr %[[CUR]], i64 7
+// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr align 1 [[T0]], i64 1, i1 false)
 struct test8 test8va (int x, ...)
 {
   struct test8 y;
@@ -144,12 +144,12 @@
   return y;
 }
 
-// Error pattern will be fixed in https://reviews.llvm.org/D18
 // CHECK: define{{.*}} void @test9va(ptr noalias sret(%struct.test9) align 1 %[[AGG_RESULT:.*]], i32 noundef signext %x, ...)
 // CHECK: %[[CUR:[^ ]+]] = load ptr, ptr %ap
 // CHECK: %[[NEXT:[^ ]+]] = getelementptr inbounds i8, ptr %[[CUR]], i64 8
 // CHECK: store ptr %[[NEXT]], ptr %ap
-// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr align 8 %[[CUR]], i64 2, i1 false)
+// CHECK: [[T0:%.*]] = getelementptr inbounds i8, ptr %[[CUR]], i64 6
+// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[AGG_RESULT]], ptr align 2 [[T0]], i64 2, i1 false)
 struct test9 test9va (int x, ...)
 {
   struct test9 y;
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -322,13 +322,17 @@
 ///   leaving one or more empty slots behind as padding.  If this
 ///   is false, the returned address might be less-aligned than
 ///   DirectAlign.
+/// \param ForceRightAdjust - Default is false. On big-endian platform and
+///   if the argument is smaller than a slot, set this flag will force
+///   right-adjust the argument in its slot irrespective of the type.
 static Address emitVoidPtrDirectVAArg(CodeGenFunction &CGF,
   Address VAListAddr,
   llvm::Type *DirectTy,
   CharUnits DirectSize,
   CharUnits DirectAlign,
   CharUnits SlotSize,
-  bool AllowHigherAlign) {
+  bool AllowHigherAlign,
+  bool ForceRightAdjust = false) {
   // Cast the element type to i8* if necessary.  Some platforms define
   // va_list as a struct containing an i8* instead of just an i8*.
   if (VAListAddr.getElementType() != CGF.Int8PtrTy)
@@ -354,7 +358,7 @@
   // If the argument is smaller than a slot, and this is a big-endian
   // target, the argument will be right-adjusted in its slot.
   if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian() &&
-  !DirectTy->isStructTy()) {
+  (!DirectTy->isStructTy() || ForceRightAdjust)) {
 Addr = CGF.Builder.CreateConstInBoundsByteGEP(Addr, SlotSize - DirectSize);
   }
 
@@ -375,11 +379,15 @@
 ///   an argument type with an alignment greater than the slot size
 ///   will be emitted on a higher-alignment address, potentially
 ///   leaving one or more empty slots behind as padding.
+/// \param ForceRightAdjust - Default is false. On big-endian platform and
+///   if the argument is smaller than a slot, set this flag will force
+///   right-adjust the argument in its slot irrespective of the type.
 static Address emitVoidPtrVAArg(CodeGenFunction &CGF, Address VAListAddr,
 QualType ValueTy, bool IsIndirect,
 TypeInfoChars ValueInfo,
 CharUnits SlotSizeAndAlign,
-bool AllowHigherAlign) {
+bool AllowHigherAlign,
+bool ForceRightAdjust = false) {
   // The size and alignment of the value that was passed directly.
   CharUnits DirectSize, DirectAlign;
   if (IsIndirect) {
@@ -395,9 +403,9 @@
   if (IsIndirect)
 DirectTy = DirectTy->getPointerTo(0);
 
-  Address Addr =
-  emitVoidPtrDirectVAArg(CGF, VAListAddr, DirectTy, DirectSize, DirectAlign,
-  

[clang-tools-extra] 5d12b13 - [clang-tidy] Dump effective diagnostics level in YAML output

[Dmitry Polukhin via cfe-commits]

Author: Dmitry Polukhin
Date: 2022-10-12T02:03:56-07:00
New Revision: 5d12b13b0b26bc58b02ee23c369da8b83240cceb

URL: 
https://github.com/llvm/llvm-project/commit/5d12b13b0b26bc58b02ee23c369da8b83240cceb
DIFF: 
https://github.com/llvm/llvm-project/commit/5d12b13b0b26bc58b02ee23c369da8b83240cceb.diff

LOG: [clang-tidy] Dump effective diagnostics level in YAML output

Before this patch YAML output had default diagnostic level instead of effective 
level reported to the user on stdout. Wrapper scripts for clang-tidy usually 
use YAML output and they pick wrong diagnostics level without this patch.

Test Plan: check-clang-tools

Differential Revision: https://reviews.llvm.org/D135367

Added: 


Modified: 
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index d455473673b09..1660d976ff0be 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -403,6 +403,8 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
 
 bool IsWarningAsError = DiagLevel == DiagnosticsEngine::Warning &&
 Context.treatAsError(CheckName);
+if (IsWarningAsError)
+  Level = ClangTidyError::Error;
 Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
 IsWarningAsError);
   }

diff  --git 
a/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp 
b/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
index 16ad4f175cab9..ca2184332f9a4 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
@@ -1,5 +1,5 @@
 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t-input.cpp
-// RUN: not clang-tidy %t-input.cpp 
-checks='-*,google-explicit-constructor,clang-diagnostic-missing-prototypes,clang-diagnostic-zero-length-array'
 -export-fixes=%t.yaml -- -Wmissing-prototypes -Wzero-length-array > %t.msg 2>&1
+// RUN: not clang-tidy %t-input.cpp 
-checks='-*,google-explicit-constructor,clang-diagnostic-missing-prototypes,clang-diagnostic-zero-length-array'
 
--warnings-as-errors='clang-diagnostic-missing-prototypes,google-explicit-constructor'
 -export-fixes=%t.yaml -- -Wmissing-prototypes -Wzero-length-array > %t.msg 2>&1
 // RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s 
-implicit-check-not='{{warning|error|note}}:'
 // RUN: FileCheck -input-file=%t.yaml -check-prefix=CHECK-YAML %s
 #define X(n) void n ## n() {}
@@ -10,9 +10,10 @@ int b[0];
 void test(x);
 struct Foo {
   member;
+  Foo(int) {}
 };
 
-// CHECK-MESSAGES: -input.cpp:2:1: warning: no previous prototype for function 
'ff' [clang-diagnostic-missing-prototypes]
+// CHECK-MESSAGES: -input.cpp:2:1: error: no previous prototype for function 
'ff' [clang-diagnostic-missing-prototypes,-warnings-as-errors]
 // CHECK-MESSAGES: -input.cpp:1:19: note: expanded from macro 'X'
 // CHECK-MESSAGES: {{^}}note: expanded from here{{$}}
 // CHECK-MESSAGES: -input.cpp:2:1: note: declare 'static' if the function is 
not intended to be used outside of this translation unit
@@ -21,6 +22,7 @@ struct Foo {
 // CHECK-MESSAGES: -input.cpp:4:7: warning: zero size arrays are an extension 
[clang-diagnostic-zero-length-array]
 // CHECK-MESSAGES: -input.cpp:6:11: error: unknown type name 'x' 
[clang-diagnostic-error]
 // CHECK-MESSAGES: -input.cpp:8:3: error: a type specifier is required for all 
declarations [clang-diagnostic-error]
+// CHECK-MESSAGES: -input.cpp:9:3: error: single-argument constructors must be 
marked explicit to avoid unintentional implicit conversions 
[google-explicit-constructor,-warnings-as-errors]
 
 // CHECK-YAML: ---
 // CHECK-YAML-NEXT: MainSourceFile:  '{{.*}}-input.cpp'
@@ -52,7 +54,7 @@ struct Foo {
 // CHECK-YAML-NEXT: FilePath:'{{.*}}-input.cpp'
 // CHECK-YAML-NEXT: FileOffset:  13
 // CHECK-YAML-NEXT: Replacements:[]
-// CHECK-YAML-NEXT: Level:   Warning
+// CHECK-YAML-NEXT: Level:   Error
 // CHECK-YAML-NEXT: BuildDirectory:  '{{.*}}'
 // CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-error
 // CHECK-YAML-NEXT: DiagnosticMessage:
@@ -94,4 +96,16 @@ struct Foo {
 // CHECK-YAML-NEXT:   Replacements:[]
 // CHECK-YAML-NEXT: Level:   Error
 // CHECK-YAML-NEXT: BuildDirectory:  '{{.*}}'
+// CHECK-YAML-NEXT:   - DiagnosticName:  google-explicit-constructor
+// CHECK-YAML-NEXT: DiagnosticMessage:
+// CHECK-YAML-NEXT:   Message: single-argument constructors must 
be marked explicit to avoid unintentional implicit conversions
+// 

[PATCH] D135367: [clang-tidy] Dump effective diagnostics level in YAML output

[Dmitry Polukhin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5d12b13b0b26: [clang-tidy] Dump effective diagnostics level 
in YAML output (authored by DmitryPolukhin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135367

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
@@ -1,5 +1,5 @@
 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t-input.cpp
-// RUN: not clang-tidy %t-input.cpp 
-checks='-*,google-explicit-constructor,clang-diagnostic-missing-prototypes,clang-diagnostic-zero-length-array'
 -export-fixes=%t.yaml -- -Wmissing-prototypes -Wzero-length-array > %t.msg 2>&1
+// RUN: not clang-tidy %t-input.cpp 
-checks='-*,google-explicit-constructor,clang-diagnostic-missing-prototypes,clang-diagnostic-zero-length-array'
 
--warnings-as-errors='clang-diagnostic-missing-prototypes,google-explicit-constructor'
 -export-fixes=%t.yaml -- -Wmissing-prototypes -Wzero-length-array > %t.msg 2>&1
 // RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s 
-implicit-check-not='{{warning|error|note}}:'
 // RUN: FileCheck -input-file=%t.yaml -check-prefix=CHECK-YAML %s
 #define X(n) void n ## n() {}
@@ -10,9 +10,10 @@
 void test(x);
 struct Foo {
   member;
+  Foo(int) {}
 };
 
-// CHECK-MESSAGES: -input.cpp:2:1: warning: no previous prototype for function 
'ff' [clang-diagnostic-missing-prototypes]
+// CHECK-MESSAGES: -input.cpp:2:1: error: no previous prototype for function 
'ff' [clang-diagnostic-missing-prototypes,-warnings-as-errors]
 // CHECK-MESSAGES: -input.cpp:1:19: note: expanded from macro 'X'
 // CHECK-MESSAGES: {{^}}note: expanded from here{{$}}
 // CHECK-MESSAGES: -input.cpp:2:1: note: declare 'static' if the function is 
not intended to be used outside of this translation unit
@@ -21,6 +22,7 @@
 // CHECK-MESSAGES: -input.cpp:4:7: warning: zero size arrays are an extension 
[clang-diagnostic-zero-length-array]
 // CHECK-MESSAGES: -input.cpp:6:11: error: unknown type name 'x' 
[clang-diagnostic-error]
 // CHECK-MESSAGES: -input.cpp:8:3: error: a type specifier is required for all 
declarations [clang-diagnostic-error]
+// CHECK-MESSAGES: -input.cpp:9:3: error: single-argument constructors must be 
marked explicit to avoid unintentional implicit conversions 
[google-explicit-constructor,-warnings-as-errors]
 
 // CHECK-YAML: ---
 // CHECK-YAML-NEXT: MainSourceFile:  '{{.*}}-input.cpp'
@@ -52,7 +54,7 @@
 // CHECK-YAML-NEXT: FilePath:'{{.*}}-input.cpp'
 // CHECK-YAML-NEXT: FileOffset:  13
 // CHECK-YAML-NEXT: Replacements:[]
-// CHECK-YAML-NEXT: Level:   Warning
+// CHECK-YAML-NEXT: Level:   Error
 // CHECK-YAML-NEXT: BuildDirectory:  '{{.*}}'
 // CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-error
 // CHECK-YAML-NEXT: DiagnosticMessage:
@@ -94,4 +96,16 @@
 // CHECK-YAML-NEXT:   Replacements:[]
 // CHECK-YAML-NEXT: Level:   Error
 // CHECK-YAML-NEXT: BuildDirectory:  '{{.*}}'
+// CHECK-YAML-NEXT:   - DiagnosticName:  google-explicit-constructor
+// CHECK-YAML-NEXT: DiagnosticMessage:
+// CHECK-YAML-NEXT:   Message: single-argument constructors must 
be marked explicit to avoid unintentional implicit conversions
+// CHECK-YAML-NEXT:   FilePath:'{{.*}}-input.cpp'
+// CHECK-YAML-NEXT:   FileOffset:  96
+// CHECK-YAML-NEXT:   Replacements:
+// CHECK-YAML-NEXT: - FilePath:'{{.*}}-input.cpp'
+// CHECK-YAML-NEXT:   Offset:  96
+// CHECK-YAML-NEXT:   Length:  0
+// CHECK-YAML-NEXT:   ReplacementText: 'explicit '
+// CHECK-YAML-NEXT: Level:   Error
+// CHECK-YAML-NEXT: BuildDirectory:  '{{.*}}'
 // CHECK-YAML-NEXT: ...
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -403,6 +403,8 @@
 
 bool IsWarningAsError = DiagLevel == DiagnosticsEngine::Warning &&
 Context.treatAsError(CheckName);
+if (IsWarningAsError)
+  Level = ClangTidyError::Error;
 Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
 IsWarningAsError);
   }


Index: clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
===
--- clang-tools-extra/test/clang-

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Just to add a few points on the module mapper discussion: overall, the current 
view is that there are two ways to handle C++20 modules from the build system's 
POV: (1) pre-scan the codebase to figure out what is what and who depends on 
whom ahead of compilation and (2) module mapper where the compiler and the 
build system discover this information interactively during compilation. 
Naturally, both approaches have advantages and disadvantages.

The major issue with the pre-scan is that there needs to be a clear boundary 
where the codebase ends. This can be an acceptable requirement for 
monorepo-style projects but not for multirepo. Even for monorepo, there are 
often external dependencies (C/C++ standard library, maybe system libraries 
like OpenSSL) that require special considerations. There is also some concern 
for the performance of the pre-scan on larger codebases. On the other hand, 
pre-scan is likely to be easier to integrate with legacy and meta-build systems 
like CMake.

The major issue with the module mapper is the need for deeper integration with 
the build system as well as the overall complexity of the client/server 
architecture. There is also some concern for the resource consumption since the 
mapper approach may need to spawn nested compiler invocations to build missing 
BMIs. The main advantage of the module mapper is probably the fact that the 
information comes from the source (the compiler) and if/when necessary, which 
sidesteps the whole issue of doing too little or too much pre-scanning (for 
example, due to imprecise boundaries, some discrepancies in the compiler 
options, etc).

GCC implements the mapper approach (with the reusable parts factored into 
libcody) and we successfully use that in build2. We would definitely like to 
see the module mapper supported by Clang, ideally with the GCC's interface (so 
that we could use the existing mapper for both). In the build2's case 
specifically, pre-scan is not a viable option since it's a multirepo-first 
build system.

Also, a comment on the earlier point made:

> I do not believe that the client-sever build system model (a.k.a. P1184 
>  / module mapper) is quite the same thing as 
> implicit modules

Strongly agree. The key difference here is that with the mapper there is a 
single entity (the build system) that decides which things need to be (re)built 
and where rather than multiple compiler instances trying to come up with 
something consistent.


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

https://reviews.llvm.org/D134267

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D134267#3852047 , @boris wrote:

> Just to add a few points on the module mapper discussion: overall, the 
> current view is that there are two ways to handle C++20 modules from the 
> build system's POV: (1) pre-scan the codebase to figure out what is what and 
> who depends on whom ahead of compilation and (2) module mapper where the 
> compiler and the build system discover this information interactively during 
> compilation. Naturally, both approaches have advantages and disadvantages.
>
> The major issue with the pre-scan is that there needs to be a clear boundary 
> where the codebase ends. This can be an acceptable requirement for 
> monorepo-style projects but not for multirepo. Even for monorepo, there are 
> often external dependencies (C/C++ standard library, maybe system libraries 
> like OpenSSL) that require special considerations. There is also some concern 
> for the performance of the pre-scan on larger codebases. On the other hand, 
> pre-scan is likely to be easier to integrate with legacy and meta-build 
> systems like CMake.
>
> The major issue with the module mapper is the need for deeper integration 
> with the build system as well as the overall complexity of the client/server 
> architecture. There is also some concern for the resource consumption since 
> the mapper approach may need to spawn nested compiler invocations to build 
> missing BMIs. The main advantage of the module mapper is probably the fact 
> that the information comes from the source (the compiler) and if/when 
> necessary, which sidesteps the whole issue of doing too little or too much 
> pre-scanning (for example, due to imprecise boundaries, some discrepancies in 
> the compiler options, etc).
>
> GCC implements the mapper approach (with the reusable parts factored into 
> libcody) and we successfully use that in build2. We would definitely like to 
> see the module mapper supported by Clang, ideally with the GCC's interface 
> (so that we could use the existing mapper for both). In the build2's case 
> specifically, pre-scan is not a viable option since it's a multirepo-first 
> build system.
>
> Also, a comment on the earlier point made:
>
>> I do not believe that the client-sever build system model (a.k.a. P1184 
>>  / module mapper) is quite the same thing as 
>> implicit modules
>
> Strongly agree. The key difference here is that with the mapper there is a 
> single entity (the build system) that decides which things need to be 
> (re)built and where rather than multiple compiler instances trying to come up 
> with something consistent.

Hi Boris, thanks for the update. (Although I feel this must not be the right 
place to discuss the topic).

Personally, I don't have strong feeling about the client-server model (pursue 
it or object it). And I'd love to support the per-scan based method since it is 
relatively easy for compiler and build systems. So that other build systems 
(including but not limited to cmake) can support modules more quickly. Then the 
more users can try to start to use modules actually.  So that then we can get 
more backport to improve. And I feel like the two option may not be mutual 
exclusive with each other. For example, my experimental support for P1689 
 is at: 
https://github.com/ChuanqiXu9/llvm-project/commit/fd809e8fcba497eb9458d7dbb6fc194044b19521.
 The implementation looks relatively trivial to me. The simplicity is pretty 
important. Then I won't feel it is too late to support the client-server model 
in clang if someday the client-server  model shows great advances.


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

https://reviews.llvm.org/D134267

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

[PATCH] D134947: [analyzer] Fix the liveness of Symbols for values in regions referred by LazyCompoundVal

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

What do you think @NoQ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134947

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

> For example, my experimental support for P1689 
>  is at: [...]. The implementation looks 
> relatively trivial to me. The simplicity is pretty important.

Two points:

1. It's worth considering the simplicity of the overall system (compiler + 
build system + user project) rather than just the compiler. I hope my previous 
comment highlighted some of the complexity that the overall system must deal 
with in the pre-scan approach with a lot of it potentially ending up on the 
user's plate.

2. I haven't looked at the code, but there are some complex problems in this 
area as highlighted by this MSVC bug: 
https://developercommunity.visualstudio.com/t/scanDependencies-does-not-take-into-acc/10029154
 I believe you may have it easier because reportedly Richard & friends have 
already implemented the necessary header import isolation semantics.


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

https://reviews.llvm.org/D134267

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

[PATCH] D135724: [HIP] Fix unbundling archive

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam accepted this revision.
saiislam added a comment.
This revision is now accepted and ready to land.

Thank you!
It is fixing our OpenMP smoke test failure.


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

https://reviews.llvm.org/D135724

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

[PATCH] D135763: [analyzer] Workaround crash on encountering Class non-type template parameters

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: NoQ, martong, Szelethus, xazax.hun.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The Clang Static Analyzer will crash on this code:

  struct Box {
int value;
  };
  template  int get() {
return V.value;
  }
  template int get();

https://godbolt.org/z/5Yb1sMMMb

The problem is that we don't account for encountering `TemplateParamObjectDecl`s
within the `DeclRefExpr` handler in the `ExprEngine`.

IMO we should create a new memregion for representing such template
param objects, to model their language semantics.
Such as:

- it should have global static storage
- for two identical values, their addresses should be identical as well

http://eel.is/c%2B%2Bdraft/temp.param#8

I was thinking of introducing a `TemplateParamObjectRegion` under `DeclRegion`
for this purpose. It could have `TemplateParamObjectDecl` as a field.

The `TemplateParamObjectDecl::getValue()` returns `APValue`, which might
represent multiple levels of structures, unions and other goodies -
making the transformation from `APValue` to `SVal` a bit complicated.

That being said, for now, I think having `Unknowns` for such cases is
definitely an improvement to crashing, hence I'm proposing this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135763

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/template-param-objects.cpp


Index: clang/test/Analysis/template-param-objects.cpp
===
--- /dev/null
+++ clang/test/Analysis/template-param-objects.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:-analyzer-config eagerly-assume=false -std=c++20 -verify %s
+
+template  void clang_analyzer_dump(T);
+void clang_analyzer_eval(bool);
+
+struct Box {
+  int value;
+};
+bool operator ==(Box lhs, Box rhs) {
+  return lhs.value == rhs.value;
+}
+template  void dumps() {
+  clang_analyzer_dump(V);// expected-warning {{lazyCompoundVal}}
+  clang_analyzer_dump(&V);   // expected-warning {{Unknown}}
+  clang_analyzer_dump(V.value);  // expected-warning {{Unknown}} FIXME: It 
should be '6 S32b'.
+  clang_analyzer_dump(&V.value); // expected-warning {{Unknown}}
+}
+template void dumps();
+
+// [temp.param].7.3.2:
+// "All such template parameters in the program of the same type with the
+// same value denote the same template parameter object."
+template  void stable_addresses() {
+  clang_analyzer_eval(&A1 == &A2); // expected-warning {{UNKNOWN}} FIXME: It 
should be TRUE.
+  clang_analyzer_eval(&B1 == &B2); // expected-warning {{UNKNOWN}} FIXME: It 
should be TRUE.
+  clang_analyzer_eval(&A1 == &B2); // expected-warning {{UNKNOWN}} FIXME: It 
should be FALSE.
+
+  clang_analyzer_eval(A1 == A2); // expected-warning {{UNKNOWN}} FIXME: It 
should be TRUE.
+  clang_analyzer_eval(B1 == B2); // expected-warning {{UNKNOWN}} FIXME: It 
should be TRUE.
+  clang_analyzer_eval(A1 == B2); // expected-warning {{UNKNOWN}} FIXME: It 
should be FALSE.
+}
+template void stable_addresses();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3158,6 +3158,12 @@
 return;
   }
 
+  if (const auto *TPO = dyn_cast(D)) {
+// FIXME: We should meaningfully implement this.
+(void)TPO;
+return;
+  }
+
   llvm_unreachable("Support for this Decl not implemented.");
 }
 


Index: clang/test/Analysis/template-param-objects.cpp
===
--- /dev/null
+++ clang/test/Analysis/template-param-objects.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:-analyzer-config eagerly-assume=false -std=c++20 -verify %s
+
+template  void clang_analyzer_dump(T);
+void clang_analyzer_eval(bool);
+
+struct Box {
+  int value;
+};
+bool operator ==(Box lhs, Box rhs) {
+  return lhs.value == rhs.value;
+}
+template  void dumps() {
+  clang_analyzer_dump(V);// expected-warning {{lazyCompoundVal}}
+  clang_analyzer_dump(&V);   // expected-warning {{Unknown}}
+  clang_analyzer_dump(V.value);  // expected-warning {{Unknown}} FIXME: It should be '6 S32b'.
+  clang_analyzer_dump(&V.value); // expected-warning {{Unknown}}
+}
+template void dumps();
+
+// [temp.param].7.3.2:
+// "All such template parameters in the program of the same type with the
+// same value denote the same template parameter object."
+template  void stable_addresses() {
+  clang_analyzer_eval(&A1 == &A2); // expected-warning {{UNKNOWN}} FIXME: It should be TRUE.

[PATCH] D135764: [clang][Interp] Implement for loops

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, tahonermann, shafik.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135764

Files:
  clang/lib/AST/Interp/ByteCodeStmtGen.cpp
  clang/lib/AST/Interp/ByteCodeStmtGen.h
  clang/test/AST/Interp/loops.cpp

Index: clang/test/AST/Interp/loops.cpp
===
--- clang/test/AST/Interp/loops.cpp
+++ clang/test/AST/Interp/loops.cpp
@@ -186,3 +186,91 @@
   }
 #endif
 };
+
+namespace ForLoop {
+  constexpr int f() {
+int i = 0;
+for (;false;) {
+  i = i + 1;
+}
+return i;
+  }
+  static_assert(f() == 0, "");
+
+  constexpr int f2() {
+int m = 0;
+for (int i = 0; i < 10; i = i + 1){
+  m = i;
+}
+return m;
+  }
+  static_assert(f2() == 9, "");
+
+  constexpr int f3() {
+int i = 0;
+for (; i != 5; i = i + 1);
+return i;
+  }
+  static_assert(f3() == 5, "");
+
+  constexpr int f4() {
+int i = 0;
+for (;;) {
+  i = i + 1;
+
+  if (i == 5)
+break;
+}
+return i;
+  }
+  static_assert(f4() == 5, "");
+
+  constexpr int f5() {
+int i = 0;
+for (;i != 5;) {
+  i = i + 1;
+  continue;
+  i = i - 1;
+}
+return i;
+  }
+  static_assert(f5() == 5, "");
+
+  constexpr int f6(bool b) {
+int i = 0;
+
+for (;true;) {
+  if (!b) {
+if (i == 5)
+  break;
+  }
+
+  if (b) {
+for (; i != 10; i = i + 1) {
+  if (i == 8)
+break;
+  continue;
+}
+  }
+
+  if (b)
+break;
+
+  i = i + 1;
+  continue;
+}
+
+return i;
+  }
+  static_assert(f6(true) == 8, "");
+  static_assert(f6(false) == 5, "");
+
+#if 0
+  /// FIXME: This is an infinite loop, which should
+  ///   be rejected.
+  constexpr int f6() {
+while(true);
+  }
+#endif
+
+};
Index: clang/lib/AST/Interp/ByteCodeStmtGen.h
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.h
+++ clang/lib/AST/Interp/ByteCodeStmtGen.h
@@ -60,6 +60,7 @@
   bool visitIfStmt(const IfStmt *IS);
   bool visitWhileStmt(const WhileStmt *S);
   bool visitDoStmt(const DoStmt *S);
+  bool visitForStmt(const ForStmt *S);
   bool visitBreakStmt(const BreakStmt *S);
   bool visitContinueStmt(const ContinueStmt *S);
 
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -171,6 +171,8 @@
 return visitWhileStmt(cast(S));
   case Stmt::DoStmtClass:
 return visitDoStmt(cast(S));
+  case Stmt::ForStmtClass:
+return visitForStmt(cast(S));
   case Stmt::BreakStmtClass:
 return visitBreakStmt(cast(S));
   case Stmt::ContinueStmtClass:
@@ -331,6 +333,39 @@
   return true;
 }
 
+template 
+bool ByteCodeStmtGen::visitForStmt(const ForStmt *S) {
+  // for (Init; Cond; Inc) { Body }
+  const Stmt *Init = S->getInit();
+  const Expr *Cond = S->getCond();
+  const Expr *Inc = S->getInc();
+  const Stmt *Body = S->getBody();
+
+  LabelTy EndLabel = this->getLabel();
+  LabelTy CondLabel = this->getLabel();
+  LabelTy IncLabel = this->getLabel();
+  LoopScope LS(this, EndLabel, IncLabel);
+
+  if (Init && !this->visitStmt(Init))
+return false;
+  this->emitLabel(CondLabel);
+  if (Cond) {
+if (!this->visitBool(Cond))
+  return false;
+if (!this->jumpFalse(EndLabel))
+  return false;
+  }
+  if (Body && !this->visitStmt(Body))
+return false;
+  this->emitLabel(IncLabel);
+  if (Inc && !this->discard(Inc))
+return false;
+  if (!this->jump(CondLabel))
+return false;
+  this->emitLabel(EndLabel);
+  return true;
+}
+
 template 
 bool ByteCodeStmtGen::visitBreakStmt(const BreakStmt *S) {
   if (!BreakLabel)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3850511 , @dblaikie wrote:

> In D135551#3850391 , @aaron.ballman 
> wrote:
>
>> In D135551#3850308 , @dblaikie 
>> wrote:
>>
>>> In D135551#3850266 , @inclyc 
>>> wrote:
>>>
 This makes sense! However I think `assert(0)` should not be used in this 
 case, we could expose another `llvm_unreachable`-like api and probably 
 `llvm_report_error` shall be fine. Are there some changed assertions 
 actually "Aspirationally unreachable" in this patch?
>>>
>>> No, I really don't think  we should go down that path.
>>>
>>> I believe these are not actually distinct cases - in either case, the 
>>> program has UB if they violated the invariants/preconditions - whether or 
>>> not they called through the C API.
>>
>> The C Index test cases I commented on earlier in the review are a good 
>> example of when there's no UB but we still want to alert people to the 
>> problem of code they should not be reaching. The assumption that "reached 
>> here unexpectedly" == "UB" is invalid. Some things are logic bugs that 
>> exhibit no UB.
>>
>>> unreachable is no more a guarantee/proven thing than an assertion - both 
>>> are written by humans and a claim "if this is reached-or-false, there is a 
>>> bug in some code, somewhere". The statement is not stronger in the 
>>> unreachable case and the style guide supports that perspective and the way 
>>> we triage/treat bugs is pretty consistent with that - we get bugs all the 
>>> time when an unreachable is reached and that doesn't seem to surprise 
>>> most/anyone - we treat it the same as a bug when an assertion fires.
>>>
>>> The discourse discussion, I think, supports this ^ perspective.
>>>
>>> As there's still disagreement, should this escalate to the RFC process to 
>>> change the style guide, Aaron?
>>
>> Yes, I would appreciate that. I don't think we're interpreting our policy 
>> the same way. Specifically "Use llvm_unreachable to mark a specific point in 
>> code that should never be reached."
>
> In the same way that an assert says "This condition should never be false" - 
> I use "should" in the same sense in both unreachable and assert, and I 
> believe that's the prevailing opinion of LLVM developers/the LLVM style guide.

I believe our code base says something different than our "prevailing opinion" 
and we should not be discounting the reality of how our code is written today.

> Perhaps we are also at a deadlock as to who should write the proposal... :/

Agreed, you and I are probably both too close to the issue to write the 
proposal right now. If nobody else does it first, maybe the two of us could 
circle back in a few months after we've had time to research and think more 
deeply and we could co-write something (even if it is a multiple choice RFC 
between conflicting directions).

>> - "should" is turning out to be interpreted in two ways:
>>
>> - "used to indicate obligation, duty, or correctness, typically when 
>> criticizing someone's actions. e.g., he should have been careful": I am 
>> asserting it is impossible to reach this.
>
> This is the "should" ^ I mean, and what every assert should mean too. This 
> code assumes this property to be true - this is a precondition of the code.
>
> We should not be using asserts where we don't mean this. I'm OK assuming 
> every assert does mean "this is a precondition" and treating them that way in 
> terms of transforming them to unreachable or anything else we might do with 
> them - and if some of them don't mean it, then they're buggy and we can fix 
> them, but assert->unreachable doesn't make the situation any worse.
>
> Any code behind/after an assert is untested and unvalidated - we can't say 
> "if you violate this constraint it'll actually be OK" because we've never 
> tested that/don't know that.

Just to double-check... are you opposed to the idea of differentiating between 
ways of saying "the reachability of this code is in question" or are you 
opposed to use of `assert` (or something that smells too similar) specifically? 
Because I don't care about *how* we spell the differentiation, just that 
there's not a policy limiting my ability to express my intent in code. I'd be 
perfectly fine with `#define some_name_we_agree_upon(msg) 
llvm_unreachable(msg)` being the facility we use instead of `assert(0);`.

>> - "used to indicate what is probable. e.g., $348 million should be enough to 
>> buy him out": I am asserting you probably won't get here, but you won't be 
>> happy if you do.

I'm arguing that we have a not-insignificant amount of uses that have this^ 
interpretation and I want a way to express that distinction in code. I have an 
allergic reaction to using `llvm_unreachable` to express 
known-to-be-potentially reachable code because my complaint is that the 
annotation c

[PATCH] D135690: [ASTMatchers] Add matcher for functions that are effectively inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for the new matcher, can you also add a release note for the addition? 
One question I have is: what's the need for adding this matcher? Do you plan to 
use it for some in-tree needs? We usually only add new matchers where there's 
an immediate need for them because of how expensive AST matchers are to compile 
(and each matcher adds a fair number of template instantiations to the final 
binary as well, so there's a bit of runtime cost too).




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7787-7788
+/// void f() {}
+/// void g();
+///   };
+/// \endcode

I think it'd be interesting to show some other cases here -- like when the 
inline keyword is specified on a declaration without a visible definition or 
when the inline keyword is used on a declaration with a non-inline definition. 
(And update the comment below accordingly with what is matched.)



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7791
+/// functionDecl(isEffectivelyInline()) will match f().
+AST_MATCHER(FunctionDecl, isEffectivelyInline) { return Node.isInlined(); }
+

Trass3r wrote:
> Is there any value in adding variables?
> I guess that would mainly target constexpr vars, for which there is 
> `isConstexpr`.
I don't see a need for covering variables yet, but I wouldn't be opposed if 
there was a use case.



Comment at: clang/include/clang/ASTMatchers/ASTMatchersMacros.h:96
   namespace internal { 
\
-  class matcher_##DefineMatcher##Matcher   
\
+  class matcher_##DefineMatcher##Matcher final 
\
   : public ::clang::ast_matchers::internal::MatcherInterface {   
\

Trass3r wrote:
> ziqingluo-90 wrote:
> > I was wondering if this change is necessary.  This definition is so general 
> > that it could affect a massive matchers.  So any change to it should be 
> > very careful and unnecessary changes may be avoided.
> > 
> > Other than that, this patch looks good to me.
> Just came across it. Since it's a macro-internal class I couldn't see any 
> potential breakage. Nobody should inherit from that. But I can take it out.
I'd remove it as it's unrelated to the changes in the patch, but it might be a 
reasonable NFC change (especially if there's some positive impact to the change 
in terms of codegen).



Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:435
   REGISTER_MATCHER(isInline);
+  REGISTER_MATCHER(isEffectivelyInline);
   REGISTER_MATCHER(isInstanceMessage);

This should be kept in alphabetical order.



Comment at: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp:310-311
+TEST(IsInlineMatcher, IsEffectivelyInline) {
+  EXPECT_TRUE(matches("class X { void f() {} void g(); };",
+  functionDecl(isEffectivelyInline(), hasName("f";
+  EXPECT_TRUE(matches("constexpr int f() { return 0; }",

I think you should also test that `g()` does not match and the additional tests 
I suggested in the documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135690

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

[PATCH] D134878: Update developer policy on potentially breaking changes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134878#3836840 , @aaron.ballman 
wrote:

> Pinging reviewers from projects other than libcxx (I'm hoping to get buy-in 
> from someone on the LLVM side of things; lldb would be nice as well).

I'll be landing this later today on the assumption that folks are comfortable 
with the changes here, so if you have concerns, bringing them up now would be 
appreciated.


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

https://reviews.llvm.org/D134878

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

[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D128745#3851225 , @ychen wrote:

> As a next step, I'll remove the ClangABICompat checks for these DRs (make 
> these DRs effective unconditionally). If we saw proof that these have 
> deployment difficulties. We can (1) re-run the rules with the committee as 
> suggested by @rjmccall; (2) consider alternatives (including reverting these 
> DRs) based on the feedback.  Please let me know if you object to this or have 
> any other concerns.

This sounds like a good plan to me. When you remove the ABI compat checks, 
please be sure to add the clang-vendors group to the review and a release note 
to the potentially breaking changes section. Once that lands, you should also 
make an announcement in Discourse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745

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

[PATCH] D135682: Fix false positive related to handling of [[noreturn]] function pointers

[Balázs Benics via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGec6da3fb9d8c: Fix false positive related to handling of 
[[noreturn]] function pointers (authored by arseniy-sonar, committed by 
steakhal).

Changed prior to commit:
  https://reviews.llvm.org/D135682?vs=466827&id=467109#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135682

Files:
  clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  clang/test/Analysis/no-return.c


Index: clang/test/Analysis/no-return.c
===
--- /dev/null
+++ clang/test/Analysis/no-return.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+typedef void(fatal_fun)() __attribute__((__noreturn__));
+fatal_fun* fatal_fptr;
+void fatal_decl() __attribute__((__noreturn__));
+
+int rng();
+
+/// This code calls a [[noreturn]] function pointer, which used to be handled
+/// inconsistently between AST builder and CSA.
+/// In the result, CSA produces a path where this function returns non-0.
+int return_zero_or_abort_by_fnptr() {
+  if (rng()) fatal_fptr();
+  return 0;
+}
+
+/// This function calls a [[noreturn]] function.
+/// If it does return, it always returns 0.
+int return_zero_or_abort_by_direct_fun() {
+  if (rng()) fatal_decl();
+  return 0;
+}
+
+/// Trigger a division by zero issue depending on the return value
+/// of the called functions.
+int caller() {
+  int x = 0;
+  // The following if branches must never be taken.
+  if (return_zero_or_abort_by_fnptr())
+return 1 / x; // no-warning: Dead code.
+  if (return_zero_or_abort_by_direct_fun())
+return 1 / x; // no-warning: Dead code.
+
+  // Make sure the warning is still reported when viable.
+  return 1 / x; // expected-warning {{Division by zero}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -44,9 +44,11 @@
   if (const FunctionDecl *FD = dyn_cast_or_null(CE.getDecl()))
 BuildSinks = FD->hasAttr() || FD->isNoReturn();
 
-  const Expr *Callee = CE.getOriginExpr();
-  if (!BuildSinks && Callee)
-BuildSinks = getFunctionExtInfo(Callee->getType()).getNoReturn();
+  if (const CallExpr *CExpr = dyn_cast_or_null(CE.getOriginExpr());
+  CExpr && !BuildSinks) {
+if (const Expr *C = CExpr->getCallee())
+  BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
+  }
 
   if (!BuildSinks && CE.isGlobalCFunction()) {
 if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {


Index: clang/test/Analysis/no-return.c
===
--- /dev/null
+++ clang/test/Analysis/no-return.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+typedef void(fatal_fun)() __attribute__((__noreturn__));
+fatal_fun* fatal_fptr;
+void fatal_decl() __attribute__((__noreturn__));
+
+int rng();
+
+/// This code calls a [[noreturn]] function pointer, which used to be handled
+/// inconsistently between AST builder and CSA.
+/// In the result, CSA produces a path where this function returns non-0.
+int return_zero_or_abort_by_fnptr() {
+  if (rng()) fatal_fptr();
+  return 0;
+}
+
+/// This function calls a [[noreturn]] function.
+/// If it does return, it always returns 0.
+int return_zero_or_abort_by_direct_fun() {
+  if (rng()) fatal_decl();
+  return 0;
+}
+
+/// Trigger a division by zero issue depending on the return value
+/// of the called functions.
+int caller() {
+  int x = 0;
+  // The following if branches must never be taken.
+  if (return_zero_or_abort_by_fnptr())
+return 1 / x; // no-warning: Dead code.
+  if (return_zero_or_abort_by_direct_fun())
+return 1 / x; // no-warning: Dead code.
+
+  // Make sure the warning is still reported when viable.
+  return 1 / x; // expected-warning {{Division by zero}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -44,9 +44,11 @@
   if (const FunctionDecl *FD = dyn_cast_or_null(CE.getDecl()))
 BuildSinks = FD->hasAttr() || FD->isNoReturn();
 
-  const Expr *Callee = CE.getOriginExpr();
-  if (!BuildSinks && Callee)
-BuildSinks = getFunctionExtInfo(Callee->getType()).getNoReturn();
+  if (const CallExpr *CExpr = dyn_cast_or_null(CE.getOriginExpr());
+  CExpr && !BuildSinks) {
+if (const Expr *C = CExpr->getCallee())
+  BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
+  }
 
   if (!BuildSinks && CE.isGlobalCFunction()

[clang] ec6da3f - Fix false positive related to handling of [[noreturn]] function pointers

[Balazs Benics via cfe-commits]

Author: Arseniy Zaostrovnykh
Date: 2022-10-12T14:46:32+02:00
New Revision: ec6da3fb9d8c7859da22d9eb7e814faf2e5a524a

URL: 
https://github.com/llvm/llvm-project/commit/ec6da3fb9d8c7859da22d9eb7e814faf2e5a524a
DIFF: 
https://github.com/llvm/llvm-project/commit/ec6da3fb9d8c7859da22d9eb7e814faf2e5a524a.diff

LOG: Fix false positive related to handling of [[noreturn]] function pointers

Before this change, the `NoReturnFunctionChecker` was missing function pointers
with a `[[noreturn]]` attribute, while `CFG` was constructed taking that into
account, which leads CSA to take impossible paths. The reason was that the
`NoReturnFunctionChecker` was looking for the attribute in the type of the
entire call expression rather than the type of the function being called.

This change makes the `[[noreturn]]` attribute of a function pointer visible
to `NoReturnFunctionChecker`. This leads to a more coherent behavior of the
CSA on the AST involving.

Reviewed By: xazax.hun

Differential Revision: https://reviews.llvm.org/D135682

Added: 
clang/test/Analysis/no-return.c

Modified: 
clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp

Removed: 




diff  --git a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
index af208e8673187..17c3cb4e9e04c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -44,9 +44,11 @@ void NoReturnFunctionChecker::checkPostCall(const CallEvent 
&CE,
   if (const FunctionDecl *FD = dyn_cast_or_null(CE.getDecl()))
 BuildSinks = FD->hasAttr() || FD->isNoReturn();
 
-  const Expr *Callee = CE.getOriginExpr();
-  if (!BuildSinks && Callee)
-BuildSinks = getFunctionExtInfo(Callee->getType()).getNoReturn();
+  if (const CallExpr *CExpr = dyn_cast_or_null(CE.getOriginExpr());
+  CExpr && !BuildSinks) {
+if (const Expr *C = CExpr->getCallee())
+  BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
+  }
 
   if (!BuildSinks && CE.isGlobalCFunction()) {
 if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {

diff  --git a/clang/test/Analysis/no-return.c b/clang/test/Analysis/no-return.c
new file mode 100644
index 0..872604ebd71d4
--- /dev/null
+++ b/clang/test/Analysis/no-return.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+typedef void(fatal_fun)() __attribute__((__noreturn__));
+fatal_fun* fatal_fptr;
+void fatal_decl() __attribute__((__noreturn__));
+
+int rng();
+
+/// This code calls a [[noreturn]] function pointer, which used to be handled
+/// inconsistently between AST builder and CSA.
+/// In the result, CSA produces a path where this function returns non-0.
+int return_zero_or_abort_by_fnptr() {
+  if (rng()) fatal_fptr();
+  return 0;
+}
+
+/// This function calls a [[noreturn]] function.
+/// If it does return, it always returns 0.
+int return_zero_or_abort_by_direct_fun() {
+  if (rng()) fatal_decl();
+  return 0;
+}
+
+/// Trigger a division by zero issue depending on the return value
+/// of the called functions.
+int caller() {
+  int x = 0;
+  // The following if branches must never be taken.
+  if (return_zero_or_abort_by_fnptr())
+return 1 / x; // no-warning: Dead code.
+  if (return_zero_or_abort_by_direct_fun())
+return 1 / x; // no-warning: Dead code.
+
+  // Make sure the warning is still reported when viable.
+  return 1 / x; // expected-warning {{Division by zero}}
+}



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

[PATCH] D135772: Stop evaluating trailing requires clause after overload resolution

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added a reviewer: clang-language-wg.
Herald added a project: All.
erichkeane requested review of this revision.

Reported as it showed up as a constriants failure after the deferred
instantiation patch, we were checking constraints TWICE after overload
resolution.  The first is during overload resolution, the second is when
diagnosing a use.

This patch modifies DiagnoseUseOfDecl to skip the trailing requires
clause check in some cases. First, of course, after choosing a candidate
after overload resolution.

The second is when evaluating a shadow using constructor, which had its
constraints checked when picking a constructor (as this is ALWAYS an
overload situation!).


https://reviews.llvm.org/D135772

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -708,3 +708,60 @@
 // expected-note@#CMVT_REQ{{because 'sizeof(int) == arity' (4 == 5) evaluated to false}}
 } // namespace ConstrainedMemberVarTemplate 
 
+// These should not diagnose, where we were unintentionally doing so before by
+// checking these two times, yet not having the ability to the 2nd time, since
+// it was no longer a dependent variant.
+namespace InheritedFromPartialSpec {
+template
+constexpr bool Check = true;
+
+template
+struct Foo {
+  template
+Foo(U&&) requires (Check){}
+  template
+void MemFunc(U&&) requires (Check){}
+  template
+static void StaticMemFunc(U&&) requires (Check){}
+  ~Foo() requires (Check){}
+};
+
+template<>
+  struct Foo : Foo {
+using Foo::Foo;
+using Foo::MemFunc;
+using Foo::StaticMemFunc;
+  };
+
+void use() {
+  Foo F {1.1};
+  F.MemFunc(1.1);
+  Foo::StaticMemFunc(1.1);
+}
+
+template
+struct counted_iterator {
+  constexpr auto operator->() const noexcept requires false {
+return T::Invalid;
+  };
+};
+
+template
+concept __has_member_pointer = requires { typename _Ip::pointer; };
+
+template
+struct __iterator_traits_member_pointer_or_arrow_or_void { using type = void; };
+template<__has_member_pointer _Ip>
+struct __iterator_traits_member_pointer_or_arrow_or_void<_Ip> { using type = typename _Ip::pointer; };
+
+template
+  requires requires(_Ip& __i) { __i.operator->(); } && (!__has_member_pointer<_Ip>)
+struct __iterator_traits_member_pointer_or_arrow_or_void<_Ip> {
+  using type = decltype(declval<_Ip&>().operator->());
+};
+
+
+void use2() {
+  __iterator_traits_member_pointer_or_arrow_or_void> f;
+}
+}// namespace InheritedFromPartialSpec 
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -14680,7 +14680,7 @@
   Method = cast(Best->Function);
   FoundDecl = Best->FoundDecl;
   CheckUnresolvedMemberAccess(UnresExpr, Best->FoundDecl);
-  if (DiagnoseUseOfDecl(Best->FoundDecl, UnresExpr->getNameLoc()))
+  if (DiagnoseUseOfOverloadedDecl(Best->FoundDecl, UnresExpr->getNameLoc()))
 break;
   // If FoundDecl is different from Method (such as if one is a template
   // and the other a specialization), make sure DiagnoseUseOfDecl is
@@ -14689,7 +14689,7 @@
   // DiagnoseUseOfDecl to accept both the FoundDecl and the decl
   // being used.
   if (Method != FoundDecl.getDecl() &&
-  DiagnoseUseOfDecl(Method, UnresExpr->getNameLoc()))
+  DiagnoseUseOfOverloadedDecl(Method, UnresExpr->getNameLoc()))
 break;
   Succeeded = true;
   break;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -222,7 +222,8 @@
  const ObjCInterfaceDecl *UnknownObjCClass,
  bool ObjCPropertyAccess,
  bool AvoidPartialAvailabilityChecks,
- ObjCInterfaceDecl *ClassReceiver) {
+ ObjCInterfaceDecl *ClassReceiver,
+ bool SkipTrailingRequiresClause) {
   SourceLocation Loc = Locs.front();
   if (getLangOpts().CPlusPlus && isa(D)) {
 // If there were any diagnostics suppressed by template argument deduction,
@@ -281,7 +282,7 @@
 // See if this is a function with constraints that need to be satisfied.
 // Check this before deducing the return type, as it might instantiate the
 // definition.
-if (FD->getTrailingRequiresClause()) {
+if (!SkipTrailingRequiresClause && FD->getTrailingRequiresClause()) {
   ConstraintSatisfaction Satisfaction;
   if (CheckFunctionConstraints(FD, Satisfaction, Loc,
 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3849583 , @erichkeane 
wrote:

> In D126907#3846591 , @erichkeane 
> wrote:
>
>> In D126907#3844788 , @BertalanD 
>> wrote:
>>
>>> Hi @erichkeane,
>>>
>>> This change broke compilation of this program 
>>> (https://godbolt.org/z/KrWGvcf8h; reduced from 
>>> https://github.com/SerenityOS/ladybird):
>>>
>>>   template
>>>   constexpr bool IsSame = false;
>>>   
>>>   template
>>>   constexpr bool IsSame = true;
>>>   
>>>   template
>>>   struct Foo {
>>>   template
>>>   Foo(U&&) requires (!IsSame);
>>>   };
>>>   
>>>   template<>
>>>   struct Foo : Foo {
>>>   using Foo::Foo;
>>>   };
>>>   
>>>   Foo test() { return 0; }
>>>
>>>
>>>
>>>   :18:27: error: invalid reference to function 'Foo': constraints 
>>> not satisfied
>>>   Foo test() { return 0; }
>>> ^
>>>   :10:24: note: because substituted constraint expression is 
>>> ill-formed: value of type '' is not contextually 
>>> convertible to 'bool'
>>>   Foo(U&&) requires (!IsSame);
>>>  ^
>>
>> Thanks for the report!  I'll look into it ASAP.
>
> Quick note: I believe I understand the cause of this, which requires a bit 
> more work than I otherwise would have expected.  I have a candidate patch I'm 
> running through my testing right now that should fix this, but it still needs 
> cleaning up.  Expect it in the next day or two if all goes well.



In D126907#3850735 , @BertalanD wrote:

> In D126907#3849583 , @erichkeane 
> wrote:
>
>> In D126907#3846591 , @erichkeane 
>> wrote:
>>
>>> In D126907#3844788 , @BertalanD 
>>> wrote:
>>>
 Hi @erichkeane,

 This change broke compilation of this program 
 (https://godbolt.org/z/KrWGvcf8h; reduced from 
 https://github.com/SerenityOS/ladybird):

   template
   constexpr bool IsSame = false;
   
   template
   constexpr bool IsSame = true;
   
   template
   struct Foo {
   template
   Foo(U&&) requires (!IsSame);
   };
   
   template<>
   struct Foo : Foo {
   using Foo::Foo;
   };
   
   Foo test() { return 0; }



   :18:27: error: invalid reference to function 'Foo': constraints 
 not satisfied
   Foo test() { return 0; }
 ^
   :10:24: note: because substituted constraint expression is 
 ill-formed: value of type '' is not contextually 
 convertible to 'bool'
   Foo(U&&) requires (!IsSame);
  ^
>>>
>>> Thanks for the report!  I'll look into it ASAP.
>>
>> Quick note: I believe I understand the cause of this, which requires a bit 
>> more work than I otherwise would have expected.  I have a candidate patch 
>> I'm running through my testing right now that should fix this, but it still 
>> needs cleaning up.  Expect it in the next day or two if all goes well.
>
> Thank you for your quick response and for creating this massive yak shave of 
> a patch :D
>
> Please ping me if you want me to test the fix on our code, or if I can help 
> in some other way.

Thanks for the offer!  I ended up taking a less-aggressive yak shave on this 
one, and have a patch here: https://reviews.llvm.org/D135772

If you could give it a try, it would be very useful!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D135472: [ODRHash] Hash attributes on declarations.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:302
+llvm::raw_string_ostream OutputStream(FullText);
+A->printPretty(OutputStream, Ctx.getPrintingPolicy());
+return OutputStream.str();

vsapsai wrote:
> aaron.ballman wrote:
> > Do we want to have more control over the printing policy here? e.g., do we 
> > really want to claim an ODR difference if one module's printing policy 
> > specifies indentation of 8 and another specifies indentation of 4? Or if 
> > one module prints `restrict` while another prints `__restrict`, etc?
> `FullAttributeText` is used for diagnostics and not for the mismatch 
> detection, so we shouldn't complain about `restrict/__ restrict` mismatches 
> (`DifferentAlignmentKeywords` tests that `__attribute__((aligned(8)))` and 
> `alignas(8)` are not a mismatch).
> 
> We use the same `ASTContext` to print both attributes, so that shouldn't be 
> confusing. Diagnostic can be unstable across clang versions and probably 
> across language modes. But I think that can happen to other diagnostic 
> messages too, so think that's acceptable.
Oh, that's great, thank you!



Comment at: clang/lib/AST/ODRHash.cpp:479-480
+
+  llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs),
+[](const Attr *A) { return !A->isImplicit(); });
+}

ChuanqiXu wrote:
> vsapsai wrote:
> > vsapsai wrote:
> > > erichkeane wrote:
> > > > aaron.ballman wrote:
> > > > > ChuanqiXu wrote:
> > > > > > vsapsai wrote:
> > > > > > > I'm not sure `isImplicit` is the best indicator of attributes to 
> > > > > > > check, so suggestions in this area are welcome. I think we can 
> > > > > > > start strict and relax some of the checks if needed.
> > > > > > > 
> > > > > > > If people have strong opinions that some attributes shouldn't be 
> > > > > > > ignored, we can add them to the tests to avoid regressions. 
> > > > > > > Personally, I believe that alignment and packed attributes should 
> > > > > > > never be silently ignored.
> > > > > > Agreed. I feel `isImplicit` is enough for now.
> > > > > The tricky part is -- sometimes certain attributes add additional 
> > > > > implicit attributes and those implicit attributes matter 
> > > > > (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L9380).
> > > > >  And some attributes seem to just do the wrong thing entirely: 
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L7344
> > > > > 
> > > > > So I think `isImplicit()` is a good approximation, but I'm more 
> > > > > wondering what the principle is for whether an attribute should or 
> > > > > should not be considered part of the ODR hash. Type attributes, 
> > > > > attributes that impact object layout, etc all seem like they should 
> > > > > almost certainly be part of ODR hashing. Others are a bit more 
> > > > > questionable though.
> > > > > 
> > > > > I think this is something that may need a per-attribute flag in 
> > > > > Attr.td so attributes can opt in or out of it because I'm not certain 
> > > > > what ODR issues could stem from `[[maybe_unused]]` or 
> > > > > `[[deprecated]]` disagreements across module boundaries.
> > > > I don't think 'isImplicit' is particularly good.  I think the idea of 
> > > > auto-adding 'type' attributes and having the 'rest' be analyzed to 
> > > > figure out which are important.
> > > > 
> > > > Alternatively, I wonder if we're better off just adding ALL attributes 
> > > > and seeing what the fallout is. We can later decide when we don't want 
> > > > them to be ODR significant (which, might be OTHERWISE meaningful 
> > > > later!).
> > > One criteria to decide which attributes should be hashed is if they 
> > > affect IRGen. But that's not exhaustive and I'm not sure how practical it 
> > > is.
> > > 
> > > The rule I'm trying to follow right now is if declarations with different 
> > > attributes can be substituted instead of each other. For example, structs 
> > > with different memory layout cannot be interchanged, so it is reasonable 
> > > to reject them.
> > > 
> > > But maybe we should review attributes on case-by-case basis. For example, 
> > > for `[[deprecated]]` I think the best for developers is not to complain 
> > > about it but merge the attributes and then have regular diagnostic about 
> > > using a deprecated entity.
> > One option was to hash `isa` attributes as they are "sticky" 
> > and should be more significant, so shouldn't be ignored. But I don't think 
> > this argument is particularly convincing.
> > 
> > At this stage I think we can still afford to add all attributes and exclude 
> > some as-needed because modules adoption is still limited. Though I don't 
> > have strong feelings about this approach, just getting more restrictive 
> > later can be hard.
> It looks not a bad idea to add all attributes as experiments, @vsapsai how do 
> you feel about th

[PATCH] D134820: [LTO][clang] Teaching Clang to Pass Plugin Options to the AIX Linker

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 updated this revision to Diff 467119.
qiongsiwu1 added a comment.

Rebase with latest main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134820

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/test/Driver/lto-aix.c

Index: clang/test/Driver/lto-aix.c
===
--- /dev/null
+++ clang/test/Driver/lto-aix.c
@@ -0,0 +1,6 @@
+// Test LTO path, mcpu and opt level options
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fuse-ld=ld -O3 2>&1 \
+// RUN:   | FileCheck -check-prefixes=LTOPATH,MCPUOPTLEVEL %s
+//
+// LTOPATH: "-bplugin:{{.*}}libLTO.{{so|dll|dylib}}"
+// MCPUOPTLEVEL: "-bplugin_opt:-mcpu={{.*}}" "-bplugin_opt:-O3"
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -190,7 +190,8 @@
  Multilib::flags_list &Flags);
 
 void addX86AlignBranchArgs(const Driver &D, const llvm::opt::ArgList &Args,
-   llvm::opt::ArgStringList &CmdArgs, bool IsLTO);
+   llvm::opt::ArgStringList &CmdArgs, bool IsLTO,
+   const StringRef PluginOptPrefix = "");
 
 void checkAMDGPUCodeObjectVersion(const Driver &D,
   const llvm::opt::ArgList &Args);
@@ -203,7 +204,8 @@
 
 void addMachineOutlinerArgs(const Driver &D, const llvm::opt::ArgList &Args,
 llvm::opt::ArgStringList &CmdArgs,
-const llvm::Triple &Triple, bool IsLTO);
+const llvm::Triple &Triple, bool IsLTO,
+const StringRef PluginOptPrefix = "");
 
 void addOpenMPDeviceRTL(const Driver &D, const llvm::opt::ArgList &DriverArgs,
 llvm::opt::ArgStringList &CC1Args,
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -65,24 +65,26 @@
 using namespace clang;
 using namespace llvm::opt;
 
-static void renderRpassOptions(const ArgList &Args, ArgStringList &CmdArgs) {
+static void renderRpassOptions(const ArgList &Args, ArgStringList &CmdArgs,
+   const StringRef PluginOptPrefix) {
   if (const Arg *A = Args.getLastArg(options::OPT_Rpass_EQ))
-CmdArgs.push_back(Args.MakeArgString(Twine("-plugin-opt=-pass-remarks=") +
- A->getValue()));
+CmdArgs.push_back(Args.MakeArgString(Twine(PluginOptPrefix) +
+ "-pass-remarks=" + A->getValue()));
 
   if (const Arg *A = Args.getLastArg(options::OPT_Rpass_missed_EQ))
 CmdArgs.push_back(Args.MakeArgString(
-Twine("-plugin-opt=-pass-remarks-missed=") + A->getValue()));
+Twine(PluginOptPrefix) + "-pass-remarks-missed=" + A->getValue()));
 
   if (const Arg *A = Args.getLastArg(options::OPT_Rpass_analysis_EQ))
 CmdArgs.push_back(Args.MakeArgString(
-Twine("-plugin-opt=-pass-remarks-analysis=") + A->getValue()));
+Twine(PluginOptPrefix) + "-pass-remarks-analysis=" + A->getValue()));
 }
 
 static void renderRemarksOptions(const ArgList &Args, ArgStringList &CmdArgs,
  const llvm::Triple &Triple,
  const InputInfo &Input,
- const InputInfo &Output) {
+ const InputInfo &Output,
+ const StringRef PluginOptPrefix) {
   StringRef Format = "yaml";
   if (const Arg *A = Args.getLastArg(options::OPT_fsave_optimization_record_EQ))
 Format = A->getValue();
@@ -96,29 +98,32 @@
 
   assert(!F.empty() && "Cannot determine remarks output name.");
   // Append "opt.ld." to the end of the file name.
-  CmdArgs.push_back(
-  Args.MakeArgString(Twine("-plugin-opt=opt-remarks-filename=") + F +
- Twine(".opt.ld.") + Format));
+  CmdArgs.push_back(Args.MakeArgString(Twine(PluginOptPrefix) +
+   "opt-remarks-filename=" + F +
+   ".opt.ld." + Format));
 
   if (const Arg *A =
   Args.getLastArg(options::OPT_foptimization_record_passes_EQ))
 CmdArgs.push_back(Args.MakeArgString(
-Twine("-plugin-opt=opt-remarks-passes=") + A->getValue()));
+Twine(PluginOptPrefix) + "opt-remarks-passes=" + A->getValue()));
 
-  CmdArgs.push_back(Args.MakeArgString(
-  Twine("-plugin-opt=opt-remarks-format=") + Format.data()));
+  CmdArgs.push_back(Args.MakeArgString(Twine(PluginOptPrefix) +

[PATCH] D129156: Add -fpass-plugin option to Flang

[Tarun Prabhu via Phabricator via cfe-commits]

tarunprabhu added a comment.

In D129156#3851843 , @awarzynski 
wrote:

> @tarunprabhu Could you confirm that with the build command above 
> "pass-plugin.f90" is failing for you? It is for me.

What compiler do you use to build this? gcc doesn't seem to like `-gmlt`, 
clang-12 complains about `-fuse-ld=lld` (could be the clang installation on the 
machine on which I am building), and clang-13 causes a compile error.


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

https://reviews.llvm.org/D129156

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

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

I think this change makes sense to me. Ultimately, this API is about the 
potential to be constexpr, so there's no need to pay for the expense of running 
the function with some ginned up arguments -- if we can compile it as a 
constexpr function, it's potentially constexpr.




Comment at: clang/lib/AST/Interp/Context.cpp:44-47
   if (!Func->isConstexpr())
 return false;
 
+  return true;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135569

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

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:590
 
+// FIXME: may be incomplete
+static unsigned CalculateTemplateDepthForConstraints(const Expr *Constr) {

I'd like some sort of assert in the case where you don't know what to do here.  
We need to collect all of these cases best we can in advance.

I'd suggest ALSO running libcxx tests against this once you have the assert in 
place, which should help.



Comment at: clang/lib/Sema/SemaConcept.cpp:706
+  std::tie(OldConstr, NewConstr) =
+  AdjustConstraintDepth::UnifyConstraintDepthFromDecl(*this, Old, 
OldConstr,
+  New, NewConstr);

Unless I'm missing something, I don't think this idea of 'unify constraint 
depth' is correct.  We should be able, from the decl itself, to determine the 
depths?  What is the difference here that I'm not getting?



Comment at: clang/lib/Sema/SemaConcept.cpp:1340
+bool Sema::IsAtLeastAsConstrained(NamedDecl *D1,
+  MutableArrayRef AC1,
+  NamedDecl *D2,

Can you explain why this is a MutableArrayRef now?  I believe this means it'll 
now modify the arrays that are passed into it, which we don't necessarily want, 
right?  A new 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128

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

[clang] 01bbe87 - [CGStmt] Use helper functions to set memory attributes (NFC)

[Nikita Popov via cfe-commits]

Author: Nikita Popov
Date: 2022-10-12T16:38:39+02:00
New Revision: 01bbe87fbb66cad9193c97843b0dd20aa2bd24ae

URL: 
https://github.com/llvm/llvm-project/commit/01bbe87fbb66cad9193c97843b0dd20aa2bd24ae
DIFF: 
https://github.com/llvm/llvm-project/commit/01bbe87fbb66cad9193c97843b0dd20aa2bd24ae.diff

LOG: [CGStmt] Use helper functions to set memory attributes (NFC)

Added: 


Modified: 
clang/lib/CodeGen/CGStmt.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index ebbc79cc8b4b6..30c955b3d43fd 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -2280,9 +2280,9 @@ static void UpdateAsmCallInst(llvm::CallBase &Result, 
bool HasSideEffect,
   // Attach readnone and readonly attributes.
   if (!HasSideEffect) {
 if (ReadNone)
-  Result.addFnAttr(llvm::Attribute::ReadNone);
+  Result.setDoesNotAccessMemory();
 else if (ReadOnly)
-  Result.addFnAttr(llvm::Attribute::ReadOnly);
+  Result.setOnlyReadsMemory();
   }
 
   // Add elementtype attribute for indirect constraints.



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

[PATCH] D135118: [clang/Sema] Fix non-deterministic order for certain kind of diagnostics

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D135118#3851050 , @akyrtzi wrote:

> @nikic, it seems to have recovered 
> (http://llvm-compile-time-tracker.com/compare.php?from=c49cde6467f9bf200640db763152a9dc7f009520&to=0456acbfb942f127359a8defd1b4f1f44420df3e&stat=instructions)
>  let me know if you have concerns.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135118

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

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

[Liming Liu via Phabricator via cfe-commits]

lime added a comment.

> Unless I'm missing something, I don't think this idea of 'unify constraint 
> depth' is correct. We should be able, from the decl itself, to determine the 
> depths? What is the difference here that I'm not getting?

It is explained by the inline comments in sequence.

> Can you explain why this is a MutableArrayRef now? I believe this means it'll 
> now modify the arrays that are passed into it, which we don't necessarily 
> want, right?

In the previous update, I mentioned using ArrayRef is not as efficient as 
MutableArrayRef. And there was no feedback on my comment for several days, so I 
changed as I said. Additionally, I found some code once called 
`IsAtLeastAsConstrained` for two constraints, and then call 
`IsAtLeastAsConstrained` again with the sequence of the two constraints 
switched. So using MutableArrayRef also saves a potential adjustment.
Any way, adjusting depths here might unique to template template parameters. If 
so, parsing the require clause in the unit test with depth equal to 0 should be 
a better solution, and things about CalculateTemplateDepthForConstraints and 
ArrayRef could remain unchanged.




Comment at: clang/lib/Sema/SemaConcept.cpp:581
 // getTemplateDepth, because it includes already instantiated parents.
 static unsigned CalculateTemplateDepthForConstraints(Sema &S,
  const NamedDecl *ND) {

1. The orignal `CalculateTemplateDepthForConstraints` calculates the depth from 
`NamedDecl`.



Comment at: clang/lib/Sema/SemaConcept.cpp:621
-  if (Old && New && Old != New) {
-unsigned Depth1 = CalculateTemplateDepthForConstraints(
-*this, Old);

2. And it was only called in `AreConstraintExpressionsEqual`.



Comment at: clang/lib/Sema/SemaConcept.cpp:706
+  std::tie(OldConstr, NewConstr) =
+  AdjustConstraintDepth::UnifyConstraintDepthFromDecl(*this, Old, 
OldConstr,
+  New, NewConstr);

erichkeane wrote:
> Unless I'm missing something, I don't think this idea of 'unify constraint 
> depth' is correct.  We should be able, from the decl itself, to determine the 
> depths?  What is the difference here that I'm not getting?
3. I extracted the calls to the original `CalculateTemplateDepthForConstraints` 
as `UnifyConstraintDepthFromDecl`. Calculating from a declaration is indeed not 
accurate, as it returns 1 for `template class>`, but the depth 
in the constraint is actually 0. This is one reason why a previous version of 
patch breaks some unit tests. But I leaves it to keep the code unchanged 
functionally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128

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

[PATCH] D134654: [clang] Detect header loops

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:323
   "whitespace recommended after macro name">;
+def warn_include_cycle : Warning<"#include cycle">,
+   InGroup>, DefaultIgnore;

urnathan wrote:
> urnathan wrote:
> > philnik wrote:
> > > aaron.ballman wrote:
> > > > urnathan wrote:
> > > > > aaron.ballman wrote:
> > > > > > This diagnostic doesn't really seem like it's going to help the 
> > > > > > user to know what's wrong with their code or how to fix it. (Also, 
> > > > > > the notes @erichkeane was hoping were emitted don't seem to be, at 
> > > > > > least according to the new test cases.) I think we need to give 
> > > > > > users a bit more guidance here.
> > > > > The test infrastructire ignores the 'included from ' chain, which is 
> > > > > emitted.  What do you suggest?
> > > > Even posting the "included from" doesn't help clarify what's wrong with 
> > > > the code though, if I'm understanding the tests properly. My concern is 
> > > > with code like:
> > > > ```
> > > > // self.h
> > > > #ifndef GUARD
> > > > #define GUARD
> > > > 
> > > > #include "self.h" // expected-warning {{#include cycle}}
> > > > #endif
> > > > ```
> > > > (This is effectively the same test as `warn-loop-macro-1.h`.) There is 
> > > > no include *cycle* ("a series of events that are regularly repeated in 
> > > > the same order.") in this code -- the header is included for the first 
> > > > time, `GUARD` is not defined, then `GUARD` is defined and the header is 
> > > > included for the second time. This time `GUARD` is defined and no 
> > > > further cycles into `self.h` occurs.
> > > > 
> > > > But I think I'm seeing now what you're trying to get at (correct me if 
> > > > I'm wrong): when processing an include stack, opening the same header 
> > > > file twice is a problem (regardless of the contents of the header or 
> > > > how you got into the same file twice)? If that's accurate, would it 
> > > > make more sense to diagnose this as:
> > > > 
> > > > `including the same header (%0) more than once in an include stack 
> > > > causes %select{incorrect behavior of Clang modules|the header to not be 
> > > > a C++ module header unit}1`
> > > > 
> > > > or something along those lines? Basically, I'd like to avoid saying 
> > > > "cycle" because that starts to sound like "you have potentially 
> > > > infinite recursion in the include stack" and I'd like to add 
> > > > information about what's actually wrong instead of pointing out what 
> > > > the code does and hoping the user knows why that's bad.
> > > FWIW in libc++ we have a script to check that we don't include the header 
> > > more than once in a stack and also call it a cycle, so there is some 
> > > precedent of calling it an include cycle, even thought it's technically 
> > > not a cycle.
> > Aaron, correct, the problem is with clang-modules/c++ header units.  The 
> > loop means that the header file does not expand to the same sequence of 
> > tokens at every inclusion.  This is particularly problematic with module 
> > maps, because the compiler will automatically start turning a header into a 
> > clang-module upon the first #include -- which might not be at the outermost 
> > level, (if it starts with a header at some other position in the loop).  
> > Then when it starts with this header, it'll think it already has converted 
> > and loaded it.  One ends up with very obscure failure modes (such as link 
> > errors concerning missing template instantations).
> > 
> > I wonder if (in addition to allow detecting this in general), the warning 
> > should be enabled by default when building a header-unit/clang-module and 
> > one reincludes a header corresponding to a module currently being built.  
> > That's the same check, because there can be a stack of modules being built, 
> > but I think we need some additional checking to avoid triggering on 
> > 'textual headers' from the module map.
> > 
> > IMHO it is still a cycle, it's a closed loop that executes exactly once :)  
> > I can see the confusion though.  I mean, 'for (auto i = 0; i < 1; i++) {}' 
> > is still a loop, right?  just not an unbounded one. 
> > 
> >  Anyway, I think your diagagnostic text is better, thanks!
> philnik, that's good to know.  
> 
Thank you for the help understanding the root of what we're diagnosing! That's 
fascinating and something I'm really glad we're getting a diagnostic for! I'm 
more strongly in favor of this being an on-by-default warning that only 
diagnoses when trying to build a module (if possible).


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

https://reviews.llvm.org/D134654

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

To avoid more tangential discussion here - I've opened 
https://discourse.llvm.org/t/generating-pcm-module-interfaces-and-regular-object-files-from-the-same-compiler-invocation/65918
  ... it would be great if the major stakeholders here especially  @rsmith 
could comment on the design intentions.


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

https://reviews.llvm.org/D134267

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

[PATCH] D135784: Add option to reparse an ASTUnit keeping its SourceManager

[Giuliano Belinassi via Phabricator via cfe-commits]

giulianobelinassi created this revision.
Herald added a project: All.
giulianobelinassi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When issuing a reparse, the SourceManager is then rebuilt from the
FileMgr object it had in ASTUnit. This, however, issues an reload
of the files MemoryBuffer which could have been changed by some
analysis process.

For example, an tooling application can call overrideFileContents with
a new MemoryBuffer with modified pieces of code, and then issue a
reparse to either rebuild the tree or update the SourceLocations. This
is useful when used with the Rewriter object because one can do:

  std::string *modified_str = new std::string(rewritebuf->begin(), 
rewritebuf->end());
  auto new_membuff = llvm::MemoryBuffer::getMemBuffer(*modified_str);
  SM.overrideFileContents(file_entry, std::move(new_membuff));
  
  AU->Reparse(..., /*KeepSourceManager=*/true);

And then the modifications to the file is loaded into the AU instance
of ASTUnit object. The test case in this patch reflects this use case.

Signed-off-by: Giuliano Belinassi 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135784

Files:
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/unittests/Frontend/ASTUnitTest.cpp

Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -178,4 +178,43 @@
   ASSERT_NE(ErrUnit->stored_diag_size(), 0U);
 }
 
+TEST_F(ASTUnitTest, ReparseWithOldSourceMgr) {
+  llvm::IntrusiveRefCntPtr InMemoryFs =
+  new llvm::vfs::InMemoryFileSystem();
+  InMemoryFs->addFile("test.cpp", 0, llvm::MemoryBuffer::getMemBuffer(R"cpp(
+  void f() {}
+)cpp"));
+
+  const char *Args[] = {"clang", "test.cpp"};
+  Diags = CompilerInstance::createDiagnostics(new DiagnosticOptions());
+  CreateInvocationOptions CIOpts;
+  CIOpts.Diags = Diags;
+  CInvok = createInvocation(Args, std::move(CIOpts));
+  ASSERT_TRUE(CInvok);
+
+  FileManager *FileMgr = new FileManager(FileSystemOptions(), InMemoryFs);
+  PCHContainerOps = std::make_shared();
+
+  auto AU = ASTUnit::LoadFromCompilerInvocation(
+  CInvok, PCHContainerOps, Diags, FileMgr, false, CaptureDiagsKind::None, 1,
+  TU_Complete, false, false, false);
+
+  ASSERT_TRUE(AU);
+
+  const FileEntry *FEntry = AU->getSourceManager().getFileEntryForID(
+  AU->getSourceManager().getMainFileID());
+  std::string NewCode = "void g() {}";
+
+  AU->getSourceManager().overrideFileContents(
+  FEntry, llvm::MemoryBuffer::getMemBuffer(NewCode));
+
+  AU->Reparse(std::make_shared(), None, nullptr,
+  /*KeepFileMgr=*/true);
+
+  auto FileMemBuf = AU->getSourceManager().getMemoryBufferForFileOrFake(FEntry);
+
+  // Verify that we still get the same memory buffer after Reparse.
+  EXPECT_TRUE(FileMemBuf.getBuffer() == NewCode);
+}
+
 } // anonymous namespace
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1094,7 +1094,9 @@
 /// contain any translation-unit information, false otherwise.
 bool ASTUnit::Parse(std::shared_ptr PCHContainerOps,
 std::unique_ptr OverrideMainBuffer,
-IntrusiveRefCntPtr VFS) {
+IntrusiveRefCntPtr VFS,
+bool KeepSourceMgr) {
+
   if (!Invocation)
 return true;
 
@@ -1130,7 +1132,8 @@
   // Ensure that Clang has a FileManager with the right VFS, which may have
   // changed above in AddImplicitPreamble.  If VFS is nullptr, rely on
   // createFileManager to create one.
-  if (VFS && FileMgr && &FileMgr->getVirtualFileSystem() == VFS)
+  if (FileMgr &&
+  (KeepSourceMgr || (VFS && &FileMgr->getVirtualFileSystem() == VFS)))
 Clang->setFileManager(&*FileMgr);
   else
 FileMgr = Clang->createFileManager(std::move(VFS));
@@ -1164,10 +1167,12 @@
   LangOpts = Clang->getInvocation().LangOpts;
   FileSystemOpts = Clang->getFileSystemOpts();
 
-  ResetForParse();
+  ResetForParse(KeepSourceMgr);
+
+  if (!KeepSourceMgr && !SourceMgr)
+SourceMgr =
+new SourceManager(getDiagnostics(), *FileMgr, UserFilesAreVolatile);
 
-  SourceMgr = new SourceManager(getDiagnostics(), *FileMgr,
-UserFilesAreVolatile);
   if (!OverrideMainBuffer) {
 checkAndRemoveNonDriverDiags(StoredDiagnostics);
 TopLevelDeclsInPreamble.clear();
@@ -1807,7 +1812,8 @@
 
 bool ASTUnit::Reparse(std::shared_ptr PCHContainerOps,
   ArrayRef RemappedFiles,
-  IntrusiveRefCntPtr VFS) {
+  IntrusiveRefCntPtr VFS,
+  bool KeepSourceMgr) {
   if (!Invocation)
 return true;
 
@@ -1835,20 +1841,22 @@
   // If we have a preamble file lying around, or if we might

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

I tried out D135772 , and our build got 
significantly farther than before! I unfortunately discovered another piece of 
code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e 
 Clang and 
GCC accept in C++20 mode, but Clang trunk does not 
(https://godbolt.org/z/q1q4nfobK):

  struct String {
String(char *);
bool operator==(String const &);
void operator!=(String const &);
  };
  
  extern char* c;
  extern String s;
  int test() {
return c == s;
  }



  :10:12: error: invalid operands to binary expression ('char *' and 
'String')
return c == s;
   ~ ^  ~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D129156#3852553 , @tarunprabhu 
wrote:

> In D129156#3851843 , @awarzynski 
> wrote:
>
>> @tarunprabhu Could you confirm that with the build command above 
>> "pass-plugin.f90" is failing for you? It is for me.
>
> @awarzynski What compiler do you use to build this? gcc doesn't seem to like 
> `-gmlt`, clang-12 complains about `-fuse-ld=lld` (could be the clang 
> installation on the machine on which I am building), and clang-13 causes a 
> compile error.
>
> [EDIT: Tag @awarzynski]

I doubt those particular flags matter here. You can trim that CMake command as 
follows:

  cmake ../llvm -D LLVM_ENABLE_PROJECTS="mlir;flang;clang;llvm" -G Ninja -D 
CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_BUILD_EXAMPLES=ON 
  ninja check-flang


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

https://reviews.llvm.org/D129156

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

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:706
+  std::tie(OldConstr, NewConstr) =
+  AdjustConstraintDepth::UnifyConstraintDepthFromDecl(*this, Old, 
OldConstr,
+  New, NewConstr);

lime wrote:
> erichkeane wrote:
> > Unless I'm missing something, I don't think this idea of 'unify constraint 
> > depth' is correct.  We should be able, from the decl itself, to determine 
> > the depths?  What is the difference here that I'm not getting?
> 3. I extracted the calls to the original 
> `CalculateTemplateDepthForConstraints` as `UnifyConstraintDepthFromDecl`. 
> Calculating from a declaration is indeed not accurate, as it returns 1 for 
> `template class>`, but the depth in the constraint is actually 
> 0. This is one reason why a previous version of patch breaks some unit tests. 
> But I leaves it to keep the code unchanged functionally.
Hmm... SO it seems based on debugging that the issue is exclusive with template 
template parameters.  It DOES make sense that the inner-template is 'deeper', 
but I think the problem is that `getTemplateInstantiationArgs` isn't correctly 
handling a ClassTemplateDecl, so the "D2" in `IsAtLeastAsConstrainedAs` is 
returning 0, instead of 1.

The "Depth" of something is the 'number of layers of template arguments'.  So 
for: 'template class>', the answer SHOULD be 1.

So I think the problem is basically that for X, the answer should ALSO be 1 
(since it has template parameters), but we aren't handling it.  So I think we 
just need to correctly just call the `ClassTemplateDecl::getTemplatedDecl` at 
one point on that one.



Comment at: clang/lib/Sema/SemaConcept.cpp:1340
+bool Sema::IsAtLeastAsConstrained(NamedDecl *D1,
+  MutableArrayRef AC1,
+  NamedDecl *D2,

erichkeane wrote:
> Can you explain why this is a MutableArrayRef now?  I believe this means 
> it'll now modify the arrays that are passed into it, which we don't 
> necessarily want, right?  A new 
I see your response... I am a bit concerned about the use of this in 
SemaTemplate, which re-uses the generated array after this however (though for 
diagnostics?).

Everywhere else doesn't seem to be using the underlying array after the fact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3852909 , @BertalanD wrote:

> I tried out D135772 , and our build got 
> significantly farther than before! I unfortunately discovered another piece 
> of code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e 
>  Clang 
> and GCC accept in C++20 mode, but Clang trunk does not 
> (https://godbolt.org/z/q1q4nfobK):
>
>   struct String {
> String(char *);
> bool operator==(String const &);
> void operator!=(String const &);
>   };
>   
>   extern char* c;
>   extern String s;
>   int test() {
> return c == s;
>   }
>
>
>
>   :10:12: error: invalid operands to binary expression ('char *' and 
> 'String')
> return c == s;
>~ ^  ~

Thanks!  I'll look into that as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

(I know this is a very contrived example with `void operator!=`, but that is 
what CVise spat out, and the actual failures are related to comparison 
operators too)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: Uthkarsh.
erichkeane added a comment.

In D126907#3852975 , @BertalanD wrote:

> (I know this is a very contrived example with `void operator!=`, but that is 
> what CVise spat out, and the actual failures are related to comparison 
> operators too)
>
> edit: hah, it repros when it returns `bool` too -- not sure how that `void` 
> came to be...

Looking more closely at that, I don't think it has anything to do with this 
patch.  I know `operator==` (and the materialized inverse) stuff was worked on 
separately recently by @Uthkarsh in 38b9d313e6945804fffc654f849cfa05ba2c713d 
.  See: 
https://reviews.llvm.org/D134529

The Equality-operator stuff has been particularly awkward in the C++ committee 
I think (whether or not reversed candidates work or not have had a bunch of 
tweaking), so you'll have to follow that up there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Erich Keane via Phabricator via cfe-commits]

erichkeane added subscribers: BertalanD, erichkeane.
erichkeane added a comment.

Note that @BertalanD  noticed an issue with what I expect to be this patch: 
https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an 
intended change?  It seems to me that the reversing behavior of the equality 
operators is a little awkward here (note that commenting out != makes that 
'work' again).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D130131: [HLSL] CodeGen hlsl cbuffer/tbuffer.

[Xiang Li via Phabricator via cfe-commits]

python3kgae added a comment.

gentle ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D134529#3852990 , @erichkeane 
wrote:

> Note that @BertalanD  noticed an issue with what I expect to be this patch: 
> https://godbolt.org/z/Wjb9rsEYG
>
> Can someone more knowledgable about this paper please make sure it is an 
> intended change?  It seems to me that the reversing behavior of the equality 
> operators is a little awkward here (note that commenting out != makes that 
> 'work' again).

This is the intended behaviour, yes. The paper aims to revert back to C++17 
behavior when we see a matching `operator !=` and this was an error in C++17: 
https://godbolt.org/z/K3cr9MMh5.
As mentioned above, the actual compiler error message is not very helpful and 
is something that could be improved, but this was not introduced by this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D129156: Add -fpass-plugin option to Flang

[Tarun Prabhu via Phabricator via cfe-commits]

tarunprabhu added a comment.

In D129156#3851843 , @awarzynski 
wrote:

> @tarunprabhu Could you confirm that with the build command above 
> "pass-plugin.f90" is failing for you? It is for me.

The tests still passed. Some relevant output from the test is below (I added 
the `-DLLVM_LIT_ARGS="-v"` to the configure line).

  UNSUPPORTED: Flang :: Driver/pass-plugin.f90 (1721 of 1809)
  PASS: Flang :: Driver/pass-plugin-not-found.f90 (560 of 1809)
  
  Testing Time: 4.81s
Unsupported  :   11
Passed   : 1780
Expectedly Failed:   18

I am not sure what I am doing differently. This is starting from an empty build 
directory.


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

https://reviews.llvm.org/D129156

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D134529#3853036 , @ilya-biryukov 
wrote:

> In D134529#3852990 , @erichkeane 
> wrote:
>
>> Note that @BertalanD  noticed an issue with what I expect to be this patch: 
>> https://godbolt.org/z/Wjb9rsEYG
>>
>> Can someone more knowledgable about this paper please make sure it is an 
>> intended change?  It seems to me that the reversing behavior of the equality 
>> operators is a little awkward here (note that commenting out != makes that 
>> 'work' again).
>
> This is the intended behaviour, yes. The paper aims to revert back to C++17 
> behavior when we see a matching `operator !=` and this was an error in C++17: 
> https://godbolt.org/z/K3cr9MMh5.
> As mentioned above, the actual compiler error message is not very helpful and 
> is something that could be improved, but this was not introduced by this 
> patch.

Thank you very much for clarifying!  I wasn't sure, and didn't want to mislead 
@BertalanD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

In D134529#3852990 , @erichkeane 
wrote:

> Note that @BertalanD  noticed an issue with what I expect to be this patch: 
> https://godbolt.org/z/Wjb9rsEYG
>
> Can someone more knowledgable about this paper please make sure it is an 
> intended change?  It seems to me that the reversing behavior of the equality 
> operators is a little awkward here (note that commenting out != makes that 
> 'work' again).

This is intentional. Example from https://eel.is/c++draft/over.match.oper#4

  struct A {};
  template bool operator==(A, T); // #1
  bool a1 = 0 == A(); // OK, calls reversed #1
  template bool operator!=(A, T);
  bool a2 = 0 == A(); // error, #1 is not a rewrite 
target

Adding a matching `operator!=` disables reversing arguments (C++17 mode). The 
LHS in this case should provide a `operator==`.

The diagnostic might not be the most helpful at this point is. You can choose to

- Remove `operator!=`. Allows assuming `x==y` is equivalent to `y==x` and 
therefore uses the same function with reversed arguments.
- Write `s == c` instead of `c == s`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

Thank you all for the help and clarification! I'll go fix our code then.

As an aside, I understand that this paper now retroactively applies to the C++ 
standard, but code has already been written that assumes that what my example 
is doing is valid. Does it deserve a mention in the Potentially Breaking 
Changes section of the release notes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hmm, as mentioned on the bug, I think wrapping the deps and generated sources 
into a library is an important part of what the build rule does.

If we need to split it up like this, I think a `generate_clang_protos` wrapper 
in FindGRPC.cmake might be the right place to encapsulate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

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

[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D128745#3851458 , @rjmccall wrote:

> Was this change in LLVM 15, or is it still unreleased?

This is still unreleased. It will be released in Clang16.

In D128745#3852409 , @aaron.ballman 
wrote:

> This sounds like a good plan to me. When you remove the ABI compat checks, 
> please be sure to add the clang-vendors group to the review and a release 
> note to the potentially breaking changes section. Once that lands, you should 
> also make an announcement in Discourse.

Will do. Thanks for the heads-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745

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

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, JonChesterfield, tra, yaxunl.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

If an invalid architecture is set we currently return an empty string.
This will cause the offloading toolchain to continue to be built and
hit an assertion elsewhere due to the invalid architecture. This patch
fixes that so we now correctly exit.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135791

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/cuda-phases.cu

Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, "device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4188,10 +4188,9 @@
 
 /// Returns the canonical name for the offloading architecture when using a HIP
 /// or CUDA architecture.
-static StringRef getCanonicalArchString(Compilation &C,
-const llvm::opt::DerivedArgList &Args,
-StringRef ArchStr,
-const llvm::Triple &Triple) {
+static Optional
+getCanonicalArchString(Compilation &C, const llvm::opt::DerivedArgList &Args,
+   StringRef ArchStr, const llvm::Triple &Triple) {
   // Lookup the CUDA / HIP architecture string. Only report an error if we were
   // expecting the triple to be only NVPTX / AMDGPU.
   CudaArch Arch = StringToCudaArch(getProcessorFromTargetID(Triple, ArchStr));
@@ -4199,7 +4198,7 @@
   (Arch == CudaArch::UNKNOWN || !IsNVIDIAGpuArch(Arch))) {
 C.getDriver().Diag(clang::diag::err_drv_offload_bad_gpu_arch)
 << "CUDA" << ArchStr;
-return StringRef();
+return None;
   } else if (Triple.isAMDGPU() &&
  (Arch == CudaArch::UNKNOWN || !IsAMDGpuArch(Arch))) {
 C.getDriver().Diag(clang::diag::err_drv_offload_bad_gpu_arch)
@@ -4208,7 +4207,7 @@
   }
 
   if (IsNVIDIAGpuArch(Arch))
-return Args.MakeArgStringRef(CudaArchToString(Arch));
+return StringRef(Args.MakeArgStringRef(CudaArchToString(Arch)));
 
   if (IsAMDGpuArch(Arch)) {
 llvm::StringMap Features;
@@ -4221,7 +4220,8 @@
   C.setContainsError();
   return StringRef();
 }
-return Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features));
+return StringRef(
+Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features)));
   }
 
   // If the input isn't CUDA or HIP just return the architecture.
@@ -4273,15 +4273,28 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+if (auto ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple()))
+  Archs.insert(*ArchStr);
+else
+  return Archs;
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == StringRef("all")) {
   Archs.clear();
-else
-  Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+} else if (auto Str =
+   getCanonicalArchString(C, Args, Arch, TC->getTriple())) {
+  auto ArchStr = getCanonicalArchString(C, Args, Arch, TC->getTriple());
+  Archs.erase(*ArchStr);
+} else {
+
+  return Archs;
+}
   }
 

[PATCH] D134637: clang-tblgen build: avoid duplicate inclusion of libLLVMSupport

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D134637#3835935 , @nhaehnle wrote:

> Thank you all for the reviews. I've integrated the suggestions except for:
>
>> A possible alternative solution would be to build clangSupport_sources as an 
>> object library, and then link that library into clangSupport and 
>> clang-tblgen which could be done unconditionally; the advantage is that you 
>> don't need to compile clangSupport_sources twice.
>
> I'm not sure how this would work. It doesn't seem to be something with 
> precedent in the LLVM tree, and seems to require using raw CMake 
> `add_library`, though it's quite likely that I missed something?

We support building object libraries, see 
https://github.com/llvm/llvm-project/blob/702d937f1e1d42892ab43d1b591f5041ce2f4e78/llvm/cmake/modules/AddLLVM.cmake#L443,
 which should be already set for `clangSupport`, see 
https://github.com/llvm/llvm-project/blob/702d937f1e1d42892ab43d1b591f5041ce2f4e78/clang/cmake/modules/AddClang.cmake#L102.
 In this case, it may be sufficient, to just link `obj.clangSupport` (that is, 
you'd use `target_link_libraries(clang-tblgen PRIVATE obj.clangSupport)`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134637

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

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 467188.
jhuber6 added a comment.

Fix missing `None` return on HIP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/cuda-phases.cu

Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, "device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4188,10 +4188,9 @@
 
 /// Returns the canonical name for the offloading architecture when using a HIP
 /// or CUDA architecture.
-static StringRef getCanonicalArchString(Compilation &C,
-const llvm::opt::DerivedArgList &Args,
-StringRef ArchStr,
-const llvm::Triple &Triple) {
+static Optional
+getCanonicalArchString(Compilation &C, const llvm::opt::DerivedArgList &Args,
+   StringRef ArchStr, const llvm::Triple &Triple) {
   // Lookup the CUDA / HIP architecture string. Only report an error if we were
   // expecting the triple to be only NVPTX / AMDGPU.
   CudaArch Arch = StringToCudaArch(getProcessorFromTargetID(Triple, ArchStr));
@@ -4199,16 +4198,16 @@
   (Arch == CudaArch::UNKNOWN || !IsNVIDIAGpuArch(Arch))) {
 C.getDriver().Diag(clang::diag::err_drv_offload_bad_gpu_arch)
 << "CUDA" << ArchStr;
-return StringRef();
+return None;
   } else if (Triple.isAMDGPU() &&
  (Arch == CudaArch::UNKNOWN || !IsAMDGpuArch(Arch))) {
 C.getDriver().Diag(clang::diag::err_drv_offload_bad_gpu_arch)
 << "HIP" << ArchStr;
-return StringRef();
+return None;
   }
 
   if (IsNVIDIAGpuArch(Arch))
-return Args.MakeArgStringRef(CudaArchToString(Arch));
+return StringRef(Args.MakeArgStringRef(CudaArchToString(Arch)));
 
   if (IsAMDGpuArch(Arch)) {
 llvm::StringMap Features;
@@ -4221,7 +4220,8 @@
   C.setContainsError();
   return StringRef();
 }
-return Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features));
+return StringRef(
+Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features)));
   }
 
   // If the input isn't CUDA or HIP just return the architecture.
@@ -4273,15 +4273,28 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+if (auto ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple()))
+  Archs.insert(*ArchStr);
+else
+  return Archs;
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == StringRef("all")) {
   Archs.clear();
-else
-  Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+} else if (auto Str =
+   getCanonicalArchString(C, Args, Arch, TC->getTriple())) {
+  auto ArchStr = getCanonicalArchString(C, Args, Arch, TC->getTriple());
+  Archs.erase(*ArchStr);
+} else {
+
+  return Archs;
+}
   }
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135724: [HIP] Fix unbundling archive

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1839
   auto Ext = IsMSVC ? ".lib" : ".a";
-  if (!Lib.startswith(":") && llvm::sys::fs::exists(Lib)) {
-ArchiveOfBundles = Lib;
-FoundAOB = true;
+  if (!Lib.startswith(":") && !Lib.startswith("-l")) {
+if (llvm::sys::fs::exists(Lib)) {

I'm puzzled a bit. What is expected to be in 'Lib' ?
If it contains the the whole `-llibrary` option passed by the user, then why do 
we check for `:` prefix?
If it contains whatever was passed via `-l`, but without `-l` 
itself, then why do we check for  `-l` prefix?



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

https://reviews.llvm.org/D135724

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

[PATCH] D135011: Add builtin_elementwise_sin and builtin_elementwise_cos

[Joshua Batista via Phabricator via cfe-commits]

bob80905 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135011

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

[PATCH] D135724: [HIP] Fix unbundling archive

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1839
   auto Ext = IsMSVC ? ".lib" : ".a";
-  if (!Lib.startswith(":") && llvm::sys::fs::exists(Lib)) {
-ArchiveOfBundles = Lib;
-FoundAOB = true;
+  if (!Lib.startswith(":") && !Lib.startswith("-l")) {
+if (llvm::sys::fs::exists(Lib)) {

tra wrote:
> I'm puzzled a bit. What is expected to be in 'Lib' ?
> If it contains the the whole `-llibrary` option passed by the user, then why 
> do we check for `:` prefix?
> If it contains whatever was passed via `-l`, but without `-l` 
> itself, then why do we check for  `-l` prefix?
> 
This function is also used for unbundling archive files specified as input 
files in the clang command line. There are three cases:

For option `-l:xxx`, Lib contains `:xxx`.

For option `-lxxx`, Lib contains `-lxxx`.

For `xxx.a` passed as input file, Lib contains `xxx.a`.


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

https://reviews.llvm.org/D135724

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

[PATCH] D135796: [HIP] Detect HIP for Debian/Fedora

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added a reviewer: tra.
Herald added subscribers: kosarev, kerbowa, jvesely.
Herald added a project: All.
yaxunl requested review of this revision.
Herald added a subscriber: MaskRay.

HIP is installed at /usr or /usr/local on Debin/Fedora,
and the version file is at {root}/share/hip/version.


https://reviews.llvm.org/D135796

Files:
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/test/Driver/hip-version.hip
  clang/test/Driver/rocm-detect.hip

Index: clang/test/Driver/rocm-detect.hip
===
--- clang/test/Driver/rocm-detect.hip
+++ clang/test/Driver/rocm-detect.hip
@@ -34,6 +34,24 @@
 // RUN:   --print-rocm-search-dirs %s 2>&1 \
 // RUN:   | FileCheck -check-prefixes=ROCM-REL %s
 
+// Test detecting HIP installed at /usr directory.
+// RUN: rm -rf %t/root/usr
+// RUN: mkdir -p %t/root/usr
+// RUN: cp -r %S/Inputs/rocm/* %t/root/usr
+// RUN: mkdir -p %t/root/usr/share/hip
+// RUN: mv %t/root/usr/bin/.hipVersion %t/root/usr/share/hip/version
+// RUN: %clang -### -v -target x86_64-linux-gnu --sysroot=%t/root 2>&1 \
+// RUN:   | FileCheck -check-prefixes=USR %s
+
+// Test detecting HIP installed at /usr/local directory.
+// RUN: rm -rf %t/root/usr
+// RUN: mkdir -p %t/root/usr/local
+// RUN: cp -r %S/Inputs/rocm/* %t/root/usr/local
+// RUN: mkdir -p %t/root/usr/local/share/hip
+// RUN: mv %t/root/usr/local/bin/.hipVersion %t/root/usr/local/share/hip/version
+// RUN: %clang -### -v -target x86_64-linux-gnu --sysroot=%t/root 2>&1 \
+// RUN:   | FileCheck -check-prefixes=USR-LOCAL %s
+
 // Test ROCm installation built by SPACK by invoke clang at %T/rocm-spack/llvm-amdgpu-*
 // directory through a soft link.
 
@@ -80,6 +98,9 @@
 // ROCM-REL: ROCm installation search path: {{.*}}/opt/rocm
 // ROCM-REL: ROCm installation search path: {{.*}}/opt/rocm-3.10.0
 
+// USR: Found HIP installation: {{.*}}root/usr, version 3.6.20214-a2917cd
+// USR-LOCAL: Found HIP installation: {{.*}}root/usr/local, version 3.6.20214-a2917cd
+
 // SPACK: ROCm installation search path (Spack 4.0.0): [[DIR:.*]]
 // SPACK: ROCm installation search path: [[CLANG:.*]]
 // SPACK: ROCm installation search path: [[DIR]]/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z
Index: clang/test/Driver/hip-version.hip
===
--- clang/test/Driver/hip-version.hip
+++ clang/test/Driver/hip-version.hip
@@ -9,6 +9,14 @@
 // RUN:   --target=amdgcn-amd-amdhsa \
 // RUN:   | FileCheck -check-prefixes=FOUND %s
 
+// RUN: rm -rf %t/Inputs
+// RUN: mkdir -p %t/Inputs
+// RUN: cp -r %S/Inputs/rocm %t/Inputs
+// RUN: mkdir -p %t/Inputs/rocm/share/hip
+// RUN: mv %t/Inputs/rocm/bin/.hipVersion %t/Inputs/rocm/share/hip/version
+// RUN: %clang -v --rocm-path=%t/Inputs/rocm 2>&1 \
+// RUN:   | FileCheck -check-prefixes=FOUND %s
+
 // FOUND: Found HIP installation: {{.*Inputs.*rocm}}, version 3.6.20214-a2917cd
 
 // When --rocm-path is set and .hipVersion is not found, use default version
Index: clang/lib/Driver/ToolChains/AMDGPU.cpp
===
--- clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -306,6 +306,11 @@
 ROCmSearchDirs.emplace_back(D.SysRoot + "/opt/" + LatestROCm,
 /*StrictChecking=*/true);
 
+  ROCmSearchDirs.emplace_back(D.SysRoot + "/usr/local",
+  /*StrictChecking=*/true);
+  ROCmSearchDirs.emplace_back(D.SysRoot + "/usr",
+  /*StrictChecking=*/true);
+
   DoPrintROCmSearchDirs();
   return ROCmSearchDirs;
 }
@@ -465,7 +470,9 @@
 llvm::sys::path::append(SharePath, "share");
 
 // If HIP version file can be found and parsed, use HIP version from there.
-for (const auto &VersionFilePath : {std::string(SharePath) + "/hipVersion", std::string(BinPath) + "/.hipVersion"}) {
+for (const auto &VersionFilePath :
+ {std::string(SharePath) + "/hip/version",
+  std::string(BinPath) + "/.hipVersion"}) {
   llvm::ErrorOr> VersionFile =
   FS.getBufferForFile(VersionFilePath);
   if (!VersionFile)
@@ -477,13 +484,12 @@
   HasHIPRuntime = true;
   return;
 }
-// Otherwise, if -rocm-path is specified (no strict checking), use the default HIP version
-// or specified by --hip-version.
+// Otherwise, if -rocm-path is specified (no strict checking), use the
+// default HIP version or specified by --hip-version.
 if (!Candidate.StrictChecking) {
   HasHIPRuntime = true;
   return;
 }
-
   }
   HasHIPRuntime = false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

LGTM with few nits.




Comment at: clang/lib/Driver/Driver.cpp:4191
 /// or CUDA architecture.
-static StringRef getCanonicalArchString(Compilation &C,
-const llvm::opt::DerivedArgList &Args,
-StringRef ArchStr,
-const llvm::Triple &Triple) {
+static Optional
+getCanonicalArchString(Compilation &C, const llvm::opt::DerivedArgList &Args,

Nit. Optional is fine, but we could've just checked returned value for 
Arch.empty().



Comment at: clang/lib/Driver/Driver.cpp:4288
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == StringRef("all")) {
   Archs.clear();

Nit: do we need explicit `StringRef()` here? If implicit type conversion does 
not work, we could use `Arch.equals("all")`.
The current code is fine, but it does stick out a bit and makes me wonder if 
there's something interesting going on there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

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

[PATCH] D135011: Add builtin_elementwise_sin and builtin_elementwise_cos

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:473
 ---
+- Add ``__builtin_elementwise_sin`` and ``__builtin_elementwise_cos`` llvm 
builtins for floating point types only.
 

Drop "llvm" from this sentence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135011

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

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:4191
 /// or CUDA architecture.
-static StringRef getCanonicalArchString(Compilation &C,
-const llvm::opt::DerivedArgList &Args,
-StringRef ArchStr,
-const llvm::Triple &Triple) {
+static Optional
+getCanonicalArchString(Compilation &C, const llvm::opt::DerivedArgList &Args,

tra wrote:
> Nit. Optional is fine, but we could've just checked returned value for 
> Arch.empty().
Probably the better option. I originally used `Optional` for the implicit 
boolean conversion, but I don't think it'll make the code that much more 
complicated.



Comment at: clang/lib/Driver/Driver.cpp:4288
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == StringRef("all")) {
   Archs.clear();

tra wrote:
> Nit: do we need explicit `StringRef()` here? If implicit type conversion does 
> not work, we could use `Arch.equals("all")`.
> The current code is fine, but it does stick out a bit and makes me wonder if 
> there's something interesting going on there.
You're right, previously this used `Arg-getValue()` but then when it changed to 
`llvm::split` it got converted to a `StringRef` so this can be updated now. 
Thanks for pointing it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

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

[clang-tools-extra] 2eaf6f9 - [AST] Preserve more structure in UsingEnumDecl node.

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2022-10-12T19:54:51+02:00
New Revision: 2eaf6f973cacd7f24691b72a9c6a3ee2a3d1a60b

URL: 
https://github.com/llvm/llvm-project/commit/2eaf6f973cacd7f24691b72a9c6a3ee2a3d1a60b
DIFF: 
https://github.com/llvm/llvm-project/commit/2eaf6f973cacd7f24691b72a9c6a3ee2a3d1a60b.diff

LOG: [AST] Preserve more structure in UsingEnumDecl node.

- store NestedNameSpecifier & Loc for the qualifiers
  This information was entirely missing from the AST.
- expose the location information for qualifier/identifier/typedefs as typeloc
  This allows many traversals/astmatchers etc to handle these generically along
  with other references. The decl vs type split can help preserve typedef
  sugar when https://github.com/llvm/llvm-project/issues/57659 is resolved.
- fix the SourceRange of UsingEnumDecl to include 'using'.

Fixes https://github.com/clangd/clangd/issues/1283

Differential Revision: https://reviews.llvm.org/D134303

Added: 


Modified: 
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
clang/include/clang/AST/DeclCXX.h
clang/include/clang/AST/RecursiveASTVisitor.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/DeclCXX.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/test/AST/ast-dump-using-enum.cpp
clang/unittests/Tooling/RecursiveASTVisitorTestTypeLocVisitor.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/FindTarget.cpp 
b/clang-tools-extra/clangd/FindTarget.cpp
index dea5cb7f50288..f76bedee03515 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -629,6 +629,12 @@ llvm::SmallVector refInDecl(const Decl *D,
DeclRelation::Underlying, Resolver)});
 }
 
+void VisitUsingEnumDecl(const UsingEnumDecl *D) {
+  // "using enum ns::E" is a non-declaration reference.
+  // The reference is covered by the embedded typeloc.
+  // Don't use the default VisitNamedDecl, which would report a 
declaration.
+}
+
 void VisitNamespaceAliasDecl(const NamespaceAliasDecl *D) {
   // For namespace alias, "namespace Foo = Target;", we add two references.
   // Add a declaration reference for Foo.

diff  --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp 
b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index fd25e8059e816..57838eafd6512 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -1267,6 +1267,15 @@ TEST_F(FindExplicitReferencesTest, All) {
 )cpp",
 "0: targets = {ns}\n"
 "1: targets = {ns::global}, qualifier = 'ns::'\n"},
+   // Using enum declarations.
+   {R"cpp(
+  namespace ns { enum class A {}; }
+  void foo() {
+using enum $0^ns::$1^A;
+  }
+)cpp",
+"0: targets = {ns}\n"
+"1: targets = {ns::A}, qualifier = 'ns::'\n"},
// Simple types.
{R"cpp(
  struct Struct { int a; };

diff  --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp 
b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
index 239566a35861e..bd3df2404fcf6 100644
--- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -531,6 +531,33 @@ TEST(SelectionTest, CommonAncestor) {
 void func() { [[__^func__]]; }
 )cpp",
"PredefinedExpr"},
+
+  // using enum
+  {R"cpp(
+namespace ns { enum class A {}; };
+using enum ns::[[^A]];
+)cpp",
+   "EnumTypeLoc"},
+  {R"cpp(
+namespace ns { enum class A {}; using B = A; };
+using enum ns::[[^B]];
+)cpp",
+   "TypedefTypeLoc"},
+  {R"cpp(
+namespace ns { enum class A {}; };
+using enum [[^ns::]]A;
+)cpp",
+   "NestedNameSpecifierLoc"},
+  {R"cpp(
+namespace ns { enum class A {}; };
+[[using ^enum ns::A]];
+)cpp",
+   "UsingEnumDecl"},
+  {R"cpp(
+namespace ns { enum class A {}; };
+[[^using enum ns::A]];
+)cpp",
+   "UsingEnumDecl"},
   };
 
   for (const Case &C : Cases) {
@@ -541,6 +568,7 @@ TEST(SelectionTest, CommonAncestor) {
 TU.Code = std::string(Test.code());
 
 TU.ExtraArgs.push_back("-xobjective-c++");
+TU.ExtraArgs.push_back("-std=c++20");
 
 auto AST = TU.build();
 auto T = makeSelectionTree(C.Code, AST);

diff  --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp 
b/clang-tools-extra/clangd/unittests/Sem

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked 5 inline comments as done.
Closed by commit rG2eaf6f973cac: [AST] Preserve more structure in UsingEnumDecl 
node. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D134303?vs=464118&id=467197#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134303

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/AST/ast-dump-using-enum.cpp
  clang/unittests/Tooling/RecursiveASTVisitorTestTypeLocVisitor.cpp

Index: clang/unittests/Tooling/RecursiveASTVisitorTestTypeLocVisitor.cpp
===
--- clang/unittests/Tooling/RecursiveASTVisitorTestTypeLocVisitor.cpp
+++ clang/unittests/Tooling/RecursiveASTVisitorTestTypeLocVisitor.cpp
@@ -88,4 +88,12 @@
   TypeLocVisitor::Lang_C));
 }
 
+TEST(RecursiveASTVisitor, VisitsUsingEnumType) {
+  TypeLocVisitor Visitor;
+  Visitor.ExpectMatch("::A", 2, 12);
+  EXPECT_TRUE(Visitor.runOver("enum class A {}; \n"
+  "using enum ::A;\n",
+  TypeLocVisitor::Lang_CXX2a));
+}
+
 } // end anonymous namespace
Index: clang/test/AST/ast-dump-using-enum.cpp
===
--- clang/test/AST/ast-dump-using-enum.cpp
+++ clang/test/AST/ast-dump-using-enum.cpp
@@ -21,7 +21,7 @@
 // CHECK-NEXT: `-EnumConstantDecl {{.*}} Foo_b 'Bob::Foo'
 
 // CHECK-LABEL: Dumping Foo:
-// CHECK-NEXT: UsingEnumDecl {{.*}} Enum {{.*}} 'Foo'
+// CHECK-NEXT: UsingEnumDecl {{.*}} <{{.*}}:16:1, col:17> {{.*}} Enum {{.*}} 'Foo'
 
 // CHECK-LABEL: Dumping Foo_a:
 // CHECK-NEXT: UsingShadowDecl {{.*}} implicit EnumConstant {{.*}} 'Foo_a' 'Bob::Foo'
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1294,7 +1294,7 @@
   VisitNamedDecl(D);
   Record.AddSourceLocation(D->getUsingLoc());
   Record.AddSourceLocation(D->getEnumLoc());
-  Record.AddDeclRef(D->getEnumDecl());
+  Record.AddTypeSourceInfo(D->getEnumType());
   Record.AddDeclRef(D->FirstUsingShadow.getPointer());
   Record.AddDeclRef(Context.getInstantiatedFromUsingEnumDecl(D));
   Code = serialization::DECL_USING_ENUM;
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1773,7 +1773,7 @@
   VisitNamedDecl(D);
   D->setUsingLoc(readSourceLocation());
   D->setEnumLoc(readSourceLocation());
-  D->Enum = readDeclAs();
+  D->setEnumType(Record.readTypeSourceInfo());
   D->FirstUsingShadow.setPointer(readDeclAs());
   if (auto *Pattern = readDeclAs())
 Reader.getContext().setInstantiatedFromUsingEnumDecl(D, Pattern);
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3294,9 +3294,11 @@
   if (SemaRef.RequireCompleteEnumDecl(EnumD, EnumD->getLocation()))
 return nullptr;
 
+  TypeSourceInfo *TSI = SemaRef.SubstType(D->getEnumType(), TemplateArgs,
+  D->getLocation(), D->getDeclName());
   UsingEnumDecl *NewUD =
   UsingEnumDecl::Create(SemaRef.Context, Owner, D->getUsingLoc(),
-D->getEnumLoc(), D->getLocation(), EnumD);
+D->getEnumLoc(), D->getLocation(), TSI);
 
   SemaRef.Context.setInstantiatedFromUsingEnumDecl(NewUD, D);
   NewUD->setAccess(D->getAccess());
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -11871,8 +11871,14 @@
   SourceLocation IdentLoc,
   IdentifierInfo &II, CXXScopeSpec *SS) {
   assert(!SS->isInvalid() && "ScopeSpec is invalid");
-  ParsedType TypeRep = getTypeName(II, IdentLoc, S, SS);
-  if (!TypeRep) {
+  TypeSourceInfo *TSI = nullptr;
+  QualType EnumTy = GetTypeFromParser(
+  getTypeName(II, Ident

[PATCH] D135796: [HIP] Detect HIP for Debian/Fedora

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:309
 
+  ROCmSearchDirs.emplace_back(D.SysRoot + "/usr/local",
+  /*StrictChecking=*/true);

Should it be done for Debian/Fedora only? See 
clang/include/clang/Driver/Distro.h for `IsDebian/IsRedhat()` helpers.


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

https://reviews.llvm.org/D135796

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

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:327
+///   if the argument is smaller than a slot, set this flag will force
+///   right-adjust the argument in its slot irrespective of the type.
 static Address emitVoidPtrDirectVAArg(CodeGenFunction &CGF,

tingwang wrote:
> rjmccall wrote:
> > Argh, Phabricator dropped one of my comments, and it's the one that 
> > explains why I CC'ed Tim Northover.
> > 
> > I'm a little worried about the existing uses of this function because this 
> > function is sensitive to the type produced by `ConvertTypeForMem`.  
> > `ConvertTypeForMem` *mostly* only generates IR struct types for C structs 
> > and unions, but there are a few places where it generates an IR struct for 
> > some fundamental type that stores multiple values.  Most of those types are 
> > at least as large as an argument slot (e.g. they contain pointers), unless 
> > there's some weird target with huge slots.  However, some of them are not; 
> > I think the most important example is `_Complex T`, which of course gets 
> > translated into a struct containing two `T`s.  So if `T` is smaller than 
> > half an argument slot, we're not going to right-align `_Complex T` on 
> > big-endian targets other than PPC64, and I don't know if that's right.
> > 
> > That would affect `_Complex _Float16` on 64-bit targets; on 32-bit targets, 
> > I think you'd need something obscure like `_Complex char` to exercise it.
> > 
> > Now, if Clang generates arguments for one of these types using a single 
> > value that's also of IR struct type, and the backend considers that when 
> > deciding whether to right-align arguments, then maybe those two decisions 
> > cancel out and we've at least got call/va_arg compatibility, even if it's 
> > not necessarily what's formally specified by the appropriate psABI.  But 
> > `DirectTy` is definitely not necessarily the type that call-argument 
> > lowering will use, so I'm a little worried.
> Thank you!
> 
> I checked the `_Complex char` case on PPC64: complex element size smaller 
> than argument slot is handled by `complexTempStructure()` 
> (https://github.com/llvm/llvm-project/blob/51d33afcbe0a81bb8508d5685f38dc9fdb2b60c9/clang/lib/CodeGen/TargetInfo.cpp#L5451),
>  and the right-adjustment is taken care by that logic. Both AIX and PPC64 use 
> `complexTempStructure()` to produce variadic callee arguments in this case.
> 
> In case `_Complex char` is encapsulated inside structure, then the whole is 
> considered as an aggregate, and is addressed by this fix. I will add a test 
> case to illustrate.
> 
> Hope these addressed your concern.
> 
This shouldn't be a problem for PPC64 because the ABI specifically makes this 
decision independent of what kind of type it is.  I'm thinking it might be a 
problem for existing big-endian targets, though, especially if any 64-bit 
big-endian targets support `_Float16`, because `_Complex _Float16` is a 
standard type that we need to make sure follows the official psABI.  `_Complex 
char` is a Clang extension, so as long as the frontend and backend agree, I'm 
not particularly worried about psABI compatibility.

Anyway, since this doesn't affect PPC64, it doesn't need to hold up your patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D18

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

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:3618
+  /// The source location of the 'enum' keyword.
+  SourceLocation EnumLoc;
+  /// 'qual::SomeEnum' as an EnumType, possibly with Elaborated/Typedef sugar.

hokein wrote:
> nit: we only rename  `EnumLocation` but not the `UsingLocation`, it is a 
> little wired, I think either we rename both or keep both unchanged.
> 
oops, at some point this was matching the use of a `TypeLoc` vs 
`SourceLocation`, but no longer does. Fixed



Comment at: clang/include/clang/AST/DeclCXX.h:3623
   /// The enum
   EnumDecl *Enum;
 

hokein wrote:
> I think we can probably get rid of the `Enum` field, since we are storing the 
> EnumType and we can get the EnumDecl by 
> `dyn_cast(EnumType->getType()->getAsTagDecl())`
Good point. Did this an added an assert in create().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134303

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

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 467199.
jhuber6 added a comment.

Making suggested changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/cuda-phases.cu


Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, 
"device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4208,7 +4208,7 @@
   }
 
   if (IsNVIDIAGpuArch(Arch))
-return Args.MakeArgStringRef(CudaArchToString(Arch));
+return StringRef(Args.MakeArgStringRef(CudaArchToString(Arch)));
 
   if (IsAMDGpuArch(Arch)) {
 llvm::StringMap Features;
@@ -4221,7 +4221,8 @@
   C.setContainsError();
   return StringRef();
 }
-return Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features));
+return StringRef(
+Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features)));
   }
 
   // If the input isn't CUDA or HIP just return the architecture.
@@ -4273,15 +4274,29 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+StringRef ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple());
+if (!ArchStr.empty())
+  Archs.insert(ArchStr);
+else
+  return Archs;
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == "all") {
   Archs.clear();
-else
-  Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+} else {
+  StringRef ArchStr =
+  getCanonicalArchString(C, Args, Arch, TC->getTriple());
+  if (!ArchStr.empty())
+Archs.erase(ArchStr);
+  else
+return Archs;
+}
   }
 }
   }


Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, "device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4208,7 +4208,7 @@
   }
 
   if (IsNVIDIAGpuArch(Arch))
-return Args.MakeArgStringRef(CudaArchToString(Arch));
+return StringRef(Args.MakeArgStringRef(CudaArchToString(Arch)));
 
   if (IsAMDGpuArch(Arch)) {
 llvm::Str

[PATCH] D135724: [HIP] Fix unbundling archive

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.

LGTM




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1839
   auto Ext = IsMSVC ? ".lib" : ".a";
-  if (!Lib.startswith(":") && llvm::sys::fs::exists(Lib)) {
-ArchiveOfBundles = Lib;
-FoundAOB = true;
+  if (!Lib.startswith(":") && !Lib.startswith("-l")) {
+if (llvm::sys::fs::exists(Lib)) {

yaxunl wrote:
> tra wrote:
> > I'm puzzled a bit. What is expected to be in 'Lib' ?
> > If it contains the the whole `-llibrary` option passed by the user, then 
> > why do we check for `:` prefix?
> > If it contains whatever was passed via `-l`, but without `-l` 
> > itself, then why do we check for  `-l` prefix?
> > 
> This function is also used for unbundling archive files specified as input 
> files in the clang command line. There are three cases:
> 
> For option `-l:xxx`, Lib contains `:xxx`.
> 
> For option `-lxxx`, Lib contains `-lxxx`.
> 
> For `xxx.a` passed as input file, Lib contains `xxx.a`.
Interesting. It may be worth adding a comment, because this convention is not 
at all obvious.


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

https://reviews.llvm.org/D135724

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

[PATCH] D135796: [HIP] Detect HIP for Debian/Fedora

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:309
 
+  ROCmSearchDirs.emplace_back(D.SysRoot + "/usr/local",
+  /*StrictChecking=*/true);

tra wrote:
> Should it be done for Debian/Fedora only? See 
> clang/include/clang/Driver/Distro.h for `IsDebian/IsRedhat()` helpers.
I think other Linux distributions may adopt it too, since /usr is the standard 
location.


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

https://reviews.llvm.org/D135796

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

[PATCH] D135011: Add builtin_elementwise_sin and builtin_elementwise_cos

[Joshua Batista via Phabricator via cfe-commits]

bob80905 updated this revision to Diff 467201.
bob80905 added a comment.

- drop llvm from release notes description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135011

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-elementwise-math.c
  clang/test/Sema/aarch64-sve-vector-trig-ops.c
  clang/test/Sema/builtins-elementwise-math.c
  clang/test/Sema/riscv-sve-vector-trig-ops.c
  clang/test/SemaCXX/builtins-elementwise-math.cpp

Index: clang/test/SemaCXX/builtins-elementwise-math.cpp
===
--- clang/test/SemaCXX/builtins-elementwise-math.cpp
+++ clang/test/SemaCXX/builtins-elementwise-math.cpp
@@ -59,3 +59,17 @@
   static_assert(!is_const::value);
   static_assert(!is_const::value);
 }
+
+void test_builtin_elementwise_cos() {
+  const float a = 42.0;
+  float b = 42.3;
+  static_assert(!is_const::value);
+  static_assert(!is_const::value);
+}
+
+void test_builtin_elementwise_sin() {
+  const float a = 42.0;
+  float b = 42.3;
+  static_assert(!is_const::value);
+  static_assert(!is_const::value);
+}
Index: clang/test/Sema/riscv-sve-vector-trig-ops.c
===
--- /dev/null
+++ clang/test/Sema/riscv-sve-vector-trig-ops.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d \
+// RUN:   -target-feature +v -target-feature +zfh -target-feature +experimental-zvfh \
+// RUN:   -disable-O0-optnone -o - -fsyntax-only %s -verify 
+
+#include 
+
+
+vfloat32mf2_t test_sin_vv_i8mf8(vfloat32mf2_t v) {
+
+  return __builtin_elementwise_sin(v);
+  // expected-error@-1 {{1st argument must be a vector, integer or floating point type}}
+}
+
+vfloat32mf2_t test_cos_vv_i8mf8(vfloat32mf2_t v) {
+
+  return __builtin_elementwise_cos(v);
+  // expected-error@-1 {{1st argument must be a vector, integer or floating point type}}
+}
Index: clang/test/Sema/builtins-elementwise-math.c
===
--- clang/test/Sema/builtins-elementwise-math.c
+++ clang/test/Sema/builtins-elementwise-math.c
@@ -280,6 +280,27 @@
   // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned4' (vector of 4 'unsigned int' values))}}
 }
 
+void test_builtin_elementwise_cos(int i, float f, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
+
+  struct Foo s = __builtin_elementwise_cos(f);
+  // expected-error@-1 {{initializing 'struct Foo' with an expression of incompatible type 'float'}}
+
+  i = __builtin_elementwise_cos();
+  // expected-error@-1 {{too few arguments to function call, expected 1, have 0}}
+
+  i = __builtin_elementwise_cos(i);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
+
+  i = __builtin_elementwise_cos(f, f);
+  // expected-error@-1 {{too many arguments to function call, expected 1, have 2}}
+
+  u = __builtin_elementwise_cos(u);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned int')}}
+
+  uv = __builtin_elementwise_cos(uv);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned4' (vector of 4 'unsigned int' values))}}
+}
+
 void test_builtin_elementwise_floor(int i, float f, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
 
   struct Foo s = __builtin_elementwise_floor(f);
@@ -322,6 +343,27 @@
   // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned4' (vector of 4 'unsigned int' values))}}
 }
 
+void test_builtin_elementwise_sin(int i, float f, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
+
+  struct Foo s = __builtin_elementwise_sin(f);
+  // expected-error@-1 {{initializing 'struct Foo' with an expression of incompatible type 'float'}}
+
+  i = __builtin_elementwise_sin();
+  // expected-error@-1 {{too few arguments to function call, expected 1, have 0}}
+
+  i = __builtin_elementwise_sin(i);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'int')}}
+
+  i = __builtin_elementwise_sin(f, f);
+  // expected-error@-1 {{too many arguments to function call, expected 1, have 2}}
+
+  u = __builtin_elementwise_sin(u);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned int')}}
+
+  uv = __builtin_elementwise_sin(uv);
+  // expected-error@-1 {{1st argument must be a floating point type (was 'unsigned4' (vector of 4 'unsigned int' values))}}
+}
+
 void test_builtin_elementwise_trunc(int i, float f, double d, float4 v, int3 iv, unsigned u, unsigned4 uv) {
 
   struct Foo s = __builtin_elementwise_trunc(f);
Index: clang/test/Sema/aarch64-sve-vector-trig-ops.c
===
--- /dev/null
+++ clang/test/Se

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D135326#3851678 , @dblaikie wrote:

> In D135326#3851672 , @dblaikie 
> wrote:
>
>> (abandoned this, but still kind of curious)
>>
>> @rjmccall - any background/history of having the CXXABI distinct from the 
>> OS? I guess the point might've been that the C ABI is part of/the definition 
>> of the OS, but maybe the CXX ABI is more "floating"/flexible on top of that? 
>> Though it means these CXX ABIs don't get the benefit of being grouped with 
>> the rest of the targetOS - instead they're a bunch of switches, which seems 
>> a bit unfortunate, but I guess there's not lots of properties here, so maybe 
>> that's OK.
>
> I guess the other question: Is this flexibility valuable/worthwhile, or could 
> we fold the CXXABI back into the TargetOS?

I don't think we really want to support targeting an OS while independently 
changing the target C++ ABI, no.  But I think "OS implies C++ ABI implies ABI 
properties" is generally the way people define platforms, and I wouldn't want 
to create a situation where targets have to manually implement all of the right 
C++ ABI properties just to get default behavior for a specific C++ ABI or else 
end up weirdly divergent.  If you have a different way to achieve that goal 
besides composition, that's worth considering.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135326

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

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:4211
   if (IsNVIDIAGpuArch(Arch))
-return Args.MakeArgStringRef(CudaArchToString(Arch));
+return StringRef(Args.MakeArgStringRef(CudaArchToString(Arch)));
 

This can be undone now, too.



Comment at: clang/lib/Driver/Driver.cpp:4224
 }
-return Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features));
+return StringRef(
+Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features)));

ditto.



Comment at: clang/lib/Driver/Driver.cpp:4295-4298
+  if (!ArchStr.empty())
+Archs.erase(ArchStr);
+  else
+return Archs;

Nit: we could save one line here:
``` 
if(ArchStr.empty()) 
  return Archs;
Archs.erase(ArchStr);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

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

[clang] a6ebd30 - [modules] Allow to validate system headers less often with `-fmodules-validate-once-per-build-session`.

[Volodymyr Sapsai via cfe-commits]

Author: Volodymyr Sapsai
Date: 2022-10-12T11:10:08-07:00
New Revision: a6ebd3083dbf8aadae58f6f2a2f1071976649d56

URL: 
https://github.com/llvm/llvm-project/commit/a6ebd3083dbf8aadae58f6f2a2f1071976649d56
DIFF: 
https://github.com/llvm/llvm-project/commit/a6ebd3083dbf8aadae58f6f2a2f1071976649d56.diff

LOG: [modules] Allow to validate system headers less often with 
`-fmodules-validate-once-per-build-session`.

Make flags `-fmodules-validate-system-headers` and
`-fmodules-validate-once-per-build-session` orthogonal, so they have
their own independent responsibilities - if system headers should be
validated and how often.

rdar://8799

Differential Revision: https://reviews.llvm.org/D135232

Added: 


Modified: 
clang/lib/Serialization/ASTReader.cpp
clang/test/Modules/fmodules-validate-once-per-build-session.c
clang/test/Modules/validate-system-headers.m

Removed: 




diff  --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index 45a6a474f1d93..e3e1d9eb36ae1 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2639,12 +2639,11 @@ ASTReader::ReadControlBlock(ModuleFile &F,
 // so we verify all input files.  Otherwise, verify only user input
 // files.
 
-unsigned N = NumUserInputs;
-if (ValidateSystemInputs ||
-(HSOpts.ModulesValidateOncePerBuildSession &&
- F.InputFilesValidationTimestamp <= HSOpts.BuildSessionTimestamp &&
- F.Kind == MK_ImplicitModule))
-  N = NumInputs;
+unsigned N = ValidateSystemInputs ? NumInputs : NumUserInputs;
+if (HSOpts.ModulesValidateOncePerBuildSession &&
+F.InputFilesValidationTimestamp > HSOpts.BuildSessionTimestamp &&
+F.Kind == MK_ImplicitModule)
+  N = NumUserInputs;
 
 for (unsigned I = 0; I < N; ++I) {
   InputFile IF = getInputFile(F, I+1, Complain);

diff  --git a/clang/test/Modules/fmodules-validate-once-per-build-session.c 
b/clang/test/Modules/fmodules-validate-once-per-build-session.c
index 0c97342a8d70a..7ab6f78c194fd 100644
--- a/clang/test/Modules/fmodules-validate-once-per-build-session.c
+++ b/clang/test/Modules/fmodules-validate-once-per-build-session.c
@@ -17,8 +17,8 @@
 
 // ===
 // Compile the module.
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
 // RUN: ls -R %t/modules-cache | grep Foo.pcm.timestamp
 // RUN: ls -R %t/modules-cache | grep Bar.pcm.timestamp
 // RUN: ls -R %t/modules-cache-user | grep Foo.pcm.timestamp
@@ -30,8 +30,8 @@
 
 // ===
 // Use it, and make sure that we did not recompile it.
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
 // RUN: ls -R %t/modules-cache | grep Foo.pcm.timestamp
 // RUN: ls -R %t/modules-cache | grep Bar.pcm.timestamp
 // RUN: ls -R %t/modules-cache-user | grep Foo.pcm.timestamp
@@ -54,8 +54,8 @@
 // ===
 // Use the module, and make sure that we did not recompile it if foo.h or
 // module.map are system fil

[PATCH] D135232: [modules] Allow to validate system headers less often with `-fmodules-validate-once-per-build-session`.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa6ebd3083dbf: [modules] Allow to validate system headers 
less often with `-fmodules-validate… (authored by vsapsai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135232

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Modules/fmodules-validate-once-per-build-session.c
  clang/test/Modules/validate-system-headers.m


Index: clang/test/Modules/validate-system-headers.m
===
--- clang/test/Modules/validate-system-headers.m
+++ clang/test/Modules/validate-system-headers.m
@@ -34,8 +34,8 @@
 // RUN: %clang_cc1 -isystem %t/Inputs/usr/include -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/ModuleCache 
-fdisable-module-hash -x objective-c-header -fsyntax-only %s 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session
 // RUN: diff %t/ModuleCache/Foo.pcm %t/Foo.pcm.saved
 
-// Now add -fmodules-validate-system-headers and rebuild
+// Now add -fmodules-validate-system-headers and rebuild. No recompilation due 
to -fmodules-validate-once-per-build-session
 // RUN: %clang_cc1 -isystem %t/Inputs/usr/include -fmodules 
-fimplicit-module-maps -fmodules-validate-system-headers 
-fmodules-cache-path=%t/ModuleCache -fdisable-module-hash -x objective-c-header 
-fsyntax-only %s -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session
-// RUN: not diff %t/ModuleCache/Foo.pcm %t/Foo.pcm.saved
+// RUN: diff %t/ModuleCache/Foo.pcm %t/Foo.pcm.saved
 
 @import Foo;
Index: clang/test/Modules/fmodules-validate-once-per-build-session.c
===
--- clang/test/Modules/fmodules-validate-once-per-build-session.c
+++ clang/test/Modules/fmodules-validate-once-per-build-session.c
@@ -17,8 +17,8 @@
 
 // ===
 // Compile the module.
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
 // RUN: ls -R %t/modules-cache | grep Foo.pcm.timestamp
 // RUN: ls -R %t/modules-cache | grep Bar.pcm.timestamp
 // RUN: ls -R %t/modules-cache-user | grep Foo.pcm.timestamp
@@ -30,8 +30,8 @@
 
 // ===
 // Use it, and make sure that we did not recompile it.
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
+// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %t/Inputs 
-fmodules-validate-system-headers -fbuild-session-timestamp=139000 
-fmodules-validate-once-per-build-session %s
 // RUN: ls -R %t/modules-cache | grep Foo.pcm.timestamp
 // RUN: ls -R %t/modules-cache | grep Bar.pcm.timestamp
 // RUN: ls -R %t/modules-cache-user | grep Foo.pcm.timestamp
@@ -54,8 +54,8 @@
 // ===
 // Use the module, and make sure that we did not recompile it if foo.h or
 // module.map are system files, even though the sources changed.
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache -fsyntax-only -isystem %t/Inputs 
-fbuild-session-timestamp=139000 -fmodules-validate-once-per-build-session 
%s
-// RUN: %clang_cc1 -cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash 
-fmodules-cache-path=%t/modules-cache-user -fsyntax-only -I %

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 467204.
jhuber6 added a comment.

Making suggested changes, Early exists is part of the LLVM style so it's 
definitely better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/cuda-phases.cu


Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, 
"device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4273,15 +4273,27 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+StringRef ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple());
+if (ArchStr.empty())
+  return Archs;
+Archs.insert(ArchStr);
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == "all") {
   Archs.clear();
-else
-  Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+} else {
+  StringRef ArchStr =
+  getCanonicalArchString(C, Args, Arch, TC->getTriple());
+  if (ArchStr.empty())
+return Archs;
+  Archs.erase(ArchStr);
+}
   }
 }
   }


Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, "device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4273,15 +4273,27 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+StringRef ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple());
+if (ArchStr.empty())
+  return Archs;
+Archs.insert(ArchStr);
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::spl

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D135326#3853324 , @rjmccall wrote:

> In D135326#3851678 , @dblaikie 
> wrote:
>
>> In D135326#3851672 , @dblaikie 
>> wrote:
>>
>>> (abandoned this, but still kind of curious)
>>>
>>> @rjmccall - any background/history of having the CXXABI distinct from the 
>>> OS? I guess the point might've been that the C ABI is part of/the 
>>> definition of the OS, but maybe the CXX ABI is more "floating"/flexible on 
>>> top of that? Though it means these CXX ABIs don't get the benefit of being 
>>> grouped with the rest of the targetOS - instead they're a bunch of 
>>> switches, which seems a bit unfortunate, but I guess there's not lots of 
>>> properties here, so maybe that's OK.
>>
>> I guess the other question: Is this flexibility valuable/worthwhile, or 
>> could we fold the CXXABI back into the TargetOS?
>
> I don't think we really want to support targeting an OS while independently 
> changing the target C++ ABI, no.  But I think "OS implies C++ ABI implies ABI 
> properties" is generally the way people define platforms, and I wouldn't want 
> to create a situation where targets have to manually implement all of the 
> right C++ ABI properties just to get default behavior for a specific C++ ABI 
> or else end up weirdly divergent.  If you have a different way to achieve 
> that goal besides composition, that's worth considering.

Thanks for the reply!

Ah, I think I see what you mean.

So we could remove the `-fc++-abi` flag? (well, it's a driver flag... - 
deprecate it? make it a no-op or an error if you ask for an abi that isn't the 
one the OS already implies?)

& then maybe ABI could be a mixin the same way TargetOS works? (TargetOS could 
mixin the C++ ABI, then TargetInfo mixes in TargetOS)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135326

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

[PATCH] D135796: [HIP] Detect HIP for Debian/Fedora

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:309
 
+  ROCmSearchDirs.emplace_back(D.SysRoot + "/usr/local",
+  /*StrictChecking=*/true);

yaxunl wrote:
> tra wrote:
> > Should it be done for Debian/Fedora only? See 
> > clang/include/clang/Driver/Distro.h for `IsDebian/IsRedhat()` helpers.
> I think other Linux distributions may adopt it too, since /usr is the 
> standard location.
Or they may do something completely different. I've been surprised in the past 
by how NVIDIA packages get split in different ways on different distributions.

I'm fine with using established locations where we do have them, but I'm 
reluctant to add something open-ended that we do not need and which may be hard 
to change later. Expanding the scope of the search if/when we need it somewhere 
is easy, removing things that users may have grown to depend on is often hard.


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

https://reviews.llvm.org/D135796

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think the disagreement here highlights the need to have a serious discussion 
about the future of error handling across the LLVM project. As you say, it 
sounds like you're not going to reach agreement on this code review, so maybe 
the best short term next step is to land the uncontroversial changes that Aaron 
agrees with.

Regarding error handling and the wide usage of assertions to guard UB across 
LLVM, we need to decide what our goals are as a community. Is it actually the 
goal of the project that Clang and LLVM that no input can lead to UB? If we 
can't guarantee that, is there some error budget we consider acceptable (fuzzer 
runs for 24hrs and can't find bugs)? How does that goal rate against our other 
goals, like performance? We could just ship with assertions enabled, sacrifice 
20% code size and performance, and call it a day. We used to do that for 
Chromium, but users complained that compiles were too slow and we stopped doing 
it.

I think the status quo has real problems. We pretend that we can do both of 
these:

- Assert liberally, with the understanding that assertion failures lead to UB 
(failed bad cast check, bounds checks, unreachable code, etc)
- We can actually find and fix all cases that violate those inputs to the point 
that clang is stable and secure enough for our satisfaction

Currently, it is really easy to run fuzzers and find crash bugs in clang. I 
think the lesson we should take from that is that we are compromising goal 2 
here, and we shouldn't kid ourselves about it.

Maybe the goal is not security, but is instead something about user or 
developer experience, but we should go through some higher level process to 
clarify that goal so we can write it down and agree on it.




Comment at: clang/lib/Analysis/CFG.cpp:1047
+  llvm_unreachable("Unexpected unary operator!");
   return llvm::None;
 }

This will create unreachable code warnings, which must be addressed before 
landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135801: [clang][Lexer] Speedup HeaderSearch when there are many HeaderMaps

[Troy Johnson via Phabricator via cfe-commits]

troyj created this revision.
troyj added a reviewer: bruno.
Herald added a project: All.
troyj requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

HeaderSearch already uses a caching system to avoid duplicate searches, but the 
initial cold miss can take a long time if a build system has supplied thousands 
of HeaderMaps. For this case, the SearchDirs vector begins with those 
HeaderMaps, so a cache miss requires testing if the sought filename is present 
in each of those maps. Instead, we can consolidate the keys of those HeaderMaps 
into one StringMap and then each cache miss can skip directly to the correct 
HeaderMap or continue its search beyond the initial sequence of HeaderMaps. In 
testing on TUs with ~15000 SearchDirs where the initial 99% are HeaderMaps, 
time spent in Clang was reduced by 15%. This patch is expected to be neutral 
for SearchDir vectors that do not begin with HeaderMaps.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135801

Files:
  clang/include/clang/Lex/HeaderMap.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp

Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -116,6 +116,7 @@
   NoCurDirSearch = noCurDirSearch;
   SearchDirToHSEntry = std::move(searchDirToHSEntry);
   //LookupFileCache.clear();
+  indexInitialHeaderMaps();
 }
 
 void HeaderSearch::AddSearchPath(const DirectoryLookup &dir, bool isAngled) {
@@ -372,6 +373,29 @@
   return Module;
 }
 
+void HeaderSearch::indexInitialHeaderMaps() {
+  llvm::StringMap Index(SearchDirs.size());
+
+  // Iterate over all filename keys and associate them with the index i.
+  unsigned i = 0;
+  for (; i != SearchDirs.size(); ++i) {
+auto &Dir = SearchDirs[i];
+
+// We're concerned with only the initial contiguous run of header
+// maps within SearchDirs, which can be 99% of SearchDirs when
+// SearchDirs.size() is ~1.
+if (!Dir.isHeaderMap())
+  break;
+
+auto Callback = [&](StringRef Filename) { Index.try_emplace(Filename, i); };
+
+Dir.getHeaderMap()->forEachKey(Callback);
+  }
+
+  SearchDirHeaderMapIndex = std::move(Index);
+  FirstNonHeaderMapSearchDirIdx = i;
+}
+
 //===--===//
 // File lookup within a DirectoryLookup scope
 //===--===//
@@ -977,22 +1001,37 @@
 
   ConstSearchDirIterator NextIt = std::next(It);
 
-  // If the entry has been previously looked up, the first value will be
-  // non-zero.  If the value is equal to i (the start point of our search), then
-  // this is a matching hit.
-  if (!SkipCache && CacheLookup.StartIt == NextIt) {
-// Skip querying potentially lots of directories for this lookup.
-if (CacheLookup.HitIt)
-  It = CacheLookup.HitIt;
-if (CacheLookup.MappedName) {
-  Filename = CacheLookup.MappedName;
-  if (IsMapped)
-*IsMapped = true;
+  if (!SkipCache) {
+if (CacheLookup.StartIt == NextIt) {
+  // HIT: Skip querying potentially lots of directories for this lookup.
+  if (CacheLookup.HitIt)
+It = CacheLookup.HitIt;
+  if (CacheLookup.MappedName) {
+Filename = CacheLookup.MappedName;
+if (IsMapped)
+  *IsMapped = true;
+  }
+} else {
+  // MISS: This is the first query, or the previous query didn't match
+  // our search start.  We will fill in our found location below, so prime
+  // the start point value.
+  CacheLookup.reset(/*NewStartIt=*/NextIt);
+
+  if (It == search_dir_begin() && FirstNonHeaderMapSearchDirIdx > 0) {
+// Handle cold misses of user includes in the presence of many header
+// maps.  We avoid searching perhaps thousands of header maps by
+// jumping directly to the correct one or jumping beyond all of them.
+auto Iter = SearchDirHeaderMapIndex.find(Filename);
+if (Iter == SearchDirHeaderMapIndex.end()) {
+  // Not in index => Skip to first SearchDir after initial header maps
+  It = search_dir_nth(FirstNonHeaderMapSearchDirIdx);
+} else {
+  // In index => Start with a specific header map
+  It = search_dir_nth(Iter->second);
+}
+  }
 }
   } else {
-// Otherwise, this is the first query, or the previous query didn't match
-// our search start.  We will fill in our found location below, so prime the
-// start point value.
 CacheLookup.reset(/*NewStartIt=*/NextIt);
   }
 
Index: clang/include/clang/Lex/HeaderSearch.h
===
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -249,6 +249,14 @@
   unsigned SystemDirIdx = 0;
   bool NoCurDirSearch = false;
 
+  /// Maps Hea

[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/AST/Interp/arrays.cpp:143
+
+};

tbaeder wrote:
> cor3ntin wrote:
> > tahonermann wrote:
> > > cor3ntin wrote:
> > > > tbaeder wrote:
> > > > > cor3ntin wrote:
> > > > > > tahonermann wrote:
> > > > > > > As others already noted, additional testing of multicharacter 
> > > > > > > literals and UCNs (including named universal characters like 
> > > > > > > `\N{LATIN_CAPITAL_LETTER_E}` would be beneficial. Some tests of 
> > > > > > > character escapes like `\t` wouldn't hurt either.
> > > > > > > 
> > > > > > > Clang does not yet support use of `-fexec-charset` to set the 
> > > > > > > literal encoding (execution character set) to anything other than 
> > > > > > > UTF-8 though work on that has been done (see D93031). If such 
> > > > > > > work was completed, it would be useful to run some tests against 
> > > > > > > a non-UTF-8 encoding. Maybe next year.
> > > > > > Yes, wide **multicharacter** literals, that's was important 
> > > > > > information missing, thanks for spotting that.
> > > > > > 
> > > > > > I have mixed feeling about adding tests for escape sequences.  
> > > > > > Their replacement doesn't happen during constant evaluation.
> > > > > > We shouldn't replicate the lexing tests here.
> > > > > > 
> > > > > > but we should compare string literal with byte values. Testing a 
> > > > > > string literal against another one doesn't ensure the code units 
> > > > > > are correct if both are equally miss evaluated.
> > > > > > 
> > > > > > Also we could add explicit tests for null termination here as they 
> > > > > > are added as part of evaluation in theory - but then again that's 
> > > > > > also something clang does earlier.
> > > > > > 
> > > > > > If we want we could consider enabling the byte code interpreter on 
> > > > > > the existing lexing test files, i actually think that's the better 
> > > > > > way to deal with the escape sequences tests.
> > > > > I changed the first test that inspects all characters of a string to 
> > > > > comparing with integers instead. Do you have a suggestion for what 
> > > > > lexing tests to enable the constant interpreter in?
> > > > I think good candidates are
> > > > 
> > > > Lexer/char-escapes.c
> > > > Lexer/char-escapes-delimited.c
> > > > Lexer/char-literal.cpp
> > > Of those, only `Lexer/char-escapes.c` does much validation of literal 
> > > values. I prefer the approach Timm has already taken relative to those 
> > > tests.
> > > 
> > > It looks like we don't have an existing set of Sema tests for character 
> > > and string literals. How about we move this test under `clang/test/Sema`? 
> > > That would be the appropriate place to exercise values relative to 
> > > `-fexec-charset` support for non-UTF-8 encodings in the future. If that 
> > > sounds amenable, then I'd like the test split to exercise character and 
> > > string literals separately.
> > > 
> > > The character literal tests don't really belong in a test named 
> > > `arrays.cpp` as is.
> > > Of those, only Lexer/char-escapes.c does much validation of literal 
> > > values. I prefer the approach Timm has already taken relative to those 
> > > tests.
> > 
> > We can do both, I was not arguing against the test we have here, I'm happy 
> > with those :)
> > I'm opposed to duplicate tests for escape sequences here.  using the new 
> > interpreter on tests that already exist (in addition of the existing run 
> > commands) is pretty easy and cheap to do.
> > 
> > I don't have opinions how the new interpreter tests are organized.
> > Ideally we would have a complete set of test that is equally suitable for 
> > both constexpr engines, but maybe that's something that does not need to be 
> > done as part of this PR 
> I've added a new run line to `test/Lexer/char-escapes.c`, which works fine. 
> To summarize, the plan is to add a new test to `clang/test/Sema`? Or split 
> the char tests out from `arrays.cpp` and add some for multicharacter char 
> sequences?
I'm a bit uncertain regarding the purpose of tests in the `Interp` directory as 
opposed to tests under the `Sema*` directories. Basically, I'm unsure why we 
wouldn't want just one set of tests that exercise the semantics of the 
language, at least with regard to constant evaluation. If the interpreter is 
also intended to (eventually) interpret full programs (e.g., evaluating 
`main()`), I think such tests would be appropriate for an `Interp` directory. 
But I say that having not closely followed the development and previous reviews 
of the new interpreter.

At a minimum, I would like to see the character and string literal testing 
pulled out of `arrays.cpp`. For the rest, I'll defer to others that have been 
more involved in these reviews.


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

https://reviews.llvm.org/D135366

___
cfe-commits mailing list
cfe-commits@lists.llvm.o

[PATCH] D135060: [HLSL] Add groupshare address space.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10299
   Error<"attribute %0 can only be applied to an OpenCL kernel function">;
-def err_opencl_return_value_with_address_space : Error<
-  "return value cannot be qualified with address space">;
+def err_opencl_hlsl_return_value_with_address_space : Error<
+  "return type cannot be qualified with address space">;

I think we can drop the opencl and hlsl from the name at this point.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11710
   "the '%select{&|*|->}0' operator is unsupported in HLSL">;
-
 // Layout randomization diagnostics.

Looks like this change can be reverted out.



Comment at: clang/lib/Parse/ParseDecl.cpp:4344
+case tok::kw_groupshared:
+  ParseHLSLQualifiers(DS.getAttributes());
+  break;

It took me a while to convince myself that this was correct -- the token is 
consumed at the end of the `while` loop.



Comment at: clang/lib/Parse/ParseDecl.cpp:5841
+case tok::kw_groupshared:
+  ParseHLSLQualifiers(DS.getAttributes());
+  break;

Same thing here -- the end of the `while` loop is what consumes the token, so 
this is also correct.



Comment at: clang/lib/Parse/ParseExprCXX.cpp:1410
+  ParseHLSLQualifiers(DS.getAttributes());
+  ConsumeToken();
+}

And this consume is correct because the parse function doesn't consume any 
tokens.



Comment at: clang/test/SemaHLSL/group_shared.hlsl:57
+ // expected-error@+1 {{no matching function for call to 'tfoo'}}
+ GSF gs2 = tfoo(gs);

Some other Sema test cases that would be useful:
```
// Do we reject it on a return type in a function pointer?
groupshared void (*fp)();
// What about on parameters in a function pointer?
void (*fp2)(groupshared float);
// Trailing return types?
auto func() -> groupshared void;
// What about in a novel qualifier order?
void groupshared f();

struct S {
  // Do we reject it as a function qualifier on a member function?
  void f() groupshared;
};

// Does it impact size or alignment?
static_assert(sizeof(float) == sizeof(groupshared float));
static_assert(alignof(double) == alignof(groupshared double));

// Does it impact type identity for templates?
template 
struct S {
  static constexpr bool value = false;
};

template <>
struct S {
  static constexpr bool value = true;
};
static_assert(!S::value);
static_assert(S::value);

// Can you overload based on the qualifier?
void func(float f) {}
void func(groupshared float f) {} // Error on redefinition?
```
(Note, I'm taking guesses on some of these test cases; if I've guessed wrong, 
feel free to correct me!)

I'd also expect tests to show up in ParserHLSL testing the parsing behavior:
```
// Do we accept novel qualifier orders?
extern groupshared float f;
extern float groupshared f; // Ok, redeclaration?

// Can we parse it on a lambda? Note, be sure to add more lambda tests with 
// more complex parsing situations to make sure you're happy with the parsing
// behavior.
[]() groupshared {};

// Can we still parse attributes written on the type?
float groupshared [[]] i = 12;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135060

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 467215.
steven_wu added a comment.
Herald added a subscriber: bmahjour.
Herald added a project: clang.

Address review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,13 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
+endmacro()
+
+function(generate_llvm_protos_library LibraryName ProtoFile)
+  cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
+  generate_protos(ProtoSource ${ProtoFile})
+
+  add_llvm_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
 endfunction()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,9 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  generate_protos(ProtoSource ${ProtoFile})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're lo

[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/AST/Interp/arrays.cpp:143
+
+};

tahonermann wrote:
> tbaeder wrote:
> > cor3ntin wrote:
> > > tahonermann wrote:
> > > > cor3ntin wrote:
> > > > > tbaeder wrote:
> > > > > > cor3ntin wrote:
> > > > > > > tahonermann wrote:
> > > > > > > > As others already noted, additional testing of multicharacter 
> > > > > > > > literals and UCNs (including named universal characters like 
> > > > > > > > `\N{LATIN_CAPITAL_LETTER_E}` would be beneficial. Some tests of 
> > > > > > > > character escapes like `\t` wouldn't hurt either.
> > > > > > > > 
> > > > > > > > Clang does not yet support use of `-fexec-charset` to set the 
> > > > > > > > literal encoding (execution character set) to anything other 
> > > > > > > > than UTF-8 though work on that has been done (see D93031). If 
> > > > > > > > such work was completed, it would be useful to run some tests 
> > > > > > > > against a non-UTF-8 encoding. Maybe next year.
> > > > > > > Yes, wide **multicharacter** literals, that's was important 
> > > > > > > information missing, thanks for spotting that.
> > > > > > > 
> > > > > > > I have mixed feeling about adding tests for escape sequences.  
> > > > > > > Their replacement doesn't happen during constant evaluation.
> > > > > > > We shouldn't replicate the lexing tests here.
> > > > > > > 
> > > > > > > but we should compare string literal with byte values. Testing a 
> > > > > > > string literal against another one doesn't ensure the code units 
> > > > > > > are correct if both are equally miss evaluated.
> > > > > > > 
> > > > > > > Also we could add explicit tests for null termination here as 
> > > > > > > they are added as part of evaluation in theory - but then again 
> > > > > > > that's also something clang does earlier.
> > > > > > > 
> > > > > > > If we want we could consider enabling the byte code interpreter 
> > > > > > > on the existing lexing test files, i actually think that's the 
> > > > > > > better way to deal with the escape sequences tests.
> > > > > > I changed the first test that inspects all characters of a string 
> > > > > > to comparing with integers instead. Do you have a suggestion for 
> > > > > > what lexing tests to enable the constant interpreter in?
> > > > > I think good candidates are
> > > > > 
> > > > > Lexer/char-escapes.c
> > > > > Lexer/char-escapes-delimited.c
> > > > > Lexer/char-literal.cpp
> > > > Of those, only `Lexer/char-escapes.c` does much validation of literal 
> > > > values. I prefer the approach Timm has already taken relative to those 
> > > > tests.
> > > > 
> > > > It looks like we don't have an existing set of Sema tests for character 
> > > > and string literals. How about we move this test under 
> > > > `clang/test/Sema`? That would be the appropriate place to exercise 
> > > > values relative to `-fexec-charset` support for non-UTF-8 encodings in 
> > > > the future. If that sounds amenable, then I'd like the test split to 
> > > > exercise character and string literals separately.
> > > > 
> > > > The character literal tests don't really belong in a test named 
> > > > `arrays.cpp` as is.
> > > > Of those, only Lexer/char-escapes.c does much validation of literal 
> > > > values. I prefer the approach Timm has already taken relative to those 
> > > > tests.
> > > 
> > > We can do both, I was not arguing against the test we have here, I'm 
> > > happy with those :)
> > > I'm opposed to duplicate tests for escape sequences here.  using the new 
> > > interpreter on tests that already exist (in addition of the existing run 
> > > commands) is pretty easy and cheap to do.
> > > 
> > > I don't have opinions how the new interpreter tests are organized.
> > > Ideally we would have a complete set of test that is equally suitable for 
> > > both constexpr engines, but maybe that's something that does not need to 
> > > be done as part of this PR 
> > I've added a new run line to `test/Lexer/char-escapes.c`, which works fine. 
> > To summarize, the plan is to add a new test to `clang/test/Sema`? Or split 
> > the char tests out from `arrays.cpp` and add some for multicharacter char 
> > sequences?
> I'm a bit uncertain regarding the purpose of tests in the `Interp` directory 
> as opposed to tests under the `Sema*` directories. Basically, I'm unsure why 
> we wouldn't want just one set of tests that exercise the semantics of the 
> language, at least with regard to constant evaluation. If the interpreter is 
> also intended to (eventually) interpret full programs (e.g., evaluating 
> `main()`), I think such tests would be appropriate for an `Interp` directory. 
> But I say that having not closely followed the development and previous 
> reviews of the new interpreter.
> 
> At a minimum, I would like to see the character and string literal testing 
> pulled out of `arrays.cpp`. For the rest, I'll defer to others that have been 
> more involved in these reviews.
> I'm a

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 467217.
steven_wu added a comment.

Was missing the latest feedback. Bump FindGRPC module to top layer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  cmake/Modules/FindGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,4 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
-endfunction()
+endmacro()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,9 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  generate_protos_source(ProtoSource ${ProtoFile})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,4 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
-endfunction()
+endmacro()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/n

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Jonathon Penix via Phabricator via cfe-commits]

jpenix-quic added a comment.

Ping @awarzynski and @klausler--could you please let me know if the driver and 
runtime portions look good to you, respectively? (Apologies in advance if this 
isn't necessary, but as you both gave me significant feedback I wanted to make 
sure you are ok with the patch as it stands.) Thanks!


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

https://reviews.llvm.org/D130513

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

[PATCH] D135804: [clang][ExtractAPI] Ignore fully anonymous RecordDecls

[Daniel Grumberg via Phabricator via cfe-commits]

dang created this revision.
dang added reviewers: zixuw, ributzka, QuietMisdreavus.
Herald added a project: All.
dang requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

ExtractAPI was emitting a separate symbol for anonymous record declaration
that define the type of a member of another record declaration. Now
ExtractAPI ignores these declarations and just records the existence of
the actual member.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135804

Files:
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/ExtractAPI/anonymous_record_no_typedef.c

Index: clang/test/ExtractAPI/anonymous_record_no_typedef.c
===
--- /dev/null
+++ clang/test/ExtractAPI/anonymous_record_no_typedef.c
@@ -0,0 +1,396 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s@INPUT_DIR@%{/t:regex_replacement}@g" \
+// RUN: %t/reference.output.json.in >> %t/reference.output.json
+// RUN: %clang_cc1 -extract-api -triple arm64-apple-macosx \
+// RUN:   -x c-header %t/input.h -o %t/output.json -verify
+
+// Generator version is not consistent across test runs, normalize it.
+// RUN: sed -e "s@\"generator\": \".*\"@\"generator\": \"?\"@g" \
+// RUN: %t/output.json >> %t/output-normalized.json
+// RUN: diff %t/reference.output.json %t/output-normalized.json
+
+//--- input.h
+/// A Vehicle
+struct Vehicle {
+/// The type of vehicle.
+enum {
+Bicycle,
+Car
+} type;
+
+/// The information about the vehicle.
+struct {
+int wheels;
+char *name;
+} information;
+};
+// expected-no-diagnostics
+
+//--- reference.output.json.in
+{
+  "metadata": {
+"formatVersion": {
+  "major": 0,
+  "minor": 5,
+  "patch": 3
+},
+"generator": "?"
+  },
+  "module": {
+"name": "",
+"platform": {
+  "architecture": "arm64",
+  "operatingSystem": {
+"minimumVersion": {
+  "major": 11,
+  "minor": 0,
+  "patch": 0
+},
+"name": "macosx"
+  },
+  "vendor": "apple"
+}
+  },
+  "relationships": [
+{
+  "kind": "memberOf",
+  "source": "c:@S@Vehicle@E@input.h@64@Bicycle",
+  "target": "c:@S@Vehicle@E@input.h@64"
+},
+{
+  "kind": "memberOf",
+  "source": "c:@S@Vehicle@E@input.h@64@Car",
+  "target": "c:@S@Vehicle@E@input.h@64"
+},
+{
+  "kind": "memberOf",
+  "source": "c:@S@Vehicle@FI@type",
+  "target": "c:@S@Vehicle"
+},
+{
+  "kind": "memberOf",
+  "source": "c:@S@Vehicle@FI@information",
+  "target": "c:@S@Vehicle"
+}
+  ],
+  "symbols": [
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "keyword",
+  "spelling": "enum"
+},
+{
+  "kind": "text",
+  "spelling": ": "
+},
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:i",
+  "spelling": "unsigned int"
+}
+  ],
+  "docComment": {
+"lines": [
+  {
+"range": {
+  "end": {
+"character": 29,
+"line": 3
+  },
+  "start": {
+"character": 9,
+"line": 3
+  }
+},
+"text": "The type of vehicle."
+  }
+]
+  },
+  "identifier": {
+"interfaceLanguage": "c",
+"precise": "c:@S@Vehicle@E@input.h@64"
+  },
+  "kind": {
+"displayName": "Enumeration",
+"identifier": "c.enum"
+  },
+  "location": {
+"position": {
+  "character": 5,
+  "line": 4
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+"navigator": [
+  {
+"kind": "identifier",
+"spelling": "Vehicle::(anonymous)"
+  }
+],
+"title": "Vehicle::(anonymous)"
+  },
+  "pathComponents": [
+"Vehicle::(anonymous)"
+  ]
+},
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "identifier",
+  "spelling": "Bicycle"
+}
+  ],
+  "identifier": {
+"interfaceLanguage": "c",
+"precise": "c:@S@Vehicle@E@input.h@64@Bicycle"
+  },
+  "kind": {
+"displayName": "Enumeration Case",
+"identifier": "c.enum.case"
+  },
+  "location": {
+"position": {
+  "character": 9,
+  "line": 5
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+"navigator": [
+  {
+"kind": "identifier",
+"spelling": "Bicycle"
+  }
+],
+"subHeading": [
+  {
+"kind": "identifier",
+"spelling": "Bicycle"
+  }
+],
+"title": 

[clang] 2c09016 - [Frontend] Recognize environment variable SOURCE_DATE_EPOCH

[Fangrui Song via cfe-commits]

Author: Fangrui Song
Date: 2022-10-12T11:55:27-07:00
New Revision: 2c090162746a6b901c5639562c090e4bb2b7327e

URL: 
https://github.com/llvm/llvm-project/commit/2c090162746a6b901c5639562c090e4bb2b7327e
DIFF: 
https://github.com/llvm/llvm-project/commit/2c090162746a6b901c5639562c090e4bb2b7327e.diff

LOG: [Frontend] Recognize environment variable SOURCE_DATE_EPOCH

See https://reproducible-builds.org/docs/source-date-epoch/ . The environment
variable ``SOURCE_DATE_EPOCH`` been recognized by many compilers.

In GCC, if `SOURCE_DATE_EPOCH` is set, it specifies a UNIX timestamp to be used
in replacement of the current date and time in the `__DATE__` and `__TIME__`
macros. Note: GCC as of today does not update `__TIMESTAMP__` (the modification
time of the current source file) but
https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros expresses the
intention to update it.

This patches parses SOURCE_DATE_EPOCH and changes all the three macros.

In addition, in case gmtime/localtime returns null (e.g. on 64-bit Windows
gmtime returns null when the timestamp is larger than 32536850399
(3001-01-19T21:59:59Z)), use `??? ?? ` as used by GCC.

Reviewed By: ychen

Differential Revision: https://reviews.llvm.org/D135045

Added: 
clang/test/Preprocessor/SOURCE_DATE_EPOCH.c

Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticFrontendKinds.td
clang/include/clang/Lex/PreprocessorOptions.h
clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/Lex/PPMacroExpansion.cpp

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9803243e61967..1fe4be39e7aba 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -338,6 +338,10 @@ Non-comprehensive list of changes in this release
   is the target triple and `driver` first tries the canonical name
   for the driver (respecting ``--driver-mode=``), and then the name found
   in the executable.
+- If the environment variable ``SOURCE_DATE_EPOCH`` is set, it specifies a UNIX
+  timestamp to be used in replacement of the current date and time in
+  the ``__DATE__``, ``__TIME__``, and ``__TIMESTAMP__`` macros. See
+  ``_.
 
 New Compiler Flags
 --

diff  --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td 
b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 8a4b69c757aaa..3e87f6256e0aa 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -130,6 +130,8 @@ def err_fe_invalid_exception_model
 def warn_fe_concepts_ts_flag : Warning<
   "-fconcepts-ts is deprecated - use '-std=c++20' for Concepts support">,
   InGroup;
+def err_fe_invalid_source_date_epoch : Error<
+"environment variable 'SOURCE_DATE_EPOCH' ('%0') must be a non-negative 
decimal integer <= %1">;
 
 def err_fe_unable_to_load_basic_block_sections_file : Error<
 "unable to load basic block sections function list: '%0'">;

diff  --git a/clang/include/clang/Lex/PreprocessorOptions.h 
b/clang/include/clang/Lex/PreprocessorOptions.h
index 4cf18e98f051f..d51f2bcbe14ed 100644
--- a/clang/include/clang/Lex/PreprocessorOptions.h
+++ b/clang/include/clang/Lex/PreprocessorOptions.h
@@ -220,6 +220,9 @@ class PreprocessorOptions {
   /// Prevents intended crashes when using #pragma clang __debug. For testing.
   bool DisablePragmaDebugCrash = false;
 
+  /// If set, the UNIX timestamp specified by SOURCE_DATE_EPOCH.
+  Optional SourceDateEpoch;
+
 public:
   PreprocessorOptions() : PrecompiledPreambleBytes(0, false) {}
 

diff  --git a/clang/lib/Frontend/CompilerInvocation.cpp 
b/clang/lib/Frontend/CompilerInvocation.cpp
index 656e5950db988..bcd5359a5f3b2 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -94,7 +94,9 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -4307,6 +4309,21 @@ static bool ParsePreprocessorArgs(PreprocessorOptions 
&Opts, ArgList &Args,
 Opts.addRemappedFile(Split.first, Split.second);
   }
 
+  if (const char *Epoch = std::getenv("SOURCE_DATE_EPOCH")) {
+// SOURCE_DATE_EPOCH, if specified, must be a non-negative decimal integer.
+// On time64 systems, pick 253402300799 (the UNIX timestamp of
+// -12-31T23:59:59Z) as the upper bound.
+const uint64_t MaxTimestamp =
+std::min(std::numeric_limits::max(), 253402300799);
+uint64_t V;
+if (StringRef(Epoch).getAsInteger(10, V) || V > MaxTimestamp) {
+  Diags.Report(diag::err_fe_invalid_source_date_epoch)
+  << Epoch << MaxTimestamp;
+} else {
+  Opts.SourceDateEpoch = V;
+}
+  }
+
   // Always avoid lexing editor placeholders when we're just running the
   // preprocessor as we never want to emit the
   // "editor placeholder in sour

[PATCH] D135045: [Frontend] Recognize environment variable SOURCE_DATE_EPOCH

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c090162746a: [Frontend] Recognize environment variable 
SOURCE_DATE_EPOCH (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135045

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/test/Preprocessor/SOURCE_DATE_EPOCH.c

Index: clang/test/Preprocessor/SOURCE_DATE_EPOCH.c
===
--- /dev/null
+++ clang/test/Preprocessor/SOURCE_DATE_EPOCH.c
@@ -0,0 +1,32 @@
+// RUN: env SOURCE_DATE_EPOCH=0 %clang_cc1 -E %s | FileCheck %s --check-prefix=19700101
+
+// 19700101:  const char date[] = "Jan  1 1970";
+// 19700101-NEXT: const char time[] = "00:00:00";
+// 19700101-NEXT: const char timestamp[] = "Thu Jan  1 00:00:00 1970";
+
+// RUN: env SOURCE_DATE_EPOCH=2147483647 %clang_cc1 -E -Wdate-time %s 2>&1 | FileCheck %s --check-prefix=Y2038
+
+// Y2038:  warning: expansion of date or time macro is not reproducible [-Wdate-time]
+// Y2038:  const char date[] = "Jan 19 2038";
+// Y2038-NEXT: const char time[] = "03:14:07";
+// Y2038-NEXT: const char timestamp[] = "Tue Jan 19 03:14:07 2038";
+
+/// Test a large timestamp if the system uses 64-bit time_t and known to support large timestamps.
+// RUN: %if !system-windows && clang-target-64-bits %{ env SOURCE_DATE_EPOCH=253402300799 %clang_cc1 -E -Wdate-time %s 2>&1 | FileCheck %s --check-prefix=1231 %}
+
+// 1231:  warning: expansion of date or time macro is not reproducible [-Wdate-time]
+// 1231:  const char date[] = "Dec 31 ";
+// 1231-NEXT: const char time[] = "23:59:59";
+// 1231-NEXT: const char timestamp[] = "Fri Dec 31 23:59:59 ";
+
+// RUN: env SOURCE_DATE_EPOCH=253402300800 not %clang_cc1 -E %s 2>&1 | FileCheck %s --check-prefix=TOOBIG
+
+// TOOBIG: error: environment variable 'SOURCE_DATE_EPOCH' ('253402300800') must be a non-negative decimal integer <= {{(2147483647|253402300799)}}
+
+// RUN: env SOURCE_DATE_EPOCH=0x0 not %clang_cc1 -E %s 2>&1 | FileCheck %s --check-prefix=NOTDECIMAL
+
+// NOTDECIMAL: error: environment variable 'SOURCE_DATE_EPOCH' ('0x0') must be a non-negative decimal integer <= {{(2147483647|253402300799)}}
+
+const char date[] = __DATE__;
+const char time[] = __TIME__;
+const char timestamp[] = __TIMESTAMP__;
Index: clang/lib/Lex/PPMacroExpansion.cpp
===
--- clang/lib/Lex/PPMacroExpansion.cpp
+++ clang/lib/Lex/PPMacroExpansion.cpp
@@ -1085,8 +1085,15 @@
 /// the identifier tokens inserted.
 static void ComputeDATE_TIME(SourceLocation &DATELoc, SourceLocation &TIMELoc,
  Preprocessor &PP) {
-  time_t TT = time(nullptr);
-  struct tm *TM = localtime(&TT);
+  time_t TT;
+  std::tm *TM;
+  if (PP.getPreprocessorOpts().SourceDateEpoch) {
+TT = *PP.getPreprocessorOpts().SourceDateEpoch;
+TM = std::gmtime(&TT);
+  } else {
+TT = std::time(nullptr);
+TM = std::localtime(&TT);
+  }
 
   static const char * const Months[] = {
 "Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"
@@ -1095,8 +1102,11 @@
   {
 SmallString<32> TmpBuffer;
 llvm::raw_svector_ostream TmpStream(TmpBuffer);
-TmpStream << llvm::format("\"%s %2d %4d\"", Months[TM->tm_mon],
-  TM->tm_mday, TM->tm_year + 1900);
+if (TM)
+  TmpStream << llvm::format("\"%s %2d %4d\"", Months[TM->tm_mon],
+TM->tm_mday, TM->tm_year + 1900);
+else
+  TmpStream << "??? ?? ";
 Token TmpTok;
 TmpTok.startToken();
 PP.CreateString(TmpStream.str(), TmpTok);
@@ -1106,8 +1116,11 @@
   {
 SmallString<32> TmpBuffer;
 llvm::raw_svector_ostream TmpStream(TmpBuffer);
-TmpStream << llvm::format("\"%02d:%02d:%02d\"",
-  TM->tm_hour, TM->tm_min, TM->tm_sec);
+if (TM)
+  TmpStream << llvm::format("\"%02d:%02d:%02d\"", TM->tm_hour, TM->tm_min,
+TM->tm_sec);
+else
+  TmpStream << "??:??:??";
 Token TmpTok;
 TmpTok.startToken();
 PP.CreateString(TmpStream.str(), TmpTok);
@@ -1593,22 +1606,24 @@
 Diag(Tok.getLocation(), diag::warn_pp_date_time);
 // MSVC, ICC, GCC, VisualAge C++ extension.  The generated string should be
 // of the form "Ddd Mmm dd hh::mm::ss ", which is returned by asctime.
-
-// Get the file that we are lexing out of.  If we're currently lexing from
-// a macro, dig into the include stack.
-const FileEntry *CurFile = nullptr;
-PreprocessorLexer *TheLexer = getCurrentFileLexer();
-
-if (TheLexer)
-  CurFile = SourceMgr.getFileEntryForID(TheLexer->getFileID())

[PATCH] D134371: [clang-doc] Add typedef/using information.

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

The premerge check is failing, because it can't apply the patch. Often this 
just means you need to rebase, so can you try rebasing your change and 
reuploading?

Also, do you have commit access? you've made enough changes that its 
appropriate to request, which should make landing your changes easier.


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

https://reviews.llvm.org/D134371

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

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peter Klausler via Phabricator via cfe-commits]

klausler accepted this revision.
klausler added a comment.

The runtime parts look good to me.


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

https://reviews.llvm.org/D130513

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

[clang] 8d72f44 - [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via cfe-commits]

Author: Joseph Huber
Date: 2022-10-12T14:07:52-05:00
New Revision: 8d72f445f79992fd860a73ca4ebd78c1bcc6324b

URL: 
https://github.com/llvm/llvm-project/commit/8d72f445f79992fd860a73ca4ebd78c1bcc6324b
DIFF: 
https://github.com/llvm/llvm-project/commit/8d72f445f79992fd860a73ca4ebd78c1bcc6324b.diff

LOG: [Clang] Do not crash when an invalid offload architecture is set

If an invalid architecture is set we currently return an empty string.
This will cause the offloading toolchain to continue to be built and
hit an assertion elsewhere due to the invalid architecture. This patch
fixes that so we now correctly exit.

Reviewed By: tra

Differential Revision: https://reviews.llvm.org/D135791

Added: 


Modified: 
clang/lib/Driver/Driver.cpp
clang/test/Driver/cuda-phases.cu

Removed: 




diff  --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 3300919d7cdd1..45530f5d68e6c 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4273,15 +4273,27 @@ Driver::getOffloadArchs(Compilation &C, const 
llvm::opt::DerivedArgList &Args,
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+StringRef ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple());
+if (ArchStr.empty())
+  return Archs;
+Archs.insert(ArchStr);
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == "all") {
   Archs.clear();
-else
-  Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+} else {
+  StringRef ArchStr =
+  getCanonicalArchString(C, Args, Arch, TC->getTriple());
+  if (ArchStr.empty())
+return Archs;
+  Archs.erase(ArchStr);
+}
   }
 }
   }

diff  --git a/clang/test/Driver/cuda-phases.cu 
b/clang/test/Driver/cuda-phases.cu
index 2622c3a1bf55e..c7e8d08c6d5b8 100644
--- a/clang/test/Driver/cuda-phases.cu
+++ b/clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, 
"device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)



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

[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8d72f445f799: [Clang] Do not crash when an invalid offload 
architecture is set (authored by jhuber6).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/cuda-phases.cu


Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, 
"device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4273,15 +4273,27 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+StringRef ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple());
+if (ArchStr.empty())
+  return Archs;
+Archs.insert(ArchStr);
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-if (Arch == StringRef("all"))
+if (Arch == "all") {
   Archs.clear();
-else
-  Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+} else {
+  StringRef ArchStr =
+  getCanonicalArchString(C, Args, Arch, TC->getTriple());
+  if (ArchStr.empty())
+return Archs;
+  Archs.erase(ArchStr);
+}
   }
 }
   }


Index: clang/test/Driver/cuda-phases.cu
===
--- clang/test/Driver/cuda-phases.cu
+++ clang/test/Driver/cuda-phases.cu
@@ -318,3 +318,16 @@
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, "device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:-ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//  INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -4273,15 +4273,27 @@
   Arg = ExtractedArg.get();
 }
 
+// Add or remove the seen architectures in order of appearance. If an
+// invalid architecture is given we simply exit.
 if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-  for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+  for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+StringRef ArchStr =
+getCanonicalArchString(C, Args, Arch, TC->getTriple());
+if (ArchStr.empty())
+  return Archs;
+Archs.insert(ArchStr);
+  }
 } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
   f

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:598
   } else {
-assert(false && "Unhandled type in array initializer initlist");
+llvm_unreachable("Unhandled type in array initializer initlist");
   }

aaron.ballman wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > The rest of the ones here are somewhat interesting in that the 
> > > interpreter is an experiment under active development and is known to be 
> > > incomplete. In all of these cases, I think the switch to unreachable is 
> > > flat-out wrong -- these asserts serve explicitly to find unimplemented 
> > > cases when we hit them.
> > & I don't see why unreachable is any different a statement than 
> > assert(false) in these cases... - it's the same statement of intent. "if 
> > this is reached you've found a bug" (in this case, a missing feature)
> > 
> > But I'd be sort of OK changing all these to report_fatal_error. But, again, 
> > I think the assert(false) -> unreachable is a valid transformation and 
> > doesn't make anything worse than it already is, but improves the code by 
> > being more consistent and removing this confusion that there might be 
> > something different about assert(false) when, I believe, there isn't.
> > & I don't see why unreachable is any different a statement than 
> > assert(false) in these cases... - it's the same statement of intent. "if 
> > this is reached you've found a bug" (in this case, a missing feature)
> 
> You are asserting it's the same statement of intent and I keep pointing out 
> that people use the different constructs in practice because they're 
> different statements of intent. I don't know how to resolve this difference 
> of opinion, but I can say as someone doing code review in this area recently 
> that your interpretation is wrong according to what we were after with this 
> code.
> 
> I'd be fine changing it to `report_fatal_error` instead of `assert(false)`; 
> I'd be strongly opposed to switching to `llvm_unreachable`.
I use llvm_unreachable as a nicer to use assert in if/else chains like this. I 
also see no difference in the intent between assert and unreachable; assert(0 
&& "message") is just uglier.

report_fatal_error is for something a user could plausibly run into but also 
isn't worth wiring into a proper error diagnostic (which happens a lot in 
codegen)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

In D134529#3853086 , @BertalanD wrote:

> Does it deserve a mention in the Potentially Breaking Changes section of the 
> release notes?

You are right. I was thinking the same after this discussion. I will add this 
to the section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D132131: [clang-format] Adds a formatter for aligning trailing comments over empty lines

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/WhitespaceManager.cpp:950-952
+  if (RestoredLineLength >= Style.ColumnLimit && Style.ColumnLimit != 0) {
+break;
+  }

Elide the braces.



Comment at: clang/unittests/Format/FormatTestComments.cpp:3045
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Leave;
+  EXPECT_EQ("int a;// do not touch\n"
+"int b; // any comments\n"

Can you do that in reverse order (more spaces at top) also?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132131

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

[PATCH] D135595: [HLSL] Add utility to convert environment to stage

[Justin Bogner via Phabricator via cfe-commits]

bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.

Even without the tooling aspect the addition of the static asserts is a strict 
improvement. looks good, thanks!




Comment at: clang/include/clang/Basic/HLSLRuntime.h:39
+  return static_cast(Pipeline);
+}
+

You're not actually introducing the dependency here (it was already there), but 
neither `ShaderStage` in LangOptions.h nor the shader stage `EnvironmentType` 
in Triple.h mention that they need to be kept in sync with the other. Can you 
add comments to both that note the relationship?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135595

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

thanks!




Comment at: cmake/Modules/FindGRPC.cmake:111
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")

changing from function to macro means all the local variables here are done in 
the parent scope, which is hard to reason about.

I think the only reason you're doing that here is to return the filename in the 
`ProtoSource` variable, in which case `set(${GeneratedSource} 
${GeneratedProtoSource} PARENT_SCOPE)` should do the job from a function.



Comment at: cmake/Modules/FindGRPC.cmake:111
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")

sammccall wrote:
> changing from function to macro means all the local variables here are done 
> in the parent scope, which is hard to reason about.
> 
> I think the only reason you're doing that here is to return the filename in 
> the `ProtoSource` variable, in which case `set(${GeneratedSource} 
> ${GeneratedProtoSource} PARENT_SCOPE)` should do the job from a function.
nit: the plural is in the wrong place: rather `generate_proto_sources`?
this generates the multiple sources for a single proto, rather than the other 
way around.

(I'm not sure why the original was plural, but this makes it somehow more 
confusing)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

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

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:540
 
-- Add `RemoveSemicolon` option for removing `;` after a non-empty function 
definition.
+- Add ``RemoveSemicolon`` option for removing ``;`` after a non-empty function 
definition.
+- Add ``RequiresExpressionIndentation`` option for configuring the alignment 
of requires-expressions.

Unrelated.



Comment at: clang/lib/Format/Format.cpp:1304
   LLVMStyle.RequiresClausePosition = FormatStyle::RCPS_OwnLine;
+  LLVMStyle.RequiresExpressionIndentation = FormatStyle::REI_Keyword;
   LLVMStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Leave;

owenpan wrote:
> Delete it.
Why delete?
Set it to OuterScope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129443

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

[PATCH] D134853: [clang-format] Correctly annotate UDLs as OverloadedOperator

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Format/TokenAnnotator.cpp:1189-1194
+// User defined literal without a space
+if (CurrentToken->Previous->is(tok::string_literal) &&
+CurrentToken->Previous->TokenText.startswith("\"\"")) {
+  CurrentToken->Previous->setType(TT_OverloadedOperator);
+}
+// User defined literal with a space




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134853

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: cmake/Modules/FindGRPC.cmake:111
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")

sammccall wrote:
> sammccall wrote:
> > changing from function to macro means all the local variables here are done 
> > in the parent scope, which is hard to reason about.
> > 
> > I think the only reason you're doing that here is to return the filename in 
> > the `ProtoSource` variable, in which case `set(${GeneratedSource} 
> > ${GeneratedProtoSource} PARENT_SCOPE)` should do the job from a function.
> nit: the plural is in the wrong place: rather `generate_proto_sources`?
> this generates the multiple sources for a single proto, rather than the other 
> way around.
> 
> (I'm not sure why the original was plural, but this makes it somehow more 
> confusing)
The reason why it is a macro is I don't know any elegant way to pass all the 
DEPEND down to `add_*_library`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

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

[PATCH] D135740: [clang-format] Fix multiple preprocessor if sections parsing incorrectly

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D135740#3851405 , @sstwcw wrote:

> Should I add a test with comments to be aligned like in the bug report?  I 
> was not sure because I didn't find an existing test for alignment of comments.

Yes you should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135740

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