[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Ian Anderson via Phabricator via cfe-commits]

iana added inline comments.



Comment at: clang/lib/Headers/stddef.h:118-122
+#ifdef __cplusplus
+namespace std {
+typedef decltype(nullptr) nullptr_t;
+}
+using ::std::nullptr_t;

aaron.ballman wrote:
> ldionne wrote:
> > iana wrote:
> > > aaron.ballman wrote:
> > > > iana wrote:
> > > > > aaron.ballman wrote:
> > > > > > iana wrote:
> > > > > > > ldionne wrote:
> > > > > > > > iana wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Related:
> > > > > > > > > > 
> > > > > > > > > > https://github.com/llvm/llvm-project/issues/37564
> > > > > > > > > > https://cplusplus.github.io/LWG/issue3484
> > > > > > > > > > 
> > > > > > > > > > CC @ldionne
> > > > > > > > > I don't _think_ this change actually changes the way 
> > > > > > > > > nullptr_t gets defined in C++, does it?
> > > > > > > > I think we absolutely don't want to touch `std::nullptr_t` from 
> > > > > > > > this header. It's libc++'s responsibility to define that, and 
> > > > > > > > in fact we define it in `std::__1`, so this is even an ABI 
> > > > > > > > break (or I guess it would be a compiler error, not sure).
> > > > > > > I'm really not touching it though. All I did is move it from 
> > > > > > > `__need_NULL` to `__need_nullptr_t`.
> > > > > > > 
> > > > > > > The old behavior is that `std::nullptr_t` would only be touched 
> > > > > > > if (no `__need_` macros were set or if `__need_NULL` was set), 
> > > > > > > and (_MSC_EXTENSIONS and _NATIVE_NULLPTR_SUPPORTED are defined).
> > > > > > > 
> > > > > > > The new behavior is that `std::nullptr_t` will only be touched if 
> > > > > > > ((no `__need_` macros are set) and (_MSC_EXTENSIONS and 
> > > > > > > _NATIVE_NULLPTR_SUPPORTED are defined)) or (the new 
> > > > > > > `__need_nullptr_t` macro is set)
> > > > > > > 
> > > > > > > So the only change is that C++ code that previously set 
> > > > > > > `__need_NULL` will no longer get `std::nullptr_t`. @efriedma felt 
> > > > > > > like that was a fine change.
> > > > > > Does libc++ provide the symbols for a freestanding compilation?
> > > > > > 
> > > > > > I was pointing out those links specifically because the C++ 
> > > > > > standard currently says that stddef.h (the C standard library 
> > > > > > header) needs to provide a definition of `std::nullptr_t`, but that 
> > > > > > LWG thinks that's perhaps not the right way to do that and may be 
> > > > > > removing that requirement.
> > > > > It is weird the standard puts that in stddef.h and not cstddef. I 
> > > > > think libc++ could provide that in their stddef.h anyway, but the 
> > > > > intent in this review is to not rock the boat and only do the minimal 
> > > > > change discussed above.
> > > > Yeah, this discussion is to figure out whether we have an existing bug 
> > > > we need to address and if so, where to address it (libc++, clang, or 
> > > > the C++ standard). I don't think your changes are exacerbating 
> > > > anything, more just that they've potentially pointed out something 
> > > > related.
> > >  
> > > Does libc++ provide the symbols for a freestanding compilation?
> > 
> > I don't think we do. We basically don't support `-ffreestanding` right now 
> > (we support embedded and funky platforms via other mechanisms).
> > 
> > But regardless, `` should never define something in namespace 
> > `std`, that should be libc++'s responsibility IMO. What we could do here 
> > instead is just
> > 
> > ```
> > #ifdef __cplusplus
> > typedef decltype(nullptr) nullptr_t;
> > #else
> > typedef typeof(nullptr) nullptr_t;
> > #endif
> > ```
> > 
> > and then let libc++'s `` do
> > 
> > ```
> > _LIBCPP_BEGIN_NAMESPACE_STD
> > using ::nullptr_t;
> > _LIBCPP_END_NAMESPACE_STD
> > ```
> > 
> > If Clang's `` did define `::nullptr_t`, we could likely remove 
> > libc++'s `` and that might simplify things.
> >> Does libc++ provide the symbols for a freestanding compilation?
> > I don't think we do. We basically don't support -ffreestanding right now 
> > (we support embedded and funky platforms via other mechanisms).
> 
> Okay, that's what I thought as well. Thanks!
> 
> > But regardless,  should never define something in namespace std, 
> > that should be libc++'s responsibility IMO. What we could do here instead 
> > is just
> 
> Ah, so you're thinking stddef.h should provide the global nullptr_t and 
> cstddef should provide the std::nullptr_t. I was thinking stddef.h should not 
> define nullptr_t in C++ mode at all; it's a C header, not a C++ header. That 
> led me to thinking about what the behavior should be in C23 given that it 
> supports nullptr_t.
> 
> Were it not for the current requirement that stddef.h provide nullptr_t, I 
> think stddef.h should do:
> ```
> typedef typeof(nullptr) nullptr_t;
> ```
> in C23 mode and not do anything special for C++ at all. C's `nullptr_t` needs 
> to be ABI compatible with C++'s `nullptr_t`, so a C++ user including the C 
> header should not get any problems linking against a C++ 

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Ian Anderson via Phabricator via cfe-commits]

iana updated this revision to Diff 552591.
iana added a comment.

Only define nullptr_t in C++ if _MSC_EXTENSIONS is defined, even if 
__need_nullptr_t is explicitly set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157757

Files:
  clang/lib/Headers/stddef.h
  clang/test/Headers/stddef.c
  clang/test/Headers/stddefneeds.c

Index: clang/test/Headers/stddefneeds.c
===
--- /dev/null
+++ clang/test/Headers/stddefneeds.c
@@ -0,0 +1,169 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=c99 -std=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=c23 -std=c23 %s
+
+// Use C99 to verify that __need_ can be used to get types that wouldn't normally be available.
+
+struct astruct { char member; };
+
+ptrdiff_t p0; // c99-error{{unknown type name 'ptrdiff_t'}} c23-error{{unknown type}}
+size_t s0; // c99-error{{unknown type name 'size_t'}} c23-error{{unknown type}}
+rsize_t r0; // c99-error{{unknown type name 'rsize_t'}} c23-error{{unknown type}}
+wchar_t wc0; // c99-error{{unknown type name 'wchar_t'}} c23-error{{unknown type}}
+void *v0 = NULL; // c99-error{{use of undeclared identifier 'NULL'}} c23-error{{undeclared identifier}}
+nullptr_t n0; // c99-error{{unknown type name 'nullptr_t'}} c23-error{{unknown type}}
+static void f0(void) { unreachable(); } // c99-error{{call to undeclared function 'unreachable'}} c23-error{{undeclared identifier 'unreachable'}}
+max_align_t m0; // c99-error{{unknown type name 'max_align_t'}} c23-error{{unknown type}}
+size_t o0 = offsetof(struct astruct, member); // c99-error{{unknown type name 'size_t'}} c99-error{{call to undeclared function 'offsetof'}} c99-error{{expected expression}} c99-error{{use of undeclared identifier 'member'}} \
+ c23-error{{unknown type name 'size_t'}} c23-error{{undeclared identifier 'offsetof'}} c23-error{{expected expression}} c23-error{{use of undeclared identifier 'member'}}
+wint_t wi0; // c99-error{{unknown type name 'wint_t'}} c23-error{{unknown type}}
+
+#define __need_ptrdiff_t
+#include 
+
+ptrdiff_t p1;
+size_t s1; // c99-error{{unknown type}} c23-error{{unknown type}}
+rsize_t r1; // c99-error{{unknown type}} c23-error{{unknown type}}
+wchar_t wc1; // c99-error{{unknown type}} c23-error{{unknown type}}
+void *v1 = NULL; // c99-error{{undeclared identifier}} c23-error{{undeclared identifier}}
+nullptr_t n1; // c99-error{{unknown type}} c23-error{{unknown type}}
+static void f1(void) { unreachable(); } // c99-error{{undeclared function}} c23-error{{undeclared identifier}}
+max_align_t m1; // c99-error{{unknown type}} c23-error{{unknown type}}
+size_t o1 = offsetof(struct astruct, member); // c99-error{{unknown type}} c99-error{{expected expression}} c99-error{{undeclared identifier}} \
+ c23-error{{unknown type}} c23-error{{undeclared identifier}} c23-error{{expected expression}} c23-error{{undeclared identifier}}
+wint_t wi1; // c99-error{{unknown type}} c23-error{{unknown type}}
+
+#define __need_size_t
+#include 
+
+ptrdiff_t p2;
+size_t s2;
+rsize_t r2; // c99-error{{unknown type}} c23-error{{unknown type}}
+// c99-note@stddef.h:*{{'size_t' declared here}} c23-note@stddef.h:*{{'size_t' declared here}}
+wchar_t wc2; // c99-error{{unknown type}} c23-error{{unknown type}}
+void *v2 = NULL; // c99-error{{undeclared identifier}} c23-error{{undeclared identifier}}
+nullptr_t n2; // c99-error{{unknown type}} c23-error{{unknown type}}
+static void f2(void) { unreachable(); } // c99-error{{undeclared function}} c23-error{{undeclared identifier}}
+max_align_t m2; // c99-error{{unknown type}} c23-error{{unknown type}}
+size_t o2 = offsetof(struct astruct, member); // c99-error{{expected expression}} c99-error{{undeclared identifier}} \
+ c23-error{{undeclared identifier}} c23-error{{expected expression}} c23-error{{undeclared identifier}}
+wint_t wi2; // c99-error{{unknown type}} c23-error{{unknown type}}
+
+#define __need_rsize_t
+#include 
+
+ptrdiff_t p3;
+size_t s3;
+rsize_t r3;
+wchar_t wc3; // c99-error{{unknown type}} c23-error{{unknown type}}
+void *v3 = NULL; // c99-error{{undeclared identifier}} c23-error{{undeclared identifier}}
+nullptr_t n3; // c99-error{{unknown type}} c23-error{{unknown type}}
+static void f3(void) { unreachable(); } // c99-error{{undeclared function}} c23-error{{undeclared identifier}}
+max_align_t m3; // c99-error{{unknown type}} c23-error{{unknown type}}
+size_t o3 = offsetof(struct astruct, member); // c99-error{{expected expression}} c99-error{{undeclared identifier}} \
+ c23-error{{undeclared identifier}} c23-error{{expected expression}} c23-error{{undeclared identifier}}
+wint_t wi3; // c99-error{{unknown type}} c23-error{{unknown type}}
+
+#define __need_wchar_t

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

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

tbaeder added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:767-772
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());

cor3ntin wrote:
> so if `visitAPValue` fails, we continue. Couldn't that lead to duplicated 
> diagnostics? Shouldn't we simply return whatever `visitAPValue` returns 
> unconditionally?
I did it this way because we don't handle all types of APValues in 
`visitAPValue()` (think lvalues with an lvalue path), so in those cases we need 
to visit the expression instead. If there is an `APValue` we're visting, we're 
not emitting any diagnostics I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158502

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

[PATCH] D158499: [analyzer] Compute FAM dynamic size

[Ding Fei via Phabricator via cfe-commits]

danix800 added a comment.

We have `getDynamicExtentWithOffset` to use, which can handle more general
dynamic memory based cases than this fix.

I'll abandon this one.

There are issues worth clarifying and fixing:

1). Debugging APIs like `clang_analyzer_dumpExtent` in `ExprInspection` might 
be expanded
to use `getDynamicExtentWithOffset` if it's intended to be used on not only 
dynamic allocated
regions but more general ones, and more testcases are needed to demonstrate the 
usage.

2). Fix type-inconsistency of several size-related `SVal`s, e.g.

  FAM fam;
  clang_analyzer_dump(clang_analyzer_getExtent());
  clang_analyzer_dump(clang_analyzer_getExtent(fam.data));
  // expected-warning@-2 {{4 S64b}}  // U64b is more reasonable when used as an 
extent
  // expected-warning@-2 {{0 U64b}}

`ArrayIndexType` might be misused here.

Simple searching results listed here (might not be complete):

1. `getDynamicExtentWithOffset` return value
2. `getElementExtent` return value
3. `ExprEngineCallAndReturn.cpp` when calling `setDynamicExtent` the `Size` arg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158499

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

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added inline comments.



Comment at: clang/lib/AST/FormatString.cpp:480-481
+  break;
+case BuiltinType::Half:
+case BuiltinType::Float16:
+case BuiltinType::Float:

aaron.ballman wrote:
> Should these be checking for `T == C.FloatTy` to return 
> `NoMatchPromotionTypeConfusion`?
I don't think it's necessary. `T` is the format specifier's expected type, and 
no format specifier expects a `float` (due to floating-point types being 
promoted to `double` by default argument promotion), so there's never a case 
where `T` is `FloatTy`.


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

https://reviews.llvm.org/D158318

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

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 552579.
fcloutier marked an inline comment as done.
fcloutier added a comment.

Add release note, ensure `bool` as a formatted formal parameter is accepted.


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

https://reviews.llvm.org/D158318

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/FormatString.cpp
  clang/test/Sema/attr-format.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -72,12 +72,20 @@
   int x = 123;
   int  = x;
   const char *s = "world";
+  bool b = false;
   format("bare string");
   format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
   format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
 
+  format("%c %c %hhd %hd %d\n", (char)'a', 'a', 'a', (short)123, (int)123);
+  format("%f %f %f\n", (__fp16)123.f, 123.f, 123.);
+  format("%Lf", (__fp16)123.f); // expected-warning{{format specifies type 'long double' but the argument has type '__fp16'}}
+  format("%Lf", 123.f); // expected-warning{{format specifies type 'long double' but the argument has type 'float'}}
+  format("%hhi %hhu %hi %hu %i %u", b, b, b, b, b, b);
+  format("%li", b); // expected-warning{{format specifies type 'long' but the argument has type 'bool'}}
+
   struct foo f;
   format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
 
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -94,13 +94,9 @@
   d3("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
 }
 
-__attribute__((format(printf, 1, 2))) void forward_fixed(const char *fmt, int i) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
-  forward_fixed(fmt, i);
-  a(fmt, i);
-}
-
-__attribute__((format(printf, 1, 2))) void forward_fixed_2(const char *fmt, int i, int j) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
-  forward_fixed_2(fmt, i, j);
-  a(fmt, i);
+__attribute__((format(printf, 1, 2)))
+void forward_fixed(const char *fmt, _Bool b, char i, short j, int k, float l, double m) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  forward_fixed(fmt, b, i, j, k, l, m);
+  a(fmt, b, i, j, k, l, m);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -458,6 +458,10 @@
 switch (BT->getKind()) {
 default:
   break;
+case BuiltinType::Bool:
+  if (T == C.IntTy || T == C.UnsignedIntTy)
+return MatchPromotion;
+  break;
 case BuiltinType::Int:
 case BuiltinType::UInt:
   if (T == C.SignedCharTy || T == C.UnsignedCharTy ||
@@ -465,6 +469,24 @@
   T == C.WideCharTy)
 return MatchPromotion;
   break;
+case BuiltinType::Char_U:
+  if (T == C.UnsignedIntTy)
+return MatchPromotion;
+  if (T == C.UnsignedShortTy)
+return NoMatchPromotionTypeConfusion;
+  break;
+case BuiltinType::Char_S:
+  if (T == C.IntTy)
+return MatchPromotion;
+  if (T == C.ShortTy)
+return NoMatchPromotionTypeConfusion;
+  break;
+case BuiltinType::Half:
+case BuiltinType::Float16:
+case BuiltinType::Float:
+  if (T == C.DoubleTy)
+return MatchPromotion;
+  break;
 case BuiltinType::Short:
 case BuiltinType::UShort:
   if (T == C.SignedCharTy || T == C.UnsignedCharTy)
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -130,6 +130,13 @@
 Attribute Changes in Clang
 --
 
+- When a non-variadic function is decorated with the ``format`` attribute,
+  Clang now checks that the format string would match the function's parameters'
+  types after default argument promotion. As a result, it's no longer an
+  automatic diagnostic to use parameters of types that the format style
+  supports but that are never the result of default argument promotion, such as
+  ``float``. (`#59824: 

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

So while working on D148474   I realized this 
PR introduced a new crash bug, see the following code: 
https://godbolt.org/z/h1EezGjbr

  template
  class B3 : A3 {
template()>
B3();
  }; B3(); 
  

which is one of my test cases. I had though it was an interaction between my 
new code and this but then realized this exists w/o my new code. We can also 
see from the godbolt above that the crash in this code changed from an 
assertion to an unreachable. My PR will fix the assertion but I need a good 
solution to the unreachable bug. I tried changing the unreachable to a `break` 
but it reduces the quality of the diagnostic and I don't think it is the 
correct fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139837

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

[PATCH] D156259: [clang-format] Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line.

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D156259#4607177 , @jp4a50 wrote:

> Addressed all comments. Please let me know if there's anything else required 
> before merging.

There are still a couple of comments unaddressed plus another that asked for 
changing `!is()` to `isNot()`. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156259

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

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 552575.
AlexVlx edited the summary of this revision.
AlexVlx added a comment.

Updating to reflect the outcome of the RFC, which is that this will be added as 
a HIP extension exclusively.


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

https://reviews.llvm.org/D155850

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenHipStdPar/unannotated-functions-get-emitted.cpp
  clang/test/CodeGenHipStdPar/unsupported-ASM.cpp
  clang/test/CodeGenHipStdPar/unsupported-builtins.cpp

Index: clang/test/CodeGenHipStdPar/unsupported-builtins.cpp
===
--- /dev/null
+++ clang/test/CodeGenHipStdPar/unsupported-builtins.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-linux-gnu \
+// RUN:   --hipstdpar -x hip -emit-llvm -fcuda-is-device -o - %s | FileCheck %s
+
+#define __global__ __attribute__((global))
+
+__global__ void foo() { return __builtin_ia32_pause(); }
+
+// CHECK: declare void @__builtin_ia32_pause__hipstdpar_unsupported()
Index: clang/test/CodeGenHipStdPar/unsupported-ASM.cpp
===
--- /dev/null
+++ clang/test/CodeGenHipStdPar/unsupported-ASM.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-linux-gnu \
+// RUN:   --hipstdpar -x hip -emit-llvm -fcuda-is-device -o - %s | FileCheck %s
+
+#define __global__ __attribute__((global))
+
+__global__ void foo(int i) {
+asm ("addl %2, %1; seto %b0" : "=q" (i), "+g" (i) : "r" (i));
+}
+
+// CHECK: declare void @__ASM__hipstdpar_unsupported([{{.*}}])
Index: clang/test/CodeGenHipStdPar/unannotated-functions-get-emitted.cpp
===
--- /dev/null
+++ clang/test/CodeGenHipStdPar/unannotated-functions-get-emitted.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -x hip -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck --check-prefix=NO-HIPSTDPAR-DEV %s
+
+// RUN: %clang_cc1 --hipstdpar -emit-llvm -fcuda-is-device \
+// RUN:   -o - %s | FileCheck --check-prefix=HIPSTDPAR-DEV %s
+
+#define __device__ __attribute__((device))
+
+// NO-HIPSTDPAR-DEV-NOT: define {{.*}} void @_Z3fooPff({{.*}})
+// HIPSTDPAR-DEV: define {{.*}} void @_Z3fooPff({{.*}})
+void foo(float *a, float b) {
+  *a = b;
+}
+
+// NO-HIPSTDPAR-DEV: define {{.*}} void @_Z3barPff({{.*}})
+// HIPSTDPAR-DEV: define {{.*}} void @_Z3barPff({{.*}})
+__device__ void bar(float *a, float b) {
+  *a = b;
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -3558,7 +3558,10 @@
   !Global->hasAttr() &&
   !Global->hasAttr() &&
   !Global->getType()->isCUDADeviceBuiltinSurfaceType() &&
-  !Global->getType()->isCUDADeviceBuiltinTextureType())
+  !Global->getType()->isCUDADeviceBuiltinTextureType() &&
+  !(LangOpts.HIPStdPar &&
+isa(Global) &&
+!Global->hasAttr()))
 return;
 } else {
   // We need to emit host-side 'shadows' for all global
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2594,10 +2594,15 @@
   std::string MissingFeature;
   llvm::StringMap CallerFeatureMap;
   CGM.getContext().getFunctionFeatureMap(CallerFeatureMap, FD);
+  // When compiling in HipStdPar mode we have to be conservative in rejecting
+  // target specific features in the FE, and defer the possible error to the
+  // AcceleratorCodeSelection pass, wherein iff an unsupported target builtin is
+  // referenced by an accelerator executable function, we emit an error.
+  bool IsHipStdPar = getLangOpts().HIPStdPar && getLangOpts().CUDAIsDevice
   if (BuiltinID) {
 StringRef FeatureList(CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID));
 if (!Builtin::evaluateRequiredTargetFeatures(
-FeatureList, CallerFeatureMap)) {
+FeatureList, CallerFeatureMap) && !IsHipStdPar) {
   CGM.getDiags().Report(Loc, diag::err_builtin_needs_feature)
   << TargetDecl->getDeclName()
   << FeatureList;
@@ -2630,7 +2635,7 @@
 return false;
   }
   return true;
-}))
+}) && !IsHipStdPar)
   CGM.getDiags().Report(Loc, diag::err_function_needs_feature)
   << FD->getDeclName() << TargetDecl->getDeclName() << MissingFeature;
   } else if (!FD->isMultiVersion() && FD->hasAttr()) {
@@ -2639,7 +2644,8 @@
 
 for (const auto  : CalleeFeatureMap) {
   if (F.getValue() && (!CallerFeatureMap.lookup(F.getKey()) ||
-  

[PATCH] D158484: [PowerPC][altivec] Correct modulo number of vec_promote on vector char

[Kai Luo 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 rG6b6ea93125bd: [PowerPC][altivec] Correct modulo number of 
vec_promote on vector char (authored by lkail).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158484

Files:
  clang/lib/Headers/altivec.h
  clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c


Index: clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
===
--- clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
+++ clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
@@ -2250,18 +2250,18 @@
 
 res_vsc = vec_promote(asc[0], 8);
 // CHECK: store <16 x i8> zeroinitializer
-// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 // CHECK-LE: store <16 x i8> zeroinitializer
-// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK-LE: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 
 res_vuc = vec_promote(auc[0], 8);
 // CHECK: store <16 x i8> zeroinitializer
-// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 // CHECK-LE: store <16 x i8> zeroinitializer
-// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK-LE: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 }
 
Index: clang/lib/Headers/altivec.h
===
--- clang/lib/Headers/altivec.h
+++ clang/lib/Headers/altivec.h
@@ -14648,14 +14648,14 @@
 static __inline__ vector signed char __ATTRS_o_ai vec_promote(signed char __a,
   int __b) {
   vector signed char __res = (vector signed char)(0);
-  __res[__b & 0x7] = __a;
+  __res[__b & 0xf] = __a;
   return __res;
 }
 
 static __inline__ vector unsigned char __ATTRS_o_ai
 vec_promote(unsigned char __a, int __b) {
   vector unsigned char __res = (vector unsigned char)(0);
-  __res[__b & 0x7] = __a;
+  __res[__b & 0xf] = __a;
   return __res;
 }
 


Index: clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
===
--- clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
+++ clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
@@ -2250,18 +2250,18 @@
 
 res_vsc = vec_promote(asc[0], 8);
 // CHECK: store <16 x i8> zeroinitializer
-// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 // CHECK-LE: store <16 x i8> zeroinitializer
-// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK-LE: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 
 res_vuc = vec_promote(auc[0], 8);
 // CHECK: store <16 x i8> zeroinitializer
-// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 // CHECK-LE: store <16 x i8> zeroinitializer
-// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK-LE: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 }
 
Index: clang/lib/Headers/altivec.h
===
--- clang/lib/Headers/altivec.h
+++ clang/lib/Headers/altivec.h
@@ -14648,14 +14648,14 @@
 static __inline__ vector signed char __ATTRS_o_ai vec_promote(signed char __a,
   int __b) {
   vector signed char __res = (vector signed char)(0);
-  __res[__b & 0x7] = __a;
+  __res[__b & 0xf] = __a;
   return __res;
 }
 
 static __inline__ vector unsigned char __ATTRS_o_ai
 vec_promote(unsigned char __a, int __b) {
   vector unsigned char __res = (vector unsigned char)(0);
-  __res[__b & 0x7] = __a;
+  __res[__b & 0xf] = __a;
   return __res;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 6b6ea93 - [PowerPC][altivec] Correct modulo number of vec_promote on vector char

[Kai Luo via cfe-commits]

Author: Kai Luo
Date: 2023-08-23T01:58:36Z
New Revision: 6b6ea93125bd834cae22149e18b742d498dc79a3

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

LOG: [PowerPC][altivec] Correct modulo number of vec_promote on vector char

According to 
https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.1?topic=functions-vec-promote,
 the index should be input modulo the number of elements in the vector. When 
the type is `vector char`, the number of elements should be 16.

Reviewed By: qiucf

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

Added: 


Modified: 
clang/lib/Headers/altivec.h
clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c

Removed: 




diff  --git a/clang/lib/Headers/altivec.h b/clang/lib/Headers/altivec.h
index c036f5ebba580e..44b5a24de89f1a 100644
--- a/clang/lib/Headers/altivec.h
+++ b/clang/lib/Headers/altivec.h
@@ -14648,14 +14648,14 @@ static __inline__ void __ATTRS_o_ai vec_stvrxl(vector 
float __a, int __b,
 static __inline__ vector signed char __ATTRS_o_ai vec_promote(signed char __a,
   int __b) {
   vector signed char __res = (vector signed char)(0);
-  __res[__b & 0x7] = __a;
+  __res[__b & 0xf] = __a;
   return __res;
 }
 
 static __inline__ vector unsigned char __ATTRS_o_ai
 vec_promote(unsigned char __a, int __b) {
   vector unsigned char __res = (vector unsigned char)(0);
-  __res[__b & 0x7] = __a;
+  __res[__b & 0xf] = __a;
   return __res;
 }
 

diff  --git a/clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c 
b/clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
index 89c361454a421b..cca2a8b2f55bd4 100644
--- a/clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
+++ b/clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
@@ -2250,18 +2250,18 @@ res_vull = vec_promote(ull, 0);
 
 res_vsc = vec_promote(asc[0], 8);
 // CHECK: store <16 x i8> zeroinitializer
-// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 // CHECK-LE: store <16 x i8> zeroinitializer
-// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK-LE: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 
 res_vuc = vec_promote(auc[0], 8);
 // CHECK: store <16 x i8> zeroinitializer
-// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 // CHECK-LE: store <16 x i8> zeroinitializer
-// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 7
+// CHECK-LE: [[IDX:%.*]] = and i32 {{.*}}, 15
 // CHECK-LE: insertelement <16 x i8> {{.*}}, i8 {{.*}}, i32 [[IDX]]
 }
 



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

[PATCH] D155833: [HIP][Clang][Sema][RFC] Add Sema support for C++ Parallel Algorithm Offload

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 552568.
AlexVlx retitled this revision from "[Clang][Sema][RFC] Add Sema support for 
C++ Parallel Algorithm Offload" to "[HIP][Clang][Sema][RFC] Add Sema support 
for C++ Parallel Algorithm Offload".
AlexVlx added a comment.

Updating this to reflect the outcome of the RFC, which is that it will be added 
as a HIP extension exclusively.


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

https://reviews.llvm.org/D155833

Files:
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/SemaHipStdPar/Inputs/hipstdpar_lib.hpp
  clang/test/SemaHipStdPar/device-can-call-host.cpp

Index: clang/test/SemaHipStdPar/device-can-call-host.cpp
===
--- /dev/null
+++ clang/test/SemaHipStdPar/device-can-call-host.cpp
@@ -0,0 +1,93 @@
+// RUN: %clang_cc1 -x hip %s -hipstdpar -triple amdgcn-amd-amdhsa --std=c++17 \
+// RUN:   -fcuda-is-device -emit-llvm -o /dev/null -verify
+
+// Note: These would happen implicitly, within the implementation of the
+//   accelerator specific algorithm library, and not from user code.
+
+// Calls from the accelerator side to implicitly host (i.e. unannotated)
+// functions are fine.
+
+// expected-no-diagnostics
+
+#define __device__ __attribute__((device))
+#define __global__ __attribute__((global))
+
+extern "C" void host_fn() {}
+
+struct Dummy {};
+
+struct S {
+  S() {}
+  ~S() { host_fn(); }
+
+  int x;
+};
+
+struct T {
+  __device__ void hd() { host_fn(); }
+
+  __device__ void hd3();
+
+  void h() {}
+
+  void operator+();
+  void operator-(const T&) {}
+
+  operator Dummy() { return Dummy(); }
+};
+
+__device__ void T::hd3() { host_fn(); }
+
+template  __device__ void hd2() { host_fn(); }
+
+__global__ void kernel() { hd2(); }
+
+__device__ void hd() { host_fn(); }
+
+template  __device__ void hd3() { host_fn(); }
+__device__ void device_fn() { hd3(); }
+
+__device__ void local_var() {
+  S s;
+}
+
+__device__ void explicit_destructor(S *s) {
+  s->~S();
+}
+
+__device__ void hd_member_fn() {
+  T t;
+
+  t.hd();
+}
+
+__device__ void h_member_fn() {
+  T t;
+  t.h();
+}
+
+__device__ void unaryOp() {
+  T t;
+  (void) +t;
+}
+
+__device__ void binaryOp() {
+  T t;
+  (void) (t - t);
+}
+
+__device__ void implicitConversion() {
+  T t;
+  Dummy d = t;
+}
+
+template 
+struct TmplStruct {
+  template  __device__ void fn() {}
+};
+
+template <>
+template <>
+__device__ void TmplStruct::fn() { host_fn(); }
+
+__device__ void double_specialization() { TmplStruct().fn(); }
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -271,7 +271,8 @@
   OutputName = Names[i]->getName();
 
 TargetInfo::ConstraintInfo Info(Literal->getString(), OutputName);
-if (!Context.getTargetInfo().validateOutputConstraint(Info)) {
+if (!Context.getTargetInfo().validateOutputConstraint(Info) &&
+!(LangOpts.HIPStdPar && LangOpts.CUDAIsDevice)) {
   targetDiag(Literal->getBeginLoc(),
  diag::err_asm_invalid_output_constraint)
   << Info.getConstraintStr();
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -19109,7 +19109,7 @@
   // Diagnose ODR-use of host global variables in device functions.
   // Reference of device global variables in host functions is allowed
   // through shadow variables therefore it is not diagnosed.
-  if (SemaRef.LangOpts.CUDAIsDevice) {
+  if (SemaRef.LangOpts.CUDAIsDevice && !SemaRef.LangOpts.HIPStdPar) {
 SemaRef.targetDiag(Loc, diag::err_ref_bad_target)
 << /*host*/ 2 << /*variable*/ 1 << Var << UserTarget;
 SemaRef.targetDiag(Var->getLocation(),
Index: clang/lib/Sema/SemaCUDA.cpp
===
--- clang/lib/Sema/SemaCUDA.cpp
+++ clang/lib/Sema/SemaCUDA.cpp
@@ -231,6 +231,15 @@
   (CallerTarget == CFT_Global && CalleeTarget == CFT_Device))
 return CFP_Native;
 
+  // HipStdPar mode is special, in that assessing whether a device side call to
+  // a host target is deferred to a subsequent pass, and cannot unambiguously be
+  // adjudicated in the AST, hence we optimistically allow them to pass here.
+  if (getLangOpts().HIPStdPar &&
+  (CallerTarget == CFT_Global || CallerTarget == CFT_Device ||
+   CallerTarget == CFT_HostDevice) &&
+  CalleeTarget == CFT_Host)
+return CFP_HostDevice;
+
   // (d) HostDevice behavior depends on compilation mode.
   if (CallerTarget == CFT_HostDevice) {
 // It's OK to call a compilation-mode matching function from an HD one.
@@ -877,7 +886,7 @@
   if (!ShouldCheck || !Capture.isReferenceCapture())
 return;
   auto DiagKind = 

[PATCH] D158469: [clang][deps] Compute command lines and file deps on-demand

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 552566.
jansvoboda11 added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158469

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -351,14 +351,24 @@
   }
 
   void mergeDeps(ModuleDepsGraph Graph, size_t InputIndex) {
-std::unique_lock ul(Lock);
-for (const ModuleDeps  : Graph) {
-  auto I = Modules.find({MD.ID, 0});
-  if (I != Modules.end()) {
-I->first.InputIndex = std::min(I->first.InputIndex, InputIndex);
-continue;
+std::vector NewMDs;
+{
+  std::unique_lock ul(Lock);
+  for (const ModuleDeps  : Graph.MDs) {
+auto I = Modules.find({MD.ID, 0});
+if (I != Modules.end()) {
+  I->first.InputIndex = std::min(I->first.InputIndex, InputIndex);
+  continue;
+}
+auto NewIt = Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
+NewMDs.push_back(>second);
   }
-  Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
+}
+// Eagerly compute the lazy members before the graph goes out of scope.
+// This is somewhat costly, so do it outside the critical section.
+for (ModuleDeps *MD : NewMDs) {
+  (void)MD->getBuildArguments();
+  (void)MD->getFileDeps();
 }
   }
 
@@ -382,7 +392,7 @@
 /*ShouldOwnClient=*/false);
 
 for (auto & : Modules)
-  if (roundTripCommand(M.second.BuildArguments, *Diags))
+  if (roundTripCommand(M.second.getBuildArguments(), *Diags))
 return true;
 
 for (auto & : Inputs)
@@ -408,10 +418,10 @@
   Object O{
   {"name", MD.ID.ModuleName},
   {"context-hash", MD.ID.ContextHash},
-  {"file-deps", toJSONSorted(MD.FileDeps)},
+  {"file-deps", toJSONSorted(MD.getFileDeps())},
   {"clang-module-deps", toJSONSorted(MD.ClangModuleDeps)},
   {"clang-modulemap-file", MD.ClangModuleMapFile},
-  {"command-line", MD.BuildArguments},
+  {"command-line", MD.getBuildArguments()},
   };
   OutModules.push_back(std::move(O));
 }
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -467,20 +467,6 @@
   serialization::ModuleFile *MF =
   MDC.ScanInstance.getASTReader()->getModuleManager().lookup(
   M->getASTFile());
-  MDC.ScanInstance.getASTReader()->visitInputFileInfos(
-  *MF, /*IncludeSystem=*/true,
-  [&](const serialization::InputFileInfo , bool IsSystem) {
-// __inferred_module.map is the result of the way in which an implicit
-// module build handles inferred modules. It adds an overlay VFS with
-// this file in the proper directory and relies on the rest of Clang to
-// handle it like normal. With explicitly built modules we don't need
-// to play VFS tricks, so replace it with the correct module map.
-if (StringRef(IFI.Filename).endswith("__inferred_module.map")) {
-  MDC.addFileDep(MD, ModuleMap->getName());
-  return;
-}
-MDC.addFileDep(MD, IFI.Filename);
-  });
 
   llvm::DenseSet SeenDeps;
   addAllSubmodulePrebuiltDeps(M, MD, SeenDeps);
@@ -510,7 +496,9 @@
   // Finish the compiler invocation. Requires dependencies and the context hash.
   MDC.addOutputPaths(CI, MD);
 
-  MD.BuildArguments = CI.getCC1CommandLine();
+  // Wire up lazy info computation.
+  MD.MDC = 
+  MDC.LazyModuleDepsInfoByID.insert({MD.ID, {MF, std::move(CI)}});
 
   return MD.ID;
 }
@@ -643,5 +631,50 @@
 void ModuleDepCollector::addFileDep(ModuleDeps , StringRef Path) {
   llvm::SmallString<256> Storage;
   Path = makeAbsoluteAndPreferred(ScanInstance, Path, Storage);
-  MD.FileDeps.insert(Path);
+  MD.FileDeps->insert(Path);
+}
+
+void ModuleDepCollector::addFileDeps(ModuleDeps ) {
+  auto It = LazyModuleDepsInfoByID.find(MD.ID);
+  assert(It != LazyModuleDepsInfoByID.end());
+
+  MD.FileDeps = llvm::StringSet<>{};
+
+  ScanInstance.getASTReader()->visitInputFileInfos(
+  *It->second.MF, /*IncludeSystem=*/true,
+  [&](const 

[PATCH] D158346: [clang-tidy] readability-container-size-empty - detect missing usage of .empty() on string_literals

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp:26
+namespace string_literals{
+string operator""s(const char *, size_t);
+}

I discovered that this test started to fail when I landed my revert 
(0d9919d362a7a70b2a7970861d897ecc47ec9e4d) of 
f2583f3acf596cc545c8c0e3cb28e712f4ebf21b. I "fixed" the test by changing this 
to `operator""_s` in ba52a10fca6fc7b791894c584233db012def68a5, but I'm not sure 
if that changes the meaning of the test. Please review that or follow up on 
this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158346

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

[PATCH] D158573: [clang][modules] Move `UNHASHED_CONTROL_BLOCK` up in the AST file

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added a reviewer: benlangmuir.
Herald added subscribers: ributzka, arphaman.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When loading (transitively) imported AST file, `ModuleManager::addModule()` 
first checks it has the expected signature via `readASTFileSignature()`. The 
signature is part of `UNHASHED_CONTROL_BLOCK`, which is placed at the end of 
the AST file. This means that just to verify signature of an AST file, we need 
to skip over all top-level blocks, paging in the whole AST file from disk. This 
is pretty slow.

This patch moves `UNHASHED_CONTROL_BLOCK` to the start of the AST file, so that 
it can be read more efficiently. To achieve this, we use dummy signature when 
first emitting the unhashed control block, and then backpatch the real 
signature at the end of the serialization process.

This speeds up dependency scanning by over 9% and significantly reduces 
run-to-run variability of my benchmarks.

Depends on D158572 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158573

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/test/Modules/ASTSignature.c

Index: clang/test/Modules/ASTSignature.c
===
--- clang/test/Modules/ASTSignature.c
+++ clang/test/Modules/ASTSignature.c
@@ -1,6 +1,6 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -iquote %S/Inputs/ASTHash/ -fsyntax-only -fmodules \
-// RUN:   -fimplicit-module-maps -fmodules-strict-context-hash \
+// RUN: %clang_cc1 -iquote %S/Inputs/ASTHash/ -fsyntax-only \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-strict-context-hash \
 // RUN:   -fmodules-cache-path=%t -fdisable-module-hash %s
 // RUN: cp %t/MyHeader2.pcm %t1.pcm
 // RUN: rm -rf %t
@@ -8,17 +8,18 @@
 // RUN:   -fmodules -fimplicit-module-maps -fmodules-strict-context-hash \
 // RUN:   -fmodules-cache-path=%t -fdisable-module-hash %s
 // RUN: cp %t/MyHeader2.pcm %t2.pcm
-// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm > %t1.dump
-// RUN: llvm-bcanalyzer --dump --disable-histogram %t2.pcm > %t2.dump
+// RUN: llvm-bcanalyzer --dump --disable-histogram --show-binary-blobs %t1.pcm > %t1.dump
+// RUN: llvm-bcanalyzer --dump --disable-histogram --show-binary-blobs %t2.pcm > %t2.dump
 // RUN: cat %t1.dump %t2.dump | FileCheck %s
 
 #include "my_header_2.h"
 
 my_int var = 42;
 
-// CHECK: [[AST_BLOCK_HASH:]]
-// CHECK: [[SIGNATURE:]]
-// CHECK: [[AST_BLOCK_HASH]]
-// CHECK-NOT: [[SIGNATURE]]
-// The modules built by this test are designed to yield the same AST. If this
-// test fails, it means that the AST block is has become non-relocatable.
+// CHECK:  blob data = '[[AST_BLOCK_HASH:.*]]'
+// CHECK:  blob data = '[[SIGNATURE:.*]]'
+// CHECK:  blob data = '[[AST_BLOCK_HASH]]'
+// CHECK-NOT:  blob data = '[[SIGNATURE]]'
+// The modules built by this test are designed to yield the same AST but distinct AST files.
+// If this test fails, it means that either the AST block has become non-relocatable,
+// or the file signature stopped hashing some parts of the AST file.
Index: clang/lib/Serialization/GlobalModuleIndex.cpp
===
--- clang/lib/Serialization/GlobalModuleIndex.cpp
+++ clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -15,6 +15,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ModuleFile.h"
 #include "clang/Serialization/PCHContainerOperations.h"
 #include "llvm/ADT/DenseMap.h"
@@ -698,8 +699,7 @@
 
 // Get Signature.
 if (State == DiagnosticOptionsBlock && Code == SIGNATURE)
-  getModuleFileInfo(File).Signature = ASTFileSignature::create(
-  Record.begin(), Record.begin() + ASTFileSignature::size);
+  getModuleFileInfo(File).Signature = ASTReader::readSignature(Blob.data());
 
 // We don't care about this record.
   }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1120,50 +1120,117 @@
 }
 
 std::pair
-ASTWriter::createSignature(StringRef AllBytes, StringRef ASTBlockBytes) {
+ASTWriter::createSignature() const {
+  StringRef AllBytes(Buffer.data(), Buffer.size());
+
   llvm::SHA1 Hasher;
-  Hasher.update(ASTBlockBytes);
+  Hasher.update(AllBytes.slice(ASTBlockRange.first, ASTBlockRange.second));
   ASTFileSignature 

[PATCH] D155826: [HIP][Clang][Preprocessor][RFC] Add preprocessor support for C++ Parallel Algorithm Offload

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 552561.
AlexVlx retitled this revision from "[Clang][Preprocessor][RFC] Add 
preprocessor support for C++ Parallel Algorithm Offload" to 
"[HIP][Clang][Preprocessor][RFC] Add preprocessor support for C++ Parallel 
Algorithm Offload".
AlexVlx added a comment.

Updating this to reflect the outcome of the RFC, which is that this will be 
added as a HIP extension exclusively.


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

https://reviews.llvm.org/D155826

Files:
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Preprocessor/predefined-macros.c


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -290,3 +290,19 @@
 // RUN:   -fcuda-is-device -fgpu-default-stream=per-thread \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-PTH
 // CHECK-PTH: #define HIP_API_PER_THREAD_DEFAULT_STREAM 1
+
+// RUN: %clang_cc1 %s -E -dM -o - -x hip -hipstdpar -triple 
x86_64-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-HIPSTDPAR
+// CHECK-HIPSTDPAR: #define __HIPSTDPAR__ 1
+
+// RUN: %clang_cc1 %s -E -dM -o - -x hip -hipstdpar -hipstdpar-interpose-alloc 
\
+// RUN:  -triple x86_64-unknown-linux-gnu | FileCheck -match-full-lines %s \
+// RUN:  --check-prefix=CHECK-HIPSTDPAR-INTERPOSE
+// CHECK-HIPSTDPAR-INTERPOSE: #define __HIPSTDPAR_INTERPOSE_ALLOC__ 1
+// CHECK-HIPSTDPAR-INTERPOSE: #define __HIPSTDPAR__ 1
+
+// RUN: %clang_cc1 %s -E -dM -o - -x hip -hipstdpar -hipstdpar-interpose-alloc 
\
+// RUN:  -triple amdgcn-amd-amdhsa -fcuda-is-device | FileCheck 
-match-full-lines \
+// RUN:  %s --check-prefix=CHECK-HIPSTDPAR-INTERPOSE-DEV-NEG
+// CHECK-HIPSTDPAR-INTERPOSE-DEV-NEG: #define __HIPSTDPAR__ 1
+// CHECK-HIPSTDPAR-INTERPOSE-DEV-NEG-NOT: #define 
__HIPSTDPAR_INTERPOSE_ALLOC__ 1
\ No newline at end of file
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -585,6 +585,11 @@
 Builder.defineMacro("__HIP_MEMORY_SCOPE_WORKGROUP", "3");
 Builder.defineMacro("__HIP_MEMORY_SCOPE_AGENT", "4");
 Builder.defineMacro("__HIP_MEMORY_SCOPE_SYSTEM", "5");
+if (LangOpts.HIPStdPar) {
+  Builder.defineMacro("__HIPSTDPAR__");
+  if (!LangOpts.CUDAIsDevice)
+Builder.defineMacro("__HIPSTDPAR_INTERPOSE_ALLOC__");
+}
 if (LangOpts.CUDAIsDevice) {
   Builder.defineMacro("__HIP_DEVICE_COMPILE__");
   if (!TI.hasHIPImageSupport()) {


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -290,3 +290,19 @@
 // RUN:   -fcuda-is-device -fgpu-default-stream=per-thread \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-PTH
 // CHECK-PTH: #define HIP_API_PER_THREAD_DEFAULT_STREAM 1
+
+// RUN: %clang_cc1 %s -E -dM -o - -x hip -hipstdpar -triple x86_64-unknown-linux-gnu \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-HIPSTDPAR
+// CHECK-HIPSTDPAR: #define __HIPSTDPAR__ 1
+
+// RUN: %clang_cc1 %s -E -dM -o - -x hip -hipstdpar -hipstdpar-interpose-alloc \
+// RUN:  -triple x86_64-unknown-linux-gnu | FileCheck -match-full-lines %s \
+// RUN:  --check-prefix=CHECK-HIPSTDPAR-INTERPOSE
+// CHECK-HIPSTDPAR-INTERPOSE: #define __HIPSTDPAR_INTERPOSE_ALLOC__ 1
+// CHECK-HIPSTDPAR-INTERPOSE: #define __HIPSTDPAR__ 1
+
+// RUN: %clang_cc1 %s -E -dM -o - -x hip -hipstdpar -hipstdpar-interpose-alloc \
+// RUN:  -triple amdgcn-amd-amdhsa -fcuda-is-device | FileCheck -match-full-lines \
+// RUN:  %s --check-prefix=CHECK-HIPSTDPAR-INTERPOSE-DEV-NEG
+// CHECK-HIPSTDPAR-INTERPOSE-DEV-NEG: #define __HIPSTDPAR__ 1
+// CHECK-HIPSTDPAR-INTERPOSE-DEV-NEG-NOT: #define __HIPSTDPAR_INTERPOSE_ALLOC__ 1
\ No newline at end of file
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -585,6 +585,11 @@
 Builder.defineMacro("__HIP_MEMORY_SCOPE_WORKGROUP", "3");
 Builder.defineMacro("__HIP_MEMORY_SCOPE_AGENT", "4");
 Builder.defineMacro("__HIP_MEMORY_SCOPE_SYSTEM", "5");
+if (LangOpts.HIPStdPar) {
+  Builder.defineMacro("__HIPSTDPAR__");
+  if (!LangOpts.CUDAIsDevice)
+Builder.defineMacro("__HIPSTDPAR_INTERPOSE_ALLOC__");
+}
 if (LangOpts.CUDAIsDevice) {
   Builder.defineMacro("__HIP_DEVICE_COMPILE__");
   if (!TI.hasHIPImageSupport()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] ba52a10 - [clang-tidy] Fix test to not depend on D153156, which was reverted

[Reid Kleckner via cfe-commits]

Author: Reid Kleckner
Date: 2023-08-22T18:39:19-07:00
New Revision: ba52a10fca6fc7b791894c584233db012def68a5

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

LOG: [clang-tidy] Fix test to not depend on D153156, which was reverted

This is a quick fix forward to get bots green again.

Added: 


Modified: 

clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Removed: 




diff  --git 
a/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
 
b/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
index 592ffc21f41a4f..a7e4977e767455 100644
--- 
a/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -23,7 +23,7 @@ template  struct set {
 }
 
 namespace string_literals{
-string operator""s(const char *, size_t);
+string operator""_s(const char *, size_t);
 }
 
 }
@@ -778,7 +778,7 @@ bool testIgnoredDummyType(const IgnoredDummyType& value) {
 bool testStringLiterals(const std::string& s)
 {
   using namespace std::string_literals;
-  return s == ""s;
+  return s == ""_s;
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be 
used
   // CHECK-FIXES: {{^  }}return s.empty()
 }
@@ -786,5 +786,5 @@ bool testStringLiterals(const std::string& s)
 bool testNotEmptyStringLiterals(const std::string& s)
 {
   using namespace std::string_literals;
-  return s == "foo"s;
+  return s == "foo"_s;
 }



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

[PATCH] D146773: [-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 552560.
NoQ added a comment.

Rebase.

Make warnings a bit more verbose than before. This plays nicely with our 
attempts to categorize unemitted fixits via `-mllvm -debug-only=SafeBuffers`, 
but this time it doesn't make sense to make the extra information debug-only; 
it makes perfect sense as part of the warning.

TODO: Some new code paths aren't covered by tests yet (captured variables, 
static member variables).


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

https://reviews.llvm.org/D146773

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-c-linkage.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-main.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -41,34 +41,34 @@
 // expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
   foo(p[1], // expected-note{{used in buffer access here}}
   pp[1][1], // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer parameter variable 'pp'}}
   1[1[pp]], // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer parameter variable 'pp'}}
   1[pp][1]  // expected-note{{used in buffer access here}}
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access through raw pointer parameter variable 'pp'}}
   );
 
   if (p[3]) {   // expected-note{{used in buffer access here}}
 void * q = p;
 
-foo(((int*)q)[10]); // expected-warning{{unsafe buffer access}}
+foo(((int*)q)[10]); // expected-warning{{unsafe buffer access through raw pointer expression}}
   }
 
-  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access}}
-  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access}}
-  charPtrCall()[3], // expected-warning{{unsafe buffer access}}
-  3[charPtrCall()]  // expected-warning{{unsafe buffer access}}
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access through raw pointer expression}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access through raw pointer expression}}
+  charPtrCall()[3], // expected-warning{{unsafe buffer access through raw pointer return value of function 'charPtrCall'}}
+  3[charPtrCall()]  // expected-warning{{unsafe buffer access through raw pointer return value of function 'charPtrCall'}}
   );
 
 int a[10];  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
 int b[10][10];  // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
   foo(a[1], 1[a],   // expected-note2{{used in buffer access here}}
-  b[3][4],  // expected-warning{{unsafe buffer access}}
+  b[3][4],  // expected-warning{{unsafe buffer access into raw array local variable 'b'}}
 // expected-note@-1{{used in buffer access here}}
-  4[b][3],  // expected-warning{{unsafe buffer access}}
+  4[b][3],  // expected-warning{{unsafe buffer access into raw array local variable 'b'}}
 // expected-note@-1{{used in buffer access here}}
-  4[3[b]]); // expected-warning{{unsafe buffer access}}
+  4[3[b]]); // expected-warning{{unsafe buffer access into raw array local variable 'b'}}
 // expected-note@-1{{used in buffer access here}}
 
   // Not to warn when index is zero
@@ -108,7 +108,7 @@
   q[1], 1[q], q[-1],// expected-note3{{used in buffer access here}}
   a[1], // expected-note{{used in buffer access here}} `a` is of pointer type
   b[1][2]   // expected-note{{used in buffer access here}} `b[1]` is of array type
-// expected-warning@-1{{unsafe buffer access}}
+// expected-warning@-1{{unsafe buffer access into raw array parameter 

[PATCH] D158523: [clang][doc] Mentions -Wno-reserved-module-identifiers

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158523

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

[PATCH] D158572: [clang][modules] Use relative offsets for input files

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added a reviewer: benlangmuir.
Herald added a subscriber: ributzka.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch replaces absolute offsets into the input files block with offsets 
relative to the block start. This makes the whole section "relocatable". I 
confirmed all other uses of `GetCurrentBitNo()` are turned into relative 
offsets before being serialized into the AST file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158572

Files:
  clang/include/clang/Serialization/ModuleFile.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1570,6 +1570,8 @@
   IFHAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
   unsigned IFHAbbrevCode = Stream.EmitAbbrev(std::move(IFHAbbrev));
 
+  uint64_t InputFilesOffsetBase = Stream.GetCurrentBitNo();
+
   // Get all ContentCache objects for files.
   std::vector UserFiles;
   std::vector SystemFiles;
@@ -1633,7 +1635,7 @@
   continue; // already recorded this file.
 
 // Record this entry's offset.
-InputFileOffsets.push_back(Stream.GetCurrentBitNo());
+InputFileOffsets.push_back(Stream.GetCurrentBitNo() - 
InputFilesOffsetBase);
 
 InputFileID = InputFileOffsets.size();
 
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2326,7 +2326,8 @@
   // Go find this input file.
   BitstreamCursor  = F.InputFilesCursor;
   SavedStreamPosition SavedPosition(Cursor);
-  if (llvm::Error Err = Cursor.JumpToBit(F.InputFileOffsets[ID - 1])) {
+  if (llvm::Error Err = Cursor.JumpToBit(F.InputFilesOffsetBase +
+ F.InputFileOffsets[ID - 1])) {
 // FIXME this drops errors on the floor.
 consumeError(std::move(Err));
   }
@@ -2410,7 +2411,8 @@
   // Go find this input file.
   BitstreamCursor  = F.InputFilesCursor;
   SavedStreamPosition SavedPosition(Cursor);
-  if (llvm::Error Err = Cursor.JumpToBit(F.InputFileOffsets[ID - 1])) {
+  if (llvm::Error Err = Cursor.JumpToBit(F.InputFilesOffsetBase +
+ F.InputFileOffsets[ID - 1])) {
 // FIXME this drops errors on the floor.
 consumeError(std::move(Err));
   }
@@ -2788,6 +2790,7 @@
   Error("malformed block record in AST file");
   return Failure;
 }
+F.InputFilesOffsetBase = F.InputFilesCursor.GetCurrentBitNo();
 continue;
 
   case OPTIONS_BLOCK_ID:
@@ -5328,6 +5331,7 @@
   bool NeedsSystemInputFiles = Listener.needsSystemInputFileVisitation();
   bool NeedsImports = Listener.needsImportVisitation();
   BitstreamCursor InputFilesCursor;
+  uint64_t InputFilesOffsetBase;
 
   RecordData Record;
   std::string ModuleDir;
@@ -5363,6 +5367,7 @@
 if (NeedsInputFiles &&
 ReadBlockAbbrevs(InputFilesCursor, INPUT_FILES_BLOCK_ID))
   return true;
+InputFilesOffsetBase = InputFilesCursor.GetCurrentBitNo();
 break;
 
   default:
@@ -5435,7 +5440,8 @@
 
 BitstreamCursor  = InputFilesCursor;
 SavedStreamPosition SavedPosition(Cursor);
-if (llvm::Error Err = Cursor.JumpToBit(InputFileOffs[I])) {
+if (llvm::Error Err =
+Cursor.JumpToBit(InputFilesOffsetBase + InputFileOffs[I])) {
   // FIXME this drops errors on the floor.
   consumeError(std::move(Err));
 }
Index: clang/include/clang/Serialization/ModuleFile.h
===
--- clang/include/clang/Serialization/ModuleFile.h
+++ clang/include/clang/Serialization/ModuleFile.h
@@ -245,7 +245,10 @@
   /// The cursor to the start of the input-files block.
   llvm::BitstreamCursor InputFilesCursor;
 
-  /// Offsets for all of the input file entries in the AST file.
+  /// Absolute offset of the start of the input-files block.
+  uint64_t InputFilesOffsetBase;
+
+  /// Relative offsets for all of the input file entries in the AST file.
   const llvm::support::unaligned_uint64_t *InputFileOffsets = nullptr;
 
   /// The input files that have been loaded from this AST file.


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1570,6 +1570,8 @@
   IFHAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
   unsigned IFHAbbrevCode = Stream.EmitAbbrev(std::move(IFHAbbrev));
 
+  uint64_t InputFilesOffsetBase = Stream.GetCurrentBitNo();
+
   // Get all ContentCache objects 

[PATCH] D155775: [HIP][Clang][Driver][RFC] Add driver support for C++ Parallel Algorithm Offload

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 552556.
AlexVlx retitled this revision from "[Clang][Driver][RFC] Add driver support 
for C++ Parallel Algorithm Offload" to "[HIP][Clang][Driver][RFC] Add driver 
support for C++ Parallel Algorithm Offload".
AlexVlx added a comment.

Updating this to reflect the outcome of the RFC, which is that this will be 
added as a HIP extension exclusively.


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

https://reviews.llvm.org/D155775

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/HIPAMD.cpp
  clang/lib/Driver/ToolChains/ROCm.h
  clang/test/Driver/Inputs/hipstdpar/hipstdpar_lib.hpp
  clang/test/Driver/hipstdpar.c

Index: clang/test/Driver/hipstdpar.c
===
--- /dev/null
+++ clang/test/Driver/hipstdpar.c
@@ -0,0 +1,17 @@
+// RUN: not %clang -### -hipstdpar --compile %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=HIPSTDPAR-MISSING-LIB %s
+// RUN: %clang -### --hipstdpar --hipstdpar-path=%S/Inputs/hipstdpar \
+// RUN:   --hipstdpar-thrust-path=%S/Inputs/hipstdpar/thrust \
+// RUN:   --hipstdpar-prim-path=%S/Inputs/hipstdpar/rocprim --compile %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=HIPSTDPAR-COMPILE %s
+// RUN: touch %t.o
+// RUN: %clang -### -hipstdpar %t.o 2>&1 | FileCheck --check-prefix=HIPSTDPAR-LINK %s
+
+// HIPSTDPAR-MISSING-LIB: error: cannot find HIP Standard Parallelism Acceleration library; provide it via '--hipstdpar-path'
+// HIPSTDPAR-COMPILE: "-x" "hip"
+// HIPSTDPAR-COMPILE: "-idirafter" "{{.*/thrust}}"
+// HIPSTDPAR-COMPILE: "-idirafter" "{{.*/rocprim}}"
+// HIPSTDPAR-COMPILE: "-idirafter" "{{.*/Inputs/hipstdpar}}"
+// HIPSTDPAR-COMPILE: "-include" "hipstdpar_lib.hpp"
+// HIPSTDPAR-LINK: "-rpath"
+// HIPSTDPAR-LINK: "-l{{.*hip.*}}"
Index: clang/lib/Driver/ToolChains/ROCm.h
===
--- clang/lib/Driver/ToolChains/ROCm.h
+++ clang/lib/Driver/ToolChains/ROCm.h
@@ -77,6 +77,9 @@
   const Driver 
   bool HasHIPRuntime = false;
   bool HasDeviceLibrary = false;
+  bool HasHIPStdParLibrary = false;
+  bool HasRocThrustLibrary = false;
+  bool HasRocPrimLibrary = false;
 
   // Default version if not detected or specified.
   const unsigned DefaultVersionMajor = 3;
@@ -96,6 +99,13 @@
   std::vector RocmDeviceLibPathArg;
   // HIP runtime path specified by --hip-path.
   StringRef HIPPathArg;
+  // HIP Standard Parallel Algorithm acceleration library specified by
+  // --hipstdpar-path
+  StringRef HIPStdParPathArg;
+  // rocThrust algorithm library specified by --hipstdpar-thrust-path
+  StringRef HIPRocThrustPathArg;
+  // rocPrim algorithm library specified by --hipstdpar-prim-path
+  StringRef HIPRocPrimPathArg;
   // HIP version specified by --hip-version.
   StringRef HIPVersionArg;
   // Wheter -nogpulib is specified.
@@ -180,6 +190,9 @@
   /// Check whether we detected a valid ROCm device library.
   bool hasDeviceLibrary() const { return HasDeviceLibrary; }
 
+  /// Check whether we detected a valid HIP STDPAR Acceleration library.
+  bool hasHIPStdParLibrary() const { return HasHIPStdParLibrary; }
+
   /// Print information about the detected ROCm installation.
   void print(raw_ostream ) const;
 
Index: clang/lib/Driver/ToolChains/HIPAMD.cpp
===
--- clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -115,6 +115,8 @@
 "--no-undefined",
 "-shared",
 "-plugin-opt=-amdgpu-internalize-symbols"};
+  if (Args.hasArg(options::OPT_hipstdpar))
+LldArgs.push_back("-plugin-opt=-amdgpu-enable-hipstdpar");
 
   auto  = getToolChain();
   auto  = TC.getDriver();
@@ -246,6 +248,8 @@
   if (!DriverArgs.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc,
   false))
 CC1Args.append({"-mllvm", "-amdgpu-internalize-symbols"});
+  if (DriverArgs.hasArgNoClaim(options::OPT_hipstdpar))
+CC1Args.append({"-mllvm", "-amdgpu-enable-hipstdpar"});
 
   StringRef MaxThreadsPerBlock =
   DriverArgs.getLastArgValue(options::OPT_gpu_max_threads_per_block_EQ);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -6552,6 +6552,12 @@
 if (Args.hasFlag(options::OPT_fgpu_allow_device_init,
  options::OPT_fno_gpu_allow_device_init, false))
   CmdArgs.push_back("-fgpu-allow-device-init");
+if (Args.hasArg(options::OPT_hipstdpar)) {
+  CmdArgs.push_back("-hipstdpar");
+
+  if 

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I went ahead and pushed a revert of this, since this change was pretty 
disruptive, at least in our experience. I could be convinced that it's worth 
sunsetting this extension for accepting C-style format string macro code, but 
that doesn't seem like the original intent of this change. Maybe that's the 
right thing to do, but it should be an intentionally considered change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D148381: [WIP][Clang] Add counted_by attribute

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 552554.
void added a comment.

Fix clang-ast-dump issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -56,6 +56,7 @@
 // CHECK-NEXT: ConsumableAutoCast (SubjectMatchRule_record)
 // CHECK-NEXT: ConsumableSetOnRead (SubjectMatchRule_record)
 // CHECK-NEXT: Convergent (SubjectMatchRule_function)
+// CHECK-NEXT: CountedBy (SubjectMatchRule_field)
 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: Destructor (SubjectMatchRule_function)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8380,6 +8380,21 @@
   D->addAttr(ZeroCallUsedRegsAttr::Create(S.Context, Kind, AL));
 }
 
+static void handleCountedByAttr(Sema , Decl *D, const ParsedAttr ) {
+  // TODO: Probably needs more processing here. See Sema::AddAlignValueAttr.
+  if (!AL.isArgIdent(0)) {
+S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
+<< AL << AANT_ArgumentIdentifier;
+return;
+  }
+
+  IdentifierLoc *IL = AL.getArgAsIdent(0);
+  CountedByAttr *CBA =
+  ::new (S.Context) CountedByAttr(S.Context, AL, IL->Ident);
+  CBA->setCountedByFieldLoc(IL->Loc);
+  D->addAttr(CBA);
+}
+
 static void handleFunctionReturnThunksAttr(Sema , Decl *D,
const ParsedAttr ) {
   StringRef KindStr;
@@ -9326,6 +9341,10 @@
 handleAvailableOnlyInDefaultEvalMethod(S, D, AL);
 break;
 
+  case ParsedAttr::AT_CountedBy:
+handleCountedByAttr(S, D, AL);
+break;
+
   // Microsoft attributes:
   case ParsedAttr::AT_LayoutVersion:
 handleLayoutVersion(S, D, AL);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -17904,6 +17904,40 @@
  "Broken injected-class-name");
 }
 
+static const FieldDecl *FindFieldWithCountedByAttr(const RecordDecl *RD) {
+  for (const Decl *D : RD->decls()) {
+if (const auto *FD = dyn_cast(D)) {
+  if (FD->hasAttr())
+return FD;
+}
+
+if (const auto *SubRD = dyn_cast(D)) {
+  return FindFieldWithCountedByAttr(SubRD);
+}
+  }
+
+  return nullptr;
+}
+
+/// CheckCountedByAttr - Return an \p IdentifierInfo if the attribute field
+/// doesn't exist in the enclosing structure. Returns \p nullptr if the
+/// attribute field does exist.
+static const IdentifierInfo *CheckCountedByAttr(const RecordDecl *RD,
+const FieldDecl *FD,
+SourceRange ) {
+  const CountedByAttr *ECA = FD->getAttr();
+
+  auto It = llvm::find_if(RD->fields(), [&](const FieldDecl *Field) {
+return Field->getName() == ECA->getCountedByField()->getName();
+  });
+  if (It == RD->field_end()) {
+Loc = ECA->getCountedByFieldLoc();
+return ECA->getCountedByField();
+  }
+
+  return nullptr;
+}
+
 void Sema::ActOnTagFinishDefinition(Scope *S, Decl *TagD,
 SourceRange BraceRange) {
   AdjustDeclIfTemplate(TagD);
@@ -17961,6 +17995,16 @@
  [](const FieldDecl *FD) { return FD->isBitField(); }))
   Diag(BraceRange.getBegin(), diag::warn_pragma_align_not_xl_compatible);
   }
+
+  // Check the "counted_by" attribute to ensure that the count field exists in
+  // the struct.
+  if (const RecordDecl *RD = dyn_cast(Tag)) {
+if (const FieldDecl *FD = FindFieldWithCountedByAttr(RD)) {
+  SourceRange SR;
+  if (const IdentifierInfo *Unknown = CheckCountedByAttr(RD, FD, SR))
+Diag(SR.getBegin(), diag::warn_counted_by_placeholder) << Unknown << SR;
+}
+  }
 }
 
 void Sema::ActOnObjCContainerFinishDefinition() {
Index: clang/lib/CodeGen/CodeGenFunction.h

[clang] 0d9919d - Revert "[Clang] CWG1473: do not err on the lack of space after operator"""

[Reid Kleckner via cfe-commits]

Author: Reid Kleckner
Date: 2023-08-22T18:10:41-07:00
New Revision: 0d9919d362a7a70b2a7970861d897ecc47ec9e4d

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

LOG: Revert "[Clang] CWG1473: do not err on the lack of space after operator"""

This reverts commit f2583f3acf596cc545c8c0e3cb28e712f4ebf21b.

There is a large body of non-conforming C-like code using format strings
like this:

  #define PRIuS "zu"
  void h(size_t foo, size_t bar) {
printf("foo is %"PRIuS", bar is %"PRIuS, foo, bar);
  }

Rejecting this code would be very disruptive. We could decide to do
that, but it's sufficiently disruptive that I think it requires
gathering more community consensus with an RFC, and Aaron indicated [1]
it's OK to revert for now so continuous testing systems can see past
this issue while we decide what to do.

[1] https://reviews.llvm.org/D153156#4607717

Added: 


Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Lex/Lexer.cpp
clang/test/CXX/drs/dr14xx.cpp
clang/test/CXX/drs/dr17xx.cpp
clang/test/CXX/drs/dr25xx.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p1.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p10.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p11.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p3.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p4.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p5.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p6.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p7.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p8.cpp
clang/test/CXX/lex/lex.literal/lex.ext/p9.cpp
clang/test/CXX/over/over.oper/over.literal/p2.cpp
clang/test/CXX/over/over.oper/over.literal/p3.cpp
clang/test/CXX/over/over.oper/over.literal/p5.cpp
clang/test/CXX/over/over.oper/over.literal/p6.cpp
clang/test/CXX/over/over.oper/over.literal/p7.cpp
clang/test/CXX/over/over.oper/over.literal/p8.cpp
clang/test/CodeGenCXX/cxx11-user-defined-literal.cpp
clang/test/CodeGenCXX/mangle-ms-cxx11.cpp
clang/test/FixIt/fixit-c++11.cpp
clang/test/OpenMP/amdgcn_ldbl_check.cpp
clang/test/PCH/cxx11-user-defined-literals.cpp
clang/test/Parser/cxx0x-literal-operators.cpp
clang/test/Parser/cxx11-user-defined-literals.cpp
clang/test/SemaCXX/cxx11-ast-print.cpp
clang/test/SemaCXX/cxx11-user-defined-literals-unused.cpp
clang/test/SemaCXX/cxx11-user-defined-literals.cpp
clang/test/SemaCXX/cxx1y-user-defined-literals.cpp
clang/test/SemaCXX/cxx1z-user-defined-literals.cpp
clang/test/SemaCXX/cxx2a-consteval.cpp
clang/test/SemaCXX/cxx98-compat.cpp
clang/test/SemaCXX/literal-operators.cpp
clang/test/SemaCXX/no-warn-user-defined-literals-in-system-headers.cpp
clang/test/SemaCXX/reserved-identifier.cpp
clang/test/SemaCXX/warn-xor-as-pow.cpp
clang/www/cxx_dr_status.html

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bed02c4555b03e..157f57d73030ce 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -94,10 +94,6 @@ C++2c Feature Support
 
 Resolutions to C++ Defect Reports
 ^
-- Implemented `CWG1473 `_ which allows spaces after 
``operator""``.
-  Clang used to err on the lack of space when the literal suffix identifier 
was invalid in
-  all the language modes, which contradicted the deprecation of the 
whitespaces.
-  Also turn on ``-Wdeprecated-literal-operator`` by default in all the 
language modes.
 
 C Language Changes
 --

diff  --git a/clang/include/clang/Basic/DiagnosticLexKinds.td 
b/clang/include/clang/Basic/DiagnosticLexKinds.td
index c5da98c4f0001e..940cca67368492 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -276,6 +276,12 @@ def warn_cxx11_compat_reserved_user_defined_literal : 
Warning<
   "identifier after literal will be treated as a reserved user-defined literal 
"
   "suffix in C++11">,
   InGroup, DefaultIgnore;
+def ext_reserved_user_defined_literal : ExtWarn<
+  "invalid suffix on literal; C++11 requires a space between literal and "
+  "identifier">, InGroup, DefaultError;
+def ext_ms_reserved_user_defined_literal : ExtWarn<
+  "invalid suffix on literal; C++11 requires a space between literal and "
+  "identifier">, InGroup;
 def err_unsupported_string_concat : Error<
   "unsupported non-standard concatenation of string literals">;
 

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index a30266d09dca7c..8c629aad89f48a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ 

[PATCH] D148381: [WIP][Clang] Add counted_by attribute

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/lib/AST/Decl.cpp:4541-4544
+bool FieldDecl::isFlexibleArrayMemberLike(
+ASTContext ,
+LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel,
+bool IgnoreTemplateOrMacroSubstitution) const {

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > There's a lambda in `isUserWritingOffTheEnd` in 
> > clang/lib/AST/ExprConstant.cpp assigned to a variable 
> > `isFlexibleArrayMember`.  Any way we can reuse code between here and there 
> > if we host that into a proper method somewhere else?
> s/host/hoist/
Maybe. The one in `ExprConstant.cpp` looks like it's used in a slightly 
different way. I'll check it out, but might do it later on...



Comment at: clang/lib/AST/Decl.cpp:4590
+  if (ConstantArrayTypeLoc CTL = TL.getAs()) {
+const Expr *SizeExpr = dyn_cast(CTL.getSizeExpr());
+if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())

nickdesaulniers wrote:
> `const Expr *` on LHS of assignment, `dyn_cast` on RHS. What 
> is going on here?
Unknown. This is copy-pasted from the `Expr.cpp` version...I'll see if we can 
do without the cast.



Comment at: clang/lib/CodeGen/CGExpr.cpp:975-976
+if (const auto *RD = Ty->getAsRecordDecl())
+  for (const FieldDecl *Field : RD->fields())
+VD = Field;
+  } else if (const auto *CE = dyn_cast(Base)) {

nickdesaulniers wrote:
> Why is this re-assigning VD? Is this trying to get the last field in the 
> RecordDecl?
> 
> Can you use `field_end()` or `--field_end()`?
> 
> In fact I see this pattern a lot in this patch. Time for a new method on 
> RecordDecl? `const FieldDecl *RecordDecl::getLastField()`?
Created the method. We can't use `field_end` or `--field_end` because it's a 
forward iterator...



Comment at: clang/lib/CodeGen/CGExpr.cpp:992-996
+  auto It = llvm::find_if(FD->getParent()->fields(),
+  [&](const FieldDecl *Field) {
+  return FieldName == Field->getName();
+  });
+  return It != FD->getParent()->field_end() ? *It : nullptr;

nickdesaulniers wrote:
> Can `llvm::is_contained` from llvm/ADT/STLExtras.h help here?
I don't think that works. I thin it requires that the element we search for is 
of the same type. Here we have only  its name. There might be a nicer way of 
doing this though.



Comment at: clang/lib/Sema/SemaDecl.cpp:17914-17915
+if (const auto *SubRD = dyn_cast(D))
+  if (const FieldDecl *FD = FindFieldWithCountedByAttr(SubRD))
+return FD;
+  }

nickdesaulniers wrote:
> I think the `if` is unnecessary, could simply `return 
> FindFieldWithContedByAttr(SubRD);`?
Nice catch.



Comment at: clang/lib/Sema/SemaDecl.cpp:17926-17930
+  auto It = llvm::find_if(
+  RD->fields(), [&](const FieldDecl *Field){
+return Field->getName() == ECA->getCountedByField()->getName();
+  });
+  if (It == RD->field_end()) {

nickdesaulniers wrote:
> `llvm::is_contained`?
It's a similar issue as the one above: the ECA returns an `IdentifierInfo`, but 
`fields()` has only `FieldDecls`.



Comment at: clang/lib/Sema/SemaDecl.cpp:18001-18003
+  const IdentifierInfo *Unknown = CheckCountedByAttr(RD, FD, SR);
+
+  if (Unknown)

nickdesaulniers wrote:
> does it fit on one line to do:
> 
> ```
> if (const IdentifierInfo *I = CheckCountedByAttr(RD, FD, SR))
> ```
Yeah. I must have missed that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

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

[PATCH] D148381: [WIP][Clang] Add counted_by attribute

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 552552.
void marked 3 inline comments as done.
void added a comment.

Address Nick's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -56,6 +56,7 @@
 // CHECK-NEXT: ConsumableAutoCast (SubjectMatchRule_record)
 // CHECK-NEXT: ConsumableSetOnRead (SubjectMatchRule_record)
 // CHECK-NEXT: Convergent (SubjectMatchRule_function)
+// CHECK-NEXT: CountedBy (SubjectMatchRule_field)
 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: Destructor (SubjectMatchRule_function)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8380,6 +8380,21 @@
   D->addAttr(ZeroCallUsedRegsAttr::Create(S.Context, Kind, AL));
 }
 
+static void handleCountedByAttr(Sema , Decl *D, const ParsedAttr ) {
+  // TODO: Probably needs more processing here. See Sema::AddAlignValueAttr.
+  if (!AL.isArgIdent(0)) {
+S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
+<< AL << AANT_ArgumentIdentifier;
+return;
+  }
+
+  IdentifierLoc *IL = AL.getArgAsIdent(0);
+  CountedByAttr *CBA =
+  ::new (S.Context) CountedByAttr(S.Context, AL, IL->Ident);
+  CBA->setCountedByFieldLoc(IL->Loc);
+  D->addAttr(CBA);
+}
+
 static void handleFunctionReturnThunksAttr(Sema , Decl *D,
const ParsedAttr ) {
   StringRef KindStr;
@@ -9326,6 +9341,10 @@
 handleAvailableOnlyInDefaultEvalMethod(S, D, AL);
 break;
 
+  case ParsedAttr::AT_CountedBy:
+handleCountedByAttr(S, D, AL);
+break;
+
   // Microsoft attributes:
   case ParsedAttr::AT_LayoutVersion:
 handleLayoutVersion(S, D, AL);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -17904,6 +17904,40 @@
  "Broken injected-class-name");
 }
 
+static const FieldDecl *FindFieldWithCountedByAttr(const RecordDecl *RD) {
+  for (const Decl *D : RD->decls()) {
+if (const auto *FD = dyn_cast(D)) {
+  if (FD->hasAttr())
+return FD;
+}
+
+if (const auto *SubRD = dyn_cast(D)) {
+  return FindFieldWithCountedByAttr(SubRD);
+}
+  }
+
+  return nullptr;
+}
+
+/// CheckCountedByAttr - Return an \p IdentifierInfo if the attribute field
+/// doesn't exist in the enclosing structure. Returns \p nullptr if the
+/// attribute field does exist.
+static const IdentifierInfo *CheckCountedByAttr(const RecordDecl *RD,
+const FieldDecl *FD,
+SourceRange ) {
+  const CountedByAttr *ECA = FD->getAttr();
+
+  auto It = llvm::find_if(RD->fields(), [&](const FieldDecl *Field) {
+return Field->getName() == ECA->getCountedByField()->getName();
+  });
+  if (It == RD->field_end()) {
+Loc = ECA->getCountedByFieldLoc();
+return ECA->getCountedByField();
+  }
+
+  return nullptr;
+}
+
 void Sema::ActOnTagFinishDefinition(Scope *S, Decl *TagD,
 SourceRange BraceRange) {
   AdjustDeclIfTemplate(TagD);
@@ -17961,6 +17995,16 @@
  [](const FieldDecl *FD) { return FD->isBitField(); }))
   Diag(BraceRange.getBegin(), diag::warn_pragma_align_not_xl_compatible);
   }
+
+  // Check the "counted_by" attribute to ensure that the count field exists in
+  // the struct.
+  if (const RecordDecl *RD = dyn_cast(Tag)) {
+if (const FieldDecl *FD = FindFieldWithCountedByAttr(RD)) {
+  SourceRange SR;
+  if (const IdentifierInfo *Unknown = CheckCountedByAttr(RD, FD, SR))
+Diag(SR.getBegin(), diag::warn_counted_by_placeholder) << Unknown << SR;
+}
+  }
 }
 
 void Sema::ActOnObjCContainerFinishDefinition() {
Index: clang/lib/CodeGen/CodeGenFunction.h

[PATCH] D155769: [HIP][Clang][docs][RFC] Add documentation for C++ Parallel Algorithm Offload

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 552550.
AlexVlx marked an inline comment as done.
AlexVlx retitled this revision from "[Clang][docs][RFC] Add documentation for 
C++ Parallel Algorithm Offload" to "[HIP][Clang][docs][RFC] Add documentation 
for C++ Parallel Algorithm Offload".
AlexVlx removed reviewers: bader, Anastasia, erichkeane.
AlexVlx added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

Updating this to reflect the outcome of the RFC, which is that this shall be a 
HIP only extension. As such, documentation lives within the HIP Support master 
document; the other patches in the series will be updated accordingly.


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

https://reviews.llvm.org/D155769

Files:
  clang/docs/HIPSupport.rst

Index: clang/docs/HIPSupport.rst
===
--- clang/docs/HIPSupport.rst
+++ clang/docs/HIPSupport.rst
@@ -176,3 +176,387 @@
* - ``HIP_API_PER_THREAD_DEFAULT_STREAM``
  - Alias to ``__HIP_API_PER_THREAD_DEFAULT_STREAM__``. Deprecated.
 
+C++ Standard Parallelism Offload Support: Compiler And Runtime
+==
+
+Introduction
+
+
+This document describes the implementation of support for offloading the
+execution of standard C++ algorithms to accelerators that can be targeted via
+HIP. Furthermore, it enumerates restrictions on user defined code, as well as
+the interactions with runtimes.
+
+Algorithm Offload: What, Why, Where
+===
+
+C++17 introduced overloads
+`for most algorithms in the standard library `_
+which allow the user to specify a desired
+`execution policy `_.
+The `parallel_unsequenced_policy `_
+maps relatively well to the execution model of many accelerators, such as GPUs.
+This, coupled with the ubiquity of GPU accelerated algorithm libraries that
+implement most / all corresponding libraries in the standard library
+(e.g. `rocThrust `_), makes
+it feasible to provide seamless accelerator offload for supported algorithms,
+when an accelerated version exists. Thus, it becomes possible to easily access
+the computational resources of an accelerator, via a well specified, familiar,
+algorithmic interface, without having to delve into low-level hardware specific
+details. Putting it all together:
+
+- **What**: standard library algorithms, when invoked with the
+  ``parallel_unsequenced_policy``
+- **Why**: democratise accelerator programming, without loss of user familiarity
+- **Where**: any and all accelerators that can be targeted by Clang/LLVM via HIP
+
+Small Example
+=
+
+Given the following C++ code, which assumes the ``std`` namespace is included:
+
+.. code-block:: C++
+
+   bool has_the_answer(const vector& v) {
+ return find(execution::par_unseq, cbegin(v), cend(v), 42) != cend(v);
+   }
+
+if Clang is invoked with the ``-hipstdpar --offload-arch=foo`` flags, the call
+to ``find`` will be offloaded to an accelerator that is part of the ``foo``
+target family. If either ``foo`` or its runtime environment do not support
+transparent on-demand paging (such as e.g. that provided in Linux via
+`HMM `_), it is necessary to also include
+the ``--hipstdpar-interpose-alloc`` flag. If the accelerator specific algorithm
+library ``foo`` uses doesn't have an implementation of a particular algorithm,
+execution seamlessly falls back to the host CPU. It is legal to specify multiple
+``--offload-arcj``s. All the flags we introduce, as well as a thorough view of
+various restrictions and their implications will be provided below.
+
+Implementation - General View
+=
+
+We built support for Algorithm Offload support atop the pre-existing HIP
+infrastructure. More specifically, when one requests offload via ``-hipstdpar``,
+compilation is switched to HIP compilation, as if ``-x hip`` was specified.
+Similarly, linking is also switched to HIP linking, as if ``--hip-link`` was
+specified. Note that these are implicit, and one should not assume that any
+interop with HIP specific language constructs is available e.g. ``__device__``
+annotations are neither necessary nor guaranteed to work.
+
+Since there are no language restriction mechanisms in place, it is necessary to
+relax HIP language specific semantic checks performed by the FE; they would
+identify otherwise valid, offloadable code, as invalid HIP code. Given that we
+know that the user intended only for certain algorithms to be offloaded, and
+encoded this by specifying the ``parallel_unsequenced_policy``, we rely on a
+pass over IR to clean up any and 

[PATCH] D158571: [clang-format][NFC] Replace !is() with isNot()

[Owen Pan via Phabricator via cfe-commits]

owenpan created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, MyDeveloperDay.
owenpan requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158571

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/DefinitionBlockSeparator.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenSource.h
  clang/lib/Format/MacroExpander.cpp
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/WhitespaceManager.cpp

Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -590,14 +590,14 @@
 
   // A new line starts, re-initialize line status tracking bools.
   // Keep the match state if a string literal is continued on this line.
-  if (i == 0 || !Changes[i].Tok->is(tok::string_literal) ||
-  !Changes[i - 1].Tok->is(tok::string_literal)) {
+  if (i == 0 || Changes[i].Tok->isNot(tok::string_literal) ||
+  Changes[i - 1].Tok->isNot(tok::string_literal)) {
 FoundMatchOnLine = false;
   }
   LineIsComment = true;
 }
 
-if (!Changes[i].Tok->is(tok::comment))
+if (Changes[i].Tok->isNot(tok::comment))
   LineIsComment = false;
 
 if (Changes[i].Tok->is(tok::comma)) {
@@ -731,10 +731,10 @@
   SpacesRequiredBefore = 0;
 }
 
-if (!Current || !Current->is(tok::identifier))
+if (!Current || Current->isNot(tok::identifier))
   return false;
 
-if (!Current->Previous || !Current->Previous->is(tok::pp_define))
+if (!Current->Previous || Current->Previous->isNot(tok::pp_define))
   return false;
 
 // For a macro function, 0 spaces are required between the
@@ -781,7 +781,7 @@
   LineIsComment = true;
 }
 
-if (!Changes[I].Tok->is(tok::comment))
+if (Changes[I].Tok->isNot(tok::comment))
   LineIsComment = false;
 
 if (!AlignMacrosMatches(Changes[I]))
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -394,7 +394,7 @@
 ParseDefault();
 continue;
   }
-  if (CanContainBracedList && !FormatTok->is(TT_MacroBlockBegin) &&
+  if (CanContainBracedList && FormatTok->isNot(TT_MacroBlockBegin) &&
   tryToParseBracedList()) {
 continue;
   }
@@ -615,7 +615,7 @@
   LBraceStack.pop_back();
   break;
 case tok::identifier:
-  if (!Tok->is(TT_StatementMacro))
+  if (Tok->isNot(TT_StatementMacro))
 break;
   [[fallthrough]];
 case tok::at:
@@ -799,8 +799,8 @@
   if (eof())
 return IfLBrace;
 
-  if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
- : !FormatTok->is(tok::r_brace)) {
+  if (MacroBlock ? FormatTok->isNot(TT_MacroBlockEnd)
+ : FormatTok->isNot(tok::r_brace)) {
 Line->Level = InitialLevel;
 FormatTok->setBlockKind(BK_Block);
 return IfLBrace;
@@ -1080,7 +1080,7 @@
   bool MaybeIncludeGuard = IfNDef;
   if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
 for (auto  : Lines) {
-  if (!Line.Tokens.front().Tok->is(tok::comment)) {
+  if (Line.Tokens.front().Tok->isNot(tok::comment)) {
 MaybeIncludeGuard = false;
 IncludeGuard = IG_Rejected;
 break;
@@ -1612,7 +1612,7 @@
 nextToken();
 if (FormatTok->is(tok::kw_public))
   nextToken();
-if (!FormatTok->is(tok::string_literal))
+if (FormatTok->isNot(tok::string_literal))
   return;
 nextToken();
 if (FormatTok->is(tok::semi))
@@ -1887,7 +1887,7 @@
   // a new unwrapped line, so they are special cased below.
   size_t TokenCount = Line->Tokens.size();
   if (Style.isJavaScript() && FormatTok->is(Keywords.kw_function) &&
-  (TokenCount > 1 || (TokenCount == 1 && !Line->Tokens.front().Tok->is(
+  (TokenCount > 1 || (TokenCount == 1 && Line->Tokens.front().Tok->isNot(
  Keywords.kw_async {
 tryToParseJSFunction();
 break;
@@ -2872,7 +2872,7 @@
   FormatTok->is(tok::l_brace)) {
 do {
   nextToken();
-} while (!FormatTok->is(tok::r_brace));
+} while (FormatTok->isNot(tok::r_brace));
 nextToken();
   }
 
@@ -2895,7 +2895,7 @@
   addUnwrappedLine();
 else
   NeedsUnwrappedLine = true;
-  } else if (!FormatTok->is(tok::kw_catch)) {
+  } else if 

[PATCH] D158570: [Darwin][StableABI][ASan] Remove version mismatch check from stable abi shim

[Brittany Blue Gaston via Phabricator via cfe-commits]

thetruestblue created this revision.
thetruestblue added reviewers: kubamracek, yln, rsundahl, wrotki, usama54321, 
MaskRay, vitalybuka.
Herald added a subscriber: Enna1.
Herald added a project: All.
thetruestblue requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

By its nature the stable abi does not require a version check symbol.
This patch sets -asan-guard-against-version-mismatch=0 for stable abi.
And updates tests to reflect this


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158570

Files:
  clang/lib/Driver/SanitizerArgs.cpp
  compiler-rt/lib/asan_abi/asan_abi_shim.cpp
  compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp


Index: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
===
--- compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
+++ compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
@@ -7,7 +7,6 @@
 // RUN: | grep " [TU] "   \
 // RUN: | grep -o "\(__asan\)[^ ]*"   \
 // RUN: | grep -v "\(__asan_abi\)[^ ]*"   \
-// RUN: | sed -e 
"s/__asan_version_mismatch_check_v[0-9]+/__asan_version_mismatch_check/" \
 // RUN: > %t.exports
 // RUN: sed -e ':a' -e 'N' -e '$!ba'  \
 // RUN: -e 's/ //g'   \
@@ -17,7 +16,9 @@
 // RUN: | grep -v -f %p/../../../../lib/asan_abi/asan_abi_tbd.txt \
 // RUN: | grep -e "INTERFACE_\(WEAK_\)\?FUNCTION" \
 // RUN: | grep -v "__sanitizer[^ ]*"  \
-// RUN: | sed -e "s/.*(//" -e "s/).*//" > %t.imports
+// RUN: | sed -e "s/.*(//" -e "s/).*//"   \
+// RUN: | sed -e "/^__asan_version_mismatch_check/d"  \
+// RUN: > %t.imports
 // RUN: sort %t.imports | uniq > %t.imports-sorted
 // RUN: sort %t.exports | uniq > %t.exports-sorted
 // RUN: diff %t.imports-sorted %t.exports-sorted
Index: compiler-rt/lib/asan_abi/asan_abi_shim.cpp
===
--- compiler-rt/lib/asan_abi/asan_abi_shim.cpp
+++ compiler-rt/lib/asan_abi/asan_abi_shim.cpp
@@ -49,7 +49,7 @@
 
   __asan_abi_init();
 }
-void __asan_version_mismatch_check_v8(void) {}
+
 void __asan_handle_no_return(void) { __asan_abi_handle_no_return(); }
 
 // Variables concerning RTL state. These provisionally exist for completeness
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -1287,6 +1287,8 @@
 CmdArgs.push_back("-asan-instrumentation-with-call-threshold=0");
 CmdArgs.push_back("-mllvm");
 CmdArgs.push_back("-asan-max-inline-poisoning-size=0");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-asan-guard-against-version-mismatch=0");
   }
 
   // Only pass the option to the frontend if the user requested,


Index: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
===
--- compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
+++ compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp
@@ -7,7 +7,6 @@
 // RUN: | grep " [TU] "   \
 // RUN: | grep -o "\(__asan\)[^ ]*"   \
 // RUN: | grep -v "\(__asan_abi\)[^ ]*"   \
-// RUN: | sed -e "s/__asan_version_mismatch_check_v[0-9]+/__asan_version_mismatch_check/" \
 // RUN: > %t.exports
 // RUN: sed -e ':a' -e 'N' -e '$!ba'  \
 // RUN: -e 's/ //g'   \
@@ -17,7 +16,9 @@
 // RUN: | grep -v -f %p/../../../../lib/asan_abi/asan_abi_tbd.txt \
 // RUN: | grep -e "INTERFACE_\(WEAK_\)\?FUNCTION" \
 // RUN: | grep -v "__sanitizer[^ ]*"  \
-// RUN: | sed -e "s/.*(//" -e "s/).*//" > %t.imports
+// RUN: | sed -e "s/.*(//" -e "s/).*//"   \
+// RUN: | sed -e "/^__asan_version_mismatch_check/d"  \
+// RUN: > %t.imports
 // RUN: sort %t.imports | uniq > %t.imports-sorted
 // RUN: sort %t.exports | uniq > %t.exports-sorted
 // RUN: diff %t.imports-sorted %t.exports-sorted
Index: compiler-rt/lib/asan_abi/asan_abi_shim.cpp
===
--- compiler-rt/lib/asan_abi/asan_abi_shim.cpp
+++ compiler-rt/lib/asan_abi/asan_abi_shim.cpp
@@ -49,7 +49,7 @@
 
   __asan_abi_init();
 }
-void __asan_version_mismatch_check_v8(void) {}
+
 void __asan_handle_no_return(void) { __asan_abi_handle_no_return(); }
 
 // Variables concerning RTL state. These provisionally exist for completeness
Index: clang/lib/Driver/SanitizerArgs.cpp

[PATCH] D158566: Add CLANGD_INCLUDE_TESTS as a separate flag to control clangd tests

[Aditya Kumar via Phabricator via cfe-commits]

hiraditya updated this revision to Diff 552543.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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

https://reviews.llvm.org/D158566

Files:
  clang-tools-extra/clangd/CMakeLists.txt


Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -208,13 +208,14 @@
   include(AddGRPC)
 endif()
 
-if(CLANG_INCLUDE_TESTS)
+if(CLANGD_INCLUDE_TESTS)
   add_subdirectory(test)
   add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is 
stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support 
for Clangd" OFF)
+option(CLANGD_INCLUDE_TESTS "Include Clangd tests" ON)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual 
installation.")
 
 add_subdirectory(index/remote)


Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -208,13 +208,14 @@
   include(AddGRPC)
 endif()
 
-if(CLANG_INCLUDE_TESTS)
+if(CLANGD_INCLUDE_TESTS)
   add_subdirectory(test)
   add_subdirectory(unittests)
 endif()
 
 # FIXME(kirillbobyrev): Document this in the LLVM docs once remote index is stable.
 option(CLANGD_ENABLE_REMOTE "Use gRPC library to enable remote index support for Clangd" OFF)
+option(CLANGD_INCLUDE_TESTS "Include Clangd tests" ON)
 set(GRPC_INSTALL_PATH "" CACHE PATH "Path to gRPC library manual installation.")
 
 add_subdirectory(index/remote)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D158559#4608410 , 
@ivanrodriguez3753 wrote:

> Also, should the underlying issue and test case be filed as an issue on 
> Github? I wasn't sure since this revision includes the bug and a description

If you're going to fix this yourself - probably it is not necessary to create a 
bug report. Otherwise, please go ahead and create a bug report.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D158559#4608397 , 
@ivanrodriguez3753 wrote:

> In D158559#4608388 , @ABataev wrote:
>
>> 1. Always provide full context in the patch.
>
> Sure, would you mind mentioning what's missing? Or do you mean the stuff from 
> debug output being shortened, as well as LLVM IR being only snippets?

I mean, provide full diff context, read the docs how to upload patches for the 
review. 
https://www.llvm.org/docs/Phabricator.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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

[clang] dddfd93 - Revert "Test commit config"

[via cfe-commits]

Author: erichkeane
Date: 2023-08-22T16:43:34-07:00
New Revision: dddfd93792b31e4bbeaeca3fe68fcefb289cfb51

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

LOG: Revert "Test commit config"
Undoing test commit.
This reverts commit 3e50bcb44b7e850032d928ce53a582891336bc87.

Added: 


Modified: 
clang/NOTES.txt

Removed: 




diff  --git a/clang/NOTES.txt b/clang/NOTES.txt
index 1f5bb17eda82e8..f06ea8c70cd340 100644
--- a/clang/NOTES.txt
+++ b/clang/NOTES.txt
@@ -102,4 +102,3 @@ delegation optimization in cases of virtual inheritance 
where:
 - they were passed indirectly.
 
 //===-===//
-



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

[clang] 3e50bcb - Test commit config

[via cfe-commits]

Author: erichkeane
Date: 2023-08-22T16:39:30-07:00
New Revision: 3e50bcb44b7e850032d928ce53a582891336bc87

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

LOG: Test commit config

Added: 


Modified: 
clang/NOTES.txt

Removed: 




diff  --git a/clang/NOTES.txt b/clang/NOTES.txt
index f06ea8c70cd340..1f5bb17eda82e8 100644
--- a/clang/NOTES.txt
+++ b/clang/NOTES.txt
@@ -102,3 +102,4 @@ delegation optimization in cases of virtual inheritance 
where:
 - they were passed indirectly.
 
 //===-===//
+



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

[PATCH] D158562: [clang][Sema] Add truncation warning on fortified snprintf

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet created this revision.
hazohelet added reviewers: aaron.ballman, serge-sans-paille, nickdesaulniers, 
tbaeder.
Herald added a project: All.
hazohelet requested review of this revision.
Herald added a project: clang.

This patch warns on `snprintf` calls whose `n` argument is known to be smaller 
than the size of the formatted string like

  char buf[5];
  snprintf(buf, 5, "Hello");

This is a counterpart of gcc's `Wformat-truncation`
Fixes https://github.com/llvm/llvm-project/issues/64871


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158562

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Analysis/taint-generic.c
  clang/test/Sema/format-strings.c
  clang/test/Sema/warn-fortify-source.c

Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -87,6 +87,10 @@
   char buf[10];
   __builtin_snprintf(buf, 10, "merp");
   __builtin_snprintf(buf, 11, "merp"); // expected-warning {{'snprintf' size argument is too large; destination buffer has size 10, but size argument is 11}}
+  __builtin_snprintf(buf, 0, "merp");
+  __builtin_snprintf(buf, 3, "merp"); // expected-warning {{'snprintf' will always be truncated; specified size is 3, but format string expands to at least 5}}
+  __builtin_snprintf(buf, 4, "merp"); // expected-warning {{'snprintf' will always be truncated; specified size is 4, but format string expands to at least 5}}
+  __builtin_snprintf(buf, 5, "merp");
 }
 
 void call_vsnprintf(void) {
@@ -94,6 +98,10 @@
   __builtin_va_list list;
   __builtin_vsnprintf(buf, 10, "merp", list);
   __builtin_vsnprintf(buf, 11, "merp", list); // expected-warning {{'vsnprintf' size argument is too large; destination buffer has size 10, but size argument is 11}}
+  __builtin_vsnprintf(buf, 0, "merp", list);
+  __builtin_vsnprintf(buf, 3, "merp", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 3, but format string expands to at least 5}}
+  __builtin_vsnprintf(buf, 4, "merp", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 4, but format string expands to at least 5}}
+  __builtin_vsnprintf(buf, 5, "merp", list);
 }
 
 void call_sprintf_chk(char *buf) {
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -202,7 +202,8 @@
   printf("%s%lv%d","unix",10,20); // expected-warning {{invalid conversion specifier 'v'}} expected-warning {{data argument not used by format string}}
   fprintf(fp,"%%%l"); // expected-warning {{incomplete format specifier}}
   sprintf(buf,"%ld%d%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}}
-  snprintf(buf, 2, "%ld%;%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}} expected-warning {{invalid conversion specifier ';'}} expected-warning {{data argument not used by format string}}
+  snprintf(buf, 2, "%ld%;%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}} expected-warning {{invalid conversion specifier ';'}} expected-warning {{data argument not used by format string}} \
+// expected-warning{{'snprintf' will always be truncated; specified size is 2, but format string expands to at least 7}}
 }
 
 void check_null_char_string(char* b)
Index: clang/test/Analysis/taint-generic.c
===
--- clang/test/Analysis/taint-generic.c
+++ clang/test/Analysis/taint-generic.c
@@ -229,6 +229,7 @@
   char addr[128];
   scanf("%s", addr);
   __builtin_snprintf(buffern, 10, "/bin/mail %s < /tmp/email", addr);
+  // expected-warning@-1 {{'snprintf' will always be truncated; specified size is 10, but format string expands to at least 24}}
   system(buffern); // expected-warning {{Untrusted data is passed to a system call}}
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1131,6 +1131,24 @@
 // Add 1 for null byte.
 return llvm::APSInt::getUnsigned(Result + 1).extOrTrunc(SizeTypeWidth);
   };
+  auto ProcessFormatStringLiteral =
+  [&](const Expr *FormatExpr, StringRef , size_t ) {
+if (auto *Format = dyn_cast(FormatExpr)) {
+  if (!Format->isOrdinary() && !Format->isUTF8())
+return false;
+
+  FormatStrRef = Format->getString();
+  const ConstantArrayType *T =
+  Context.getAsConstantArrayType(Format->getType());
+  assert(T && "String literal not of constant array type!");
+  size_t 

[PATCH] D157296: [AST][Coroutine] Fix CoyieldExpr missing end loc

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:322
+  auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc();
+  return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr);
 }

We should use 
[bugprone-argument-comment](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
 when using literals for arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157296

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

[PATCH] D158476: [driver] Search for compatible Android runtime directories

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158476#4608428 , @thakis wrote:

> I think this is a useful feature, for the reasons mentioned on the thread.
>
> Since this is a superset of D115049  (... 
> right?), this should probably revert the changes over there?

Wouldn't it break Chrome if I reverted those changes as part of this diff? I 
was thinking I could land this, Chrome could move over to a versioned dir 
(assuming that works for it), and then we could undo D115049 
. I'm happy to make the revert as part of 
this change if it'll work out for Chrome though.

> Maybe we should dump searched paths in `-v` mode?

As in output all the versioned directories that were found and considered?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

[PATCH] D141414: [clang] add warning on shifting boolean type

[Angelo Matni via Phabricator via cfe-commits]

angelomatnigoogle added a comment.

Pardon me for losing sight on this change. Within the week, I will revisit this 
change based on the current outstanding comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141414

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

[PATCH] D158476: [driver] Search for compatible Android runtime directories

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

I think this is a useful feature, for the reasons mentioned on the thread.

Since this is a superset of D115049  (... 
right?), this should probably revert the changes over there?

Maybe we should dump searched paths in `-v` mode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

[PATCH] D158561: [-Wunsafe-buffer-usage] Add AST info to the unclaimed DRE debug notes for analysis

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: jkorous, NoQ, t-rasmud, malavikasamak.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The debug note for an unclaimed DRE highlights a usage of an unsafe pointer 
that is not supported by our analysis.

For a better understand of what the unsupported cases are,  we add more 
information to the debug note---a string of ancestor AST nodes of the unclaimed 
DRE.For example, an unclaimed DRE `p` in an expression `*(p++)` will result 
in a string `DRE, UnaryOperator(++), Paren, UnaryOperator(*)`.

To find out the most common patterns of those unsupported use cases,  we add a 
simple script to build a prefix tree over those strings and count each prefix.  
 The script reads input line by line, assumes a line is a list of words 
separated by `,`s, and builds a prefix tree over those lists.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158561

Files:
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/utils/analyze_safe_buffer_debug_notes.py

Index: clang/utils/analyze_safe_buffer_debug_notes.py
===
--- /dev/null
+++ clang/utils/analyze_safe_buffer_debug_notes.py
@@ -0,0 +1,44 @@
+class Trie:
+def __init__(self, name):
+self.name = name
+self.children = {}
+self.count = 1
+
+def add(self, name):
+if name in self.children:
+self.children[name].count += 1
+else:
+self.children[name] = Trie(name)
+return self.children[name]
+
+def print(self, depth):
+if depth > 0:
+print('|', end="")
+for i in range(depth):
+print('-', end="")
+if depth > 0:
+print(end=" ")
+print(self.name, '#', self.count)
+for key, child in self.children.items():
+child.print(depth + 1)
+
+
+Root = Trie("Root")
+
+def main():
+while True:
+try:
+line = input()
+except EOFError:
+break
+
+words = line.split(',')
+words = [word.strip() for word in words]
+MyTrie = Root;
+for word in words:
+MyTrie = MyTrie.add(word)
+
+Root.print(0)
+
+if __name__ == "__main__":
+main()
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -22,6 +23,84 @@
 using namespace clang;
 using namespace ast_matchers;
 
+#ifndef NDEBUG
+namespace {
+static std::string printUnaryOperator(const Stmt *Stmt) {
+  const UnaryOperator *UO = cast(Stmt);
+  return "UnaryOperator(" + UO->getOpcodeStr(UO->getOpcode()).str() + ")";
+}
+
+static std::string printBinaryOperator(const Stmt *Stmt) {
+  const BinaryOperator *BO = cast(Stmt);
+  StringRef BOOpStr = BO->getOpcodeStr();
+
+  if (BO->getOpcode() == BinaryOperator::Opcode::BO_Comma)
+BOOpStr = "COMMA"; // Do not print `,` as it is used as the separator
+  return "BinaryOperator(" + BOOpStr.str() + ")";
+}
+
+static std::string printCompoundAssignOperator(const Stmt *Stmt) {
+  return printBinaryOperator(Stmt);
+}
+
+static std::string printImplicitCastExpr(const Stmt *Stmt) {
+  const CastExpr *CE = cast(Stmt);
+  return "ImplicitCastExpr(" + std::string(CE->getCastKindName()) + ")";
+}
+
+// Prints the `StmtClass` of the given `Stmt`.  For `UnaryOperator`s and
+// `BinaryOperator`s, it also prints the operator.  For `CastExpr`, it also
+// prints the cast kind.
+static std::optional printStmtClass(const Stmt *Stmt) {
+  switch (Stmt->getStmtClass()) {
+#define STMT(CLASS, PARENT)\
+  case Stmt::CLASS##Class: \
+return #CLASS;
+#define UNARYOPERATOR(TYPE, BASE)  \
+  case Stmt::TYPE##Class:  \
+return print##TYPE(Stmt);
+#define BINARYOPERATOR(TYPE, BASE) \
+  case Stmt::TYPE##Class:  \
+return print##TYPE(Stmt);
+#define IMPLICITCASTEXPR(TYPE, BASE)   \
+  case Stmt::TYPE##Class:  \
+return print##TYPE(Stmt);
+#define ABSTRACT_STMT(STMT)
+#include "clang/AST/StmtNodes.inc"
+  default:
+return std::nullopt;
+  }
+}
+
+// Returns a string of 

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

Also, should the underlying issue and test case be filed as an issue on Github? 
I wasn't sure since this revision includes the bug and a description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 added a comment.

In D158559#4608388 , @ABataev wrote:

> 1. Always provide full context in the patch.

Sure, would you mind mentioning what's missing? Or do you mean the stuff from 
debug output being shortened, as well as LLVM IR being only snippets?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

1. Always provide full context in the patch.
2. It looks like we're counting the pointer size (or the size of just the first 
element of the array), because  we do not account array section here, either 
just the pointer or the first element only (most probably second option). Need 
to teach the compiler about array section here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

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

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

dblaikie wrote:
> MaskRay wrote:
> > dexonsmith wrote:
> > > MaskRay wrote:
> > > > dexonsmith wrote:
> > > > > MaskRay wrote:
> > > > > > hans wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > hans wrote:
> > > > > > > > > dexonsmith wrote:
> > > > > > > > > > MaskRay wrote:
> > > > > > > > > > > dexonsmith wrote:
> > > > > > > > > > > > Why would we want to use the old name here? An alias 
> > > > > > > > > > > > seems strictly better to me. 
> > > > > > > > > > > Making `overriding-t-option` an alias for 
> > > > > > > > > > > `overriding-option` would make `-Wno-overriding-t-option` 
> > > > > > > > > > > applies to future overriding option diagnostics, which is 
> > > > > > > > > > > exactly what I want to avoid.
> > > > > > > > > > > 
> > > > > > > > > > I understand that you don't want `-t-` to apply to work on 
> > > > > > > > > > future overriding option diagnostics, but I think the 
> > > > > > > > > > platform divergence you're adding here is worse.
> > > > > > > > > > 
> > > > > > > > > > Having a few Darwin-specific options use 
> > > > > > > > > > `-Woverriding-t-option` (and everything else use 
> > > > > > > > > > `-Woverriding-option`) as the canonical spelling is hard to 
> > > > > > > > > > reason about for maintainers, and for users.
> > > > > > > > > > 
> > > > > > > > > > And might not users on other platforms have 
> > > > > > > > > > `-Woverriding-t-option` hardcoded in  build settings? (So 
> > > > > > > > > > @dblaikie's argument that we shouldn't arbitrarily make 
> > > > > > > > > > things hard for users would apply to all options?)
> > > > > > > > > > 
> > > > > > > > > > IMO, if we're not comfortable removing 
> > > > > > > > > > `-Woverriding-t-option` entirely, then it should live on as 
> > > > > > > > > > an alias (easy to reason about), not as 
> > > > > > > > > > canonical-in-special-cases (hard to reason about).
> > > > > > > > > > IMO, if we're not comfortable removing 
> > > > > > > > > > -Woverriding-t-option entirely, then it should live on as 
> > > > > > > > > > an alias (easy to reason about), not as 
> > > > > > > > > > canonical-in-special-cases (hard to reason about).
> > > > > > > > > 
> > > > > > > > > +1 if we can't drop the old spelling, an alias seems like the 
> > > > > > > > > best option.
> > > > > > > > Making `overriding-t-option` an alias for `overriding-option`, 
> > > > > > > > as I mentioned, will make `-Wno-overriding-t-option` affect new 
> > > > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > > > 
> > > > > > > > I know there are some `-Wno-overriding-t-option` uses. 
> > > > > > > > Honestly, they are far fewer than other diagnostics we are 
> > > > > > > > introducing or changing in Clang. I can understand the argument 
> > > > > > > > "make -Werror users easier for this specific diagnostic" (but 
> > > > > > > > `-Werror` will complain about other new diagnostics), but do we 
> > > > > > > > really want to in the Darwin case? I think no. They can remove 
> > > > > > > > the version from the target triple like 
> > > > > > > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > > > > > > 　or make the version part consistent with `-m.*os-version-min`.
> > > > > > > > 
> > > > > > > > This change may force these users to re-think how they should 
> > > > > > > > fix  their build. I apology to these users, but I don't feel 
> > > > > > > > that adding an alias is really necessary.
> > > > > > > > Making overriding-t-option an alias for overriding-option, as I 
> > > > > > > > mentioned, will make -Wno-overriding-t-option affect new 
> > > > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > > 
> > > > > > > Is keeping them separate actually important, though? 
> > > > > > > -Wno-overriding-option has the same issue in that case, that 
> > > > > > > using the flag will also affect any new overriding-options uses, 
> > > > > > > and I don't think that's a problem.
> > > > > > `-Wno-overriding-option` is properly named, so affecting new 
> > > > > > overriding-options uses looks fine to me.
> > > > > > `-Wno-overriding-t-option` is awkward, and making it affect new 
> > > > > > uses makes me nervous.
> > > > > > 
> > > > > > The gist of my previous comment is whether the uses cases really 
> > > > > > justify a tiny bit of tech bit in clang and I think the answer is 
> > > > > > no.
> > > > > > 
> > > > > > This change is not about changing a semantic warning that has 

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D158557#4608262 , @erichkeane 
wrote:

> Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING 
> on these?  If someone passes a negative size, we should probably at least do 
> the warning that it was converted/truncated.

The arguments have type `size_t` so using `-Wsign-conversion` would warn for 
the previous value but that is not really the point of the fix, which is why I 
updated the value to clarify.


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

https://reviews.llvm.org/D158557

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

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 552518.
shafik added a comment.

-Updated values used in test


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

https://reviews.llvm.org/D158557

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-string.cpp


Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -676,3 +676,14 @@
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error 
{{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, 0x);
+  __builtin_memcmp(0, 0, 0x);
+  __builtin_bcmp(0, 0, 0x);
+  __builtin_wmemcmp(0, 0, 0x);
+  __builtin_memchr((const void*)0, 1, 0x);
+  __builtin_wmemchr((const wchar_t*)0, 1, 0x);
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9357,7 +9357,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 // We cannot find the value if there are no candidates to match against.
 if (MaxLength == 0u)
@@ -12381,7 +12381,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 
 // Empty substrings compare equal by definition.


Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -676,3 +676,14 @@
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error {{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, 0x);
+  __builtin_memcmp(0, 0, 0x);
+  __builtin_bcmp(0, 0, 0x);
+  __builtin_wmemcmp(0, 0, 0x);
+  __builtin_memchr((const void*)0, 1, 0x);
+  __builtin_wmemchr((const wchar_t*)0, 1, 0x);
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9357,7 +9357,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 // We cannot find the value if there are no candidates to match against.
 if (MaxLength == 0u)
@@ -12381,7 +12381,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 
 // Empty substrings compare equal by definition.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 552516.
shafik added a comment.

- Diff w/ context


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

https://reviews.llvm.org/D158557

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-string.cpp


Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -676,3 +676,14 @@
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error 
{{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, -511LL);
+  __builtin_memcmp(0, 0, -511LL);
+  __builtin_bcmp(0, 0, -511LL);
+  __builtin_wmemcmp(0, 0, -511LL);
+  __builtin_memchr((const void*)0, 1, -511LL);
+  __builtin_wmemchr((const wchar_t*)0, 1, -511LL);
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9357,7 +9357,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 // We cannot find the value if there are no candidates to match against.
 if (MaxLength == 0u)
@@ -12381,7 +12381,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 
 // Empty substrings compare equal by definition.


Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -676,3 +676,14 @@
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error {{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, -511LL);
+  __builtin_memcmp(0, 0, -511LL);
+  __builtin_bcmp(0, 0, -511LL);
+  __builtin_wmemcmp(0, 0, -511LL);
+  __builtin_memchr((const void*)0, 1, -511LL);
+  __builtin_wmemchr((const wchar_t*)0, 1, -511LL);
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9357,7 +9357,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 // We cannot find the value if there are no candidates to match against.
 if (MaxLength == 0u)
@@ -12381,7 +12381,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 
 // Empty substrings compare equal by definition.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 0d592c8 - [Driver][PS5] Simplify a condition

[Paul Robinson via cfe-commits]

Author: Paul Robinson
Date: 2023-08-22T15:01:55-07:00
New Revision: 0d592c8d49bc734e3807bd7093f61e634e8194cf

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

LOG: [Driver][PS5] Simplify a condition

Added: 


Modified: 
clang/lib/Driver/ToolChains/PS4CPU.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp 
b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 2f43d33bf0f1c8..33b81d09e8b00e 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -157,14 +157,12 @@ void tools::PScpu::Linker::ConstructJob(Compilation , 
const JobAction ,
   const bool UseJMC =
   Args.hasFlag(options::OPT_fjmc, options::OPT_fno_jmc, false);
   const bool IsPS4 = TC.getTriple().isPS4();
-  const bool IsPS5 = TC.getTriple().isPS5();
-  assert(IsPS4 || IsPS5);
 
   const char *PS4LTOArgs = "";
   auto AddCodeGenFlag = [&](Twine Flag) {
 if (IsPS4)
   PS4LTOArgs = Args.MakeArgString(Twine(PS4LTOArgs) + " " + Flag);
-else if (IsPS5)
+else
   CmdArgs.push_back(Args.MakeArgString(Twine("-plugin-opt=") + Flag));
   };
 



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

[PATCH] D158474: [clang][ExtractAPI] Fix bool spelling coming from the macro definition.

[Erick Velez 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 rGe81744563a53: [clang][ExtractAPI] Fix bool spelling coming 
from the macro definition. (authored by evelez7).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158474

Files:
  clang/include/clang/ExtractAPI/DeclarationFragments.h
  clang/lib/ExtractAPI/DeclarationFragments.cpp
  clang/test/ExtractAPI/bool.c
  clang/test/ExtractAPI/bool.cpp

Index: clang/test/ExtractAPI/bool.cpp
===
--- /dev/null
+++ clang/test/ExtractAPI/bool.cpp
@@ -0,0 +1,203 @@
+// 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
+bool Foo;
+
+bool IsFoo(bool Bar);
+/// 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": [],
+  "symbols": [
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "identifier",
+  "spelling": "Foo"
+},
+{
+  "kind": "text",
+  "spelling": ";"
+}
+  ],
+  "identifier": {
+"interfaceLanguage": "c++",
+"precise": "c:@Foo"
+  },
+  "kind": {
+"displayName": "Global Variable",
+"identifier": "c++.var"
+  },
+  "location": {
+"position": {
+  "character": 6,
+  "line": 1
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+"navigator": [
+  {
+"kind": "identifier",
+"spelling": "Foo"
+  }
+],
+"subHeading": [
+  {
+"kind": "identifier",
+"spelling": "Foo"
+  }
+],
+"title": "Foo"
+  },
+  "pathComponents": [
+"Foo"
+  ]
+},
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "identifier",
+  "spelling": "IsFoo"
+},
+{
+  "kind": "text",
+  "spelling": "("
+},
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "internalParam",
+  "spelling": "Bar"
+},
+{
+  "kind": "text",
+  "spelling": ");"
+}
+  ],
+  "functionSignature": {
+"parameters": [
+  {
+"declarationFragments": [
+  {
+"kind": "typeIdentifier",
+"preciseIdentifier": "c:b",
+"spelling": "bool"
+  },
+  {
+"kind": "text",
+"spelling": " "
+  },
+  {
+"kind": "internalParam",
+"spelling": "Bar"
+  }
+],
+"name": "Bar"
+  }
+],
+"returns": [
+  {
+"kind": "typeIdentifier",
+"preciseIdentifier": "c:b",
+"spelling": "bool"
+  }
+]
+  },
+  "identifier": {
+"interfaceLanguage": "c++",
+"precise": "c:@F@IsFoo#b#"
+  },
+  "kind": {
+"displayName": "Function",
+"identifier": "c++.func"
+  },
+  "location": {
+"position": {
+  "character": 6,
+  "line": 3
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+   

[clang] e817445 - [clang][ExtractAPI] Fix bool spelling coming from the macro definition.

[Erick Velez via cfe-commits]

Author: Erick Velez
Date: 2023-08-22T15:00:14-07:00
New Revision: e81744563a53b1ed0aaa2cefda885287974a9e21

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

LOG: [clang][ExtractAPI] Fix bool spelling coming from the macro definition.

getFragmentsForType resulted in a bool typeIdentifier fragment to be spelled 
"_Bool".
This fixes the spelling to be "bool" and checks it in C and C++.

Reviewed By: dang

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

Added: 
clang/test/ExtractAPI/bool.c
clang/test/ExtractAPI/bool.cpp

Modified: 
clang/include/clang/ExtractAPI/DeclarationFragments.h
clang/lib/ExtractAPI/DeclarationFragments.cpp

Removed: 




diff  --git a/clang/include/clang/ExtractAPI/DeclarationFragments.h 
b/clang/include/clang/ExtractAPI/DeclarationFragments.h
index 3c05f43e829c60..316d83df13e935 100644
--- a/clang/include/clang/ExtractAPI/DeclarationFragments.h
+++ b/clang/include/clang/ExtractAPI/DeclarationFragments.h
@@ -170,6 +170,11 @@ class DeclarationFragments {
 return *this;
   }
 
+  DeclarationFragments (std::string NewSpelling, unsigned Position) {
+Fragments.at(Position).Spelling = NewSpelling;
+return *this;
+  }
+
   /// Append a text Fragment of a space character.
   ///
   /// \returns a reference to the DeclarationFragments object itself after

diff  --git a/clang/lib/ExtractAPI/DeclarationFragments.cpp 
b/clang/lib/ExtractAPI/DeclarationFragments.cpp
index f1fff6bf513df6..3c15d5073b01eb 100644
--- a/clang/lib/ExtractAPI/DeclarationFragments.cpp
+++ b/clang/lib/ExtractAPI/DeclarationFragments.cpp
@@ -397,6 +397,9 @@ DeclarationFragments 
DeclarationFragmentsBuilder::getFragmentsForType(
   DeclarationFragments QualsFragments = getFragmentsForQualifiers(SQT.Quals),
TypeFragments =
getFragmentsForType(SQT.Ty, Context, After);
+  if (QT.getAsString() == "_Bool")
+TypeFragments.replace("bool", 0);
+
   if (QualsFragments.getFragments().empty())
 return TypeFragments;
 

diff  --git a/clang/test/ExtractAPI/bool.c b/clang/test/ExtractAPI/bool.c
new file mode 100644
index 00..fc013792c67991
--- /dev/null
+++ b/clang/test/ExtractAPI/bool.c
@@ -0,0 +1,204 @@
+// 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 -extract-api -target arm64-apple-macosx \
+// RUN: %t/input.h -o %t/output.json
+
+// 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
+#include 
+bool Foo;
+
+bool IsFoo(bool Bar);
+/// 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": [],
+  "symbols": [
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "identifier",
+  "spelling": "Foo"
+},
+{
+  "kind": "text",
+  "spelling": ";"
+}
+  ],
+  "identifier": {
+"interfaceLanguage": "c",
+"precise": "c:@Foo"
+  },
+  "kind": {
+"displayName": "Global Variable",
+"identifier": "c.var"
+  },
+  "location": {
+"position": {
+  "character": 6,
+  "line": 2
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+"navigator": [
+  {
+"kind": "identifier",
+"spelling": "Foo"
+  }
+],
+"subHeading": [
+  {
+"kind": "identifier",
+"spelling": "Foo"
+  }
+],
+"title": "Foo"
+  },
+  "pathComponents": [
+"Foo"
+  ]
+},
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Ivan Rodriguez via Phabricator via cfe-commits]

ivanrodriguez3753 created this revision.
ivanrodriguez3753 added a reviewer: OpenMP.
Herald added subscribers: pengfei, guansong, tpr, yaxunl.
Herald added a project: All.
ivanrodriguez3753 requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, jplehr, sstefan1.
Herald added a project: clang.

It seems that the OpenMP CodeGen is incorrectly generating a pointer for a size 
calculation on the combined entry of a partially mapped struct. Here is the 
reduced test case:

  scrubbed-user@scrubbed-server: cat reduced.cpp
  #include 
  #include 
  #include 
  
  #define N 1000
  
  struct T {
int dep_1[N];
int dep_2[N];
  };
  
  using namespace std;
  int main() {
#define SMALL 2
T t;
#pragma omp target map(tofrom: t.dep_1, t.dep_2[0:SMALL])
{
  for (int i = 0; i < SMALL; i++) {
t.dep_1[i] = 1;
t.dep_2[i] = 1;
  }
}
  
for (int i = 0; i < SMALL; i++) {
  assert(t.dep_1[i] == 1);
  assert(t.dep_2[i] == 1);
}
  }

Originally, we were mapping `t.dep_2[0:N]`, but I reduced to the smallest size 
that still breaks the runtime. We'll see why we need at least 2 in a second... 
Here is some output from the runtime library crashing

  scrubbed-user@scrubbed-server: 
/ptmp/scrubbed-user/llvm-project/build/bin/clang++ -I 
/ptmp/scrubbed-user/llvm-project/build/projects/openmp/runtime/src -L 
/ptmp/scrubbed-user/llvm-project/build/projects/openmp/libomptarget/ -fopenmp 
-fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa 
-march=gfx908 reduced.cpp -g
  scrubbed-user@scrubbed-server: LIBOMPTARGET_DEBUG=1 ./a.out # only including 
relevant output, run yourself for the full verbose debug messaging
  
  PluginInterface --> Entry point 0x maps to 
__omp_offloading_4e_6ccfb3ae_main_l16 (0x55b886d524d8)
  Libomptarget --> Entry  0: Base=0x7ffd9cac727c, Begin=0x7ffd9cac727c, 
Size=4004, Type=0x20, Name=unknown
  Libomptarget --> Entry  1: Base=0x7ffd9cac727c, Begin=0x7ffd9cac727c, 
Size=4000, Type=0x10003, Name=unknown
  Libomptarget --> Entry  2: Base=0x7ffd9cac727c, Begin=0x7ffd9cac821c, 
Size=8, Type=0x10003, Name=unknown
  
  a.out:237581 terminated with signal 6 at PC=7f409bf30c6b SP=7ffd9cac6a00.  
Backtrace:
  /lib64/libc.so.6(gsignal+0x10d)[0x7f409bf30c6b]
  /lib64/libc.so.6(abort+0x177)[0x7f409bf32305]
  
/ptmp/scrubbed-user/llvm-project/build/bin/../lib/libomptarget.so.18git(+0x7452c1)[0x7f409ca652c1]
  
/ptmp/scrubbed-user/llvm-project/build/bin/../lib/libomptarget.so.18git(+0x740fe6)[0x7f409ca60fe6]
  
/ptmp/scrubbed-user/llvm-project/build/bin/../lib/libomptarget.so.18git(__tgt_target_kernel+0xe5)[0x7f409ca60335]
  ./a.out(+0x3385)[0x55b8850d5385]
  /lib64/libc.so.6(__libc_start_main+0xef)[0x7f409bf1b24d]
  ./a.out(+0x312a)[0x55b8850d512a]

If my understanding is correct, the combined entry should have a size equal to 
the highest pointer minus the lowest pointer (in the most ideal scenario). I'm 
not sure if upstream clang uses a tight or loose bounding box for the combined 
entry, but in any case, it's wrong. It should be either 4008 or 8000, depending 
on whether we are being clever or not.

Running in GDB:

  scrubbed-user@scrubbed-server: gdb a.out
  (gdb) r
  Starting program: 
/cray/css/users/scrubbed-user/sandbox/test_cleanup/target_depend_lvalue_01/reduced/upstream_build/a.out
 
  Missing separate debuginfos, use: zypper install 
glibc-debuginfo-2.31-150300.46.1.x86_64
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib64/libthread_db.so.1".
  [New Thread 0x7fffceb2c700 (LWP 247765)]
  [New Thread 0x7ffece1ff700 (LWP 247766)]
  [Thread 0x7ffece1ff700 (LWP 247766) exited]
  Libomptarget message: explicit extension not allowed: host address specified 
is 0x7fff786c (8 bytes), but device allocation maps to host at 
0x7fff68cc (4004 bytes)
  Libomptarget error: Call to getTargetPointer returned null pointer (device 
failure or illegal mapping).
  Libomptarget error: Call to targetDataBegin failed, abort target.
  Libomptarget error: Failed to process data before launching the kernel.
  Libomptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for 
debugging options.
  reduced.cpp:16:3: Libomptarget fatal error 1: failure of target construct 
while offloading is mandatory
  
  Thread 1 "a.out" received signal SIGABRT, Aborted.
  0x762ccc6b in raise () from /lib64/libc.so.6
  Missing separate debuginfos, use: zypper install 
comgr5.5.0-debuginfo-2.5.0.50500-sles153.63.x86_64 
hip-runtime-amd5.5.0-debuginfo-5.5.30201.50500-sles153.63.x86_64 
hsa-rocr5.5.0-debuginfo-1.8.0.50500-sles153.63.x86_64 
libatomic1-debuginfo-12.2.1+git416-15.1.7.1.x86_64 
libdrm2-debuginfo-2.4.107-150400.1.8.x86_64 
libdrm_amdgpu1-debuginfo-2.4.107-150400.1.8.x86_64 
libefa1-debuginfo-38.1-150400.4.6.x86_64 
libelf1-debuginfo-0.185-150400.5.3.1.x86_64 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};

tbaeder wrote:
> aaronpuchert wrote:
> > As with the cleanup function, a definition shouldn't be necessary.
> Is there a way to test whether the contents of the cleanup function are being 
> checked as well? From these tests, I only know we consider them called, but 
> not whether we (properly) analyze their bodies in the context as well. Or is 
> that separate from this patch?
For now we're just adding a new element to the CFG and adapting the respective 
tests. The CFG is generated on a per-function basis, and the generation of one 
function's CFG will never look into another function's body. It might use some 
(declaration) properties of course, like whether it has `[[noreturn]]` or 
`noexcept`. Of course we should also generate a CFG for the cleanup function, 
but that's independent of this change.

Users of the CFG will naturally need to be taught about this new element type 
to “understand” it. Otherwise they should simply skip it. Since the CFG 
previously did not contain such elements, there should be no change for them. 
So we can also initially just add the element and not tell anybody about it.

We could also add understanding of the new element type to other CFG users, but 
you don't have to do this. If you only care about Thread Safety Analysis, then 
it's totally fine to handle it only there.

But let's move all changes to Thread Safety Analysis into a follow-up, so that 
we don't have to bother the CFG maintainers with that.


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

https://reviews.llvm.org/D157385

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

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

MaskRay wrote:
> dexonsmith wrote:
> > MaskRay wrote:
> > > dexonsmith wrote:
> > > > MaskRay wrote:
> > > > > hans wrote:
> > > > > > MaskRay wrote:
> > > > > > > hans wrote:
> > > > > > > > dexonsmith wrote:
> > > > > > > > > MaskRay wrote:
> > > > > > > > > > dexonsmith wrote:
> > > > > > > > > > > Why would we want to use the old name here? An alias 
> > > > > > > > > > > seems strictly better to me. 
> > > > > > > > > > Making `overriding-t-option` an alias for 
> > > > > > > > > > `overriding-option` would make `-Wno-overriding-t-option` 
> > > > > > > > > > applies to future overriding option diagnostics, which is 
> > > > > > > > > > exactly what I want to avoid.
> > > > > > > > > > 
> > > > > > > > > I understand that you don't want `-t-` to apply to work on 
> > > > > > > > > future overriding option diagnostics, but I think the 
> > > > > > > > > platform divergence you're adding here is worse.
> > > > > > > > > 
> > > > > > > > > Having a few Darwin-specific options use 
> > > > > > > > > `-Woverriding-t-option` (and everything else use 
> > > > > > > > > `-Woverriding-option`) as the canonical spelling is hard to 
> > > > > > > > > reason about for maintainers, and for users.
> > > > > > > > > 
> > > > > > > > > And might not users on other platforms have 
> > > > > > > > > `-Woverriding-t-option` hardcoded in  build settings? (So 
> > > > > > > > > @dblaikie's argument that we shouldn't arbitrarily make 
> > > > > > > > > things hard for users would apply to all options?)
> > > > > > > > > 
> > > > > > > > > IMO, if we're not comfortable removing 
> > > > > > > > > `-Woverriding-t-option` entirely, then it should live on as 
> > > > > > > > > an alias (easy to reason about), not as 
> > > > > > > > > canonical-in-special-cases (hard to reason about).
> > > > > > > > > IMO, if we're not comfortable removing -Woverriding-t-option 
> > > > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > > > about), not as canonical-in-special-cases (hard to reason 
> > > > > > > > > about).
> > > > > > > > 
> > > > > > > > +1 if we can't drop the old spelling, an alias seems like the 
> > > > > > > > best option.
> > > > > > > Making `overriding-t-option` an alias for `overriding-option`, as 
> > > > > > > I mentioned, will make `-Wno-overriding-t-option` affect new 
> > > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > > 
> > > > > > > I know there are some `-Wno-overriding-t-option` uses. Honestly, 
> > > > > > > they are far fewer than other diagnostics we are introducing or 
> > > > > > > changing in Clang. I can understand the argument "make -Werror 
> > > > > > > users easier for this specific diagnostic" (but `-Werror` will 
> > > > > > > complain about other new diagnostics), but do we really want to 
> > > > > > > in the Darwin case? I think no. They can remove the version from 
> > > > > > > the target triple like 
> > > > > > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > > > > > 　or make the version part consistent with `-m.*os-version-min`.
> > > > > > > 
> > > > > > > This change may force these users to re-think how they should fix 
> > > > > > >  their build. I apology to these users, but I don't feel that 
> > > > > > > adding an alias is really necessary.
> > > > > > > Making overriding-t-option an alias for overriding-option, as I 
> > > > > > > mentioned, will make -Wno-overriding-t-option affect new 
> > > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > 
> > > > > > Is keeping them separate actually important, though? 
> > > > > > -Wno-overriding-option has the same issue in that case, that using 
> > > > > > the flag will also affect any new overriding-options uses, and I 
> > > > > > don't think that's a problem.
> > > > > `-Wno-overriding-option` is properly named, so affecting new 
> > > > > overriding-options uses looks fine to me.
> > > > > `-Wno-overriding-t-option` is awkward, and making it affect new uses 
> > > > > makes me nervous.
> > > > > 
> > > > > The gist of my previous comment is whether the uses cases really 
> > > > > justify a tiny bit of tech bit in clang and I think the answer is no.
> > > > > 
> > > > > This change is not about changing a semantic warning that has mixed 
> > > > > opinions, e.g. `-Wbitwise-op-parentheses` (many consider it not 
> > > > > justified).
> > > > > The gist of my previous comment is whether the uses cases really 
> 

[PATCH] D158534: [clang-tidy] Disable trivially-destructible check for darwin

[Aditya Kumar via Phabricator via cfe-commits]

hiraditya abandoned this revision.
hiraditya added a comment.

In D158534#4607719 , @PiotrZSL wrote:

> 626849c71e85d546a004cc91866beab610222194 
>  didn't 
> fix this issue already ?

ah you're right. that fixes it. i should have tested on lastest clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158534

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

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

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

reverted in 9b6b6bbc9127d604881cdc9dcea0e7c74ef821fd 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D158558: [clang] - Add missing builtin name to AtomicExpr JSON dump

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added a reviewer: aaron.ballman.
Herald added a project: All.
serge-sans-paille requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As a side effect, introduce AtomicExpr::getOpAsString() to dump the
AtomicOp string representation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158558

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/JSONNodeDumper.h
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-atomic-json.c

Index: clang/test/AST/ast-dump-atomic-json.c
===
--- /dev/null
+++ clang/test/AST/ast-dump-atomic-json.c
@@ -0,0 +1,128 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -ast-dump=json %s | FileCheck %s
+
+int foo(int * ptr) {
+  return __atomic_load_n(ptr, __ATOMIC_SEQ_CST);
+}
+
+// NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
+// using --filters=AtomicExpr
+
+
+// CHECK-NOT: {{^}}Dumping
+// CHECK:  "kind": "AtomicExpr",
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"offset": 132,
+// CHECK-NEXT:"col": 10,
+// CHECK-NEXT:"tokLen": 15
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"offset": 169,
+// CHECK-NEXT:"col": 47,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "type": {
+// CHECK-NEXT:   "qualType": "int"
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "valueCategory": "prvalue",
+// CHECK-NEXT:  "name": "__atomic_load_n",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "ImplicitCastExpr",
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "offset": 148,
+// CHECK-NEXT:  "col": 26,
+// CHECK-NEXT:  "tokLen": 3
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "offset": 148,
+// CHECK-NEXT:  "col": 26,
+// CHECK-NEXT:  "tokLen": 3
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"type": {
+// CHECK-NEXT: "qualType": "int *"
+// CHECK-NEXT:},
+// CHECK-NEXT:"valueCategory": "prvalue",
+// CHECK-NEXT:"castKind": "LValueToRValue",
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x{{.*}}",
+// CHECK-NEXT:  "kind": "DeclRefExpr",
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"offset": 148,
+// CHECK-NEXT:"col": 26,
+// CHECK-NEXT:"tokLen": 3
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"offset": 148,
+// CHECK-NEXT:"col": 26,
+// CHECK-NEXT:"tokLen": 3
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "type": {
+// CHECK-NEXT:   "qualType": "int *"
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "valueCategory": "lvalue",
+// CHECK-NEXT:  "referencedDecl": {
+// CHECK-NEXT:   "id": "0x{{.*}}",
+// CHECK-NEXT:   "kind": "ParmVarDecl",
+// CHECK-NEXT:   "name": "ptr",
+// CHECK-NEXT:   "type": {
+// CHECK-NEXT:"qualType": "int *"
+// CHECK-NEXT:   }
+// CHECK-NEXT:  }
+// CHECK-NEXT: }
+// CHECK-NEXT:]
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "IntegerLiteral",
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "spellingLoc": {
+// CHECK-NEXT:   "offset": 417,
+// CHECK-NEXT:   "file": "",
+// CHECK-NEXT:   "line": 13,
+// CHECK-NEXT:   "presumedLine": 12,
+// CHECK-NEXT:   "col": 26,
+// CHECK-NEXT:   "tokLen": 1
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "expansionLoc": {
+// CHECK-NEXT:   "offset": 153,
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 4,
+// CHECK-NEXT:   "col": 31,
+// CHECK-NEXT:   "tokLen": 16
+// CHECK-NEXT:  }
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "spellingLoc": {
+// CHECK-NEXT:   "offset": 417,
+// CHECK-NEXT:   "file": "",
+// CHECK-NEXT:   "line": 13,
+// CHECK-NEXT:   "presumedLine": 12,
+// CHECK-NEXT:   "col": 26,
+// CHECK-NEXT:   "tokLen": 1
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "expansionLoc": {
+// CHECK-NEXT:   "offset": 153,
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 4,
+// CHECK-NEXT:   "col": 31,
+// CHECK-NEXT:   "tokLen": 16
+// CHECK-NEXT:  }
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"type": {
+// CHECK-NEXT: "qualType": "int"
+// CHECK-NEXT:},
+// CHECK-NEXT:"valueCategory": "prvalue",
+// CHECK-NEXT:"value": "5"
+// CHECK-NEXT:   }
+// CHECK-NEXT:  ]
+// CHECK-NEXT: }
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ 

[clang] 9b6b6bb - Revert "[Profile] Allow online merging with debug info correlation."

[Arthur Eubanks via cfe-commits]

Author: Arthur Eubanks
Date: 2023-08-22T14:34:46-07:00
New Revision: 9b6b6bbc9127d604881cdc9dcea0e7c74ef821fd

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

LOG: Revert "[Profile] Allow online merging with debug info correlation."

This reverts commit cf2cf195d5fb0a07c122c5c274bd6deb0790e015.
This breaks merging profiles when nothing is instrumented, see comments in 
https://reviews.llvm.org/D157632.

This also reverts follow-up commit bfc965c54fdbfe33a0785b45903b53fc11165f13.

Added: 


Modified: 
clang/lib/CodeGen/BackendUtil.cpp
compiler-rt/lib/profile/InstrProfilingMerge.c
compiler-rt/test/profile/Darwin/instrprof-debug-info-correlate.c
compiler-rt/test/profile/Linux/instrprof-debug-info-correlate.c
compiler-rt/test/profile/instrprof-merge-error.c

Removed: 




diff  --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index 3e8b2b78a3928b..373d38672284d6 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -102,7 +102,7 @@ namespace {
 
 // Default filename used for profile generation.
 std::string getDefaultProfileGenName() {
-  return DebugInfoCorrelate ? "default_%m.proflite" : "default_%m.profraw";
+  return DebugInfoCorrelate ? "default_%p.proflite" : "default_%m.profraw";
 }
 
 class EmitAssemblyHelper {

diff  --git a/compiler-rt/lib/profile/InstrProfilingMerge.c 
b/compiler-rt/lib/profile/InstrProfilingMerge.c
index fdf7bd0c157ae2..241891da454bb4 100644
--- a/compiler-rt/lib/profile/InstrProfilingMerge.c
+++ b/compiler-rt/lib/profile/InstrProfilingMerge.c
@@ -47,6 +47,7 @@ uint64_t lprofGetLoadModuleSignature(void) {
 COMPILER_RT_VISIBILITY
 int __llvm_profile_check_compatibility(const char *ProfileData,
uint64_t ProfileSize) {
+  /* Check profile header only for now  */
   __llvm_profile_header *Header = (__llvm_profile_header *)ProfileData;
   __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
   SrcDataStart =
@@ -101,6 +102,13 @@ static uintptr_t signextIfWin64(void *V) {
 COMPILER_RT_VISIBILITY
 int __llvm_profile_merge_from_buffer(const char *ProfileData,
  uint64_t ProfileSize) {
+  if (__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE) {
+PROF_ERR(
+"%s\n",
+"Debug info correlation does not support profile merging at runtime. "
+"Instead, merge raw profiles using the llvm-profdata tool.");
+return 1;
+  }
   if (__llvm_profile_get_version() & VARIANT_MASK_TEMPORAL_PROF) {
 PROF_ERR("%s\n",
  "Temporal profiles do not support profile merging at runtime. "
@@ -110,8 +118,7 @@ int __llvm_profile_merge_from_buffer(const char 
*ProfileData,
 
   __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
   __llvm_profile_header *Header = (__llvm_profile_header *)ProfileData;
-  char *SrcCountersStart, *DstCounter;
-  const char *SrcCountersEnd, *SrcCounter;
+  char *SrcCountersStart;
   const char *SrcNameStart;
   const char *SrcValueProfDataStart, *SrcValueProfData;
   uintptr_t CountersDelta = Header->CountersDelta;
@@ -121,36 +128,14 @@ int __llvm_profile_merge_from_buffer(const char 
*ProfileData,
   Header->BinaryIdsSize);
   SrcDataEnd = SrcDataStart + Header->NumData;
   SrcCountersStart = (char *)SrcDataEnd;
-  SrcCountersEnd = SrcCountersStart +
-   Header->NumCounters * __llvm_profile_counter_entry_size();
-  SrcNameStart = SrcCountersEnd;
+  SrcNameStart = SrcCountersStart +
+ Header->NumCounters * __llvm_profile_counter_entry_size();
   SrcValueProfDataStart =
   SrcNameStart + Header->NamesSize +
   __llvm_profile_get_num_padding_bytes(Header->NamesSize);
   if (SrcNameStart < SrcCountersStart)
 return 1;
 
-  // Merge counters when there is no data section and debug info correlation is
-  // enabled.
-  if (Header->NumData == 0) {
-if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
-  PROF_ERR("%s\n", "Missing profile data section.");
-  return 1;
-}
-for (SrcCounter = SrcCountersStart,
-DstCounter = __llvm_profile_begin_counters();
- SrcCounter < SrcCountersEnd;) {
-  if (__llvm_profile_get_version() & VARIANT_MASK_BYTE_COVERAGE) {
-*DstCounter &= *SrcCounter;
-  } else {
-*(uint64_t *)DstCounter += *(uint64_t *)SrcCounter;
-  }
-  SrcCounter += __llvm_profile_counter_entry_size();
-  DstCounter += __llvm_profile_counter_entry_size();
-}
-return 0;
-  }
-
   for (SrcData = SrcDataStart,
   DstData = (__llvm_profile_data *)__llvm_profile_begin_data(),
   SrcValueProfData = SrcValueProfDataStart;

diff  --git 

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on 
these?  If someone passes a negative size, we should probably at least do the 
warning that it was converted/truncated.




Comment at: clang/lib/AST/ExprConstant.cpp:9357
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))

Context missing throughout


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

https://reviews.llvm.org/D158557

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

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: aaron.ballman, erichkeane, davide, rsmith.
Herald added a project: All.
shafik requested review of this revision.

The implementation of `__builtin_strncmp` and other related builtins function 
use `getExtValue()` to evaluate the size argument. This can cause a crash when 
the value does not fit into an `int64_t` value, which is can be expected since 
the type of the argument is `size_t`.

The fix is to switch to using `getZExtValue()`.

This fixes:

https://github.com/llvm/llvm-project/issues/64876


https://reviews.llvm.org/D158557

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-string.cpp


Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -676,3 +676,14 @@
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error 
{{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, -511LL);
+  __builtin_memcmp(0, 0, -511LL);
+  __builtin_bcmp(0, 0, -511LL);
+  __builtin_wmemcmp(0, 0, -511LL);
+  __builtin_memchr((const void*)0, 1, -511LL);
+  __builtin_wmemchr((const wchar_t*)0, 1, -511LL);
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9357,7 +9357,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 // We cannot find the value if there are no candidates to match against.
 if (MaxLength == 0u)
@@ -12381,7 +12381,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 
 // Empty substrings compare equal by definition.


Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -676,3 +676,14 @@
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error {{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, -511LL);
+  __builtin_memcmp(0, 0, -511LL);
+  __builtin_bcmp(0, 0, -511LL);
+  __builtin_wmemcmp(0, 0, -511LL);
+  __builtin_memchr((const void*)0, 1, -511LL);
+  __builtin_wmemchr((const wchar_t*)0, 1, -511LL);
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9357,7 +9357,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 // We cannot find the value if there are no candidates to match against.
 if (MaxLength == 0u)
@@ -12381,7 +12381,7 @@
   APSInt N;
   if (!EvaluateInteger(E->getArg(2), N, Info))
 return false;
-  MaxLength = N.getExtValue();
+  MaxLength = N.getZExtValue();
 }
 
 // Empty substrings compare equal by definition.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

aeubanks wrote:
> ellis wrote:
> > aeubanks wrote:
> > > aeubanks wrote:
> > > > we're running into this error message even though we don't have debug 
> > > > info correlation turned on, is that expected in some configurations? 
> > > > I'm still trying to understand how we could end up in this code path 
> > > > without debug info correlation
> > > > 
> > > > https://crbug.com/1473935
> > > also it seems like this code path isn't tested?
> > It's very strange that you are hitting this code path because I didn't 
> > think it was possible. Were you ever able to reproduce locally?
> no, it's only showed up on some bots (both mac and linux), I'm still trying 
> to understand what's going on
managed to repro having no `DataSize` (`NumData` now).

```
$ cat /tmp/a.cc
int main() {}
$ cat /tmp/funlist
[clang]
default:skip
$ clang++ -O1 -fprofile-instr-generate -fcoverage-mapping -o /tmp/a 
-fuse-ld=lld /tmp/z.cc -fprofile-list=/tmp/funlist
$ export LLVM_PROFILE_FILE='a%m.profraw'
$ /tmp/a
$ /tmp/a
LLVM Profile Error: Missing profile data section.
LLVM Profile Error: Invalid profile data to merge
LLVM Profile Error: Profile Merging of file a18405209413954990729_0.profraw 
failed: Success
LLVM Profile Error: Failed to write file "a18405209413954990729_0.profraw": 
Success
```

basically if nothing is instrumented we'll hit this

will revert


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D158474: [clang][ExtractAPI] Fix bool spelling coming from the macro definition.

[Erick Velez via Phabricator via cfe-commits]

evelez7 updated this revision to Diff 552504.
evelez7 added a comment.

Use clang instead of clang_cc1 like other C tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158474

Files:
  clang/include/clang/ExtractAPI/DeclarationFragments.h
  clang/lib/ExtractAPI/DeclarationFragments.cpp
  clang/test/ExtractAPI/bool.c
  clang/test/ExtractAPI/bool.cpp

Index: clang/test/ExtractAPI/bool.cpp
===
--- /dev/null
+++ clang/test/ExtractAPI/bool.cpp
@@ -0,0 +1,203 @@
+// 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
+bool Foo;
+
+bool IsFoo(bool Bar);
+/// 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": [],
+  "symbols": [
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "identifier",
+  "spelling": "Foo"
+},
+{
+  "kind": "text",
+  "spelling": ";"
+}
+  ],
+  "identifier": {
+"interfaceLanguage": "c++",
+"precise": "c:@Foo"
+  },
+  "kind": {
+"displayName": "Global Variable",
+"identifier": "c++.var"
+  },
+  "location": {
+"position": {
+  "character": 6,
+  "line": 1
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+"navigator": [
+  {
+"kind": "identifier",
+"spelling": "Foo"
+  }
+],
+"subHeading": [
+  {
+"kind": "identifier",
+"spelling": "Foo"
+  }
+],
+"title": "Foo"
+  },
+  "pathComponents": [
+"Foo"
+  ]
+},
+{
+  "accessLevel": "public",
+  "declarationFragments": [
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "identifier",
+  "spelling": "IsFoo"
+},
+{
+  "kind": "text",
+  "spelling": "("
+},
+{
+  "kind": "typeIdentifier",
+  "preciseIdentifier": "c:b",
+  "spelling": "bool"
+},
+{
+  "kind": "text",
+  "spelling": " "
+},
+{
+  "kind": "internalParam",
+  "spelling": "Bar"
+},
+{
+  "kind": "text",
+  "spelling": ");"
+}
+  ],
+  "functionSignature": {
+"parameters": [
+  {
+"declarationFragments": [
+  {
+"kind": "typeIdentifier",
+"preciseIdentifier": "c:b",
+"spelling": "bool"
+  },
+  {
+"kind": "text",
+"spelling": " "
+  },
+  {
+"kind": "internalParam",
+"spelling": "Bar"
+  }
+],
+"name": "Bar"
+  }
+],
+"returns": [
+  {
+"kind": "typeIdentifier",
+"preciseIdentifier": "c:b",
+"spelling": "bool"
+  }
+]
+  },
+  "identifier": {
+"interfaceLanguage": "c++",
+"precise": "c:@F@IsFoo#b#"
+  },
+  "kind": {
+"displayName": "Function",
+"identifier": "c++.func"
+  },
+  "location": {
+"position": {
+  "character": 6,
+  "line": 3
+},
+"uri": "file://INPUT_DIR/input.h"
+  },
+  "names": {
+"navigator": [
+  {
+"kind": "identifier",
+"spelling": "IsFoo"
+  }
+],
+

[PATCH] D158430: [flang][driver] Mark -fuse-ld as visible in Flang

[Hao Jin via Phabricator via cfe-commits]

erjin updated this revision to Diff 552498.
erjin added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158430

Files:
  clang/include/clang/Driver/Options.td
  flang/test/Driver/misc-flags.f90


Index: flang/test/Driver/misc-flags.f90
===
--- flang/test/Driver/misc-flags.f90
+++ flang/test/Driver/misc-flags.f90
@@ -4,6 +4,9 @@
 ! Make sure that `-Wl` is "visible" to Flang's driver
 ! RUN: %flang -Wl,abs -### %s
 
+! Make sure that `-fuse-ld' is "visible" to Flang's driver
+! RUN: %flang -fuse-ld= -### %s
+
 program hello
   write(*,*), "Hello world!"
 end program hello
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5887,7 +5887,7 @@
   "generated assembly will consider GNU as support. 'none' means that all ELF "
   "features can be used, regardless of binutils support. Defaults to 2.26.">;
 def fuse_ld_EQ : Joined<["-"], "fuse-ld=">, Group,
-  Flags<[LinkOption]>, Visibility<[ClangOption, CLOption]>;
+  Flags<[LinkOption]>, Visibility<[ClangOption, FlangOption, CLOption]>;
 def ld_path_EQ : Joined<["--"], "ld-path=">, Group;
 
 defm align_labels : BooleanFFlag<"align-labels">, 
Group;


Index: flang/test/Driver/misc-flags.f90
===
--- flang/test/Driver/misc-flags.f90
+++ flang/test/Driver/misc-flags.f90
@@ -4,6 +4,9 @@
 ! Make sure that `-Wl` is "visible" to Flang's driver
 ! RUN: %flang -Wl,abs -### %s
 
+! Make sure that `-fuse-ld' is "visible" to Flang's driver
+! RUN: %flang -fuse-ld= -### %s
+
 program hello
   write(*,*), "Hello world!"
 end program hello
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5887,7 +5887,7 @@
   "generated assembly will consider GNU as support. 'none' means that all ELF "
   "features can be used, regardless of binutils support. Defaults to 2.26.">;
 def fuse_ld_EQ : Joined<["-"], "fuse-ld=">, Group,
-  Flags<[LinkOption]>, Visibility<[ClangOption, CLOption]>;
+  Flags<[LinkOption]>, Visibility<[ClangOption, FlangOption, CLOption]>;
 def ld_path_EQ : Joined<["--"], "ld-path=">, Group;
 
 defm align_labels : BooleanFFlag<"align-labels">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:9512
 uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+  return false;

I think we should issue a diagnostic, we don't have any indication that this is 
failing, let alone because the destination object size is zero. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157252

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

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

dexonsmith wrote:
> MaskRay wrote:
> > dexonsmith wrote:
> > > MaskRay wrote:
> > > > hans wrote:
> > > > > MaskRay wrote:
> > > > > > hans wrote:
> > > > > > > dexonsmith wrote:
> > > > > > > > MaskRay wrote:
> > > > > > > > > dexonsmith wrote:
> > > > > > > > > > Why would we want to use the old name here? An alias seems 
> > > > > > > > > > strictly better to me. 
> > > > > > > > > Making `overriding-t-option` an alias for `overriding-option` 
> > > > > > > > > would make `-Wno-overriding-t-option` applies to future 
> > > > > > > > > overriding option diagnostics, which is exactly what I want 
> > > > > > > > > to avoid.
> > > > > > > > > 
> > > > > > > > I understand that you don't want `-t-` to apply to work on 
> > > > > > > > future overriding option diagnostics, but I think the platform 
> > > > > > > > divergence you're adding here is worse.
> > > > > > > > 
> > > > > > > > Having a few Darwin-specific options use 
> > > > > > > > `-Woverriding-t-option` (and everything else use 
> > > > > > > > `-Woverriding-option`) as the canonical spelling is hard to 
> > > > > > > > reason about for maintainers, and for users.
> > > > > > > > 
> > > > > > > > And might not users on other platforms have 
> > > > > > > > `-Woverriding-t-option` hardcoded in  build settings? (So 
> > > > > > > > @dblaikie's argument that we shouldn't arbitrarily make things 
> > > > > > > > hard for users would apply to all options?)
> > > > > > > > 
> > > > > > > > IMO, if we're not comfortable removing `-Woverriding-t-option` 
> > > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > > about), not as canonical-in-special-cases (hard to reason 
> > > > > > > > about).
> > > > > > > > IMO, if we're not comfortable removing -Woverriding-t-option 
> > > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > > about), not as canonical-in-special-cases (hard to reason 
> > > > > > > > about).
> > > > > > > 
> > > > > > > +1 if we can't drop the old spelling, an alias seems like the 
> > > > > > > best option.
> > > > > > Making `overriding-t-option` an alias for `overriding-option`, as I 
> > > > > > mentioned, will make `-Wno-overriding-t-option` affect new 
> > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > 
> > > > > > I know there are some `-Wno-overriding-t-option` uses. Honestly, 
> > > > > > they are far fewer than other diagnostics we are introducing or 
> > > > > > changing in Clang. I can understand the argument "make -Werror 
> > > > > > users easier for this specific diagnostic" (but `-Werror` will 
> > > > > > complain about other new diagnostics), but do we really want to in 
> > > > > > the Darwin case? I think no. They can remove the version from the 
> > > > > > target triple like 
> > > > > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > > > > 　or make the version part consistent with `-m.*os-version-min`.
> > > > > > 
> > > > > > This change may force these users to re-think how they should fix  
> > > > > > their build. I apology to these users, but I don't feel that adding 
> > > > > > an alias is really necessary.
> > > > > > Making overriding-t-option an alias for overriding-option, as I 
> > > > > > mentioned, will make -Wno-overriding-t-option affect new 
> > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > 
> > > > > Is keeping them separate actually important, though? 
> > > > > -Wno-overriding-option has the same issue in that case, that using 
> > > > > the flag will also affect any new overriding-options uses, and I 
> > > > > don't think that's a problem.
> > > > `-Wno-overriding-option` is properly named, so affecting new 
> > > > overriding-options uses looks fine to me.
> > > > `-Wno-overriding-t-option` is awkward, and making it affect new uses 
> > > > makes me nervous.
> > > > 
> > > > The gist of my previous comment is whether the uses cases really 
> > > > justify a tiny bit of tech bit in clang and I think the answer is no.
> > > > 
> > > > This change is not about changing a semantic warning that has mixed 
> > > > opinions, e.g. `-Wbitwise-op-parentheses` (many consider it not 
> > > > justified).
> > > > The gist of my previous comment is whether the uses cases really 
> > > > justify a tiny bit of tech bit in clang and I think the answer is no.
> > > > 
> > > 
> > > I think we agree that we should add the minimal technical debt to clang.
> 

[clang] f2b150b - Summary:

[via cfe-commits]

Author: zhijian
Date: 2023-08-22T16:29:22-04:00
New Revision: f2b150b4501ff8978ce19b0727225a6817be1dc7

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

LOG: Summary:

Fixed a clang: error: OBJECT_MODE setting any is not recognized and is not a 
valid setting
in 
https://lab.llvm.org/buildbot/#/builders/214/builds/9125/steps/6/logs/FAIL__Clang__dwarf-version_c

The error is caused by the modification of clang/test/lit.cfg.py of the commit 
of https://reviews.llvm.org/D142660

Added: 


Modified: 
clang/test/lit.cfg.py

Removed: 




diff  --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 815ea219aaa81e..60843ef8a14204 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -388,7 +388,9 @@ def exclude_unsupported_files_for_aix(dirname):
 # "OBJECT_MODE" to "any" by default on AIX OS.
 
 if "system-aix" in config.available_features:
-   config.environment["OBJECT_MODE"] = "any"
+   config.substitutions.append(("llvm-nm", "env OBJECT_MODE=any llvm-nm"))
+   config.substitutions.append(("llvm-ar", "env OBJECT_MODE=any llvm-ar"))
+   config.substitutions.append(("llvm-ranlib", "env OBJECT_MODE=any 
llvm-ranlib"))
 
 # It is not realistically possible to account for all options that could
 # possibly be present in system and user configuration files, so disable



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

[PATCH] D157565: [CodeGen] Add AArch64 behavior to existing MFS tests

[Daniel Hoekwater via Phabricator via cfe-commits]

dhoekwater marked an inline comment as done.
dhoekwater added a comment.

Relanded in 90ab85a1b2e72f63039fadf6669b23f52192defd 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157565

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

[PATCH] D157933: [OpenMP 5.1] Parsing and Sema support for `scope` construct

[Alexey Bataev via Phabricator via cfe-commits]

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

LG. Please, update docs/OpenMPSupport.rst


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

https://reviews.llvm.org/D157933

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

[PATCH] D157933: [OpenMP 5.1] Parsing and Sema support for `scope` construct

[Fazlay Rabbi via Phabricator via cfe-commits]

mdfazlay added a comment.

In D157933#4591627 , @ABataev wrote:

> Could you also add the nesting tests for outer scope directive? Currently it 
> tests only for inner

Added. Please take a look.


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

https://reviews.llvm.org/D157933

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

[PATCH] D157441: [-Wunsafe-buffer-usage] Use `Strategy` to determine whether to fix a parameter

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D157441#4575220 , @ziqingluo-90 
wrote:

>>> Extend PointerAssignmentGadget and PointerInitGadget to generate fix-its 
>>> for cases where the left-hand side 
>>> has won't fix strategy and the right-hand side has std::span strategy.
>>
>> Hmm, is this intertwined with other changes, or is this completely separate? 
>> Also my head appears to be blank;
>
> The two `FixableGadget`s previously gave incorrect fix-its for the cases 
> where only RHS need fix.  After the change made to `Strategy`, they returned 
> `std::nullopt` as the case was not implemented.
> So to avoid regression, I made the extension to the two Gadgets.
>
>> did we reach any conclusion about the nature of "single" RHS-es and 
>> usefulness of annotating them as such?
>> Which could be an approach that would encourage people to propagate more 
>> bounds in the backwards direction by default.
>
> Can you elaborate the question a bit more?

If previously we were emitting an incorrect fixit and now we're not emitting 
any fixit, then I think it's a very good improvement, not a regression 樂

Uhh, I meant LHSes, sorry 

Anyway, the point was that it's hard to tell whether `.data()` is the right 
fix, so I think we wanted to hold off implementing it, because we probably want 
to do something better instead in most cases. Like, implement a strategy 
discovery procedure that would give us a better idea of //how exactly// is the 
LHS pointer used safely. Then we might emit different fixes based on that. So I 
suspect that it should be a separate effort, so it probably makes sense to 
separate it into a different patch and think more about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157441

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

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

MaskRay wrote:
> dexonsmith wrote:
> > MaskRay wrote:
> > > hans wrote:
> > > > MaskRay wrote:
> > > > > hans wrote:
> > > > > > dexonsmith wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > dexonsmith wrote:
> > > > > > > > > Why would we want to use the old name here? An alias seems 
> > > > > > > > > strictly better to me. 
> > > > > > > > Making `overriding-t-option` an alias for `overriding-option` 
> > > > > > > > would make `-Wno-overriding-t-option` applies to future 
> > > > > > > > overriding option diagnostics, which is exactly what I want to 
> > > > > > > > avoid.
> > > > > > > > 
> > > > > > > I understand that you don't want `-t-` to apply to work on future 
> > > > > > > overriding option diagnostics, but I think the platform 
> > > > > > > divergence you're adding here is worse.
> > > > > > > 
> > > > > > > Having a few Darwin-specific options use `-Woverriding-t-option` 
> > > > > > > (and everything else use `-Woverriding-option`) as the canonical 
> > > > > > > spelling is hard to reason about for maintainers, and for users.
> > > > > > > 
> > > > > > > And might not users on other platforms have 
> > > > > > > `-Woverriding-t-option` hardcoded in  build settings? (So 
> > > > > > > @dblaikie's argument that we shouldn't arbitrarily make things 
> > > > > > > hard for users would apply to all options?)
> > > > > > > 
> > > > > > > IMO, if we're not comfortable removing `-Woverriding-t-option` 
> > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > about), not as canonical-in-special-cases (hard to reason about).
> > > > > > > IMO, if we're not comfortable removing -Woverriding-t-option 
> > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > about), not as canonical-in-special-cases (hard to reason about).
> > > > > > 
> > > > > > +1 if we can't drop the old spelling, an alias seems like the best 
> > > > > > option.
> > > > > Making `overriding-t-option` an alias for `overriding-option`, as I 
> > > > > mentioned, will make `-Wno-overriding-t-option` affect new 
> > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > 
> > > > > I know there are some `-Wno-overriding-t-option` uses. Honestly, they 
> > > > > are far fewer than other diagnostics we are introducing or changing 
> > > > > in Clang. I can understand the argument "make -Werror users easier 
> > > > > for this specific diagnostic" (but `-Werror` will complain about 
> > > > > other new diagnostics), but do we really want to in the Darwin case? 
> > > > > I think no. They can remove the version from the target triple like 
> > > > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > > > 　or make the version part consistent with `-m.*os-version-min`.
> > > > > 
> > > > > This change may force these users to re-think how they should fix  
> > > > > their build. I apology to these users, but I don't feel that adding 
> > > > > an alias is really necessary.
> > > > > Making overriding-t-option an alias for overriding-option, as I 
> > > > > mentioned, will make -Wno-overriding-t-option affect new 
> > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > 
> > > > Is keeping them separate actually important, though? 
> > > > -Wno-overriding-option has the same issue in that case, that using the 
> > > > flag will also affect any new overriding-options uses, and I don't 
> > > > think that's a problem.
> > > `-Wno-overriding-option` is properly named, so affecting new 
> > > overriding-options uses looks fine to me.
> > > `-Wno-overriding-t-option` is awkward, and making it affect new uses 
> > > makes me nervous.
> > > 
> > > The gist of my previous comment is whether the uses cases really justify 
> > > a tiny bit of tech bit in clang and I think the answer is no.
> > > 
> > > This change is not about changing a semantic warning that has mixed 
> > > opinions, e.g. `-Wbitwise-op-parentheses` (many consider it not 
> > > justified).
> > > The gist of my previous comment is whether the uses cases really justify 
> > > a tiny bit of tech bit in clang and I think the answer is no.
> > > 
> > 
> > I think we agree that we should add the minimal technical debt to clang.
> > 
> > This patch is harder-to-reason about, and thus bigger IMO, technical debt 
> > than adding an alias would be.
> Honestly when people asked whether we need back compatibility for `-Werror` 
> uses. I disagree with the idea after 

[PATCH] D156032: Implement CWG2137 (list-initialization from objects of the same type)

[Mital Ashok via Phabricator via cfe-commits]

MitalAshok updated this revision to Diff 552477.
MitalAshok added a comment.

Address comments; better implementation for elision (check after considering 
only initializer list constructors)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156032

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CXX/drs/dr14xx.cpp
  clang/test/CXX/drs/dr21xx.cpp
  clang/test/CXX/drs/dr23xx.cpp
  clang/www/cxx_dr_status.html
  libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp

Index: libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
===
--- libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
+++ libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
@@ -121,7 +121,26 @@
 test_pair_rv();
 test_pair_rv();
 
-test_pair_rv();
+/* For ExplicitTypes::CopyOnly, two of the viable candidates for initializing from a non-const xvalue are:
+ *   pair(const pair&);  // (defaulted copy constructor)
+ *   template explicit pair(const pair&&); [U1 = ExplicitTypes::CopyOnly, U2 = int]
+ * This results in diverging behavior for test_convertible which uses copy-list-initialization
+ * Prior to CWG2137, this would have selected the first (non-explicit) ctor as explicit ctors would not be considered
+ * Afterwards, it should select the second since it is a better match, and then failed because it is explicit
+ *
+ * This may change with future defect reports, and some compilers only have partial support for CWG2137,
+ * so use std::is_convertible directly to avoid a copy-list-initialization
+ */
+{
+  using P1  = std::pair;
+  using P2  = std::pair;
+  using UP1 = std::pair&&;
+  using UP2 = std::pair&&;
+  static_assert(std::is_constructible::value, "");
+  static_assert(std::is_convertible::value, "");
+  static_assert(std::is_constructible::value, "");
+  static_assert(std::is_convertible::value, "");
+}
 test_pair_rv();
 test_pair_rv();
 
Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -12629,7 +12629,7 @@
 https://cplusplus.github.io/CWG/issues/2137.html;>2137
 CD4
 List-initialization from object of same type
-Unknown
+Clang 18
   
   
 https://cplusplus.github.io/CWG/issues/2138.html;>2138
@@ -13673,7 +13673,7 @@
 https://cplusplus.github.io/CWG/issues/2311.html;>2311
 open
 Missed case for guaranteed copy elision
-Not resolved
+Clang 18
   
   
 https://cplusplus.github.io/CWG/issues/2312.html;>2312
Index: clang/test/CXX/drs/dr23xx.cpp
===
--- clang/test/CXX/drs/dr23xx.cpp
+++ clang/test/CXX/drs/dr23xx.cpp
@@ -5,6 +5,16 @@
 // RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -std=c++23 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 
+namespace std {
+  __extension__ typedef __SIZE_TYPE__ size_t;
+
+  template struct initializer_list {
+const E *p; size_t n;
+initializer_list(const E *p, size_t n);
+initializer_list();
+  };
+}
+
 #if __cplusplus >= 201103L
 namespace dr2303 { // dr2303: 12
 template 
@@ -37,6 +47,80 @@
 } //namespace dr2303
 #endif
 
+namespace dr2311 {  // dr2311: 18 open
+#if __cplusplus >= 201707L
+template
+void test() {
+  // Ensure none of these expressions try to call a move constructor.
+  T a = T{T(0)};
+  T b{T(0)};
+  auto c{T(0)};
+  T d = {T(0)};
+  auto e = {T(0)};
+#if __cplusplus >= 202302L
+  auto f = auto{T(0)};
+#endif
+  void(*fn)(T);
+  fn({T(0)});
+}
+
+struct NonMovable {
+  NonMovable(int);
+  NonMovable(NonMovable&&) = delete;
+};
+struct NonMovableNonApplicableIList {
+  NonMovableNonApplicableIList(int);
+  NonMovableNonApplicableIList(NonMovableNonApplicableIList&&) = delete;
+  NonMovableNonApplicableIList(std::initializer_list);
+};
+struct ExplicitMovable {
+  ExplicitMovable(int);
+  explicit ExplicitMovable(ExplicitMovable&&);
+};
+struct ExplicitNonMovable {
+  ExplicitNonMovable(int);
+  explicit ExplicitNonMovable(ExplicitNonMovable&&) = delete;
+};
+struct ExplicitNonMovableNonApplicableIList {
+  ExplicitNonMovableNonApplicableIList(int);
+  explicit ExplicitNonMovableNonApplicableIList(ExplicitNonMovableNonApplicableIList&&) = delete;
+  ExplicitNonMovableNonApplicableIList(std::initializer_list);
+};
+struct CopyOnly {
+  CopyOnly(int);
+  CopyOnly(const CopyOnly&);
+  CopyOnly(CopyOnly&&) = delete;
+};
+struct 

[PATCH] D158488: [NFC] Initialize member pointers to nullptr.

[Sindhu Chittireddy via Phabricator via cfe-commits]

schittir abandoned this revision.
schittir added a comment.

In D158488#4607754 , @Manna wrote:

> This has already been addressed by https://reviews.llvm.org/D157989

Thank you for letting me know. Abandoning this change.


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

https://reviews.llvm.org/D158488

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

[PATCH] D158540: Improve error message for constexpr constructors of virtual base classes

[Nouman Amir via Phabricator via cfe-commits]

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

The changes are for better diagnostic/error-messages. The error message of 
Clang, MSVC, and GCC were compared and MSVC gives more detailed error message 
so that is used now.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158540

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td


Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9393,8 +9393,7 @@
   "the parameter for an explicitly-defaulted copy assignment operator must be 
an "
   "lvalue reference type">;
 def err_incorrect_defaulted_constexpr : Error<
-  "defaulted definition of %sub{select_special_member_kind}0 "
-  "is not constexpr">;
+  "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class or 
struct with virtual base classes">;
 def err_incorrect_defaulted_consteval : Error<
   "defaulted declaration of %sub{select_special_member_kind}0 "
   "cannot be consteval because implicit definition is not constexpr">;


Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9393,8 +9393,7 @@
   "the parameter for an explicitly-defaulted copy assignment operator must be an "
   "lvalue reference type">;
 def err_incorrect_defaulted_constexpr : Error<
-  "defaulted definition of %sub{select_special_member_kind}0 "
-  "is not constexpr">;
+  "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class or struct with virtual base classes">;
 def err_incorrect_defaulted_consteval : Error<
   "defaulted declaration of %sub{select_special_member_kind}0 "
   "cannot be consteval because implicit definition is not constexpr">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] c3c8f16 - Fixing the memory leak using split() instead of strtok

[Abhina Sreeskantharajan via cfe-commits]

Author: Harini Chilamantula
Date: 2023-08-22T15:26:34-04:00
New Revision: c3c8f16fc32cccf5239a2aedd10c6be9babf2945

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

LOG: Fixing the memory leak using split() instead of strtok

Reviewed By: abhina.sreeskantharajan

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

Added: 


Modified: 
clang/lib/Driver/ToolChains/ZOS.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/ZOS.cpp 
b/clang/lib/Driver/ToolChains/ZOS.cpp
index db10567ca28ec2..05548fbec68b63 100644
--- a/clang/lib/Driver/ToolChains/ZOS.cpp
+++ b/clang/lib/Driver/ToolChains/ZOS.cpp
@@ -188,11 +188,10 @@ void zos::Linker::ConstructJob(Compilation , const 
JobAction ,
   CmdArgs.push_back(
   Args.MakeArgString("//'" + LEHLQ + ".SCEELIB(CELQS003)'"));
 } else {
-  char *ld_side_deck = strdup(ld_env_var.str().c_str());
-  ld_side_deck = strtok(ld_side_deck, ":");
-  while (ld_side_deck != nullptr) {
-CmdArgs.push_back(ld_side_deck);
-ld_side_deck = strtok(nullptr, ":");
+  SmallVector ld_side_deck;
+  ld_env_var.split(ld_side_deck, ":");
+  for (StringRef ld_loc : ld_side_deck) {
+CmdArgs.push_back((ld_loc.str()).c_str());
   }
 }
   }



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

[PATCH] D158254: Fixing the memory leak using split() instead of strtok

[Abhina Sree 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 rGc3c8f16fc32c: Fixing the memory leak using split() instead 
of strtok (authored by hchilama, committed by abhina.sreeskantharajan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158254

Files:
  clang/lib/Driver/ToolChains/ZOS.cpp


Index: clang/lib/Driver/ToolChains/ZOS.cpp
===
--- clang/lib/Driver/ToolChains/ZOS.cpp
+++ clang/lib/Driver/ToolChains/ZOS.cpp
@@ -188,11 +188,10 @@
   CmdArgs.push_back(
   Args.MakeArgString("//'" + LEHLQ + ".SCEELIB(CELQS003)'"));
 } else {
-  char *ld_side_deck = strdup(ld_env_var.str().c_str());
-  ld_side_deck = strtok(ld_side_deck, ":");
-  while (ld_side_deck != nullptr) {
-CmdArgs.push_back(ld_side_deck);
-ld_side_deck = strtok(nullptr, ":");
+  SmallVector ld_side_deck;
+  ld_env_var.split(ld_side_deck, ":");
+  for (StringRef ld_loc : ld_side_deck) {
+CmdArgs.push_back((ld_loc.str()).c_str());
   }
 }
   }


Index: clang/lib/Driver/ToolChains/ZOS.cpp
===
--- clang/lib/Driver/ToolChains/ZOS.cpp
+++ clang/lib/Driver/ToolChains/ZOS.cpp
@@ -188,11 +188,10 @@
   CmdArgs.push_back(
   Args.MakeArgString("//'" + LEHLQ + ".SCEELIB(CELQS003)'"));
 } else {
-  char *ld_side_deck = strdup(ld_env_var.str().c_str());
-  ld_side_deck = strtok(ld_side_deck, ":");
-  while (ld_side_deck != nullptr) {
-CmdArgs.push_back(ld_side_deck);
-ld_side_deck = strtok(nullptr, ":");
+  SmallVector ld_side_deck;
+  ld_env_var.split(ld_side_deck, ":");
+  for (StringRef ld_loc : ld_side_deck) {
+CmdArgs.push_back((ld_loc.str()).c_str());
   }
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

In D148997#4561620 , @v.g.vassilev 
wrote:

> So, in that case we should bring back the boolean flag for incremental 
> processing and keep the `IncrementalExtensions` LanguageOption separate. In 
> that case `IncrementalExtensions` would mean that we must turn on incremental 
> processing for managing lifetime and only use the language option when 
> extending the parsing logic. However, I think the problem would be what to do 
> with the `tok::eof` and `tok::annot_repl_input_end`? I'd probably need 
> @aaron.ballman or @rsmith here...

Would you be happy to make that change, or should I put it up? Separating the 
options and what to do about the token in general could be separate PRs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148997

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

[PATCH] D158363: [clang-format] Fix segmentation fault when formatting nested namespaces

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

HazardyKnusperkeks added a comment.

In D158363#4606159 , @d0nc1h0t wrote:

> In D158363#4604468 , 
> @HazardyKnusperkeks wrote:
>
>> Please upload the patch with the full context.
>
> I'm creating a patch via 'git diff' from the root of the project. What does 
> full context mean?

If you look at your diff here there is `Context not available.`. Please use 
`git diff -U9`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158363

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

[PATCH] D158254: Fixing the memory leak using split() instead of strtok

[Harini Chilamantula via Phabricator via cfe-commits]

hchilama added a comment.

Please use my Intel email Chilamantula, Harini  .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158254

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

[PATCH] D158538: [MS-ABI] Remove comdat attribute for inheriting ctor.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 created this revision.
jyu2 added reviewers: rnk, rsmith, majnemer, cfe-commits.
jyu2 added a project: clang.
Herald added a project: All.
jyu2 requested review of this revision.

Currently, for MS, the linkage for the inheriting constructors is set to
internal.  However, the comdat attribute is also set like:

define internal noundef ptr @"??0?$B@_N@@qeaa@AEBVF@@aebua@@@z"(ptr noundef 
nonnull returned align 1 dereferenceable(1) %this, ptr noundef nonnull align 1 
dereferenceable(1) %0, ptr noundef nonnull align 1 dereferenceable(1) %1) 
unnamed_addr comdat

This could cause linker to fail.

The change is to remove comdat attribute for the inheriting constructor
to make linker happy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158538

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/CodeGenCXX/ms-inheriting-ctor.cpp


Index: clang/test/CodeGenCXX/ms-inheriting-ctor.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ms-inheriting-ctor.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -fcxx-exceptions -triple=x86_64-windows-msvc -emit-llvm %s 
-o - | FileCheck %s
+
+class F {
+public:
+  F(wchar_t *);
+};
+using a = F;
+struct A {};
+struct b {
+  b(a, F, A);
+};
+template  struct c : b {
+  c(const a , const A ) : b(p1, 0, d) {}
+};
+template  struct B : c {
+  using c::c;
+};
+class f {
+public:
+  f(...);
+}
+
+typedef g;
+class C {
+public:
+  C(g, f);
+};
+static wchar_t h;
+class D {
+public:
+  static C E();
+};
+
+C D::E() {
+  C i(B(, {}), f());
+  return i;
+}
+
+// Inheriting ctor has internal linkage, should not with comdat.
+
+// CHECK-LABEL: define internal noundef ptr 
@"??0?$B@_N@@QEAA@AEBVF@@AEBUA@@@Z"(ptr noundef nonnull returned align 1 
dereferenceable(1) %this, ptr noundef nonnull align 1 dereferenceable(1) %0, 
ptr noundef nonnull align 1 dereferenceable(1) %1) unnamed_addr #2 align 2
+// CHECK-LABEL: define linkonce_odr dso_local noundef ptr 
@"??0?$c@_NUbQEAA@AEBVF@@AEBUA@@@Z"(ptr noundef nonnull returned align 1 
dereferenceable(1) %this, ptr noundef nonnull align 1 dereferenceable(1) %p1, 
ptr noundef nonnull align 1 dereferenceable(1) %d) unnamed_addr #2 comdat align 
2
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11688,6 +11688,14 @@
   if (FD->isMSExternInline())
 return GVA_StrongODR;
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+  isa(FD) &&
+  cast(FD)->isInheritingConstructor())
+// Our approach to inheriting constructors is fundamentally different from
+// that used by the MS ABI, so keep our inheriting constructor thunks
+// internal rather than trying to pick an unambiguous mangling for them.
+return GVA_Internal;
+
   return GVA_DiscardableODR;
 }
 


Index: clang/test/CodeGenCXX/ms-inheriting-ctor.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ms-inheriting-ctor.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -fcxx-exceptions -triple=x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+class F {
+public:
+  F(wchar_t *);
+};
+using a = F;
+struct A {};
+struct b {
+  b(a, F, A);
+};
+template  struct c : b {
+  c(const a , const A ) : b(p1, 0, d) {}
+};
+template  struct B : c {
+  using c::c;
+};
+class f {
+public:
+  f(...);
+}
+
+typedef g;
+class C {
+public:
+  C(g, f);
+};
+static wchar_t h;
+class D {
+public:
+  static C E();
+};
+
+C D::E() {
+  C i(B(, {}), f());
+  return i;
+}
+
+// Inheriting ctor has internal linkage, should not with comdat.
+
+// CHECK-LABEL: define internal noundef ptr @"??0?$B@_N@@QEAA@AEBVF@@AEBUA@@@Z"(ptr noundef nonnull returned align 1 dereferenceable(1) %this, ptr noundef nonnull align 1 dereferenceable(1) %0, ptr noundef nonnull align 1 dereferenceable(1) %1) unnamed_addr #2 align 2
+// CHECK-LABEL: define linkonce_odr dso_local noundef ptr @"??0?$c@_NUbQEAA@AEBVF@@AEBUA@@@Z"(ptr noundef nonnull returned align 1 dereferenceable(1) %this, ptr noundef nonnull align 1 dereferenceable(1) %p1, ptr noundef nonnull align 1 dereferenceable(1) %d) unnamed_addr #2 comdat align 2
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11688,6 +11688,14 @@
   if (FD->isMSExternInline())
 return GVA_StrongODR;
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+  isa(FD) &&
+  cast(FD)->isInheritingConstructor())
+// Our approach to inheriting constructors is fundamentally different from
+// that used by the MS ABI, so keep our inheriting constructor thunks
+// internal rather than trying to pick an unambiguous mangling for them.
+return GVA_Internal;
+
   return GVA_DiscardableODR;
 }
 
___
cfe-commits mailing list

[PATCH] D158254: Fixing the memory leak using split() instead of strtok

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan added a comment.

In D158254#4607027 , @hchilama wrote:

> Hi @abhina.sreeskantharajan , Thanks for letting me know the process and 
> please help me in commiting this patch.

I'd like to put your name and email as the commit's author. What email should I 
use? `git commit --amend --author="Harini Chilamantula "`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158254

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

[PATCH] D158488: [NFC] Initialize member pointers to nullptr.

[Soumi Manna via Phabricator via cfe-commits]

Manna added a comment.

This has already been addressed by https://reviews.llvm.org/D157989


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

https://reviews.llvm.org/D158488

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D153156#4607671 , @hans wrote:

> In D153156#4607655 , @aaron.ballman 
> wrote:
>
>> I agree this should be reverted from 17.x so we have more time to consider 
>> an appropriate path forward. Supporting evidence that this will likely be 
>> disruptive: 
>> https://sourcegraph.com/search?q=context:global+%5E%28%22%5B%5E%22%5D%2B%22PRI.*%29%24+lang:C%2B%2B+-file:.*test.*=regexp=yes=0=repo
>
> I don't believe this is on 17.x. I'm suggesting reverting to green on trunk.

Heh, review fatigue caught me; I even mentioned on this review that it should 
not go into 17.x so we get more bake time with it! Sorry for that.

I think it is beneficial for us to get more feedback on problems caused by the 
changes, in case they help us decide how we want to move forward solving them. 
However, if the changes on trunk are sufficiently disruptive that we probably 
won't get more (novel) feedback anyway, then I think a revert is reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D158534: [clang-tidy] Disable trivially-destructible check for darwin

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

626849c71e85d546a004cc91866beab610222194 
 didn't 
fix this issue already ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158534

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aeubanks wrote:
> aaron.ballman wrote:
> > aeubanks wrote:
> > > aaron.ballman wrote:
> > > > What happens for tentative definitions where the value isn't known? 
> > > > e.g.,
> > > > ```
> > > > [[clang::unnamed_addr]] int i1, i2;
> > > > ```
> > > > 
> > > > What happens if the types are similar but not the same?
> > > > ```
> > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > ```
> > > > 
> > > > Should we diagnose taking the address of such an attributed variable so 
> > > > users have some hope of spotting the non-conforming situations?
> > > > 
> > > > Does this attribute have impacts across translation unit boundaries 
> > > > (perhaps only when doing LTO) or only within a single TU?
> > > > 
> > > > What does this attribute do in C++ in the presence of constructors and 
> > > > destructors? e.g.,
> > > > ```
> > > > struct S {
> > > >   S();
> > > >   ~S();
> > > > };
> > > > 
> > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's only 
> > > > one ctor/dtor call?
> > > > ```
> > > globals are only mergeable if they're known to be constant and have the 
> > > same value/size. this can be done at compile time only if the optimizer 
> > > can see the constant values, or at link time
> > > 
> > > so nothing would happen in any of the cases you've given.
> > > 
> > > but yeah that does imply that we should warn when the attribute is used 
> > > on non const, non-POD globals. I'll update this patch to do that
> > > 
> > > as mentioned in the description, we actually do want to take the address 
> > > of these globals for table-driven parsing, but we don't care about 
> > > identity equality
> > > globals are only mergeable if they're known to be constant and have the 
> > > same value/size. this can be done at compile time only if the optimizer 
> > > can see the constant values, or at link time
> > >
> > > so nothing would happen in any of the cases you've given.
> > 
> > A that's good to know. So I assume we *will* merge these?
> > 
> > ```
> > struct S {
> >   int i, j;
> >   float f;
> > };
> > 
> > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > [[clang::unnamed_addr]] const S s3 = s2;
> > ```
> > 
> > > but yeah that does imply that we should warn when the attribute is used 
> > > on non const, non-POD globals. I'll update this patch to do that
> > 
> > Thank you, I think that will be more user-friendly
> > 
> > > as mentioned in the description, we actually do want to take the address 
> > > of these globals for table-driven parsing, but we don't care about 
> > > identity equality
> > 
> > Yeah, I still wonder if we want to diagnose just the same -- if the address 
> > is never taken, there's not really a way to notice the optimization, but if 
> > the address is taken, you basically get UB (and I think we should 
> > explicitly document it as such). Given how easy it is to accidentally take 
> > the address of something (like via a reference in C++), I think we should 
> > warn by default, but still have a warning group for folks who want to live 
> > life dangerously.
> > > globals are only mergeable if they're known to be constant and have the 
> > > same value/size. this can be done at compile time only if the optimizer 
> > > can see the constant values, or at link time
> > >
> > > so nothing would happen in any of the cases you've given.
> > 
> > A that's good to know. So I assume we *will* merge these?
> > 
> > ```
> > struct S {
> >   int i, j;
> >   float f;
> > };
> > 
> > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > [[clang::unnamed_addr]] const S s3 = s2;
> > ```
> yeah those are merged even just by clang
> 
> ```
> struct S {
>   int i, j;
>   float f;
> };
> 
> [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s3 = s2;
> 
> const void * f1() {
>   return 
> }
> 
> const void * f2() {
>   return 
> }
> 
> const void * f3() {
>   return 
> }
> 
> $ ./build/rel/bin/clang++ -S -emit-llvm -o - -O2 /tmp/a.cc
> ```
> > 
> > > but yeah that does imply that we should warn when the attribute is used 
> > > on non const, non-POD globals. I'll update this patch to do that
> > 
> > Thank you, I think that will be more user-friendly
> > 
> > > as mentioned in the description, we actually do want to take the address 
> > > of these globals for table-driven parsing, but we don't care about 
> > > identity equality
> > 
> > Yeah, I still wonder if we want to diagnose just the same 

[PATCH] D158534: [clang-tidy] Disable trivially-destructible check for darwin

[Aditya Kumar via Phabricator via cfe-commits]

hiraditya created this revision.
hiraditya added reviewers: PiotrZSL, carlosgalvezp.
Herald added a subscriber: xazax.hun.
Herald added a project: All.
hiraditya requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

For fat binaries this test fails. 
https://github.com/llvm/llvm-project/issues/60304


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158534

Files:
  
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
@@ -3,6 +3,9 @@
 // RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -fix 
--
 // RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' 
-warnings-as-errors='-*,performance-trivially-destructible' --
 
+// For fat binaries this test fails: 
https://github.com/llvm/llvm-project/issues/60304
+// UNSUPPORTED: system-darwin
+
 struct TriviallyDestructible1 {
   int a;
 };


Index: clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
@@ -3,6 +3,9 @@
 // RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -fix --
 // RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -warnings-as-errors='-*,performance-trivially-destructible' --
 
+// For fat binaries this test fails: https://github.com/llvm/llvm-project/issues/60304
+// UNSUPPORTED: system-darwin
+
 struct TriviallyDestructible1 {
   int a;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D153156#4607655 , @aaron.ballman 
wrote:

> I agree this should be reverted from 17.x so we have more time to consider an 
> appropriate path forward. Supporting evidence that this will likely be 
> disruptive: 
> https://sourcegraph.com/search?q=context:global+%5E%28%22%5B%5E%22%5D%2B%22PRI.*%29%24+lang:C%2B%2B+-file:.*test.*=regexp=yes=0=repo

I don't believe this is on 17.x. I'm suggesting reverting to green on trunk.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D157497: feat: Migrate isArch16Bit

[Alex Langford via Phabricator via cfe-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.

In D157497#4592330 , @Pivnoy wrote:

> At the moment, the TargetParser architecture is extensible. This complicates 
> the addition of new architectures, operating systems, and so on.
> In the main discussion, it was proposed to redesign the architecture of this 
> module in order to increase the possibility of adding new architectures, etc.
> The main idea of   the rework was to separate the Triple entity into a 
> data-class, and create a number of interfaces from components that would 
> include Triple, through the implementation of which it would be possible to 
> represent various data bindings of the components. At the moment, Triple is 
> overflowing with various methods that do not fully meet the ideas of OOP.
> Since the TargetParser module is quite large and has many dependencies 
> throughout the llvm-project, it was first of all supposed to remove these 
> methods from Triple, since they would not correspond to OOP ideas.
> This would help to gradually rid Triple of unnecessary dependencies, and 
> gradually change the architecture inside Triple, without breaking code of 
> another LLVM developers.

I'm still not sure how this would make things simpler. I'll be as specific is 
possible in what does not click for me.

There is an inherent complexity in parsing triples. Architectures can have 
variants and subvariants, compilers and other tools do different things for the 
same architecture and OS when there are different vendors, the environment can 
subtly change things like ABI, the list goes on. I don't think you're going to 
be able to wholesale reduce complexity here. The proposal you've laid out here 
is certainly very OOP-like (in some sense of the phrase "OOP") but you present 
your ideas under the assumption that this style of OOP is the ideal to strive 
for. Why is that? Why is this better than what exists today? Is it easier to 
debug? Is it more performant? Is it easier to maintain? I personally do not 
think it will be better than what already exists in any of those ways.

In the original discourse thread, you also made the argument that the potential 
performance overhead and binary size increase should be unnoticeable and that 
with modern machines we do not need to fight for each microsecond. Without any 
numbers for performance or size, this is not an argument I can accept. 
Knowingly adding a performance/size regression to your build tools without an 
appropriate tradeoff does not make sense to do.

If you want to do this, please provide a concrete argument for what benefit 
this brings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157497

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D153156#4607576 , @hans wrote:

> Given the concerns raised (the PRIuS one in https://godbolt.org/z/v3boc9E6T 
> seems like a good example), and that the fix in D158372 
>  doesn't seem straight-forward, could this 
> be reverted to unbreak things until a revised version is ready?
>
> @aaron.ballman what do you think?

I agree this should be reverted from 17.x so we have more time to consider an 
appropriate path forward. Supporting evidence that this will likely be 
disruptive: 
https://sourcegraph.com/search?q=context:global+%5E%28%22%5B%5E%22%5D%2B%22PRI.*%29%24+lang:C%2B%2B+-file:.*test.*=regexp=yes=0=repo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Ian Anderson via Phabricator via cfe-commits]

iana added inline comments.



Comment at: clang/lib/Headers/stddef.h:1
 /*=== stddef.h - Basic type definitions ===
  *

aaron.ballman wrote:
> ldionne wrote:
> > Making a thread out of this:
> > 
> > > The relationship between clang's stddef.h and the C Standard Library 
> > > stddef.h is that there is no relationship. clang's header doesn't 
> > > #include_next, and it is in the search path before the OS's cstdlib.
> > 
> > So in that case what is the purpose of the SDK/system providing a 
> > `` header? They basically all provide one and it's never used?
> > 
> The compiler provides `` for the same reason it provides 
> `` and others: the compiler is responsible for defining these 
> interfaces because it's the only thing that knows the correct definitions it 
> expects. The system might know some of it, but for example, `size_t` relates 
> to the maximum size of an object, which is something only the compiler knows 
> the answer to.
I think the purpose is for the SDK/system to support compilers that don't 
provide ``. In the early Apple days that was CodeWarrior, maybe gcc 
didn't used to provide that header? I don't know.

But basically yes, they all provide one and it's practically never used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157757

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

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64CallingConvention.td:555-557
+def CSR_Win_AArch64_Arm64EC_Thunk : CalleeSavedRegs<(add X19, X20, X21, X22, 
X23, X24,
+ X25, X26, X27, X28, 
FP, LR,
+ (sequence "Q%u", 6, 
15))>;

dpaoliello wrote:
> I've been hitting asserts in `computeCalleeSaveRegisterPairs` due to this: 
> LLVM doesn't support gaps when saving registers for Windows (~line 2744 of 
> AArch64FrameLowering.cpp), so placing the smaller registers before the larger 
> ones causes issues if those smaller ones aren't paired (the `assert(OffsetPre 
> % Scale == 0);` fires since `OffsetPre` will be a multiple of 8 but `Scale` 
> will be 16).
That looks right; I thought I had that fix in here, but I guess I mixed up my 
patches and pulled in a different version.

(I'll reply to the other review comments in more detail soon.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157547

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aaron.ballman wrote:
> aeubanks wrote:
> > aaron.ballman wrote:
> > > What happens for tentative definitions where the value isn't known? e.g.,
> > > ```
> > > [[clang::unnamed_addr]] int i1, i2;
> > > ```
> > > 
> > > What happens if the types are similar but not the same?
> > > ```
> > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > ```
> > > 
> > > Should we diagnose taking the address of such an attributed variable so 
> > > users have some hope of spotting the non-conforming situations?
> > > 
> > > Does this attribute have impacts across translation unit boundaries 
> > > (perhaps only when doing LTO) or only within a single TU?
> > > 
> > > What does this attribute do in C++ in the presence of constructors and 
> > > destructors? e.g.,
> > > ```
> > > struct S {
> > >   S();
> > >   ~S();
> > > };
> > > 
> > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's only 
> > > one ctor/dtor call?
> > > ```
> > globals are only mergeable if they're known to be constant and have the 
> > same value/size. this can be done at compile time only if the optimizer can 
> > see the constant values, or at link time
> > 
> > so nothing would happen in any of the cases you've given.
> > 
> > but yeah that does imply that we should warn when the attribute is used on 
> > non const, non-POD globals. I'll update this patch to do that
> > 
> > as mentioned in the description, we actually do want to take the address of 
> > these globals for table-driven parsing, but we don't care about identity 
> > equality
> > globals are only mergeable if they're known to be constant and have the 
> > same value/size. this can be done at compile time only if the optimizer can 
> > see the constant values, or at link time
> >
> > so nothing would happen in any of the cases you've given.
> 
> A that's good to know. So I assume we *will* merge these?
> 
> ```
> struct S {
>   int i, j;
>   float f;
> };
> 
> [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s3 = s2;
> ```
> 
> > but yeah that does imply that we should warn when the attribute is used on 
> > non const, non-POD globals. I'll update this patch to do that
> 
> Thank you, I think that will be more user-friendly
> 
> > as mentioned in the description, we actually do want to take the address of 
> > these globals for table-driven parsing, but we don't care about identity 
> > equality
> 
> Yeah, I still wonder if we want to diagnose just the same -- if the address 
> is never taken, there's not really a way to notice the optimization, but if 
> the address is taken, you basically get UB (and I think we should explicitly 
> document it as such). Given how easy it is to accidentally take the address 
> of something (like via a reference in C++), I think we should warn by 
> default, but still have a warning group for folks who want to live life 
> dangerously.
> > globals are only mergeable if they're known to be constant and have the 
> > same value/size. this can be done at compile time only if the optimizer can 
> > see the constant values, or at link time
> >
> > so nothing would happen in any of the cases you've given.
> 
> A that's good to know. So I assume we *will* merge these?
> 
> ```
> struct S {
>   int i, j;
>   float f;
> };
> 
> [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s3 = s2;
> ```
yeah those are merged even just by clang

```
struct S {
  int i, j;
  float f;
};

[[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s3 = s2;

const void * f1() {
  return 
}

const void * f2() {
  return 
}

const void * f3() {
  return 
}

$ ./build/rel/bin/clang++ -S -emit-llvm -o - -O2 /tmp/a.cc
```
> 
> > but yeah that does imply that we should warn when the attribute is used on 
> > non const, non-POD globals. I'll update this patch to do that
> 
> Thank you, I think that will be more user-friendly
> 
> > as mentioned in the description, we actually do want to take the address of 
> > these globals for table-driven parsing, but we don't care about identity 
> > equality
> 
> Yeah, I still wonder if we want to diagnose just the same -- if the address 
> is never taken, there's not really a way to notice the optimization, but if 
> the address is taken, you basically get UB (and I think we should explicitly 
> document it as such). Given how easy it is to accidentally take the address 
> of something (like via a reference in 

[PATCH] D158522: [NFC][CLANG] Fix static analyzer bugs about large copy by values

[Soumi Manna via Phabricator via cfe-commits]

Manna added a comment.

In D158522#4607628 , @tahonermann 
wrote:

> Looks fine to me!

Thanks @tahonermann for reviews!


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

https://reviews.llvm.org/D158522

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

[PATCH] D158522: [NFC][CLANG] Fix static analyzer bugs about large copy by values

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Looks fine to me!


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

https://reviews.llvm.org/D158522

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aeubanks wrote:
> aaron.ballman wrote:
> > What happens for tentative definitions where the value isn't known? e.g.,
> > ```
> > [[clang::unnamed_addr]] int i1, i2;
> > ```
> > 
> > What happens if the types are similar but not the same?
> > ```
> > [[clang::unnamed_addr]] signed int i1 = 32;
> > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > ```
> > 
> > Should we diagnose taking the address of such an attributed variable so 
> > users have some hope of spotting the non-conforming situations?
> > 
> > Does this attribute have impacts across translation unit boundaries 
> > (perhaps only when doing LTO) or only within a single TU?
> > 
> > What does this attribute do in C++ in the presence of constructors and 
> > destructors? e.g.,
> > ```
> > struct S {
> >   S();
> >   ~S();
> > };
> > 
> > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's only one 
> > ctor/dtor call?
> > ```
> globals are only mergeable if they're known to be constant and have the same 
> value/size. this can be done at compile time only if the optimizer can see 
> the constant values, or at link time
> 
> so nothing would happen in any of the cases you've given.
> 
> but yeah that does imply that we should warn when the attribute is used on 
> non const, non-POD globals. I'll update this patch to do that
> 
> as mentioned in the description, we actually do want to take the address of 
> these globals for table-driven parsing, but we don't care about identity 
> equality
> globals are only mergeable if they're known to be constant and have the same 
> value/size. this can be done at compile time only if the optimizer can see 
> the constant values, or at link time
>
> so nothing would happen in any of the cases you've given.

A that's good to know. So I assume we *will* merge these?

```
struct S {
  int i, j;
  float f;
};

[[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s3 = s2;
```

> but yeah that does imply that we should warn when the attribute is used on 
> non const, non-POD globals. I'll update this patch to do that

Thank you, I think that will be more user-friendly

> as mentioned in the description, we actually do want to take the address of 
> these globals for table-driven parsing, but we don't care about identity 
> equality

Yeah, I still wonder if we want to diagnose just the same -- if the address is 
never taken, there's not really a way to notice the optimization, but if the 
address is taken, you basically get UB (and I think we should explicitly 
document it as such). Given how easy it is to accidentally take the address of 
something (like via a reference in C++), I think we should warn by default, but 
still have a warning group for folks who want to live life dangerously.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Given the concerns raised (the PRIuS one in https://godbolt.org/z/v3boc9E6T 
seems like a good example), and that the fix in D158372 
 doesn't seem straight-forward, could this be 
reverted to unbreak things until a revised version is ready?

@aaron.ballman what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Headers/stddef.h:1
 /*=== stddef.h - Basic type definitions ===
  *

ldionne wrote:
> Making a thread out of this:
> 
> > The relationship between clang's stddef.h and the C Standard Library 
> > stddef.h is that there is no relationship. clang's header doesn't 
> > #include_next, and it is in the search path before the OS's cstdlib.
> 
> So in that case what is the purpose of the SDK/system providing a 
> `` header? They basically all provide one and it's never used?
> 
The compiler provides `` for the same reason it provides `` 
and others: the compiler is responsible for defining these interfaces because 
it's the only thing that knows the correct definitions it expects. The system 
might know some of it, but for example, `size_t` relates to the maximum size of 
an object, which is something only the compiler knows the answer to.



Comment at: clang/lib/Headers/stddef.h:118-122
+#ifdef __cplusplus
+namespace std {
+typedef decltype(nullptr) nullptr_t;
+}
+using ::std::nullptr_t;

ldionne wrote:
> iana wrote:
> > aaron.ballman wrote:
> > > iana wrote:
> > > > aaron.ballman wrote:
> > > > > iana wrote:
> > > > > > ldionne wrote:
> > > > > > > iana wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Related:
> > > > > > > > > 
> > > > > > > > > https://github.com/llvm/llvm-project/issues/37564
> > > > > > > > > https://cplusplus.github.io/LWG/issue3484
> > > > > > > > > 
> > > > > > > > > CC @ldionne
> > > > > > > > I don't _think_ this change actually changes the way nullptr_t 
> > > > > > > > gets defined in C++, does it?
> > > > > > > I think we absolutely don't want to touch `std::nullptr_t` from 
> > > > > > > this header. It's libc++'s responsibility to define that, and in 
> > > > > > > fact we define it in `std::__1`, so this is even an ABI break (or 
> > > > > > > I guess it would be a compiler error, not sure).
> > > > > > I'm really not touching it though. All I did is move it from 
> > > > > > `__need_NULL` to `__need_nullptr_t`.
> > > > > > 
> > > > > > The old behavior is that `std::nullptr_t` would only be touched if 
> > > > > > (no `__need_` macros were set or if `__need_NULL` was set), and 
> > > > > > (_MSC_EXTENSIONS and _NATIVE_NULLPTR_SUPPORTED are defined).
> > > > > > 
> > > > > > The new behavior is that `std::nullptr_t` will only be touched if 
> > > > > > ((no `__need_` macros are set) and (_MSC_EXTENSIONS and 
> > > > > > _NATIVE_NULLPTR_SUPPORTED are defined)) or (the new 
> > > > > > `__need_nullptr_t` macro is set)
> > > > > > 
> > > > > > So the only change is that C++ code that previously set 
> > > > > > `__need_NULL` will no longer get `std::nullptr_t`. @efriedma felt 
> > > > > > like that was a fine change.
> > > > > Does libc++ provide the symbols for a freestanding compilation?
> > > > > 
> > > > > I was pointing out those links specifically because the C++ standard 
> > > > > currently says that stddef.h (the C standard library header) needs to 
> > > > > provide a definition of `std::nullptr_t`, but that LWG thinks that's 
> > > > > perhaps not the right way to do that and may be removing that 
> > > > > requirement.
> > > > It is weird the standard puts that in stddef.h and not cstddef. I think 
> > > > libc++ could provide that in their stddef.h anyway, but the intent in 
> > > > this review is to not rock the boat and only do the minimal change 
> > > > discussed above.
> > > Yeah, this discussion is to figure out whether we have an existing bug we 
> > > need to address and if so, where to address it (libc++, clang, or the C++ 
> > > standard). I don't think your changes are exacerbating anything, more 
> > > just that they've potentially pointed out something related.
> >  
> > Does libc++ provide the symbols for a freestanding compilation?
> 
> I don't think we do. We basically don't support `-ffreestanding` right now 
> (we support embedded and funky platforms via other mechanisms).
> 
> But regardless, `` should never define something in namespace 
> `std`, that should be libc++'s responsibility IMO. What we could do here 
> instead is just
> 
> ```
> #ifdef __cplusplus
> typedef decltype(nullptr) nullptr_t;
> #else
> typedef typeof(nullptr) nullptr_t;
> #endif
> ```
> 
> and then let libc++'s `` do
> 
> ```
> _LIBCPP_BEGIN_NAMESPACE_STD
> using ::nullptr_t;
> _LIBCPP_END_NAMESPACE_STD
> ```
> 
> If Clang's `` did define `::nullptr_t`, we could likely remove 
> libc++'s `` and that might simplify things.
>> Does libc++ provide the symbols for a freestanding compilation?
> I don't think we do. We basically don't support -ffreestanding right now (we 
> support embedded and funky platforms via other mechanisms).

Okay, that's what I thought as well. Thanks!

> But regardless,  should never define something in namespace std, 
> that should be libc++'s responsibility IMO. What we could do 

[PATCH] D158532: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza added a comment.

The delta from https://reviews.llvm.org/D155895 is in 
`clang/test/SemaCXX/attr-trivial-abi.cpp` where 1) a comment has been added 
above the non-trivial move constructor of the `Trivial` test helper and 2) test 
expectations have been tweaked to account for `_WIN64` and `__MINGW32__` 
(cargo-culting these conditions from the already-existing tests in the top part 
of this test file).

I have considered making Windows and non-Windows behaving in the same way, 
which would require making `Trivial` trivially-copyable (and consequently 
trivially-movable).  This wouldn't repro the original problem, so I abandoned 
this direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158532

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

[PATCH] D158532: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza created this revision.
lukasza added a reviewer: gribozavr2.
lukasza added a project: clang.
Herald added a subscriber: mstorsjo.
Herald added a project: All.
lukasza requested review of this revision.
Herald added a subscriber: cfe-commits.

This is a reland of https://reviews.llvm.org/D155895.

Consider the test input below:

  struct [[clang::trivial_abi]] Trivial {
Trivial() {}
Trivial(Trivial&& other) {}
Trivial& operator=(Trivial&& other) { return *this; }
~Trivial() {}
  };
  static_assert(__is_trivially_relocatable(Trivial), "");
  
  struct [[clang::trivial_abi]] S {
S(S&& other) {}
S& operator=(S&& other) { return *this; }
~S() {}
union { Trivial field; };
  };
  static_assert(__is_trivially_relocatable(S), "");

Before the fix Clang would warn that 'trivial_abi' is disallowed on 'S'
because it has a field of a non-trivial class type (the type of the
anonymous union is non-trivial, because it doesn't have the
`[[clang::trivial_abi]]` attribute applied to it). Consequently, before
the fix the `static_assert` about `__is_trivially_relocatable` would
fail.

Note that `[[clang::trivial_abi]]` cannot be applied to the anonymous
union, because Clang warns that 'trivial_abi' is disallowed on '(unnamed
union at ...)' because its copy constructors and move constructors are
all deleted. Also note that it is impossible to provide copy nor move
constructors for anonymous unions and structs that contain
fields with a non-trivial copy constructors or move constructors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158532

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp

Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17
 
 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
 
@@ -169,3 +170,166 @@
 static_assert(__is_trivially_relocatable(S20), "");
 #endif
 } // namespace deletedCopyMoveConstructor
+
+namespace anonymousUnionsAndStructs {
+  // Test helper:
+  struct [[clang::trivial_abi]] Trivial {
+Trivial() {}
+
+// Non-trivial move constructor and move assignment operator are defined
+// below primarily because (in a subset of tests below) we want to hit the
+// following scenario from https://en.cppreference.com/w/cpp/language/union:
+//
+// > If a union contains a non-static data member with a non-trivial special
+// > member function (copy/move constructor, copy/move assignment, or
+// > destructor), that function is deleted by default in the union and needs
+// > to be defined explicitly by the programmer.
+//
+// (Note that explicit definition of a move constructor is impossible for
+// an *anonymous* union.)
+//
+// Hitting this scenario is require to repro warnings like:
+//
+// * 'trivial_abi' cannot be applied to '(unnamed union}`
+//   copy constructors and move constructors are all deleted
+//   (this repros regardless of the presence of the fix / CLANG_ABI_COMPAT).
+// * 'trivial_abi' cannot be applied to 'StructWithAnonymousUnion'
+//   'trivial_abi' is disallowed 'StructWithAnonymousUnion' because it has a
+//   field of a non-trivial class type
+//   (repros only before the fix - `CLANG_ABI_COMPAT <= 17`)
+//
+// OTOH, hitting this scenario means that `Trivial` is not trivially
+// copyable.  Sadly, this means that `__is_trivially_relocatable` asserts
+// need different expectations on Windows/MSVC, where to be
+// trivial-for-calls, an object must be trivially copyable.  (And it is only
+// trivially relocatable, currently, if it is trivial for calls.)
+Trivial(Trivial&& other) {}
+Trivial& operator=(Trivial&& other) { return *this; }
+
+~Trivial() {}
+  };
+#if !defined(_WIN64) || defined(__MINGW32__)
+  static_assert(__is_trivially_relocatable(Trivial), "");
+#else
+  static_assert(!__is_trivially_relocatable(Trivial), "");
+#endif
+
+  // Test helper:
+  struct Nontrivial {
+Nontrivial() {}
+Nontrivial(Nontrivial&& other) {}
+Nontrivial& operator=(Nontrivial&& other) { return *this; }
+~Nontrivial() {}
+  };
+  static_assert(!__is_trivially_relocatable(Nontrivial), "");
+
+  // Basic smoke test, not yet related to anonymous unions or structs:
+#if !defined(_WIN64) || defined(__MINGW32__)
+  struct [[clang::trivial_abi]] BasicStruct {
+#else
+  struct [[clang::trivial_abi]] BasicStruct { // expected-warning {{'trivial_abi' cannot be applied to 'BasicStruct'}} expected-note {{trivial_abi' is 

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

ellis wrote:
> aeubanks wrote:
> > aeubanks wrote:
> > > we're running into this error message even though we don't have debug 
> > > info correlation turned on, is that expected in some configurations? I'm 
> > > still trying to understand how we could end up in this code path without 
> > > debug info correlation
> > > 
> > > https://crbug.com/1473935
> > also it seems like this code path isn't tested?
> It's very strange that you are hitting this code path because I didn't think 
> it was possible. Were you ever able to reproduce locally?
no, it's only showed up on some bots (both mac and linux), I'm still trying to 
understand what's going on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Ellis Hoag via Phabricator via cfe-commits]

ellis added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

aeubanks wrote:
> aeubanks wrote:
> > we're running into this error message even though we don't have debug info 
> > correlation turned on, is that expected in some configurations? I'm still 
> > trying to understand how we could end up in this code path without debug 
> > info correlation
> > 
> > https://crbug.com/1473935
> also it seems like this code path isn't tested?
It's very strange that you are hitting this code path because I didn't think it 
was possible. Were you ever able to reproduce locally?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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