[PATCH] D139586: [Clang][C++23] Lifetime extension in range-based for loops

[Hubert Tong via Phabricator via cfe-commits]

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

Waiting on changes to fix the visitation to locate the appropriate temporaries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139586

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

[PATCH] D142358: Opting out of Clang 16 ABI Changes for AIX and z/OS

[Hubert Tong 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 rG0f5099cd9422: Opting out of Clang 16 ABI Changes for AIX and 
z/OS (authored by nicolerabjohn, committed by hubert.reinterpretcast).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142358

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -7,8 +7,17 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
+// RUN: %clang_cc1 -triple powerpc-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple powerpc-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+
 // expected-no-diagnostics
 
+#if !defined(__MVS__) && !defined(_AIX)
+
 #define SA(n, p) int a##n[(p) ? 1 : -1]
 
 struct A {
@@ -611,6 +620,8 @@
 #pragma pack(pop)
 }
 
+#endif // !defined(__MVS__) && !defined(__AIX__)
+
 namespace non_pod {
 struct t1 {
 protected:
@@ -670,11 +681,15 @@
 struct t2 {
   t1 v1;
 } __attribute__((packed));
+#if (defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15) || !defined(__MVS__)
 _Static_assert(_Alignof(t2) == 1, "");
+#else
+_Static_assert(_Alignof(t2) == 4, "");
+#endif
 struct t3 : t1 {
   char c;
 };
-#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+#if (defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15) || defined(__MVS__)
 _Static_assert(sizeof(t3) == 8, "");
 #else
 _Static_assert(sizeof(t3) == 12, "");
Index: clang/lib/Basic/Targets/OSTargets.h
===
--- clang/lib/Basic/Targets/OSTargets.h
+++ clang/lib/Basic/Targets/OSTargets.h
@@ -773,6 +773,10 @@
   }
 
   bool defaultsToAIXPowerAlignment() const override { return true; }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // z/OS target
@@ -831,6 +835,10 @@
 this->UseLeadingZeroLengthBitfield = false;
 this->ZeroLengthBitfieldBoundary = 32;
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 void addWindowsDefines(const llvm::Triple , const LangOptions ,
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1968,7 +1968,8 @@
  FieldClass->hasAttr() ||
  Context.getLangOpts().getClangABICompat() <=
  LangOptions::ClangABI::Ver15 ||
- Target.isPS() || Target.isOSDarwin())) ||
+ Target.isPS() || Target.isOSDarwin() ||
+ Target.isOSAIX())) ||
  D->hasAttr();
 
   // When used as part of a typedef, or together with a 'packed' attribute, the
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -838,13 +838,14 @@
 
 - GCC doesn't pack non-POD members in packed structs unless the packed
   attribute is also specified on the member. Clang historically did perform
-  such packing. Clang now matches the gcc behavior (except on Darwin and PS4).
+  such packing. Clang now matches the gcc behavior
+  (except on Darwin, PS4 and AIX).
   You can switch back to the old ABI behavior with the flag:
   ``-fclang-abi-compat=15.0``.
 - GCC allows POD types to have defaulted special members. Clang historically
   classified such types as 

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Since this is a change only for AIX and z/OS, and considering that this is a 
patch to prevent ABI changes compared to the current ABI baselines for those 
platforms, I will land this ahead of the LLVM 16 branching.


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

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

Thanks @nicolerabjohn; this LGTM!




Comment at: clang/test/SemaCXX/class-layout.cpp:685
+#if (defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15) || !defined(__MVS__)
 _Static_assert(_Alignof(t2) == 1, "");
+#else

Just a note: As mentioned above, for some releases of Open XL on z/OS, the 
`-fclang-abi-compat` level that gives this result is less than 15 (but, then 
again, the behaviour expected by this line persisted in vanilla Clang 15). What 
is more important is the default behaviour (confirmed below) anyway.


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:688
 };
 #if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
 _Static_assert(sizeof(t3) == 8, "");

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Because z/OS has some of the Clang 16 changes but not all, I think we can't 
> > rely on claiming that it uses the Clang 15 ABI for the purposes of the 
> > testing (and need to make a change here to say that it does follow the 
> > "Clang 15 ABI" for this aspect).
> Sorry, my last suggestion for this line should be reverted. z/OS works just 
> like Linux for this right now: no special code needed here.
> Sorry, my last suggestion for this line should be reverted. z/OS works just 
> like Linux for this right now: no special code needed here.

I was actually confused about being confused before (and now I am just very 
confused). This is the test corresponding to the changes to 
`areDefaultedSMFStillPOD`. z/OS does not work just like Linux for this part. 
Under `__MVS__`, we really should always be returning `8` for the size of `t3`. 
I am not sure how we ended up with `12` as I was told offline.



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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:18
 // expected-no-diagnostics
 
+#if !defined(__MVS__) && !defined(_AIX)

dblaikie wrote:
> What's the reason this part is #ifdef'd out? Does it contain code that's 
> unsupported on these platforms?
The correct values need to be evaluated for those parts. This test was never 
run with those platforms as targets. We're trying to get the newer tests 
enabled first.


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:688
 };
 #if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
 _Static_assert(sizeof(t3) == 8, "");

hubert.reinterpretcast wrote:
> Because z/OS has some of the Clang 16 changes but not all, I think we can't 
> rely on claiming that it uses the Clang 15 ABI for the purposes of the 
> testing (and need to make a change here to say that it does follow the "Clang 
> 15 ABI" for this aspect).
Sorry, my last suggestion for this line should be reverted. z/OS works just 
like Linux for this right now: no special code needed here.


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:14
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+//RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 
-Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 
-Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15

hubert.reinterpretcast wrote:
> See comment for line 688 of this Diff.
Should change `//RUN:` to `// RUN:` to match other lines as well.


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:14
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+//RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 
-Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 
-Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15

See comment for line 688 of this Diff.



Comment at: clang/test/SemaCXX/class-layout.cpp:688
 };
 #if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
 _Static_assert(sizeof(t3) == 8, "");

Because z/OS has some of the Clang 16 changes but not all, I think we can't 
rely on claiming that it uses the Clang 15 ABI for the purposes of the testing 
(and need to make a change here to say that it does follow the "Clang 15 ABI" 
for this aspect).


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:817
   attribute is also specified on the member. Clang historically did perform
   such packing. Clang now matches the gcc behavior (except on Darwin and PS4).
   You can switch back to the old ABI behavior with the flag:

This line also needs update.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1972
+ Target.isPS() || Target.isOSDarwin() ||
+ Target.isOSAIX())) || 
  D->hasAttr();

I agree we are good with z/OS having this change take effect going forward 
because the ABI was "stabilized" with a version where this change was effective.

I don't know if @SeanP has any comments on `-fclang-abi-compat=15` exposing the 
pre-stable ABI behaviour (at least with the code here). My understanding is 
that the Open XL product exposed the "Clang 16" behaviour with 
`-fclang-abi-compat=n` for some `n` less than 16.



Comment at: clang/test/SemaCXX/class-layout.cpp:639
 _Static_assert(_Alignof(t1) == 4, "");
 _Static_assert(_Alignof(t2) == 1, "");
 #else

I am having trouble understanding how this line is passing with
```
//RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 
-Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
```

The Clang 16 behaviour should be effective on z/OS given the state of this 
patch.


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

https://reviews.llvm.org/D142358

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

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:10
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 

There are no AIX and z/OS `RUN` lines here? For AIX, we expect 
`-DCLANG_ABI_COMPAT=15` behaviour with and without the `-fclang-abi-compat=15` 
option.


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

https://reviews.llvm.org/D142358

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

[PATCH] D137756: [z/OS][pg] Throw error when using -pg on z/OS

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D137756#4018666 , @MaskRay wrote:

> Most targets reject `-p` now. It's unnecessary to have another z/OS specific 
> diagnostic. So this patch can be abandoned.
>
>   % fclang -p a.cc
>   clang-16: error: unsupported option '-p' for target 
> 'x86_64-unknown-linux-gnu'

@francii, if you can confirm:

1. that there is a test that covers that error for z/OS, and
2. any comments in the code associated with generating the error above do not 
misrepresent the rationale for why `-p` is unsupported on z/OS,

then I'm good with leaving it at that.

Otherwise, we should make this into an NFC patch that adds the test/adjusts the 
comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137756

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

[PATCH] D139586: [Clang][C++23] Lifetime extension in range-based for loops

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

At least on the surface, it looks like there will be a lot of trouble to deal 
with default arguments:

  struct A { A(); ~A(); int x[3]; };
  int ((const A & = A()))[3];
  void bar(int);
  void foo() {
for (auto e : f()) { bar(e); }
  }

We get a `CXXDefaultArgExpr`, and the expression on the parameter declaration 
is what has the `MaterializeTemporaryExpr`. It looks like we now have cases 
where different call sites require different semantics (and perhaps different 
diagnostics, somewhat like template instantiations).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139586

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

[PATCH] D139586: [Clang][C++23] Lifetime extension in range-based for loops

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D139586#3984118 , @cor3ntin wrote:

> I will try to do a more detailed review later, but at least I think we might 
> want more tests. Maybe codegen tests that do not rely on 
> [[clang::lifetimebound]], and tests with more chaining (`a().b().c()`) .

We need to cover AST structure more than chaining. For example the patch 
currently does not find the `B()` temporary:

  struct A { ~A(); int x[3]; };
  struct B : A {};
  int ((const A *))[3];
  const A *g(const A &);
  void bar(int);
  void foo() {
for (auto e : f(g(B( {
  bar(e);
}
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139586

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

[PATCH] D139586: [Clang][C++23] Lifetime extension in range-based for loops

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D139586#3987252 , @royjacobson 
wrote:

> The (non-wording) paper makes a pretty convincing case to just apply this 
> retroactively to any C++11 code 
> (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2644r1.pdf). I 
> think we should apply this retroactively, maybe add a pedantic warning when 
> we do a lifetime extension on code before C++23.

Just noting that the committee did not vote this in as a Defect Report, but I 
mostly agree that people should code for the new behaviour and that the old 
behaviour is unlikely to be relied on. I suspect this should be highlighted as 
technically a breaking change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139586

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

[PATCH] D137986: [Clang][CodeGen][AIX] Map __builtin_frexpl, __builtin_ldexpl, and __builtin_modfl to 'double' version lib calls in 64-bit 'long double' mode

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast 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/D137986/new/

https://reviews.llvm.org/D137986

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

[PATCH] D137986: [Clang][CodeGen][AIX] Map __builtin_frexpl, __builtin_ldexpl, and __builtin_modfl to 'double' version lib calls in 64-bit 'long double' mode

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:112
+  // double' mode.
+  static SmallDenseMap AIXLongDoubleBuiltins{
+  {Builtin::BI__builtin_frexpl, "frexp"},

Please rename to "AIXLongDouble64Builtins".



Comment at: clang/test/CodeGen/aix-builtin-mapping.c:3
+// 'long double'. Check that the compiler generates calls to the 'double'
+// versions for correspoding builtin functions in 64-bit 'long double' mode.
+

Fix typo.



Comment at: clang/test/CodeGen/aix-builtin-mapping.c:5-6
+
+// RUN: %clang_cc1 -triple powerpc-ibm-aix -mlong-double-64 -S -o - %s | 
FileCheck %s -check-prefix=CHECK
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix -mlong-double-64 -S -o - %s | 
FileCheck %s -check-prefix=CHECK
+

This should be an IR test. Checking assembly means actually needing the PowerPC 
target enabled in the LLVM configuration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137986

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

[PATCH] D137437: [lit][AIX] Convert clang tests to use 'target={{.*}}aix{{.*}}'

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/ClangScanDeps/modules-no-undeclared-includes.c:3
 // section in XCOFF yet.
-// UNSUPPORTED: aix
+// UNSUPPORTED: target={{.*}}aix{{.*}}
 

Perhaps we should normalize on `-aix` while we are doing this?


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

https://reviews.llvm.org/D137437

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

[Diffusion] rGfdab9f1203ee: [Clang] Check for response file existence prior to check for recursion

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a subscriber: cfe-commits.

BRANCHES
  main

Users:
  sepavloff (Author)

https://reviews.llvm.org/rGfdab9f1203ee

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

[PATCH] D136568: [Clang] Support constexpr builtin ilogb

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/Sema/constant-builtins-ilogb.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics

aaron.ballman wrote:
> hubert.reinterpretcast wrote:
> > There seems to be no C language test in the patch (although the builtin 
> > presumably is okay at least as part of arithmetic constant expressions).
> > 
> > @aaron.ballman, what are your thoughts re: integer constant expression 
> > contexts? For example:
> > ```
> > struct C { int x : __builtin_ilogb(1. + 1.); };
> > ```
> WG14 adopted https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2713.htm for 
> C2x to clarify that implementations are not allowed to extend what they 
> consider to be an integer constant expression. The operand in this case is a 
> function call expression, which is not one of the permissible things in an 
> ICE, so the standard doesn't want us to make it one.
> 
> I believe that Clang's response to that paper is to implement it to the 
> letter rather than to the intent. e.g., issue a warning that a constant 
> expression is being folded into an ICE but otherwise accept the code. We have 
> too many situations already in which we fold a constant expression to an ICE 
> and changing that behavior would be observable (and a performance 
> regression). So I think it's fine for us to treat the builtin call as an ICE 
> so long as we issue a (pedantic) warning about folding it.
> 
> (FWIW, I don't think it qualifies as an arithmetic constant expression 
> because that also doesn't allow function call expressions. But we can extend 
> the definition of an arithmetic constant expression.)
Thanks Aaron. I think a C language test for this patch may be a good way to 
explore the extension space (and any pedantic conformance diagnostics).

Particular to the example above and ICEs is that the argument involves 
floating-point operations. Would the desired model be that the call is 
considered a literal for the purposes of the language requirements?

It seems that built-ins that are accepted in constant expressions in C are a 
more general issue though (and I am not sure if, for this patch, we defer the 
issue for some general approach that would take care of the class of problems 
as a whole).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136568

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

[PATCH] D136568: [Clang] Support constexpr builtin ilogb

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast requested changes to this revision.
hubert.reinterpretcast added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/AST/ExprConstant.cpp:12452
+int Ilogb;
+if (APFloat::opStatus St = ilogb(F, Ilogb); !isConstantOpStatus(St))
+  return false;

Izaron wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > hubert.reinterpretcast wrote:
> > > > Izaron wrote:
> > > > > majnemer wrote:
> > > > > > Izaron wrote:
> > > > > > > jcranmer-intel wrote:
> > > > > > > > Izaron wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > jcranmer-intel wrote:
> > > > > > > > > > > `long double` is `ppc_fp128` on at least some PPC 
> > > > > > > > > > > targets, and while I'm not entirely certain of what 
> > > > > > > > > > > `ilogb` properly returns in the corner cases of the 
> > > > > > > > > > > `ppc_fp128`, I'm not entirely confident that the 
> > > > > > > > > > > implementation of `APFloat` is correct in those cases. 
> > > > > > > > > > > I'd like someone with PPC background to comment in here.
> > > > > > > > > > Paging @hubert.reinterpretcast for help finding a good 
> > > > > > > > > > person to comment on the PPC questions.
> > > > > > > > > @jcranmer-intel constexpr evaluation is quite 
> > > > > > > > > machine-/target-independent. Clang evaluates it based on its 
> > > > > > > > > **internal** representation of float variables.
> > > > > > > > > 
> > > > > > > > > [[ 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L1256
> > > > > > > > >  | int ilogb ]] uses `Arg.getIEEE()`, that [[ 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L819-L825
> > > > > > > > >  | returns Clang's internal float representation ]].
> > > > > > > > > 
> > > > > > > > > Whichever float semantics is being used, [[ 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/lib/Support/APFloat.cpp#L54-L61
> > > > > > > > >  | minExponent and maxExponent are representable as 
> > > > > > > > > APFloatBase::ExponentType ]], which is `int32_t`:
> > > > > > > > > ```
> > > > > > > > > /// A signed type to represent a floating point numbers 
> > > > > > > > > unbiased exponent.
> > > > > > > > > typedef int32_t ExponentType;
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > We already use `int ilogb` in some constexpr evaluation code: 
> > > > > > > > > [[ 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > > > > > >  | link ]], it is working correct because it is working on 
> > > > > > > > > Clang's float representations.
> > > > > > > > > We already use `int ilogb` in some constexpr evaluation code: 
> > > > > > > > > [[ 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > > > > > >  | link ]], it is working correct because it is working on 
> > > > > > > > > Clang's float representations.
> > > > > > > > 
> > > > > > > > `APFloat::getIEEE()`, if I'm following it correctly, only 
> > > > > > > > returns the details of the high double in `ppc_fp128` floats, 
> > > > > > > > and I'm not sufficiently well-versed in the `ppc_fp128` format 
> > > > > > > > to establish whether or not the low double comes into play 
> > > > > > > > here. glibc seems to think that the low double comes into play 
> > > > > > > > in at least one case: 
> > > > > > > > https://github.com/bminor/glibc/blob/30891f35fa7da832b66d80d0807610df361851f3/sysdeps/ieee754/ldbl-128ibm/e_ilogbl.c
> > > > > > > Thanks for the link to the glibc code! It helped me to understand 
> > > > > > > the IEEE754 standard better.
> > > > > > > 
> > > > > > > I did some research and it seems like AST supports a fixed set of 
> > > > > > > float types, each working good with `ilogb`:
> > > > > > > ```
> > > > > > > half (__fp16, only for OpenCL), float16, float, double, long 
> > > > > > > double, float128
> > > > > > > ```
> > > > > > > [[ 
> > > > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/lib/Sema/SemaExpr.cpp#L3911-L3931
> > > > > > >  | link to SemaExpr.cpp ]]
> > > > > > > 
> > > > > > > It means that the constant evaluator doesn't deal with other 
> > > > > > > float types including `ppc_fp128`.
> > > > > > > If `ppc_fp128` was supported on the AST level, it would anyway 
> > > > > > > come through type casting, and `__builtin_ilog` would 
> > > > > > > deal with a value of a known type.
> > > > > > > 
> > > > > > > I checked the list of builtins - each builtin argument of float 
> > > > > > > type also supports only common float types:
> > > > > > > [[ 
> > > > > > > 

[Diffusion] rGe4ec6ce8a75c: Clang: Add release note for defaulted-special-members-POD GCC ABI fix

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.

BRANCHES
  main

/clang/docs/ReleaseNotes.rst:612-613 Thanks!

Users:
  dblaikie (Author)

https://reviews.llvm.org/rGe4ec6ce8a75c

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

[Diffusion] rGe4ec6ce8a75c: Clang: Add release note for defaulted-special-members-POD GCC ABI fix

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.

BRANCHES
  main

/clang/docs/ReleaseNotes.rst:612-613 Shouldn't the usage of POD here be 
clarified to say that we mean "POD for the purposes of Itanium ABI layout"?

Users:
  dblaikie (Author)

https://reviews.llvm.org/rGe4ec6ce8a75c

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

[Diffusion] rGe4ec6ce8a75c: Clang: Add release note for defaulted-special-members-POD GCC ABI fix

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a subscriber: cfe-commits.

BRANCHES
  main

Users:
  dblaikie (Author)

https://reviews.llvm.org/rGe4ec6ce8a75c

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

[PATCH] D136568: [Clang] Support constexpr builtin ilogb

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:12452
+int Ilogb;
+if (APFloat::opStatus St = ilogb(F, Ilogb); !isConstantOpStatus(St))
+  return false;

hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Izaron wrote:
> > > majnemer wrote:
> > > > Izaron wrote:
> > > > > jcranmer-intel wrote:
> > > > > > Izaron wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > jcranmer-intel wrote:
> > > > > > > > > `long double` is `ppc_fp128` on at least some PPC targets, 
> > > > > > > > > and while I'm not entirely certain of what `ilogb` properly 
> > > > > > > > > returns in the corner cases of the `ppc_fp128`, I'm not 
> > > > > > > > > entirely confident that the implementation of `APFloat` is 
> > > > > > > > > correct in those cases. I'd like someone with PPC background 
> > > > > > > > > to comment in here.
> > > > > > > > Paging @hubert.reinterpretcast for help finding a good person 
> > > > > > > > to comment on the PPC questions.
> > > > > > > @jcranmer-intel constexpr evaluation is quite 
> > > > > > > machine-/target-independent. Clang evaluates it based on its 
> > > > > > > **internal** representation of float variables.
> > > > > > > 
> > > > > > > [[ 
> > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L1256
> > > > > > >  | int ilogb ]] uses `Arg.getIEEE()`, that [[ 
> > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L819-L825
> > > > > > >  | returns Clang's internal float representation ]].
> > > > > > > 
> > > > > > > Whichever float semantics is being used, [[ 
> > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/lib/Support/APFloat.cpp#L54-L61
> > > > > > >  | minExponent and maxExponent are representable as 
> > > > > > > APFloatBase::ExponentType ]], which is `int32_t`:
> > > > > > > ```
> > > > > > > /// A signed type to represent a floating point numbers unbiased 
> > > > > > > exponent.
> > > > > > > typedef int32_t ExponentType;
> > > > > > > ```
> > > > > > > 
> > > > > > > We already use `int ilogb` in some constexpr evaluation code: [[ 
> > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > > > >  | link ]], it is working correct because it is working on 
> > > > > > > Clang's float representations.
> > > > > > > We already use `int ilogb` in some constexpr evaluation code: [[ 
> > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > > > >  | link ]], it is working correct because it is working on 
> > > > > > > Clang's float representations.
> > > > > > 
> > > > > > `APFloat::getIEEE()`, if I'm following it correctly, only returns 
> > > > > > the details of the high double in `ppc_fp128` floats, and I'm not 
> > > > > > sufficiently well-versed in the `ppc_fp128` format to establish 
> > > > > > whether or not the low double comes into play here. glibc seems to 
> > > > > > think that the low double comes into play in at least one case: 
> > > > > > https://github.com/bminor/glibc/blob/30891f35fa7da832b66d80d0807610df361851f3/sysdeps/ieee754/ldbl-128ibm/e_ilogbl.c
> > > > > Thanks for the link to the glibc code! It helped me to understand the 
> > > > > IEEE754 standard better.
> > > > > 
> > > > > I did some research and it seems like AST supports a fixed set of 
> > > > > float types, each working good with `ilogb`:
> > > > > ```
> > > > > half (__fp16, only for OpenCL), float16, float, double, long double, 
> > > > > float128
> > > > > ```
> > > > > [[ 
> > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/lib/Sema/SemaExpr.cpp#L3911-L3931
> > > > >  | link to SemaExpr.cpp ]]
> > > > > 
> > > > > It means that the constant evaluator doesn't deal with other float 
> > > > > types including `ppc_fp128`.
> > > > > If `ppc_fp128` was supported on the AST level, it would anyway come 
> > > > > through type casting, and `__builtin_ilog` would deal with a 
> > > > > value of a known type.
> > > > > 
> > > > > I checked the list of builtins - each builtin argument of float type 
> > > > > also supports only common float types:
> > > > > [[ 
> > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L27-L31
> > > > >  | link to Builtins.def 1 ]]
> > > > > [[ 
> > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L53-L54
> > > > >  | link to Builtins.def 2 ]]
> > > > Won't long double map to ppc_fp128 for some targets?
> > > Hi! It will map, but only **after** all the constant (constexpr) 
> > > calculations are done 

[PATCH] D136568: [Clang] Support constexpr builtin ilogb

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:12452
+int Ilogb;
+if (APFloat::opStatus St = ilogb(F, Ilogb); !isConstantOpStatus(St))
+  return false;

hubert.reinterpretcast wrote:
> Izaron wrote:
> > majnemer wrote:
> > > Izaron wrote:
> > > > jcranmer-intel wrote:
> > > > > Izaron wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > jcranmer-intel wrote:
> > > > > > > > `long double` is `ppc_fp128` on at least some PPC targets, and 
> > > > > > > > while I'm not entirely certain of what `ilogb` properly returns 
> > > > > > > > in the corner cases of the `ppc_fp128`, I'm not entirely 
> > > > > > > > confident that the implementation of `APFloat` is correct in 
> > > > > > > > those cases. I'd like someone with PPC background to comment in 
> > > > > > > > here.
> > > > > > > Paging @hubert.reinterpretcast for help finding a good person to 
> > > > > > > comment on the PPC questions.
> > > > > > @jcranmer-intel constexpr evaluation is quite 
> > > > > > machine-/target-independent. Clang evaluates it based on its 
> > > > > > **internal** representation of float variables.
> > > > > > 
> > > > > > [[ 
> > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L1256
> > > > > >  | int ilogb ]] uses `Arg.getIEEE()`, that [[ 
> > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L819-L825
> > > > > >  | returns Clang's internal float representation ]].
> > > > > > 
> > > > > > Whichever float semantics is being used, [[ 
> > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/lib/Support/APFloat.cpp#L54-L61
> > > > > >  | minExponent and maxExponent are representable as 
> > > > > > APFloatBase::ExponentType ]], which is `int32_t`:
> > > > > > ```
> > > > > > /// A signed type to represent a floating point numbers unbiased 
> > > > > > exponent.
> > > > > > typedef int32_t ExponentType;
> > > > > > ```
> > > > > > 
> > > > > > We already use `int ilogb` in some constexpr evaluation code: [[ 
> > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > > >  | link ]], it is working correct because it is working on Clang's 
> > > > > > float representations.
> > > > > > We already use `int ilogb` in some constexpr evaluation code: [[ 
> > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > > >  | link ]], it is working correct because it is working on Clang's 
> > > > > > float representations.
> > > > > 
> > > > > `APFloat::getIEEE()`, if I'm following it correctly, only returns the 
> > > > > details of the high double in `ppc_fp128` floats, and I'm not 
> > > > > sufficiently well-versed in the `ppc_fp128` format to establish 
> > > > > whether or not the low double comes into play here. glibc seems to 
> > > > > think that the low double comes into play in at least one case: 
> > > > > https://github.com/bminor/glibc/blob/30891f35fa7da832b66d80d0807610df361851f3/sysdeps/ieee754/ldbl-128ibm/e_ilogbl.c
> > > > Thanks for the link to the glibc code! It helped me to understand the 
> > > > IEEE754 standard better.
> > > > 
> > > > I did some research and it seems like AST supports a fixed set of float 
> > > > types, each working good with `ilogb`:
> > > > ```
> > > > half (__fp16, only for OpenCL), float16, float, double, long double, 
> > > > float128
> > > > ```
> > > > [[ 
> > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/lib/Sema/SemaExpr.cpp#L3911-L3931
> > > >  | link to SemaExpr.cpp ]]
> > > > 
> > > > It means that the constant evaluator doesn't deal with other float 
> > > > types including `ppc_fp128`.
> > > > If `ppc_fp128` was supported on the AST level, it would anyway come 
> > > > through type casting, and `__builtin_ilog` would deal with a 
> > > > value of a known type.
> > > > 
> > > > I checked the list of builtins - each builtin argument of float type 
> > > > also supports only common float types:
> > > > [[ 
> > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L27-L31
> > > >  | link to Builtins.def 1 ]]
> > > > [[ 
> > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L53-L54
> > > >  | link to Builtins.def 2 ]]
> > > Won't long double map to ppc_fp128 for some targets?
> > Hi! It will map, but only **after** all the constant (constexpr) 
> > calculations are done (that is, after the AST parsing stage) - in the 
> > Codegen stage.
> > 
> > The Clang's constant evaluator itself never deals with ppc_fp128 and 
> > doesn't care about the target.
> > While 

[PATCH] D136568: [Clang] Support constexpr builtin ilogb

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:12452
+int Ilogb;
+if (APFloat::opStatus St = ilogb(F, Ilogb); !isConstantOpStatus(St))
+  return false;

Izaron wrote:
> majnemer wrote:
> > Izaron wrote:
> > > jcranmer-intel wrote:
> > > > Izaron wrote:
> > > > > aaron.ballman wrote:
> > > > > > jcranmer-intel wrote:
> > > > > > > `long double` is `ppc_fp128` on at least some PPC targets, and 
> > > > > > > while I'm not entirely certain of what `ilogb` properly returns 
> > > > > > > in the corner cases of the `ppc_fp128`, I'm not entirely 
> > > > > > > confident that the implementation of `APFloat` is correct in 
> > > > > > > those cases. I'd like someone with PPC background to comment in 
> > > > > > > here.
> > > > > > Paging @hubert.reinterpretcast for help finding a good person to 
> > > > > > comment on the PPC questions.
> > > > > @jcranmer-intel constexpr evaluation is quite 
> > > > > machine-/target-independent. Clang evaluates it based on its 
> > > > > **internal** representation of float variables.
> > > > > 
> > > > > [[ 
> > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L1256
> > > > >  | int ilogb ]] uses `Arg.getIEEE()`, that [[ 
> > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L819-L825
> > > > >  | returns Clang's internal float representation ]].
> > > > > 
> > > > > Whichever float semantics is being used, [[ 
> > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/lib/Support/APFloat.cpp#L54-L61
> > > > >  | minExponent and maxExponent are representable as 
> > > > > APFloatBase::ExponentType ]], which is `int32_t`:
> > > > > ```
> > > > > /// A signed type to represent a floating point numbers unbiased 
> > > > > exponent.
> > > > > typedef int32_t ExponentType;
> > > > > ```
> > > > > 
> > > > > We already use `int ilogb` in some constexpr evaluation code: [[ 
> > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > >  | link ]], it is working correct because it is working on Clang's 
> > > > > float representations.
> > > > > We already use `int ilogb` in some constexpr evaluation code: [[ 
> > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592
> > > > >  | link ]], it is working correct because it is working on Clang's 
> > > > > float representations.
> > > > 
> > > > `APFloat::getIEEE()`, if I'm following it correctly, only returns the 
> > > > details of the high double in `ppc_fp128` floats, and I'm not 
> > > > sufficiently well-versed in the `ppc_fp128` format to establish whether 
> > > > or not the low double comes into play here. glibc seems to think that 
> > > > the low double comes into play in at least one case: 
> > > > https://github.com/bminor/glibc/blob/30891f35fa7da832b66d80d0807610df361851f3/sysdeps/ieee754/ldbl-128ibm/e_ilogbl.c
> > > Thanks for the link to the glibc code! It helped me to understand the 
> > > IEEE754 standard better.
> > > 
> > > I did some research and it seems like AST supports a fixed set of float 
> > > types, each working good with `ilogb`:
> > > ```
> > > half (__fp16, only for OpenCL), float16, float, double, long double, 
> > > float128
> > > ```
> > > [[ 
> > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/lib/Sema/SemaExpr.cpp#L3911-L3931
> > >  | link to SemaExpr.cpp ]]
> > > 
> > > It means that the constant evaluator doesn't deal with other float types 
> > > including `ppc_fp128`.
> > > If `ppc_fp128` was supported on the AST level, it would anyway come 
> > > through type casting, and `__builtin_ilog` would deal with a 
> > > value of a known type.
> > > 
> > > I checked the list of builtins - each builtin argument of float type also 
> > > supports only common float types:
> > > [[ 
> > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L27-L31
> > >  | link to Builtins.def 1 ]]
> > > [[ 
> > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L53-L54
> > >  | link to Builtins.def 2 ]]
> > Won't long double map to ppc_fp128 for some targets?
> Hi! It will map, but only **after** all the constant (constexpr) calculations 
> are done (that is, after the AST parsing stage) - in the Codegen stage.
> 
> The Clang's constant evaluator itself never deals with ppc_fp128 and doesn't 
> care about the target.
> While parsing the AST, the constant evaluator works on the same level with 
> it, providing calculated values to the AST being built on-the-fly. At the 
> moment AST is built, constant evaluation is over.
> The 

[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D135919#3859491 , @tahonermann 
wrote:

>> the case from https://github.com/llvm/llvm-project/issues/57828 is not for a 
>> block-scope variable, but the case in the patch description is...
>
> Thanks, Hubert. Yes, I found that the reported issue occurred for any case 
> where thread safe guard variables are not required. I chose the block-scope 
> variable example for the patch summary because I felt it better presented the 
> issue.
>
> Your question inspired me to do some additional testing though and I see both 
> gcc and icc also exhibit the re-initialization behavior for that case, but 
> not the case reported in https://github.com/llvm/llvm-project/issues/57828.

For the block-scope case, https://eel.is/c++draft/stmt.dcl#3: If control 
re-enters the declaration recursively while the variable is being initialized, 
the behavior is undefined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135919

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

[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D135919#3858808 , @rjmccall wrote:

> We can't set the flag if initialization is aborted by an exception, which is 
> not ruled out by the use of thread-unsafe statics.  So this is not a correct 
> change.

@tahonermann, the case from https://github.com/llvm/llvm-project/issues/57828 
is not for a block-scope variable, but the case in the patch description 
//is//...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135919

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

[PATCH] D135848: [clang][Module][AIX] Mark test unsupported since objc doesn't have xcoff support

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

Confirming LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135848

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

[PATCH] D135384: [AIX] Enable the use of the -pg flag

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM with minor comment; thanks!




Comment at: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp:39
+Triple TargetTriple(M.getTargetTriple());
+if (Func == "__mcount" && TargetTriple.isOSAIX()) {
+  Type *SizeTy = M.getDataLayout().getIntPtrType(C);

The construction for the `Triple` already did the string processing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135384

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

[PATCH] D135848: [clang][Module][AIX] Mark test unsupported since objc doesn't have xcoff support

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM with comment (matches similar files already present in the directory).




Comment at: clang/test/Modules/module-file-home-is-cwd.m:1
+// UNSUPPORTED: aix
 // RUN: cd %S

We see GOFF in the same list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135848

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

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

> I reached out to the GCC author to confirm that the committee acknowledges 
> that there should be a change to temp.func.order p6.2.1, but no consensus on 
> what changes to make [3].

A more accurate description is that some changes elsewhere to overload 
resolution are probably needed, but no solution has been developed sufficiently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128750

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

[PATCH] D135384: [AIX] Enable the use of the -pg flag

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp:38-40
+const Twine targetTwine(M.getTargetTriple());
+Triple targetTriple(targetTwine);
+if (targetTriple.isOSAIX()) {

A `Twine` can be implicitly constructed from a `std::string`. The implicit 
conversion works in this context.
Also: Change the variable name to use the LLVM naming convention for 
non-"functions".



Comment at: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp:40
+Triple targetTriple(targetTwine);
+if (targetTriple.isOSAIX()) {
+  Type *SizeTy = M.getDataLayout().getIntPtrType(C);

I see no reason why we should assume that all of the other function names 
should have the extra argument passed on AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135384

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

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D128745#3834435 , @ychen wrote:

> New flag or not, as long as we allow users to use the old behavior, it is 
> already fracturing the ecosystem. Do you mean (1) we shouldn't give the user 
> the choices or  (2) we allow users to use the old behavior but only for a few 
> releases and then remove the flag? I think (1) is a little bit harsh but I 
> would not say it is disruptive. (2) is more user-friendly.

(1) has been the default absent information to indicate that the new behaviour 
is not ready for deployment (because it would cause widespread disruption). To 
consider alternatives would require information about the nature of the 
deployment difficulties.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745

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

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D128745#3834313 , @ychen wrote:

> I think the consensus is some flag is needed to put it back to legacy 
> behavior just in case.

I am not sure there is strong consensus to add a flag "just in case". 
Historically, many DRs had effects on overload resolution (via SFINAE or 
otherwise). Once we start adding flags "just in case", we might end up having a 
lot of them. These flags can exacerbate fracturing of the ecosystem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745

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

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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D128745#3833291 , @aaron.ballman 
wrote:

> @rjmccall is correct about us not being required to apply DRs when they're 
> disruptive, but at the same time, WG21 DRs are intended to be handled as if 
> the original standard they were reported against had always been using the 
> fixed form. So *not* applying a DR in order to avoid problems for existing 
> code can cause problems for existing and future code in terms of portability 
> between compilers (and ABI impacts that stem from the semantic changes). So I 
> think we wish to apply the DRs as broadly as we can. The question is: do we 
> think users with existing code should not have to change or do we think it's 
> reasonable to give them a flag to opt into the old behavior? My personal 
> feeling is -- the default compiler mode should be as conforming as we can 
> make it be within reason, and since this has some impact but not a massive 
> break (no major OS SDKs or third party libraries seem to be impacted as best 
> I can tell), my feeling is that we should strongly consider adding a feature 
> flag (other than ABI compat, as that does seem like a bit of an odd choice to 
> key on) to opt into the older behavior, esp since the break is a loud one and 
> not a silent one.
>
> Adding @hubert.reinterpretcast as C++ conformance code owner in case he's got 
> opinions as well.

I happen to be on vacation this week ahead of Canadian Thanksgiving (or trying 
as much as I can anyway). I agree that broad application of DRs is generally 
what has been expected in the context of Clang and GCC (except where it is 
believed the DR resolution itself is defective, in which case the committee is 
consulted).

It seems there is a question of whether the DR resolutions are defective/too 
breaking to be advisable. Do we have a useful summary of what led to that 
opinion?

In considering a compatibility flag, I think some thought should be given to 
whether to time-limit it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128745

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

[PATCH] D134284: [AIX] change the clang tests with llvm-nm -Xany

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Instead of applying `OBJECT_MODE` to everything, maybe we can apply it only to 
the tool invocations via a LIT expansion?
Something like `%llvm-nm` that expands (for AIX) to `OBJECT_MODE=any llvm-nm`?

Would need to check that the LIT internal shell accepts that syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134284

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

[PATCH] D131541: [Sema] Fix friend destructor declarations after D130936

[Hubert Tong via Phabricator via cfe-commits]

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

This LGTM; thanks!

In D131541#3721383 , @royjacobson 
wrote:

> I still don't diagnose the dependent friend case, but I didn't see how to do 
> it easily.

Maybe adding code to `TemplateDeclInstantiator::VisitFriendDecl` will do the 
trick.




Comment at: clang/test/SemaCXX/member-class-11.cpp:36-40
+// FIXME: We should diagnose here.
+template 
+struct E {
+  friend T::S::~V();
+};

royjacobson wrote:
> hubert.reinterpretcast wrote:
> > Please replace this with the case where there is an instantiation. Also, 
> > the prior change to the release notes in https://reviews.llvm.org/D130936 
> > should be adjusted to reflect the new scope of what is fixed.
> Updated the test cases accordingly.
> 
> I don't think there's things to add in the release notes as this is just 
> fixing breakage from my previous patch, not really diagnosing new cases?
Okay; I guess "invalid destructor names were incorrectly accepted on template 
classes" uses "on" and not "in" (and can be read to match the current scope of 
the fix).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131541

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

[PATCH] D131541: [Sema] Fix friend destructor declarations after D130936

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:11522
+  if (NewFD->getFriendObjectKind() == Decl::FriendObjectKind::FOK_None ||
+  !NewFD->isDependentContext()) {
+QualType ClassType = Destructor->getThisObjectType();

This condition appears to be true even when the friend itself is not dependent. 
The error message changes for:
```
struct C;
struct B { ~B(); };
template 
struct A {
  friend B::~C();
};
```

(causing the template and non-template cases to generate different messages).




Comment at: clang/test/SemaCXX/member-class-11.cpp:36-40
+// FIXME: We should diagnose here.
+template 
+struct E {
+  friend T::S::~V();
+};

Please replace this with the case where there is an instantiation. Also, the 
prior change to the release notes in https://reviews.llvm.org/D130936 should be 
adjusted to reflect the new scope of what is fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131541

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

[PATCH] D131541: [Sema] Fix friend destructor declarations after D130936

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:11518-11520
+  // FIXME: We still don't diagnose on this case
+  // template 
+  // struct A { friend T::S::~V(); };

There's nothing we can diagnose about this without an instantiation (because 
`S` could be an alias for a class having `V` as the injected-class-name).

It is, however, true that we don't diagnose this even with problematic 
instantiations:
```
struct R { struct V; ~R(); };
struct QQ { using S = R; };

template 
struct A { friend T::S::~V(); };

A a;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131541

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

[PATCH] D130936: [SemaCXX] Validate destructor is valid for dependent classes

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

This does not work for friend declarations.

  template 
  struct A {
friend T::S::~S();
  private:
static constexpr int secret = 42;
  };
  
  struct Q {
struct S { ~S(); };
  };
  
  Q::S::~S() {
void foo(int);
foo(A::secret);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130936

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

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: libcxx/trunk/include/istream:1223
+__state |= ios_base::badbit;
 return -1;
 }

@ldionne, another dubious aspect of this patch is that it initializes `__r` to 
`0` and never sets it. Seems like this line should have been to set `__r` and 
not to return directly.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49863

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

[PATCH] D129401: [libLTO] Set data-sections by default in libLTO.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

With respect to the compiler changes: they look good. I checked, and the 
injected options being passed to the linker are added before user-specified 
ones (and that is good).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129401

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

[PATCH] D129401: [libLTO] Set data-sections by default in libLTO.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Tests require fixing.




Comment at: llvm/test/LTO/PowerPC/data-sections-aix.ll:18-19
+
+; CHECK-NOT:  (csect: .data)
+; CHECK:   g O .data {{.*}} var
+

Unfortunately (without the change) this would pass even if the line was
```
 g O .data (csect: .data)    var
```




Comment at: llvm/test/LTO/PowerPC/data-sections-linux.ll:20
+
+; CHECK-NO-DATA-SECTIONS-NOT: .var
+; CHECK-NO-DATA-SECTIONS:  g O .bss {{.*}} var

This is not needed. The whitespace before the `{{.*}}` will not match if 
`.bss.var` is encountered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129401

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

[PATCH] D129401: [libLTO] Set data-sections by default in libLTO for ELF and XCOFF.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D129401#3662857 , @quinnp wrote:

> @hubert.reinterpretcast please correct me if I am wrong about why this change 
> is needed.

I think your description is correct.




Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:351
+  llvm::Triple::ObjectFormatType ObjectFormat = Triple.getObjectFormat();
+  if (!codegen::getExplicitDataSections() &&
+  (ObjectFormat == llvm::Triple::ObjectFormatType::ELF ||

quinnp wrote:
> hubert.reinterpretcast wrote:
> > w2yehia wrote:
> > > quinnp wrote:
> > > > w2yehia wrote:
> > > > > any reason we do this for ELF and XCOFF only?
> > > > I don't think there is a particular reason that we do this for ELF and 
> > > > XCOFF only. We needed this fixed for `AIX` (`XCOFF`) and wanted to 
> > > > change `Linux` (`ELF`) to match the behaviour of `lld`/`gold` at the 
> > > > same time. I'm not sure what other file formats need for this so I did 
> > > > not include them.
> > > > 
> > > > @hubert.reinterpretcast might have a better answer for this.
> > > I don't know either about the other formats, was just wondering.
> > > I think it's safe to do it for the file formats that we know are 
> > > currently different between libLTO and lld/gold. The proposed change is 
> > > an improvement with minimal risk.
> > I agree with @w2yehia that we should change the data-sections to "on" by 
> > default in libLTO for the other file formats where one of lld/the gold 
> > plugin sets it to "on".
> @hubert.reinterpretcast I think that if we want to change `data-sections` to 
> "on" by default for any file format which  `lld` or `gold plugin` set 
> data-sections to "on", we would set `data-sections` to "on" for all file 
> formats. This is because `gold plugin` does not check the file format when it 
> is setting `data-sections`. You can see where `gold plugin` sets 
> `data-sections` here: 
> https://github.com/llvm/llvm-project/blob/main/llvm/tools/gold/gold-plugin.cpp#L893
> 
> Do you suggest that we remove the checks for file format when setting 
> `data-sections` in `libLTO`? ie. change the `if` statement to this:
> ```
> if (!codegen::getExplicitDataSections())
>   Config.Options.DataSections = true;
> ```
I can't imagine setting data-sections on by default for LTO being wrong, but I 
am not sure that the gold-plugin supports the same range of object file formats 
either. I guess using `lld` as the precedent is conservatively correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129401

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

[PATCH] D129912: [Tooling/DependencyScanning] Enable passing a `vfs::FileSystem` object to `DependencyScanningTool`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D129912#3661282 , @akyrtzi wrote:

> Sorry about that, it should be fixed via 
> https://github.com/llvm/llvm-project/commit/d1b58cada61aa8bc44d8e8ef9c23ed12ef7b549b

Thanks for the fix; it does look promising.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129912

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

[PATCH] D129912: [Tooling/DependencyScanning] Enable passing a `vfs::FileSystem` object to `DependencyScanningTool`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

The added test is not passing on the AIX builder: 
https://lab.llvm.org/buildbot/#/builders/214/builds/2388/steps/6/logs/FAIL__Clang-Unit__83

Note that Clang on that platform generates assembly by default (then invokes 
the system assembler).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129912

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

[PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D128059#3640564 , @cor3ntin wrote:

> Good point. The error was a bit misleading but i guess what's happening is a 
> segfault when running `clang-ast-dump`. 
> I'm reverting for now and I don't really know how to go from there as I don't 
> have the hardware to test that code path.

Sending you info via e-mail on how to request access to systems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128059

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

[PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D128059#3640424 , @cor3ntin wrote:

> @aaron.ballman Thanks for the review. I landed the changes and got a bunch of 
> bots screaming at me for changes that are completely unrelated
>
> https://lab.llvm.org/buildbot/#/builders/21/builds/45146
> https://lab.llvm.org/buildbot/#/builders/36/builds/22838
> https://lab.llvm.org/buildbot/#/builders/121/builds/21183

What do you mean it is completely unrelated? You have `__ALTIVEC__`-specific 
changes and it knocks out the PPC bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128059

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

[PATCH] D129165: [AIX][clang/test] Set/propagate AIXTHREAD_STK for AIX

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbd8b55e609c8: [AIX][clang/test] Set/propagate AIXTHREAD_STK 
for AIX (authored by hubert.reinterpretcast).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129165

Files:
  clang/test/Driver/arm-float-abi-lto.c
  clang/test/Index/lit.local.cfg
  clang/test/lit.cfg.py


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -264,3 +264,13 @@
   '/ASTMerge/anonymous-fields', 
'/ASTMerge/injected-class-name-decl'):
 exclude_unsupported_files_for_aix(config.test_source_root + directory)
 
+# Some tests perform deep recursion, which requires a larger pthread stack size
+# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
+# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
+# a larger pthread stack size for the tests. Various applications and runtime
+# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
+# that as a default value here.
+if 'AIXTHREAD_STK' in os.environ:
+config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
+elif platform.system() == 'AIX':
+config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Index/lit.local.cfg
===
--- clang/test/Index/lit.local.cfg
+++ /dev/null
@@ -1,12 +0,0 @@
-import platform
-
-# Some tests perform deep recursion, which requires a larger pthread stack size
-# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
-# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
-# a larger pthread stack size for the tests. Various applications and runtime
-# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
-# that as a default value here.
-if 'AIXTHREAD_STK' in os.environ:
-config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
-elif platform.system() == 'AIX':
-config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Driver/arm-float-abi-lto.c
===
--- clang/test/Driver/arm-float-abi-lto.c
+++ clang/test/Driver/arm-float-abi-lto.c
@@ -1,5 +1,3 @@
-// FIXME: Produces a segmentation fault on AIX after the introduction of 
opaque pointers (D125847). 
-// UNSUPPORTED: system-aix
 // REQUIRES: arm-registered-target
 
 // RUN: %clang --target=arm-none-eabi -mcpu=cortex-m33 -mfloat-abi=hard -O1 %s 
-S -o - -emit-llvm -DCALL_LIB -DDEFINE_LIB | FileCheck %s


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -264,3 +264,13 @@
   '/ASTMerge/anonymous-fields', '/ASTMerge/injected-class-name-decl'):
 exclude_unsupported_files_for_aix(config.test_source_root + directory)
 
+# Some tests perform deep recursion, which requires a larger pthread stack size
+# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
+# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
+# a larger pthread stack size for the tests. Various applications and runtime
+# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
+# that as a default value here.
+if 'AIXTHREAD_STK' in os.environ:
+config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
+elif platform.system() == 'AIX':
+config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Index/lit.local.cfg
===
--- clang/test/Index/lit.local.cfg
+++ /dev/null
@@ -1,12 +0,0 @@
-import platform
-
-# Some tests perform deep recursion, which requires a larger pthread stack size
-# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
-# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
-# a larger pthread stack size for the tests. Various applications and runtime
-# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
-# that as a default value here.
-if 'AIXTHREAD_STK' in os.environ:
-config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
-elif platform.system() == 'AIX':
-config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Driver/arm-float-abi-lto.c
===
--- clang/test/Driver/arm-float-abi-lto.c
+++ clang/test/Driver/arm-float-abi-lto.c
@@ -1,5 +1,3 @@
-// FIXME: Produces a segmentation fault on AIX after the introduction of opaque pointers (D125847). 
-// UNSUPPORTED: system-aix
 // REQUIRES: arm-registered-target
 
 // RUN: %clang --target=arm-none-eabi -mcpu=cortex-m33 -mfloat-abi=hard -O1 %s -S 

[PATCH] D129165: [AIX][clang/test] Set/propagate AIXTHREAD_STK for AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast created this revision.
hubert.reinterpretcast added reviewers: xingxue, daltenty, Jake-Egan, 
cebowleratibm.
Herald added subscribers: ormris, arphaman, steven_wu, hiraditya, kristof.beyls.
Herald added a project: All.
hubert.reinterpretcast requested review of this revision.
Herald added a project: clang.

Some tests perform deep recursion, which requires a larger pthread stack
size than the relatively low default of 192 KiB for 64-bit processes on
AIX. The `AIXTHREAD_STK` environment variable provides a non-intrusive
way to request a larger pthread stack size for the tests. The required
pthread stack size depends on the build configuration.

A 4 MiB default is generous compared to the 512 KiB of macOS; however,
it is known that some compilers on AIX produce code that uses
comparatively more stack space.

This patch expands the solution from D65688  
to apply to all Clang LIT
tests.

This also reverts commit c3c75d805c2174388417080f762230961b3433d6 
,
"[clang][test] Mark test arm-float-abi-lto.c unsupported on AIX".

The problem was caused by the test running up against the per-thread
stack limit on AIX. This is resolved by having the tests run with
`AIXTHREAD_STK` set for 4 MiB.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129165

Files:
  clang/test/Driver/arm-float-abi-lto.c
  clang/test/Index/lit.local.cfg
  clang/test/lit.cfg.py


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -264,3 +264,13 @@
   '/ASTMerge/anonymous-fields', 
'/ASTMerge/injected-class-name-decl'):
 exclude_unsupported_files_for_aix(config.test_source_root + directory)
 
+# Some tests perform deep recursion, which requires a larger pthread stack size
+# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
+# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
+# a larger pthread stack size for the tests. Various applications and runtime
+# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
+# that as a default value here.
+if 'AIXTHREAD_STK' in os.environ:
+config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
+elif platform.system() == 'AIX':
+config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Index/lit.local.cfg
===
--- clang/test/Index/lit.local.cfg
+++ /dev/null
@@ -1,12 +0,0 @@
-import platform
-
-# Some tests perform deep recursion, which requires a larger pthread stack size
-# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
-# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
-# a larger pthread stack size for the tests. Various applications and runtime
-# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
-# that as a default value here.
-if 'AIXTHREAD_STK' in os.environ:
-config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
-elif platform.system() == 'AIX':
-config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Driver/arm-float-abi-lto.c
===
--- clang/test/Driver/arm-float-abi-lto.c
+++ clang/test/Driver/arm-float-abi-lto.c
@@ -1,5 +1,3 @@
-// FIXME: Produces a segmentation fault on AIX after the introduction of 
opaque pointers (D125847). 
-// UNSUPPORTED: system-aix
 // REQUIRES: arm-registered-target
 
 // RUN: %clang --target=arm-none-eabi -mcpu=cortex-m33 -mfloat-abi=hard -O1 %s 
-S -o - -emit-llvm -DCALL_LIB -DDEFINE_LIB | FileCheck %s


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -264,3 +264,13 @@
   '/ASTMerge/anonymous-fields', '/ASTMerge/injected-class-name-decl'):
 exclude_unsupported_files_for_aix(config.test_source_root + directory)
 
+# Some tests perform deep recursion, which requires a larger pthread stack size
+# than the relatively low default of 192 KiB for 64-bit processes on AIX. The
+# `AIXTHREAD_STK` environment variable provides a non-intrusive way to request
+# a larger pthread stack size for the tests. Various applications and runtime
+# libraries on AIX use a default pthread stack size of 4 MiB, so we will use
+# that as a default value here.
+if 'AIXTHREAD_STK' in os.environ:
+config.environment['AIXTHREAD_STK'] = os.environ['AIXTHREAD_STK']
+elif platform.system() == 'AIX':
+config.environment['AIXTHREAD_STK'] = '4194304'
Index: clang/test/Index/lit.local.cfg
===
--- clang/test/Index/lit.local.cfg
+++ /dev/null
@@ -1,12 +0,0 @@
-import platform
-
-# Some tests perform deep 

[PATCH] D128645: Update developer policy.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

LGTM!




Comment at: llvm/docs/DeveloperPolicy.rst:88
+#. Patches should be unified diffs with "infinite context" (i.e. using 
something
+   like `git diff -U99 main`).
+

vext01 wrote:
> hubert.reinterpretcast wrote:
> > Using `git diff` like this, there are risks that `main` is now ahead of 
> > your branch's base. Probably safer to use `HEAD~n` where `n` is the number 
> > of commits you have on your branch.
> > 
> The exact command you want to issue is going to vary according to 
> circumstance and personal workflow preference. Sometimes `main` is fine, if 
> it isn't `HEAD~n` is fine, but if you have a lot of commits then determining 
> (i.e. manually counting) `n` will be impractical and you probably want to 
> just use an absolute git hash.
> 
> I don't think we need to delve into a great level of detail. I think the 
> prose "using something like" is adequate.
okay


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

https://reviews.llvm.org/D128645

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

[PATCH] D128645: Update developer policy.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:403-405
+Note, these mailing lists are moderated and it is not unusual for a large
+commit to require a moderator to approve the email, so do not be concerned if a
+commit does not immediately appear in the archives.

"Note," => "Note that"
Add comma before "and".


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

https://reviews.llvm.org/D128645

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

[PATCH] D128645: Update developer policy.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:88
+#. Patches should be unified diffs with "infinite context" (i.e. using 
something
+   like `git diff -U99 main`).
+

Using `git diff` like this, there are risks that `main` is now ahead of your 
branch's base. Probably safer to use `HEAD~n` where `n` is the number of 
commits you have on your branch.



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

https://reviews.llvm.org/D128645

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

[PATCH] D127544: Add no_sanitize('hwaddress') (and 'memtag', but that's a no-op).

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D127544#3609393 , @hctim wrote:

> I see that the clang-ppc64le-linux bot is green with the second attempt 
> (https://lab.llvm.org/buildbot/#/builders/105/builds/27200). Please let me 
> know if you have further issues.

Thanks. Will do (hopefully there won't be more issues).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127544

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

[PATCH] D127544: Add no_sanitize('hwaddress') (and 'memtag', but that's a no-op).

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D127544#3609335 , @hctim wrote:

> Sent fadc98b06befb674fa47da4f3d8606bf61bed681 
>  to 
> fix-forward.

I thought `*-registered-target` is true as long as the target is available, not 
only when the target happens to be the default.
Also, if there would be more such tests in the future, maybe a LIT feature that 
the default target supports HWAsan makes sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127544

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

[PATCH] D127544: Add no_sanitize('hwaddress') (and 'memtag', but that's a no-op).

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

From the content of this patch, it probably is 
`llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp`:

  switch (TargetTriple.getArch()) {
  case Triple::x86_64:
// The signal handler will find the data address in rdi.
Asm = InlineAsm::get(
FunctionType::get(IRB.getVoidTy(), {PtrLong->getType()}, false),
"int3\nnopl " +
itostr(0x40 + (AccessInfo & HWASanAccessInfo::RuntimeMask)) +
"(%rax)",
"{rdi}",
/*hasSideEffects=*/true);
break;
  case Triple::aarch64:
  case Triple::aarch64_be:
// The signal handler will find the data address in x0.
Asm = InlineAsm::get(
FunctionType::get(IRB.getVoidTy(), {PtrLong->getType()}, false),
"brk #" + itostr(0x900 + (AccessInfo & HWASanAccessInfo::RuntimeMask)),
"{x0}",
/*hasSideEffects=*/true);
break;
  default:
report_fatal_error("unsupported architecture");
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127544

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

[PATCH] D127544: Add no_sanitize('hwaddress') (and 'memtag', but that's a no-op).

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

This is causing "unsupported architecture" errors on bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127544

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

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D124556#3482224 , @ken-matsui 
wrote:

> Thank you!

Thanks in turn.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124556

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

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG52ce95a1a554: [NFC] Prevent shadowing a variable declared in 
`if` (authored by ken-matsui, committed by hubert.reinterpretcast).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124556

Files:
  clang/lib/Basic/Diagnostic.cpp


Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -983,13 +983,13 @@
   if (const char *S = tok::getPunctuatorSpelling(Kind))
 // Quoted token spelling for punctuators.
 Out << '\'' << S << '\'';
-  else if (const char *S = tok::getKeywordSpelling(Kind))
+  else if ((S = tok::getKeywordSpelling(Kind)))
 // Unquoted token spelling for keywords.
 Out << S;
-  else if (const char *S = getTokenDescForDiagnostic(Kind))
+  else if ((S = getTokenDescForDiagnostic(Kind)))
 // Unquoted translatable token name.
 Out << S;
-  else if (const char *S = tok::getTokenName(Kind))
+  else if ((S = tok::getTokenName(Kind)))
 // Debug name, shouldn't appear in user-facing diagnostics.
 Out << '<' << S << '>';
   else


Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -983,13 +983,13 @@
   if (const char *S = tok::getPunctuatorSpelling(Kind))
 // Quoted token spelling for punctuators.
 Out << '\'' << S << '\'';
-  else if (const char *S = tok::getKeywordSpelling(Kind))
+  else if ((S = tok::getKeywordSpelling(Kind)))
 // Unquoted token spelling for keywords.
 Out << S;
-  else if (const char *S = getTokenDescForDiagnostic(Kind))
+  else if ((S = getTokenDescForDiagnostic(Kind)))
 // Unquoted translatable token name.
 Out << S;
-  else if (const char *S = tok::getTokenName(Kind))
+  else if ((S = tok::getTokenName(Kind)))
 // Debug name, shouldn't appear in user-facing diagnostics.
 Out << '<' << S << '>';
   else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D124556#3480228 , @ken-matsui 
wrote:

> @hubert.reinterpretcast
>
> Sorry, I'm a newbie here, but is there anything I should do after getting 
> approved?

I'm not sure if the instructions are a bit out-of-date: 
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

If you intend to contribute going forward, the idea would be to get a few 
patches approved and committed on your behalf and then to request commit 
access. Until you have commit access, you will need to ask people on the review 
to help you commit.

I can commit this patch for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124556

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

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM; thanks for the patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124556

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

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D124556#3479833 , @ken-matsui 
wrote:

> @hubert.reinterpretcast,
>
> Sorry to have missed providing a summary.

You can still provide one by using the "Edit Revision" link.

I suggest something like:
Prevents confusion over which `S` is referenced in the final `else` branch if 
such a use is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124556

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

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

@ken-matsui, can you provide some rationale for the change (got compiler 
warning/error)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124556

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

[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM with minor comments.




Comment at: clang/test/CodeGen/PowerPC/atomic-alignment.c:1-12
 // RUN: %clang_cc1 -verify -triple powerpc-unknown-unknown -emit-llvm -o - %s 
| \
 // RUN:   FileCheck %s --check-prefixes=PPC,PPC32
 // RUN: %clang_cc1 -verify -triple powerpc64le-unknown-linux -emit-llvm -o - 
%s | \
 // RUN:   FileCheck %s --check-prefixes=PPC,PPC64
+// RUN: %clang_cc1 -verify -triple powerpc64le-unknown-linux -emit-llvm -o - 
%s \
+// RUN:   -target-cpu pwr8 | FileCheck %s --check-prefixes=PPC,PPC64
 // RUN: %clang_cc1 -verify -triple powerpc64-unknown-aix -emit-llvm -o - %s | \

Use `-Werror` in place of `-verify` with no diagnostics.



Comment at: clang/test/CodeGen/PowerPC/quadword-atomics.c:1-6
+// RUN: %clang_cc1 -verify -Wno-atomic-alignment -triple powerpc64le-linux-gnu 
\
+// RUN:   -target-cpu pwr8 -emit-llvm -o - %s | FileCheck %s 
--check-prefix=PPC64-PWR8
+// RUN: %clang_cc1 -verify -Wno-atomic-alignment -triple powerpc64le-linux-gnu 
\
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=PPC64
+// RUN: %clang_cc1 -verify -Wno-atomic-alignment -triple powerpc64-unknown-aix 
\
+// RUN:   -target-cpu pwr7 -emit-llvm -o - %s | FileCheck %s 
--check-prefix=PPC64

Same comment as for the other file.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:18057
+bool PPCTargetLowering::shouldInlineQuadwordAtomics() const {
+  // TODO: 16-byte atomic type support for AIX is in progress, we should be 
able
+  // to inline 16-byte atomic ops on AIX too in the future.

Minor nit: Use semicolon.



Comment at: llvm/test/CodeGen/PowerPC/atomics-i128.ll:160
+; LE-PWR8-NEXT:adde r8, r5, r6
+; LE-PWR8-NEXT:stqcx. r8, 0, r3
+; LE-PWR8-NEXT:bne cr0, .LBB1_1

I have verified that the registers in the pairs are used correctly for this 
case. I've skimmed the other cases for the instructions applied to the loaded 
value (or, for non-inline cases, the functions called).

I did not check that the set-up for the calls, etc.

I also haven't looked into the memory barrier instruction usage (but that 
should be common with other widths).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/CodeGen/PowerPC/atomic-alignment.c:14
 // PPC: @c = global i8 0, align 1{{$}}
 _Atomic(char) c; // expected-no-diagnostics
 

I was recently informed that, instead of using `expected-no-diagnostics` and 
`-verify`, using `-Werror` achieves the same with less processing cost. Sorry 
for the churn.

Noting also that `expected-no-diagnostics` only needs to be added once the the 
file.



Comment at: clang/test/CodeGen/PowerPC/quadword-atomics.c:5
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=PPC64
+
+typedef struct {

Can we add an AIX run line for `pwr7`? It should match the `PPC64` check lines.



Comment at: clang/test/CodeGen/PowerPC/quadword-atomics.c:42-44
+void test_add(int128_t *ptr, int128_t x) {
+  // expected-no-diagnostics
+  __c11_atomic_fetch_add((_Atomic(int128_t) *)ptr, x, __ATOMIC_RELAXED);

The pointer cast is avoidable; let's avoid it.



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:18031
+bool PPCTargetLowering::shouldInlineQuadwordAtomics() const {
+  // TODO: 16-byte atomic type support for AIX is on progress, we should be 
able
+  // inline 16-byte atomic ops on AIX too in the future.

Typo fix; grammar fix.



Comment at: llvm/test/CodeGen/PowerPC/atomics-i128.ll:17
+; RUN:   --check-prefix=AIX64-PWR8 %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-unknown -mcpu=pwr8 \
+; RUN:   -ppc-quadword-atomics -ppc-asm-full-reg-names 
-ppc-track-subreg-liveness < %s \

I think this case deserves an extra comment to explain the 
`-ppc-quadword-atomics`.



Comment at: llvm/test/CodeGen/PowerPC/atomics-i128.ll:77
+; AIX64-PWR8-NEXT:sync
+; AIX64-PWR8-NEXT:bl .__sync_lock_test_and_set_16[PR]
+; AIX64-PWR8-NEXT:nop

What library is this expected to provide this symbol?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D122983#3429060 , @aaron.ballman 
wrote:

> Morally, yes, that's reasonable in CodeGen because you're ensuring you get no 
> diagnostics. Practically, that's a convoluted, more expensive, less 
> maintainable way to spell `-Werror` for the test. When diagnostics are 
> introduced, this pattern encourages people to remove the `// 
> expected-no-diagnostics` comment and start adding `// expected-warning {{}}` 
> comments. Running the diagnostic verifier also slows down test execution 
> because of the extra verification step (which adds up over thousands of 
> tests).

Thanks for the tip. I never thought to use `-Werror` that way.


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

https://reviews.llvm.org/D122983

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

[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

I am not sure that the choice of `isOSBinFormatELF` to (afaik) primarily scope 
this change from affecting AIX (where we know the library calls are not 
implemented to be lock-free yet) is better than alternative where the condition 
is for little-endian mode or specifically for not AIX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.h:443
 
-// PPC64 supports atomics up to 8 bytes.
-MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+// PPC64 supports atomics up to 16 bytes.
+MaxAtomicPromoteWidth = 128;

Clarify that the statement is not for PPC64 in general.



Comment at: clang/lib/Basic/Targets/PPC.h:445
+MaxAtomicPromoteWidth = 128;
+// PPC64 supports inlining atomics up to 8 bytes.
+MaxAtomicInlineWidth = 64;

Clarify that the support up to 8 bytes is for "baseline" PPC64 (i.e., 
non-baseline implementations may support lock-free inline 16-byte atomic 
operations).



Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:18033
+ (EnableQuadwordAtomics ||
+  Subtarget.getTargetTriple().isOSBinFormatELF()) &&
+ Subtarget.hasQuadwordAtomics();

Can we have a comment here to explain the `isOSBinFormatELF` check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.h:448
+  void setMaxAtomicWidth() override {
+// For layout on ELF targets, we support up to 16 bytes.
+if (getTriple().isOSBinFormatELF())

hubert.reinterpretcast wrote:
> lkail wrote:
> > hubert.reinterpretcast wrote:
> > > I believe this should be presented more as this override being 
> > > implemented currently only for ELF targets. The current presentation 
> > > seems to imply more design intent for non-ELF targets than there is 
> > > consensus for.
> > > 
> > > For example:
> > > ```
> > > if (!getTriple().isOSBinFormatELF())
> > >   return PPCTargetInfo::setMaxAtomicWidth();
> > > ```
> > It looks a chance to adjust according to arch level to me(Considering we 
> > finally will make xcoff and elf targets consistent here). If you have 
> > strong opinion on this, I'll make a change here.
> This function currently modifies two different things. One that should be 
> arch-level-sensitive (and, iiuc, is already so) and one that should not be 
> arch-level-sensitive.
> 
> So, the `isOSBinFormatELF` checks are here mainly to scope the patch 
> (temporarily).
> 
> I only suggested to use the call to the base class as a shorthand for 
> "nothing special that this function is doing". The associated comment should 
> read something like "Restrict 16-byte atomic changes to ELF targets for now. 
> TODO: Consider other targets as well."
> 
> At the very least (for this patch), I do not believe the `isOSBinFormatELF` 
> should be checked twice in terms of control flow.
The new version of the change already addresses this; thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.h:448
+  void setMaxAtomicWidth() override {
+// For layout on ELF targets, we support up to 16 bytes.
+if (getTriple().isOSBinFormatELF())

lkail wrote:
> hubert.reinterpretcast wrote:
> > I believe this should be presented more as this override being implemented 
> > currently only for ELF targets. The current presentation seems to imply 
> > more design intent for non-ELF targets than there is consensus for.
> > 
> > For example:
> > ```
> > if (!getTriple().isOSBinFormatELF())
> >   return PPCTargetInfo::setMaxAtomicWidth();
> > ```
> It looks a chance to adjust according to arch level to me(Considering we 
> finally will make xcoff and elf targets consistent here). If you have strong 
> opinion on this, I'll make a change here.
This function currently modifies two different things. One that should be 
arch-level-sensitive (and, iiuc, is already so) and one that should not be 
arch-level-sensitive.

So, the `isOSBinFormatELF` checks are here mainly to scope the patch 
(temporarily).

I only suggested to use the call to the base class as a shorthand for "nothing 
special that this function is doing". The associated comment should read 
something like "Restrict 16-byte atomic changes to ELF targets for now. TODO: 
Consider other targets as well."

At the very least (for this patch), I do not believe the `isOSBinFormatELF` 
should be checked twice in terms of control flow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D122983#3426716 , @erichkeane 
wrote:

> We typically avoid doing -verify in CodeGen (unless the diagnostic is 
> ACTUALLY in CodeGen) as a matter of business.

I hope that `-verify` and `// expected-no-diagnostics` in CodeGen tests is 
compatible with the above. I believe it is valuable to confirm that the test 
itself is not written problematically.


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

https://reviews.llvm.org/D122983

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D122983#3426541 , @paulwalker-arm 
wrote:

> The tests verify a set of builtins do not exist when the associated feature 
> flag is not present.  They sit within CodeGen because the tests were 
> plentiful and it did not seem worth duplicating them.

This sounds like a situation where the test files should have more comments 
near the RUN lines (if the situation is not resolved via a different solution).


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

https://reviews.llvm.org/D122983

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

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D122983#3426450 , @aaron.ballman 
wrote:

> Yeah, those tests seem to be overly-constraining. There's no reason to be 
> validating whether there's an implicit function declaration warning in a 
> *codegen* test. I will change all of those AAarch64 tests to require -std=c99 
> explicitly whenever possible, remove the `-verify` flag because there's no 
> reason for a codegen test to verify diagnostic behavior that isn't generated 
> by the CodeGen library, and remove the `// expected-warning` comments. I plan 
> to do that as an NFC change that I'll land outside of this patch, unless any 
> of the AArch64 folks speak up pretty quickly.

Maybe a smaller hammer can be used here, e.g., 
`-Wno-error=implicit-function-declaration`? The use of the `-verify` flag in a 
CodeGen test to validate that the test is written as "cleanly" as intended is 
something that I am sympathetic to.


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

https://reviews.llvm.org/D122983

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

[PATCH] D122377: [PowerPC][Linux] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Basic/Targets/PPC.h:448
+  void setMaxAtomicWidth() override {
+// For layout on ELF targets, we support up to 16 bytes.
+if (getTriple().isOSBinFormatELF())

I believe this should be presented more as this override being implemented 
currently only for ELF targets. The current presentation seems to imply more 
design intent for non-ELF targets than there is consensus for.

For example:
```
if (!getTriple().isOSBinFormatELF())
  return PPCTargetInfo::setMaxAtomicWidth();
```



Comment at: clang/test/Sema/atomic-ops.c:13
+// RUN:   -target-cpu pwr8 -DPPC64_PWR8
 
 // Basic parsing/Sema tests for __c11_atomic_*

This will need a separate patch to cover ppc32 (likely with AIX).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122377: [PowerPC][Linux] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/CodeGen/PowerPC/atomic-alignment.c:34
 
 // PPC32: @o = global %struct.O zeroinitializer, align 1{{$}}
 // PPC64: @o = global %struct.O zeroinitializer, align 8{{$}}

Just noting that GCC increases the alignment even for ppc32:
```
typedef struct A8 { char x[8]; } A8;
typedef struct A16 { char x[16]; } A16;
extern char q8[_Alignof(_Atomic(A8))], q8[8]; // okay for GCC targeting ppc32
extern char q16[_Alignof(_Atomic(A16))], q16[16]; // okay for GCC targeting 
ppc32
```

Apparently, the change for i686 in GCC occurred with version 11.
https://godbolt.org/z/fTTGoqWW1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D121992: [Clang] [Driver] Add option to set alternative toolchain path

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D121992#3418443 , @MaskRay wrote:

> If you intend to overlay ld.so, you'll necessarily overlay libc, then 
> --sysroot seems just unneeded at all.

Why? The header and library search paths are not restricted to artifacts from 
libc. I had consulted some Advance Toolchain developers and they indicted that 
it was important for paths from the `--sysroot` to be incorporated (as GCC from 
the Advance Toolchain does).

The library search order has the issue of needing the multiarch/"OS lib dir" 
paths from both the overlay toolchain and the sysroot before attempting 
fallback plain-"lib" paths. Aside from a new option, is there a way to achieve 
that?

I agree, however, that the new option doesn't have to incorporate effects on 
what the `--dyld-prefix` default is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121992

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

[PATCH] D122377: [PowerPC][Linux] Support 16-byte lock free atomics on pwr8 and up

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/CodeGen/PowerPC/atomic-alignment.c:45
+// PPC64: @q = global %struct.Q zeroinitializer, align 16{{$}}
+// AIX64: @q = global %struct.Q zeroinitializer, align 1{{$}}
 _Atomic(Q) q; // expected-no-diagnostics

Although it is a change that affects layout compatibility with the short 
history of 16-byte atomic types compiled with LLVM-based compilers on AIX, 
having this 16-byte atomic type aligned also on AIX for PPC64 would be 
reasonable. The consequences of not doing so is that operations on some types 
would be lock-free only on ELF-based platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377

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

[PATCH] D122249: [Clang] Add a compatibiliy warning for non-literals in constexpr.

[Hubert Tong via Phabricator via cfe-commits]

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

This LGTM (with minor comment). Please wait for Aaron to respond re: the 
handling of template instantiations.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1912-1913
+   diag::err_constexpr_local_var_non_literal_type,
+   isa(Dcl)))
   return false;
 if (!VD->getType()->isDependentType() &&

Minor nit: The coding standards were updated (some time ago now) to recommend 
keeping if-else chains consistently braced or not-braced.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1905
+if (SemaRef.LangOpts.CPlusPlus2b) {
+  if (!VD->getType()->isLiteralType(SemaRef.Context))
+SemaRef.Diag(VD->getLocation(),

cor3ntin wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > This seems to trigger even when the type is dependent:
> > > > > ```
> > > > > :1:36: warning: definition of a variable of non-literal type 
> > > > > in a constexpr function is incompatible with C++ standards before 
> > > > > C++2b [-Wpre-c++2b-compat]
> > > > > auto qq = [](auto x) { decltype(x) n; };
> > > > >^
> > > > > 1 warning generated.
> > > > > ```
> > > > > 
> > > > > This also seems to emit even when `Kind` is not 
> > > > > `Sema::CheckConstexprKind::Diagnose` (unlike the 
> > > > > `static`/`thread_local` case above). Is the `CheckLiteralType` logic 
> > > > > not reusable for this?
> > > > You are right, thanks for noticing that, it was rather bogus.
> > > > The reason I'm not using CheckLiteralType is to avoid duplicating a 
> > > > diagnostics message, as CheckLiteralType doesn't allow us to pass 
> > > > parameter to the diagnostic message.
> > > > 
> > > > It leaves us with an uncovered scenario though: We do not emit the 
> > > > warning on template instantiation, and I don't think there is an  easy 
> > > > way to do that.
> > > > The reason I'm not using CheckLiteralType is to avoid duplicating a 
> > > > diagnostics message, as CheckLiteralType doesn't allow us to pass 
> > > > parameter to the diagnostic message.
> > > 
> > > Huh?
> > > 
> > > ```
> > > static bool CheckLiteralType(Sema , Sema::CheckConstexprKind Kind,
> > >  SourceLocation Loc, QualType T, unsigned 
> > > DiagID,
> > >  Ts &&...DiagArgs) {
> > >   ...
> > > }
> > > ```
> > > I would hope `DiagArgs` should do exactly that? :-)
> > > It leaves us with an uncovered scenario though: We do not emit the 
> > > warning on template instantiation, and I don't think there is an easy way 
> > > to do that.
> > 
> > I believe the code paths that lead us here all come from 
> > `Sema::CheckConstexprFunctionDefinition()` which is called from 
> > `Sema::ActOnFinishFunctionBody()` which seems to be called when 
> > instantiating templates in `Sema::InstantiateFunctionDefinition()`, so 
> > perhaps some more investigation is needed as to why we're not reaching this 
> > for template instantiations.
> We could add something in addition of `Sema::CheckConstexprKind::CheckValid` 
> and `Sema::CheckConstexprKind::Diagnose`, but 
> 
> * not for implicit lambdas, because we should not warn on lambdas that won't 
> be constexpr
> * for explicit constexpr lambdas / functions, it would force us to call 
> CheckConstexprFunctionDefinition  on instanciation, which we currently don't 
> do, and is not free for that one warning - and we would have to not-reemit 
> the other warnings. It seems like quite a fair amount of work for a 
> diagnostic not enabled by default.
> so perhaps some more investigation is needed as to why we're not reaching 
> this for template instantiations.

@aaron.ballman, do you have any position on whether we want this investigation 
before moving forward with this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122249

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

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGce21c926f8ef: [Clang] Work with multiple pragmas weak before 
definition (authored by hubert.reinterpretcast).

Changed prior to commit:
  https://reviews.llvm.org/D121927?vs=417755=418092#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Weak.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/pragma-weak.c
  clang/test/PCH/pragma-weak-functional.c
  clang/test/PCH/pragma-weak-functional.h

Index: clang/test/PCH/pragma-weak-functional.h
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.h
@@ -0,0 +1,6 @@
+// Header for PCH test pragma-weak-functional.c
+
+#pragma weak undecfunc_alias2 = undecfunc
+#pragma weak undecfunc_alias4 = undecfunc_alias1
+#pragma weak undecfunc_alias3 = undecfunc_alias1
+#pragma weak undecfunc_alias1 = undecfunc
Index: clang/test/PCH/pragma-weak-functional.c
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.c
@@ -0,0 +1,17 @@
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/pragma-weak-functional.h %s -verify -emit-llvm -o - | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c-header -emit-pch -o %t %S/pragma-weak-functional.h
+// RUN: %clang_cc1 -include-pch %t %s -verify -emit-llvm -o - | FileCheck %s
+
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
+
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+void undecfunc_alias1(void);
+void undecfunc(void) { }
+// expected-warning@pragma-weak-functional.h:4 {{alias will always resolve to undecfunc}}
+// expected-warning@pragma-weak-functional.h:5 {{alias will always resolve to undecfunc}}
Index: clang/test/CodeGen/pragma-weak.c
===
--- clang/test/CodeGen/pragma-weak.c
+++ clang/test/CodeGen/pragma-weak.c
@@ -17,6 +17,10 @@
 // CHECK-DAG: @mix2 = weak{{.*}} alias void (), void ()* @__mix2
 // CHECK-DAG: @a1 = weak{{.*}} alias void (), void ()* @__a1
 // CHECK-DAG: @xxx = weak{{.*}} alias void (), void ()* @__xxx
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
 
 
 
@@ -136,6 +140,15 @@
 __attribute((pure,noinline,const)) void __xxx(void) { }
 // CHECK: void @__xxx() [[RN:#[0-9]+]]
 
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+#pragma weak undecfunc_alias1 = undecfunc
+#pragma weak undecfunc_alias1 = undecfunc // Try specifying same alias/target pair a second time.
+#pragma weak undecfunc_alias3 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias4 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias2 = undecfunc
+void undecfunc_alias2(void);
+void undecfunc(void) { }
+
 / PR10878: Make sure we can call a weak alias
 void SHA512Pad(void *context) {}
 #pragma weak SHA384Pad = SHA512Pad
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4559,13 +4559,14 @@
   // entire table, since later PCH files in a PCH chain are only interested in
   // the results at the end of the chain.
   RecordData WeakUndeclaredIdentifiers;
-  for (auto  : SemaRef.WeakUndeclaredIdentifiers) {
-IdentifierInfo *II = WeakUndeclaredIdentifier.first;
-WeakInfo  = WeakUndeclaredIdentifier.second;
-AddIdentifierRef(II, WeakUndeclaredIdentifiers);
-AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
-AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
-WeakUndeclaredIdentifiers.push_back(WI.getUsed());
+  for (const auto  :
+   SemaRef.WeakUndeclaredIdentifiers) {
+const IdentifierInfo *const II = WeakUndeclaredIdentifierList.first;
+for (const auto  : WeakUndeclaredIdentifierList.second) {
+  AddIdentifierRef(II, WeakUndeclaredIdentifiers);
+  AddIdentifierRef(WI.getAlias(), 

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast updated this revision to Diff 417755.
hubert.reinterpretcast added a comment.

- Adjust per observation: Use DenseMapInfo for the alias pointer value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Weak.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/pragma-weak.c
  clang/test/PCH/pragma-weak-functional.c
  clang/test/PCH/pragma-weak-functional.h

Index: clang/test/PCH/pragma-weak-functional.h
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.h
@@ -0,0 +1,6 @@
+// Header for PCH test pragma-weak-functional.c
+
+#pragma weak undecfunc_alias2 = undecfunc
+#pragma weak undecfunc_alias4 = undecfunc_alias1
+#pragma weak undecfunc_alias3 = undecfunc_alias1
+#pragma weak undecfunc_alias1 = undecfunc
Index: clang/test/PCH/pragma-weak-functional.c
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.c
@@ -0,0 +1,17 @@
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/pragma-weak-functional.h %s -verify -emit-llvm -o - | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c-header -emit-pch -o %t %S/pragma-weak-functional.h
+// RUN: %clang_cc1 -include-pch %t %s -verify -emit-llvm -o - | FileCheck %s
+
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
+
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+void undecfunc_alias1(void);
+void undecfunc(void) { }
+// expected-warning@pragma-weak-functional.h:4 {{alias will always resolve to undecfunc}}
+// expected-warning@pragma-weak-functional.h:5 {{alias will always resolve to undecfunc}}
Index: clang/test/CodeGen/pragma-weak.c
===
--- clang/test/CodeGen/pragma-weak.c
+++ clang/test/CodeGen/pragma-weak.c
@@ -17,6 +17,10 @@
 // CHECK-DAG: @mix2 = weak{{.*}} alias void (), void ()* @__mix2
 // CHECK-DAG: @a1 = weak{{.*}} alias void (), void ()* @__a1
 // CHECK-DAG: @xxx = weak{{.*}} alias void (), void ()* @__xxx
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
 
 
 
@@ -136,6 +140,15 @@
 __attribute((pure,noinline,const)) void __xxx(void) { }
 // CHECK: void @__xxx() [[RN:#[0-9]+]]
 
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+#pragma weak undecfunc_alias1 = undecfunc
+#pragma weak undecfunc_alias1 = undecfunc // Try specifying same alias/target pair a second time.
+#pragma weak undecfunc_alias3 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias4 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias2 = undecfunc
+void undecfunc_alias2(void);
+void undecfunc(void) { }
+
 / PR10878: Make sure we can call a weak alias
 void SHA512Pad(void *context) {}
 #pragma weak SHA384Pad = SHA512Pad
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4559,13 +4559,14 @@
   // entire table, since later PCH files in a PCH chain are only interested in
   // the results at the end of the chain.
   RecordData WeakUndeclaredIdentifiers;
-  for (auto  : SemaRef.WeakUndeclaredIdentifiers) {
-IdentifierInfo *II = WeakUndeclaredIdentifier.first;
-WeakInfo  = WeakUndeclaredIdentifier.second;
-AddIdentifierRef(II, WeakUndeclaredIdentifiers);
-AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
-AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
-WeakUndeclaredIdentifiers.push_back(WI.getUsed());
+  for (const auto  :
+   SemaRef.WeakUndeclaredIdentifiers) {
+const IdentifierInfo *const II = WeakUndeclaredIdentifierList.first;
+for (const auto  : WeakUndeclaredIdentifierList.second) {
+  AddIdentifierRef(II, WeakUndeclaredIdentifiers);
+  AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
+  AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
+}
   }
 
   

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Sema/Weak.h:47
+static unsigned getHashValue(const WeakInfo ) {
+  return W.getAlias() ? DenseMapInfo::getHashValue(W.getAlias()->getName())
+  : DenseMapInfo::getHashValue("");

The fact that `WeakUndeclaredIdentifiers` is keyed by `IdentifierInfo *` must 
mean that it is safe to use pointer comparison. This is further bolstered by 
`IdentifierInfo` having only private or deleted constructors (with 
`IdentifierTable` being a friend class).

I will adjust to make this class simply forward to `DenseMapInfo` with the alias members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

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

[PATCH] D121992: [Clang] [Driver] Add option to set alternative toolchain path

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

LGTM; thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121992

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

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

@aaron.ballman, I believe I have responded to all of the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

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

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast updated this revision to Diff 417666.
hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added a comment.

- Address review comments: Return `const` from getAlias()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Weak.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/pragma-weak.c
  clang/test/PCH/pragma-weak-functional.c
  clang/test/PCH/pragma-weak-functional.h

Index: clang/test/PCH/pragma-weak-functional.h
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.h
@@ -0,0 +1,6 @@
+// Header for PCH test pragma-weak-functional.c
+
+#pragma weak undecfunc_alias2 = undecfunc
+#pragma weak undecfunc_alias4 = undecfunc_alias1
+#pragma weak undecfunc_alias3 = undecfunc_alias1
+#pragma weak undecfunc_alias1 = undecfunc
Index: clang/test/PCH/pragma-weak-functional.c
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.c
@@ -0,0 +1,17 @@
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/pragma-weak-functional.h %s -verify -emit-llvm -o - | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c-header -emit-pch -o %t %S/pragma-weak-functional.h
+// RUN: %clang_cc1 -include-pch %t %s -verify -emit-llvm -o - | FileCheck %s
+
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
+
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+void undecfunc_alias1(void);
+void undecfunc(void) { }
+// expected-warning@pragma-weak-functional.h:4 {{alias will always resolve to undecfunc}}
+// expected-warning@pragma-weak-functional.h:5 {{alias will always resolve to undecfunc}}
Index: clang/test/CodeGen/pragma-weak.c
===
--- clang/test/CodeGen/pragma-weak.c
+++ clang/test/CodeGen/pragma-weak.c
@@ -17,6 +17,10 @@
 // CHECK-DAG: @mix2 = weak{{.*}} alias void (), void ()* @__mix2
 // CHECK-DAG: @a1 = weak{{.*}} alias void (), void ()* @__a1
 // CHECK-DAG: @xxx = weak{{.*}} alias void (), void ()* @__xxx
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
 
 
 
@@ -136,6 +140,15 @@
 __attribute((pure,noinline,const)) void __xxx(void) { }
 // CHECK: void @__xxx() [[RN:#[0-9]+]]
 
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+#pragma weak undecfunc_alias1 = undecfunc
+#pragma weak undecfunc_alias1 = undecfunc // Try specifying same alias/target pair a second time.
+#pragma weak undecfunc_alias3 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias4 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias2 = undecfunc
+void undecfunc_alias2(void);
+void undecfunc(void) { }
+
 / PR10878: Make sure we can call a weak alias
 void SHA512Pad(void *context) {}
 #pragma weak SHA384Pad = SHA512Pad
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4559,13 +4559,14 @@
   // entire table, since later PCH files in a PCH chain are only interested in
   // the results at the end of the chain.
   RecordData WeakUndeclaredIdentifiers;
-  for (auto  : SemaRef.WeakUndeclaredIdentifiers) {
-IdentifierInfo *II = WeakUndeclaredIdentifier.first;
-WeakInfo  = WeakUndeclaredIdentifier.second;
-AddIdentifierRef(II, WeakUndeclaredIdentifiers);
-AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
-AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
-WeakUndeclaredIdentifiers.push_back(WI.getUsed());
+  for (const auto  :
+   SemaRef.WeakUndeclaredIdentifiers) {
+const IdentifierInfo *const II = WeakUndeclaredIdentifierList.first;
+for (const auto  : WeakUndeclaredIdentifierList.second) {
+  AddIdentifierRef(II, WeakUndeclaredIdentifiers);
+  AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
+  AddSourceLocation(WI.getLocation(), 

[PATCH] D122249: [Clang] Add a compatibiliy warning for non-literals in constexpr.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1905-1907
+  CheckLiteralType(SemaRef, Kind, VD->getLocation(), VD->getType(),
+   diag::warn_cxx20_compat_constexpr_var,
+   isa(Dcl), 2);

Apply Aaron's suggestion 
(https://reviews.llvm.org/D122249?id=417358#inline-1168823) to the updated code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122249

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

[PATCH] D122261: [Clang][NFC] Some `const` for `IdentifierInfo *`s feeding `DeclarationName`

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGda167a53c87f: [Clang][NFC] Some `const` for `IdentifierInfo 
*`s feeding `DeclarationName` (authored by hubert.reinterpretcast).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122261

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -9016,8 +9016,8 @@
 
 /// DeclClonePragmaWeak - clone existing decl (maybe definition),
 /// \#pragma weak needs a non-definition decl and source may not have one.
-NamedDecl * Sema::DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
-  SourceLocation Loc) {
+NamedDecl *Sema::DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
+ SourceLocation Loc) {
   assert(isa(ND) || isa(ND));
   NamedDecl *NewD = nullptr;
   if (auto *FD = dyn_cast(ND)) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2020,7 +2020,7 @@
 
 VarDecl::VarDecl(Kind DK, ASTContext , DeclContext *DC,
  SourceLocation StartLoc, SourceLocation IdLoc,
- IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
+ const IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
  StorageClass SC)
 : DeclaratorDecl(DK, DC, IdLoc, Id, T, TInfo, StartLoc),
   redeclarable_base(C) {
@@ -2035,10 +2035,9 @@
   // Everything else is implicitly initialized to false.
 }
 
-VarDecl *VarDecl::Create(ASTContext , DeclContext *DC,
- SourceLocation StartL, SourceLocation IdL,
- IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
- StorageClass S) {
+VarDecl *VarDecl::Create(ASTContext , DeclContext *DC, SourceLocation StartL,
+ SourceLocation IdL, const IdentifierInfo *Id,
+ QualType T, TypeSourceInfo *TInfo, StorageClass S) {
   return new (C, DC) VarDecl(Var, C, DC, StartL, IdL, Id, T, TInfo, S);
 }
 
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -10175,7 +10175,7 @@
   void ActOnPragmaVisibility(const IdentifierInfo* VisType,
  SourceLocation PragmaLoc);
 
-  NamedDecl *DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
+  NamedDecl *DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
  SourceLocation Loc);
   void DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, WeakInfo );
 
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -1041,7 +1041,7 @@
   };
 
   VarDecl(Kind DK, ASTContext , DeclContext *DC, SourceLocation StartLoc,
-  SourceLocation IdLoc, IdentifierInfo *Id, QualType T,
+  SourceLocation IdLoc, const IdentifierInfo *Id, QualType T,
   TypeSourceInfo *TInfo, StorageClass SC);
 
   using redeclarable_base = Redeclarable;
@@ -1071,8 +1071,8 @@
 
   static VarDecl *Create(ASTContext , DeclContext *DC,
  SourceLocation StartLoc, SourceLocation IdLoc,
- IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
- StorageClass S);
+ const IdentifierInfo *Id, QualType T,
+ TypeSourceInfo *TInfo, StorageClass S);
 
   static VarDecl *CreateDeserialized(ASTContext , unsigned ID);
 


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -9016,8 +9016,8 @@
 
 /// DeclClonePragmaWeak - clone existing decl (maybe definition),
 /// \#pragma weak needs a non-definition decl and source may not have one.
-NamedDecl * Sema::DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
-  SourceLocation Loc) {
+NamedDecl *Sema::DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
+ SourceLocation Loc) {
   assert(isa(ND) || isa(ND));
   NamedDecl *NewD = nullptr;
   if (auto *FD = dyn_cast(ND)) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2020,7 +2020,7 @@
 
 VarDecl::VarDecl(Kind DK, ASTContext , DeclContext *DC,
  SourceLocation 

[PATCH] D122249: [Clang] Add a compatibiliy warning for non-literals in constexpr.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:219
+  NonLiteral n; // cxx2b-note {{non-literal type 'NonLiteral' cannot be 
used in a constant expression}} \
+// cxx2b-warning {{definition of a variable of non-literal 
type in a constexpr function is incompatible with C++ standards before C++2b}}
 return 0;

Not sure how much we want the message in this case. The lambda is not marked 
`constexpr` (although it is implicitly `constexpr` in C++2b).

Note that we don't get a message for this:
```
auto qq = [] { return 0; static int x = 42; };
constexpr int qx = qq();
```

I am not sure how difficult it would be to come at this from the 
used-in-constant-evaluation side, but there is probably a larger class of 
messages in the same situation (so it would probably be a separate endeavour).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122249

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

[PATCH] D122249: [Clang] Add a compatibiliy warning for non-literals in constexpr.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1905
+if (SemaRef.LangOpts.CPlusPlus2b) {
+  if (!VD->getType()->isLiteralType(SemaRef.Context))
+SemaRef.Diag(VD->getLocation(),

This seems to trigger even when the type is dependent:
```
:1:36: warning: definition of a variable of non-literal type in a 
constexpr function is incompatible with C++ standards before C++2b 
[-Wpre-c++2b-compat]
auto qq = [](auto x) { decltype(x) n; };
   ^
1 warning generated.
```

This also seems to emit even when `Kind` is not 
`Sema::CheckConstexprKind::Diagnose` (unlike the `static`/`thread_local` case 
above). Is the `CheckLiteralType` logic not reusable for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122249

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

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast updated this revision to Diff 417443.
hubert.reinterpretcast added a comment.

- Address review comments: Add release notes, expand comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Weak.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/pragma-weak.c
  clang/test/PCH/pragma-weak-functional.c
  clang/test/PCH/pragma-weak-functional.h

Index: clang/test/PCH/pragma-weak-functional.h
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.h
@@ -0,0 +1,6 @@
+// Header for PCH test pragma-weak-functional.c
+
+#pragma weak undecfunc_alias2 = undecfunc
+#pragma weak undecfunc_alias4 = undecfunc_alias1
+#pragma weak undecfunc_alias3 = undecfunc_alias1
+#pragma weak undecfunc_alias1 = undecfunc
Index: clang/test/PCH/pragma-weak-functional.c
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.c
@@ -0,0 +1,17 @@
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/pragma-weak-functional.h %s -verify -emit-llvm -o - | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c-header -emit-pch -o %t %S/pragma-weak-functional.h
+// RUN: %clang_cc1 -include-pch %t %s -verify -emit-llvm -o - | FileCheck %s
+
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
+
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+void undecfunc_alias1(void);
+void undecfunc(void) { }
+// expected-warning@pragma-weak-functional.h:4 {{alias will always resolve to undecfunc}}
+// expected-warning@pragma-weak-functional.h:5 {{alias will always resolve to undecfunc}}
Index: clang/test/CodeGen/pragma-weak.c
===
--- clang/test/CodeGen/pragma-weak.c
+++ clang/test/CodeGen/pragma-weak.c
@@ -17,6 +17,10 @@
 // CHECK-DAG: @mix2 = weak{{.*}} alias void (), void ()* @__mix2
 // CHECK-DAG: @a1 = weak{{.*}} alias void (), void ()* @__a1
 // CHECK-DAG: @xxx = weak{{.*}} alias void (), void ()* @__xxx
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
 
 
 
@@ -136,6 +140,15 @@
 __attribute((pure,noinline,const)) void __xxx(void) { }
 // CHECK: void @__xxx() [[RN:#[0-9]+]]
 
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+#pragma weak undecfunc_alias1 = undecfunc
+#pragma weak undecfunc_alias1 = undecfunc // Try specifying same alias/target pair a second time.
+#pragma weak undecfunc_alias3 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias4 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias2 = undecfunc
+void undecfunc_alias2(void);
+void undecfunc(void) { }
+
 / PR10878: Make sure we can call a weak alias
 void SHA512Pad(void *context) {}
 #pragma weak SHA384Pad = SHA512Pad
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4559,13 +4559,14 @@
   // entire table, since later PCH files in a PCH chain are only interested in
   // the results at the end of the chain.
   RecordData WeakUndeclaredIdentifiers;
-  for (auto  : SemaRef.WeakUndeclaredIdentifiers) {
-IdentifierInfo *II = WeakUndeclaredIdentifier.first;
-WeakInfo  = WeakUndeclaredIdentifier.second;
-AddIdentifierRef(II, WeakUndeclaredIdentifiers);
-AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
-AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
-WeakUndeclaredIdentifiers.push_back(WI.getUsed());
+  for (const auto  :
+   SemaRef.WeakUndeclaredIdentifiers) {
+const IdentifierInfo *const II = WeakUndeclaredIdentifierList.first;
+for (const auto  : WeakUndeclaredIdentifierList.second) {
+  AddIdentifierRef(II, WeakUndeclaredIdentifiers);
+  AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
+  AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
+}
   }
 
   // Build 

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Sema/Weak.h:33
+  : alias(Alias), loc(Loc) {}
+  inline IdentifierInfo *getAlias() const { return alias; }
   inline SourceLocation getLocation() const { return loc; }

hubert.reinterpretcast wrote:
> aaron.ballman wrote:
> > Would it be onerous to make the return type be `const IdentifierInfo *` 
> > given that the function is `const`? (If it is, just ignore the suggestion 
> > -- I love adding const correctness where we can get it basically for free.)
> It's not free but I can post an NFC patch to add the `const` through. On the 
> chain of calls that `const` needs to be added to, the pointer eventually 
> feeds a `DeclarationName` (which already takes `const IdentifierInfo *`).
https://reviews.llvm.org/D122261 has the NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

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

[PATCH] D122261: [Clang][NFC] Some `const` for `IdentifierInfo *`s feeding `DeclarationName`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast created this revision.
hubert.reinterpretcast added a reviewer: aaron.ballman.
Herald added a project: All.
hubert.reinterpretcast requested review of this revision.
Herald added a project: clang.

`DeclarationName` already takes `const IdentifierInfo *`. Propagate the `const` 
outward to various APIs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122261

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -9016,8 +9016,8 @@
 
 /// DeclClonePragmaWeak - clone existing decl (maybe definition),
 /// \#pragma weak needs a non-definition decl and source may not have one.
-NamedDecl * Sema::DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
-  SourceLocation Loc) {
+NamedDecl *Sema::DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
+ SourceLocation Loc) {
   assert(isa(ND) || isa(ND));
   NamedDecl *NewD = nullptr;
   if (auto *FD = dyn_cast(ND)) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2020,7 +2020,7 @@
 
 VarDecl::VarDecl(Kind DK, ASTContext , DeclContext *DC,
  SourceLocation StartLoc, SourceLocation IdLoc,
- IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
+ const IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
  StorageClass SC)
 : DeclaratorDecl(DK, DC, IdLoc, Id, T, TInfo, StartLoc),
   redeclarable_base(C) {
@@ -2035,10 +2035,9 @@
   // Everything else is implicitly initialized to false.
 }
 
-VarDecl *VarDecl::Create(ASTContext , DeclContext *DC,
- SourceLocation StartL, SourceLocation IdL,
- IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
- StorageClass S) {
+VarDecl *VarDecl::Create(ASTContext , DeclContext *DC, SourceLocation StartL,
+ SourceLocation IdL, const IdentifierInfo *Id,
+ QualType T, TypeSourceInfo *TInfo, StorageClass S) {
   return new (C, DC) VarDecl(Var, C, DC, StartL, IdL, Id, T, TInfo, S);
 }
 
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -10175,7 +10175,7 @@
   void ActOnPragmaVisibility(const IdentifierInfo* VisType,
  SourceLocation PragmaLoc);
 
-  NamedDecl *DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
+  NamedDecl *DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
  SourceLocation Loc);
   void DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, WeakInfo );
 
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -1041,7 +1041,7 @@
   };
 
   VarDecl(Kind DK, ASTContext , DeclContext *DC, SourceLocation StartLoc,
-  SourceLocation IdLoc, IdentifierInfo *Id, QualType T,
+  SourceLocation IdLoc, const IdentifierInfo *Id, QualType T,
   TypeSourceInfo *TInfo, StorageClass SC);
 
   using redeclarable_base = Redeclarable;
@@ -1071,8 +1071,8 @@
 
   static VarDecl *Create(ASTContext , DeclContext *DC,
  SourceLocation StartLoc, SourceLocation IdLoc,
- IdentifierInfo *Id, QualType T, TypeSourceInfo *TInfo,
- StorageClass S);
+ const IdentifierInfo *Id, QualType T,
+ TypeSourceInfo *TInfo, StorageClass S);
 
   static VarDecl *CreateDeserialized(ASTContext , unsigned ID);
 


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -9016,8 +9016,8 @@
 
 /// DeclClonePragmaWeak - clone existing decl (maybe definition),
 /// \#pragma weak needs a non-definition decl and source may not have one.
-NamedDecl * Sema::DeclClonePragmaWeak(NamedDecl *ND, IdentifierInfo *II,
-  SourceLocation Loc) {
+NamedDecl *Sema::DeclClonePragmaWeak(NamedDecl *ND, const IdentifierInfo *II,
+ SourceLocation Loc) {
   assert(isa(ND) || isa(ND));
   NamedDecl *NewD = nullptr;
   if (auto *FD = dyn_cast(ND)) {
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2020,7 +2020,7 @@
 
 VarDecl::VarDecl(Kind DK, ASTContext , DeclContext 

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast updated this revision to Diff 417386.
hubert.reinterpretcast marked 2 inline comments as done.
hubert.reinterpretcast added a comment.

- Address review comments: Use default member init in WeakInfo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

Files:
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Weak.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/pragma-weak.c
  clang/test/PCH/pragma-weak-functional.c
  clang/test/PCH/pragma-weak-functional.h

Index: clang/test/PCH/pragma-weak-functional.h
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.h
@@ -0,0 +1,6 @@
+// Header for PCH test pragma-weak-functional.c
+
+#pragma weak undecfunc_alias2 = undecfunc
+#pragma weak undecfunc_alias4 = undecfunc_alias1
+#pragma weak undecfunc_alias3 = undecfunc_alias1
+#pragma weak undecfunc_alias1 = undecfunc
Index: clang/test/PCH/pragma-weak-functional.c
===
--- /dev/null
+++ clang/test/PCH/pragma-weak-functional.c
@@ -0,0 +1,17 @@
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/pragma-weak-functional.h %s -verify -emit-llvm -o - | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -x c-header -emit-pch -o %t %S/pragma-weak-functional.h
+// RUN: %clang_cc1 -include-pch %t %s -verify -emit-llvm -o - | FileCheck %s
+
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
+
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+void undecfunc_alias1(void);
+void undecfunc(void) { }
+// expected-warning@pragma-weak-functional.h:4 {{alias will always resolve to undecfunc}}
+// expected-warning@pragma-weak-functional.h:5 {{alias will always resolve to undecfunc}}
Index: clang/test/CodeGen/pragma-weak.c
===
--- clang/test/CodeGen/pragma-weak.c
+++ clang/test/CodeGen/pragma-weak.c
@@ -17,6 +17,10 @@
 // CHECK-DAG: @mix2 = weak{{.*}} alias void (), void ()* @__mix2
 // CHECK-DAG: @a1 = weak{{.*}} alias void (), void ()* @__a1
 // CHECK-DAG: @xxx = weak{{.*}} alias void (), void ()* @__xxx
+// CHECK-DAG: @undecfunc_alias1 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias2 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias3 = weak{{.*}} alias void (), void ()* @undecfunc
+// CHECK-DAG: @undecfunc_alias4 = weak{{.*}} alias void (), void ()* @undecfunc
 
 
 
@@ -136,6 +140,15 @@
 __attribute((pure,noinline,const)) void __xxx(void) { }
 // CHECK: void @__xxx() [[RN:#[0-9]+]]
 
+/ PR28611: Try multiple aliases of same undeclared symbol or alias
+#pragma weak undecfunc_alias1 = undecfunc
+#pragma weak undecfunc_alias1 = undecfunc // Try specifying same alias/target pair a second time.
+#pragma weak undecfunc_alias3 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias4 = undecfunc_alias2 // expected-warning {{alias will always resolve to undecfunc}}
+#pragma weak undecfunc_alias2 = undecfunc
+void undecfunc_alias2(void);
+void undecfunc(void) { }
+
 / PR10878: Make sure we can call a weak alias
 void SHA512Pad(void *context) {}
 #pragma weak SHA384Pad = SHA512Pad
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4559,13 +4559,14 @@
   // entire table, since later PCH files in a PCH chain are only interested in
   // the results at the end of the chain.
   RecordData WeakUndeclaredIdentifiers;
-  for (auto  : SemaRef.WeakUndeclaredIdentifiers) {
-IdentifierInfo *II = WeakUndeclaredIdentifier.first;
-WeakInfo  = WeakUndeclaredIdentifier.second;
-AddIdentifierRef(II, WeakUndeclaredIdentifiers);
-AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
-AddSourceLocation(WI.getLocation(), WeakUndeclaredIdentifiers);
-WeakUndeclaredIdentifiers.push_back(WI.getUsed());
+  for (const auto  :
+   SemaRef.WeakUndeclaredIdentifiers) {
+const IdentifierInfo *const II = WeakUndeclaredIdentifierList.first;
+for (const auto  : WeakUndeclaredIdentifierList.second) {
+  AddIdentifierRef(II, WeakUndeclaredIdentifiers);
+  AddIdentifierRef(WI.getAlias(), WeakUndeclaredIdentifiers);
+  AddSourceLocation(WI.getLocation(), 

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Sema/Weak.h:33
+  : alias(Alias), loc(Loc) {}
+  inline IdentifierInfo *getAlias() const { return alias; }
   inline SourceLocation getLocation() const { return loc; }

aaron.ballman wrote:
> Would it be onerous to make the return type be `const IdentifierInfo *` given 
> that the function is `const`? (If it is, just ignore the suggestion -- I love 
> adding const correctness where we can get it basically for free.)
It's not free but I can post an NFC patch to add the `const` through. On the 
chain of calls that `const` needs to be added to, the pointer eventually feeds 
a `DeclarationName` (which already takes `const IdentifierInfo *`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

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

[PATCH] D121927: [Clang] Work with multiple pragmas weak before definition

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Sema/Weak.h:62
+return false;
+  return LHS.getAlias()->getName() == RHS.getAlias()->getName();
+}

aaron.ballman wrote:
> Previously we cared about the source locations being the same. But... is 
> there a situation where the aliases are the same but the source locations are 
> different? I can't think of any, so perhaps that's worth an assert that the 
> locations match?
We didn't really case about the source locations being the same. The removed 
operators were just dead code (featuring such things as 
compare-by-pointer-value). I have verified that splitting the deletion of them 
to a separate patch does not cause any build or LIT `check-all` failures. I can 
commit that part as an NFC patch first if you prefer.

Also, the aliases //can// be the same with different source locations. The 
location comes from where the `#pragma weak` is (from the parser by way of 
`ActOnPragmaWeakAlias`). That duplicates are ignored (meaning we only diagnose 
one instance) is mentioned in the patch description.

It is already the status quo that having the same alias specified with 
different targets (at least those not yet declared) neither produces a 
diagnostic nor operates by "last one wins". Instead, Clang prefers the first 
declared target and GCC just emits both and leaves it to the assembler to 
figure out (https://godbolt.org/z/EasK375j3).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121927

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

[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Confirming LGTM with minor comments.

In D111400#3399929 , @cor3ntin wrote:

> - We may still miss a compatibility warning for non-literal in c++23 mode?

My understanding is that Aaron and I both want the warning added (but are okay 
with having this patch landed).




Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:23
+  static _Thread_local int m = 0; // expected-note {{control flows through 
the definition of a thread_local variable}} \
+// cxx2b-warning {{definition of a 
static variable in a constexpr function is incompatible with C++ standards 
before C++2b}}
+  return m;

Formatting nit.



Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:29
+  static __thread int m = 0;// expected-note {{control flows 
through the definition of a thread_local variable}} \
+ / cxx2b-warning {{definition of a static variable 
in a constexpr function is incompatible with C++ standards before C++2b}}
+  return m;

Formatting nit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111400

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

[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:69
+def note_constexpr_static_local : Note<
+  "control flows through the declaration of a %select{static|thread_local}0 
variable">;
 def note_constexpr_subobject_declared_here : Note<

erichkeane wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > Hmm... this feels a little awkward to me.  Though, I usually have Aaron 
> > > do the bikeshedding, so if he didn't come up with better I guess we can 
> > > let it go.
> > I didn't think it was overly awkward, about the only question I had was 
> > "should we quote the `static` and `thread_local` as keywords?" when I 
> > reviewed.
> Ok, I can live with that.  AS far as quoting, I think we are consistently 
> inconsistent on that one.
Minor nit:
The code already behaves as-if the resolution of CWG 2552 is applied. Let's use 
the corresponding wording.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111400

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

[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:17-22
+constexpr int g() { // expected-error {{constexpr function never produces a 
constant expression}}
+  goto test;// expected-note {{subexpression not valid in a constant 
expression}} \
+   // expected-warning {{use of this statement in a constexpr function 
is incompatible with C++ standards before C++2b}}
+test:
+  return 0;
+}

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > cor3ntin wrote:
> > > hubert.reinterpretcast wrote:
> > > > Still trying to make the choice of tests consistent between the 
> > > > different cases (static local variable, goto statement, etc.) and 
> > > > consistent within files...
> > > > 
> > > > The test that is here (function will unconditionally evaluate) seems to 
> > > > belong in `constant-expression-cxx2b.cpp`... or at least that is where 
> > > > the equivalent tests for the static local variable case, etc. is (and 
> > > > correctly, I think).
> > > > 
> > > > The test that should be here is the "conditionally will evaluate" case 
> > > > where the function is not called. That is, this file has tests that 
> > > > check that the warnings are produced purely from the definition being 
> > > > present.
> > > I'm not sure I understand. No call is made in that file.
> > The case here is unconditional and not called. So, it should be in 
> > `constant-expression-cxx2b.cpp` because that is the file with other 
> > unconditional (and not called) cases. That a call never produces a constant 
> > expression is a property of the evaluation rules.
> > 
> > The other file currently has `eval_goto::f`, which does have a condition 
> > gating the evaluation of the `goto` (and is called). A copy of that 
> > function (but no call) would be what fits in this file.
> The idea is that `p3-2b.cpp` would check for things without evaluating them, 
> whether `constant-expression-cxx2b.cpp` does evaluate them - aka define vs 
> use. Let me know if that clarifies or if you would still prefer i remove 
> redundant test from `p3-2b.cpp` 
Regarding the test here (and related possible tests), I'm willing to go with 
what's in the patch now.

I was pointing out that `constant-expression-cxx2b.cpp` //does// have cases 
that are not evaluated (`::f` and `::g`) where the construct is reached 
unconditionally (i.e., cases like this one, except not for `goto`); therefore, 
this test and those tests should be in the same file.

At the same time, the compat warning should be tested (in this file) as being 
present on the definition even if it is only conditionally reachable and there 
are no uses of the function (`dtor.cpp` covers the same for the extension 
warning).

As for which file tests like the current one should go, I would characterize 
the difference between `p3-2b.cpp` and `constant-expression-cxx2b.cpp` as not 
"define versus use", but instead as "definition rules versus evaluation rules". 
I would argue that the truth of "never produces a constant expression" is a 
consequence from the evaluation rules.



Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:32-34
+constexpr void non_literal() { // expected-error {{constexpr function never 
produces a constant expression}}
+  NonLiteral n;// expected-note {{non-literal type 
'NonLiteral' cannot be used in a constant expression}}
+}

hubert.reinterpretcast wrote:
> For the reason above, I think this should be in 
> `constant-expression-cxx2b.cpp` instead.
If moving tests around re: "never produces a constant expression", move this 
one (`non_literal()`) too.



Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp:38
+  if (!b)
+NonLiteral n;
+}

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > For consistency, this should warn (under `-Wpre-c++2b-compat`).
> I though we decided *not* to do that
Actually, I think we only decided that it should always be an error in older 
modes (and we didn't decide not to add a compat warning). That is, the 
extension and compat warnings just happen to be paired currently in the patch. 
Now that the code has cleared out of my system a bit, I believe that there is 
no fundamental reason for the two warnings to be paired.

I'm fine with getting this patch landed and then fixing this after. Maybe 
@aaron.ballman would comment as well.



Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp:1-3
+// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu 
-std=c++11 -Werror=c++14-extensions -Werror=c++20-extensions 
-Werror=c++2b-extensions %s
+// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu 
-std=c++14 -DCXX14 -Werror=c++20-extensions -Werror=c++2b-extensions %s
+// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu 
-std=c++20 -DCXX14 -DCXX20 

[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM with minor nit. Thank you.




Comment at: clang/test/SemaCXX/constant-expression-cxx2b.cpp:4
+
+struct NonLiteral { //cxx2a-note {{'NonLiteral' is not literal}}
+  NonLiteral() {}

Minor nit: Formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111400

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