[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 added inline comments.



Comment at: clang/test/SemaCXX/offsetof.cpp:106
+int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}
+int x4[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+

cor3ntin wrote:
> yichi170 wrote:
> > cor3ntin wrote:
> > > yichi170 wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > aaron.ballman wrote:
> > > > > > yichi170 wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > There's one more test I'd like to see:
> > > > > > > > ```
> > > > > > > > struct S {
> > > > > > > >   int Foo;
> > > > > > > > };
> > > > > > > > 
> > > > > > > > template 
> > > > > > > > void func() {
> > > > > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > > > > }
> > > > > > > > 
> > > > > > > > void inst() {
> > > > > > > >   func();
> > > > > > > > }
> > > > > > > > ```
> > > > > > > It would get the compile error in the current patch, but I think 
> > > > > > > it should be compiled without any error, right?
> > > > > > Correct, that should be accepted: https://godbolt.org/z/1f6a9Yaxa
> > > > > Should expect this to pass too:
> > > > > ```
> > > > > template 
> > > > > struct Z {
> > > > >   static_assert(!__builtin_offsetof(T, template Q::x));
> > > > > };
> > > > > 
> > > > > struct A {
> > > > >   template  using Q = T;
> > > > >   int x;
> > > > > };
> > > > > 
> > > > > Z za;
> > > > > ```
> > > > Wow. Does it mean we cannot simply parse the identifier, `::`, `.` and 
> > > > brackets in `__builtin_offsetof`?
> > > GCC seems to support that. 
> > > 
> > > We probably want to call `ParseOptionalCXXScopeSpecifier` and store the 
> > > `NestedNameSpecifierLoc` we'd get from it (and then parse the (sequence 
> > > of) identifier(s) corresponding to the member) as we do now.
> > > 
> > > The documentation of 
> > > https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Offsetof.html#index-_005f_005fbuiltin_005foffsetof
> > >  
> > > seems inaccurate,
> > > 
> > > it seems to be
> > > 
> > > `"__builtin_offsetof" "(" typename ","  nested-name-specifier 
> > > offsetof_member_designator ")"`
> > > 
> > > 
> > > Note that you will have to take care of transforming the nested name in 
> > > TreeTransform and handle type dependencies. Let me know if you have 
> > > further questions,
> > > it's more involved than what you signed for. Sorry for not spotting that 
> > > earlier (Thanks @hubert.reinterpretcast !)
> > Thank you for all the help! I'll take a look at it!
> I was wrong, we need another approach.
> 
> I think the grammar is actually
> ```
> member-designator:
>   qualified-id
>   member-designator.qualified-id
>   member-designator.qualified-id
> ```
> IE, we should support https://godbolt.org/z/eEq8snMc8
> 
> Unfortunately, this looks like a much bigger change that we envisioned when 
> we tagged this as a good first issue, to the extent I'm not sure what is 
> actually the right design is would be.
> 
> For each component I imagine we want to store
> `NestedNameSpecifierLoc + DeclarationNameInfo`
> 
> The parser would have to produce a CXXScopeSpec + UnqualifiedId pair for each 
> component.
> 
> The expression is dependent if any of the component is type dependent,
> 
> `OffsetOfNode` would have to change, but i think we can get away
> Only changing the identifier case (ie the dependent case)  
> 
Would it be better for me to close this patch and submit a new one if I find 
out how to implement it? I hope others won't hesitate to contribute because I'm 
working on this. I don't want to cause any delays in the release plan!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

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

[PATCH] D152793: [RISCV] Add MC layer support for Zicfiss.

[Yeting Kuo via Phabricator via cfe-commits]

fakepaper56 planned changes to this revision.
fakepaper56 added a comment.
Herald added a subscriber: sunshaoce.

The spec does some change, I need to sync with the spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152793

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

[PATCH] D158152: [clang-tidy]mark record initList as non-const param

[Congcong Cai via Phabricator via cfe-commits]

HerrCai0907 updated this revision to Diff 551802.
HerrCai0907 added a comment.

fix typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158152

Files:
  clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp
@@ -222,6 +222,18 @@
 void recordpointer(struct XY *xy) {
   *(xy->x) = 0;
 }
+void recordInitList(int *x) {
+  XY xy = {x, nullptr};
+}
+
+struct XYConst {
+  int const *x;
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:30: warning: pointer parameter 'x' can be 
pointer to const
+void recordInitListDiag(int *x) {
+  // CHECK-FIXES: {{^}}void recordInitListDiag(const int *x) {{{$}}
+  XYConst xy = {x};
+}
 
 class C {
 public:
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -227,6 +227,10 @@
   do-while loops into account for the `AllowIntegerConditions` and
   `AllowPointerConditions` options.
 
+- Improved :doc:`readability-non-const-parameter
+  ` check to ignore
+  false-positives in initializer list of record.
+
 - Improved :doc:`readability-static-accessed-through-instance
   ` check to
   identify calls to static member functions with out-of-class inline 
definitions.
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -103,7 +103,7 @@
   } else if (const auto *VD = Result.Nodes.getNodeAs("Mark")) {
 const QualType T = VD->getType();
 if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
-T->isArrayType())
+T->isArrayType() || T->isRecordType())
   markCanNotBeConst(VD->getInit(), true);
 else if (T->isLValueReferenceType() &&
  !T->getPointeeType().isConstQualified())


Index: clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp
@@ -222,6 +222,18 @@
 void recordpointer(struct XY *xy) {
   *(xy->x) = 0;
 }
+void recordInitList(int *x) {
+  XY xy = {x, nullptr};
+}
+
+struct XYConst {
+  int const *x;
+};
+// CHECK-MESSAGES: :[[@LINE+1]]:30: warning: pointer parameter 'x' can be pointer to const
+void recordInitListDiag(int *x) {
+  // CHECK-FIXES: {{^}}void recordInitListDiag(const int *x) {{{$}}
+  XYConst xy = {x};
+}
 
 class C {
 public:
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -227,6 +227,10 @@
   do-while loops into account for the `AllowIntegerConditions` and
   `AllowPointerConditions` options.
 
+- Improved :doc:`readability-non-const-parameter
+  ` check to ignore
+  false-positives in initializer list of record.
+
 - Improved :doc:`readability-static-accessed-through-instance
   ` check to
   identify calls to static member functions with out-of-class inline definitions.
Index: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -103,7 +103,7 @@
   } else if (const auto *VD = Result.Nodes.getNodeAs("Mark")) {
 const QualType T = VD->getType();
 if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
-T->isArrayType())
+T->isArrayType() || T->isRecordType())
   markCanNotBeConst(VD->getInit(), true);
 else if (T->isLValueReferenceType() &&
  !T->getPointeeType().isConstQualified())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D156821#4600605 , @craig.topper 
wrote:

> In D156821#4600550 , @MaskRay wrote:
>
>>> Currenly both Clang and GCC support the following set of flags that control
>>
>> code gen of signed overflow:
>>
>>> [...]
>>> Howerver, clang ignores these flags for __builtin_abs(int) and its 
>>> higher-width
>>
>> versions, so passing minimum integer value always causes poison.
>>
>> This paragraph reads as if GCC emits a trap for `__builtin_abs` in -ftrapv 
>> mode, but it doesn't. That said, its `-fsanitize=signed-integer-overflow` 
>> does handle `__builtin_abs`.
>
> On X86 at least, gcc does call `__negvsi2` for `__builtin_abs` under -ftrapv. 
> https://godbolt.org/z/8dhn9bsv5

Thank you. You are right, sorry for my neglection. GCC may use either 
`__negvsi2` or `__subvsi3` for different ports.


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

https://reviews.llvm.org/D156821

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

[clang] 8b6f09e - Revert "[clang][X86] Add __cpuidex function to cpuid.h"

[Aiden Grossman via cfe-commits]

Author: Aiden Grossman
Date: 2023-08-19T17:18:10-07:00
New Revision: 8b6f09e257b521947b50761737290e5bf31c80f3

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

LOG: Revert "[clang][X86] Add __cpuidex function to cpuid.h"

This reverts commit 58696d2f5bbae32dddcaec6891293e769465e77c.

Accidentally had this in my branch for my structural hash patch.

Added: 


Modified: 
clang/lib/Headers/cpuid.h
clang/test/Headers/cpuid.c

Removed: 
clang/test/Headers/__cpuidex_conflict.c



diff  --git a/clang/lib/Headers/cpuid.h b/clang/lib/Headers/cpuid.h
index 9b31db9f7d4f32..1ad6853a97c9d2 100644
--- a/clang/lib/Headers/cpuid.h
+++ b/clang/lib/Headers/cpuid.h
@@ -328,14 +328,4 @@ static __inline int __get_cpuid_count (unsigned int __leaf,
 return 1;
 }
 
-// In some configurations, __cpuidex is defined as a builtin (primarily
-// -fms-extensions) which will conflict with the __cpuidex definition below.
-#if !(__has_builtin(__cpuidex))
-static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
-{
-  __cpuid_count(__leaf, __subleaf, __cpu_info[0], __cpu_info[1], __cpu_info[2],
-__cpu_info[3]);
-}
-#endif
-
 #endif /* __CPUID_H */

diff  --git a/clang/test/Headers/__cpuidex_conflict.c 
b/clang/test/Headers/__cpuidex_conflict.c
deleted file mode 100644
index 8687a6aa2f897a..00
--- a/clang/test/Headers/__cpuidex_conflict.c
+++ /dev/null
@@ -1,22 +0,0 @@
-// Make sure that __cpuidex in cpuid.h doesn't conflict with the MS
-// extensions built in by ensuring compilation succeeds:
-// RUN: %clang_cc1 %s -ffreestanding -fms-extensions -fms-compatibility \
-// RUN:  -fms-compatibility-version=19.00 -triple x86_64-pc-windows-msvc 
-emit-llvm -o -
-// %clang_cc1 %s -ffreestanding -triple x86_64-w64-windows-gnu -fms-extensions 
-emit-llvm -o -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
-
-typedef __SIZE_TYPE__ size_t;
-
-// We declare __cpuidex here as where the buitlin should be exposed (MSVC), the
-// declaration is in , but  is not available from all the
-// targets that are being tested here.
-void __cpuidex (int[4], int, int);
-
-#include 
-
-int cpuid_info[4];
-
-void test_cpuidex(unsigned level, unsigned count) {
-  __cpuidex(cpuid_info, level, count);
-}
-

diff  --git a/clang/test/Headers/cpuid.c b/clang/test/Headers/cpuid.c
index 6ed12eca7a61d4..7e485495c10665 100644
--- a/clang/test/Headers/cpuid.c
+++ b/clang/test/Headers/cpuid.c
@@ -6,19 +6,14 @@
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A 
cpuid\0A xchgq %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
-// CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
 
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, 
i32 %{{[a-z0-9]+}})
-// CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, 
i32 %{{[a-z0-9]+}})
 
 unsigned eax0, ebx0, ecx0, edx0;
 unsigned eax1, ebx1, ecx1, edx1;
 
-int cpuid_info[4];
-
 void test_cpuid(unsigned level, unsigned count) {
   __cpuid(level, eax1, ebx1, ecx1, edx1);
   __cpuid_count(level, count, eax0, ebx0, ecx0, edx0);
-  __cpuidex(cpuid_info, level, count);
 }



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

[clang] 58696d2 - [clang][X86] Add __cpuidex function to cpuid.h

[Aiden Grossman via cfe-commits]

Author: Aiden Grossman
Date: 2023-08-19T17:12:23-07:00
New Revision: 58696d2f5bbae32dddcaec6891293e769465e77c

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

LOG: [clang][X86] Add __cpuidex function to cpuid.h

MSVC has a __cpuidex function implemented to call the underlying cpuid
instruction which accepts a leaf, subleaf, and data array that the output
data is written into. This patch adds this functionality into clang
under the cpuid.h header. This also makes clang match GCC's behavior.
GCC has had __cpuidex in its cpuid.h since 2020.

Added: 
clang/test/Headers/__cpuidex_conflict.c

Modified: 
clang/lib/Headers/cpuid.h
clang/test/Headers/cpuid.c

Removed: 




diff  --git a/clang/lib/Headers/cpuid.h b/clang/lib/Headers/cpuid.h
index 1ad6853a97c9d2..9b31db9f7d4f32 100644
--- a/clang/lib/Headers/cpuid.h
+++ b/clang/lib/Headers/cpuid.h
@@ -328,4 +328,14 @@ static __inline int __get_cpuid_count (unsigned int __leaf,
 return 1;
 }
 
+// In some configurations, __cpuidex is defined as a builtin (primarily
+// -fms-extensions) which will conflict with the __cpuidex definition below.
+#if !(__has_builtin(__cpuidex))
+static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
+{
+  __cpuid_count(__leaf, __subleaf, __cpu_info[0], __cpu_info[1], __cpu_info[2],
+__cpu_info[3]);
+}
+#endif
+
 #endif /* __CPUID_H */

diff  --git a/clang/test/Headers/__cpuidex_conflict.c 
b/clang/test/Headers/__cpuidex_conflict.c
new file mode 100644
index 00..8687a6aa2f897a
--- /dev/null
+++ b/clang/test/Headers/__cpuidex_conflict.c
@@ -0,0 +1,22 @@
+// Make sure that __cpuidex in cpuid.h doesn't conflict with the MS
+// extensions built in by ensuring compilation succeeds:
+// RUN: %clang_cc1 %s -ffreestanding -fms-extensions -fms-compatibility \
+// RUN:  -fms-compatibility-version=19.00 -triple x86_64-pc-windows-msvc 
-emit-llvm -o -
+// %clang_cc1 %s -ffreestanding -triple x86_64-w64-windows-gnu -fms-extensions 
-emit-llvm -o -
+// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+
+typedef __SIZE_TYPE__ size_t;
+
+// We declare __cpuidex here as where the buitlin should be exposed (MSVC), the
+// declaration is in , but  is not available from all the
+// targets that are being tested here.
+void __cpuidex (int[4], int, int);
+
+#include 
+
+int cpuid_info[4];
+
+void test_cpuidex(unsigned level, unsigned count) {
+  __cpuidex(cpuid_info, level, count);
+}
+

diff  --git a/clang/test/Headers/cpuid.c b/clang/test/Headers/cpuid.c
index 7e485495c10665..6ed12eca7a61d4 100644
--- a/clang/test/Headers/cpuid.c
+++ b/clang/test/Headers/cpuid.c
@@ -6,14 +6,19 @@
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A 
cpuid\0A xchgq %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
+// CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
 
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, 
i32 %{{[a-z0-9]+}})
+// CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, 
i32 %{{[a-z0-9]+}})
 
 unsigned eax0, ebx0, ecx0, edx0;
 unsigned eax1, ebx1, ecx1, edx1;
 
+int cpuid_info[4];
+
 void test_cpuid(unsigned level, unsigned count) {
   __cpuid(level, eax1, ebx1, ecx1, edx1);
   __cpuid_count(level, count, eax0, ebx0, ecx0, edx0);
+  __cpuidex(cpuid_info, level, count);
 }



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

[PATCH] D149867: [Clang][M68k] Add Clang support for the new M68k_RTD CC

[Min-Yih Hsu via Phabricator via cfe-commits]

myhsu added inline comments.



Comment at: clang/test/Sema/m68k-mrtd.c:4-9
+#ifdef MRTD
+// expected-error@+3 {{function with no prototype cannot use the m68k_rtd 
calling convention}}
+#endif
+void foo(int arg) {
+  bar(arg);
+}

aaron.ballman wrote:
> myhsu wrote:
> > aaron.ballman wrote:
> > > A better way to do this is to use `-verify=mrtd` on the line enabling 
> > > rtd, and using `// rtd-error {{whatever}}` on the line being diagnosed. 
> > > (Same comment applies throughout the file.)
> > > 
> > > Huh, I was unaware that implicit function declarations are using 
> > > something other than the default calling convention (which is C, not 
> > > m68k_rtd). Is this intentional?
> > > Huh, I was unaware that implicit function declarations are using 
> > > something other than the default calling convention (which is C, not 
> > > m68k_rtd). Is this intentional?
> > 
> > I'm not sure if I understand you correctly, but this diagnostic is emitted 
> > if the CC does not support variadic function call. 
> `bar` is not declared, in C89 this causes an implicit function declaration to 
> be generated with the signature: `extern int ident();` What surprised me is 
> that we seem to be generating something more like this instead: 
> `__attribute__((m68k_rtd)) int ident();` despite that not being valid.
I understand what you meant, but the standard only says that using implicit 
declared identifier is as if `extern int ident();` appears in the source code. 
My interpretation is that in this case since the very source code is compiled 
with `-mrtd`, every functions use m68k_rtd rather than C calling convention. 

Another example is stdcall: i386 Clang emits a similar warning ("function with 
no prototype cannot use the stdcall calling convention") when `-mrtd` is used 
(to set default CC to stdcall) on the same snippet. Should `bar` was not called 
with stdcall under the influence of `-mrtd`, the aforementioned message would 
not be emitted in the first place.
i386 GCC also calls `bar` with stdcall when `-mrtd` is given (though no warning 
message is emitted).



Comment at: clang/test/Sema/m68k-mrtd.c:45
+extern void (*d)(int, ...);
+__attribute__((m68k_rtd)) extern void (*d)(int, ...);

aaron.ballman wrote:
> myhsu wrote:
> > aaron.ballman wrote:
> > > Missing tests for:
> > > 
> > > * Function without a prototype
> > > * Applying the attribute to a non-function
> > > * Providing arguments to the attribute
> > > * What should happen for C++ and things like member functions?
> > > Function without a prototype
> > 
> > I thought the first check was testing function without a prototype.
> > 
> > > What should happen for C++ and things like member functions?
> > 
> > I believe we don't have any special handling for C++.
> > 
> > I addressed rest of the bullet items you mentioned, please take a look.
> >> Function without a prototype
> > I thought the first check was testing function without a prototype.
> 
> Nope, I meant something more like this:
> ```
> __attribute__((m68k_rtd)) void foo(); // Should error
> __attribute__((m68k_rtd)) void bar(a, b) int a, b; {} // Should error
> ```
> 
> >> What should happen for C++ and things like member functions?
> > I believe we don't have any special handling for C++.
> 
> Test coverage should still exist to ensure the behavior is what you'd expect 
> when adding the calling convention to a member function and a lambda, for 
> example. (Both Sema and CodeGen tests for this)
> 
> Also missing coverage for the changes to `-fdefault-calling-conv=`
> 
> I'm also still wondering whether there's a way to use m68k with a Windows ABI 
> triple (basically, do we need changes in MicrosoftMangle.cpp?)

> ```
> __attribute__((m68k_rtd)) void bar(a, b) int a, b; {} // Should error
> ```

Right now Clang only gives a warning about potential behavior change after C23, 
rather than an error for this kind of syntax. Because it seems like Clang 
doesn't check this situation against unsupported calling convention. I'm not 
sure if we should throw the same error as `__attribute__((m68k_rtd)) void 
foo()`.

> I'm also still wondering whether there's a way to use m68k with a Windows ABI 
> triple (basically, do we need changes in MicrosoftMangle.cpp?)

I don't think it's worth it to support m68k with Windows ABI as this 
combination never existed (and unlikely to happen in the future)


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

https://reviews.llvm.org/D149867

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

[PATCH] D149867: [Clang][M68k] Add Clang support for the new M68k_RTD CC

[Min-Yih Hsu via Phabricator via cfe-commits]

myhsu updated this revision to Diff 551797.
myhsu marked 4 inline comments as done.
myhsu added a comment.

- Add more C++ tests for `m68k_rtd`, both Sema and CodeGen ones
- Addressed other comments


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

https://reviews.llvm.org/D149867

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/M68k.cpp
  clang/lib/Basic/Targets/M68k.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/mrtd.c
  clang/test/CodeGenCXX/default_calling_conv.cpp
  clang/test/CodeGenCXX/m68k-rtdcall.cpp
  clang/test/Sema/m68k-rtdcall.c
  clang/test/SemaCXX/m68k-rtdcall.cpp
  clang/tools/libclang/CXType.cpp

Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -678,6 +678,7 @@
   TCALLINGCONV(SwiftAsync);
   TCALLINGCONV(PreserveMost);
   TCALLINGCONV(PreserveAll);
+  TCALLINGCONV(M68kRTD);
 case CC_SpirFunction: return CXCallingConv_Unexposed;
 case CC_AMDGPUKernelCall: return CXCallingConv_Unexposed;
 case CC_OpenCLKernel: return CXCallingConv_Unexposed;
Index: clang/test/SemaCXX/m68k-rtdcall.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/m68k-rtdcall.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple m68k-linux-gnu -fsyntax-only %s
+
+class A {
+public:
+  void __attribute__((m68k_rtd)) member() {}
+};
+
+void test() {
+  A a;
+  a.member();
+
+  auto f = [](int b) __attribute__((m68k_rtd)) {};
+  f(87);
+};
Index: clang/test/Sema/m68k-rtdcall.c
===
--- /dev/null
+++ clang/test/Sema/m68k-rtdcall.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple m68k-unknown-unknown -mrtd -std=c89 -verify -verify=rtd %s
+// RUN: %clang_cc1 -triple m68k-unknown-unknown -std=c89 -verify -verify=nortd %s
+
+// rtd-error@+2 {{function with no prototype cannot use the m68k_rtd calling convention}}
+void foo(int arg) {
+  bar(arg);
+}
+
+// nortd-note@+4 {{previous declaration is here}}
+// nortd-error@+4 {{function declared 'm68k_rtd' here was previously declared without calling convention}}
+// nortd-note@+4 {{previous declaration is here}}
+// nortd-error@+4 {{function declared 'm68k_rtd' here was previously declared without calling convention}}
+void nonvariadic1(int a, int b, int c);
+void __attribute__((m68k_rtd)) nonvariadic1(int a, int b, int c);
+void nonvariadic2(int a, int b, int c);
+void __attribute__((m68k_rtd)) nonvariadic2(int a, int b, int c) { }
+
+// expected-error@+2 {{variadic function cannot use m68k_rtd calling convention}}
+void variadic(int a, ...);
+void __attribute__((m68k_rtd)) variadic(int a, ...);
+
+// rtd-note@+2 {{previous declaration is here}}
+// rtd-error@+2 {{redeclaration of 'a' with a different type: 'void ((*))(int, int) __attribute__((cdecl))' vs 'void (*)(int, int) __attribute__((m68k_rtd))'}}
+extern void (*a)(int, int);
+__attribute__((cdecl)) extern void (*a)(int, int);
+
+extern void (*b)(int, ...);
+__attribute__((cdecl)) extern void (*b)(int, ...);
+
+// nortd-note@+2 {{previous declaration is here}}
+// nortd-error@+2 {{redeclaration of 'c' with a different type: 'void ((*))(int, int) __attribute__((m68k_rtd))' vs 'void (*)(int, int)'}}
+extern void (*c)(int, int);
+__attribute__((m68k_rtd)) extern void (*c)(int, int);
+
+// expected-error@+2 {{variadic function cannot use m68k_rtd calling convention}}
+extern void (*d)(int, ...);
+__attribute__((m68k_rtd)) extern void (*d)(int, ...);
+
+// expected-warning@+1 {{'m68k_rtd' only applies to function types; type here is 'int'}}
+__attribute__((m68k_rtd)) static int g = 0;
+
+// expected-error@+1 {{'m68k_rtd' attribute takes no arguments}}
+void __attribute__((m68k_rtd("invalid"))) z(int a);
+
+// expected-error@+1 {{function with no prototype cannot use the m68k_rtd calling convention}}
+void __attribute__((m68k_rtd)) e();
Index: clang/test/CodeGenCXX/m68k-rtdcall.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/m68k-rtdcall.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple m68k-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+class A {
+public:
+// CHECK: define{{.*}} m68k_rtdcc void @_ZN1A6memberEv
+  void __attribute__((m68k_rtd)) member() {}
+};
+
+void test() {
+  A a;
+  a.member();
+
+// CHECK: define{{.*}} m68k_rtdcc 

[PATCH] D158231: [clang][test] Fix clang machine-function-split tests

[Fangrui Song via Phabricator via cfe-commits]

MaskRay resigned from this revision.
MaskRay added a comment.
This revision now requires review to proceed.

Sorry, after reading through D157750 , I 
think the patch should be reverted.




Comment at: clang/test/CodeGen/fsplit-machine-functions.c:2
+// REQUIRES: system-linux
+// REQUIRES: x86-registered-target
+// REQUIRES: arm-registered-target

shenhan wrote:
> MaskRay wrote:
> > We usually place x86 tests under CodeGen/X86 and arm tests under CodeGen/ARM
> I can moved the X86 cases to clang/test/CodeGen/X86, but there is no 
> clang/test/CodeGen/Arm or similar, shall I create "clang/test/CodeGen/Arm" 
> instead? 
Sorry, this is not llvm/test/CodeGen. For clang/test/CodeGen this is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158231

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

[PATCH] D158346: [clang-tidy] [readability-container-size-empty] improved check to detect missing usage of .empty() on string_literals Fixes #64547

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 updated this revision to Diff 551789.
felix642 added a comment.

Clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158346

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -22,6 +22,10 @@
 };
 }
 
+namespace string_literals{
+string operator""s(const char *, size_t);
+}
+
 }
 
 template 
@@ -461,7 +465,7 @@
 constexpr Lazy L;
 static_assert(!L.size(), "");
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: the 'empty' method should be used
-// CHECK-MESSAGES: :90:18: note: method 'Lazy'::empty() defined here
+// CHECK-MESSAGES: :94:18: note: method 'Lazy'::empty() defined here
 // CHECK-FIXES: {{^}}static_assert(L.empty(), "");
 
 struct StructWithLazyNoexcept {
@@ -492,17 +496,17 @@
   if (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   CHECKSIZE(templated_container);
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
 }
 
@@ -575,74 +579,74 @@
   if (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   while (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}while (!templated_container.empty()){{$}}
 
   do {
   }
   while (templated_container.size());
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}while (!templated_container.empty());
 
   for (; templated_container.size();)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}for (; !templated_container.empty();){{$}}
 
   if (true && templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (true && !templated_container.empty()){{$}}
 
   if (true || templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (true || 

[PATCH] D158346: [clang-tidy] [readability-container-size-empty] improved check to detect missing usage of .empty() on string_literals Fixes #64547

[Piotr Zegar via Phabricator via cfe-commits]

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

LGTM, run clang-format on this code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158346

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

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13278
+llvm::Value *
+CodeGenFunction::EmitX86CpuSupports(std::array FeatureMask) {
   Value *Result = Builder.getTrue();

erichkeane wrote:
> Hmm... I guess size-wise this is on the edge of "const ref vs pass by value". 
>  I think its fine now, but 'next time' this grows we'll have to think about 
> making this a const-ref.
Yes. Right now passing 2 registers on a 64-bit target is more efficient.
'next time' may be quite a while from now:)



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13311
+  llvm::Constant *CpuFeatures2 =
+  CGM.CreateRuntimeVariable(ATy, "__cpu_features2");
+  cast(CpuFeatures2)->setDSOLocal(true);

erichkeane wrote:
> This won't double-create this if used more than 1x, right?  There doesn't 
> need to be something like GetOrCreate... here?
`CodeGenModule::CreateRuntimeVariable` calls GetOrCreate internally. It's fine 
to be called multiple times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329

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

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 551787.
MaskRay marked 4 inline comments as done.
MaskRay added a comment.

Use Lo_32/Hi_32


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/attr-target-clones.c
  clang/test/CodeGen/builtin-cpu-supports.c
  clang/test/Sema/attr-target-clones.c
  compiler-rt/lib/builtins/cpu_model.c
  llvm/lib/TargetParser/X86TargetParser.cpp

Index: llvm/lib/TargetParser/X86TargetParser.cpp
===
--- llvm/lib/TargetParser/X86TargetParser.cpp
+++ llvm/lib/TargetParser/X86TargetParser.cpp
@@ -317,6 +317,7 @@
 // listed here before, which means it doesn't support -march, -mtune and so on.
 // FIXME: Remove OnlyForCPUDispatchSpecific after all CPUs here support both
 // cpu_dispatch/specific() feature and -march, -mtune, and so on.
+// clang-format off
 constexpr ProcInfo Processors[] = {
  // Empty processor. Include X87 and CMPXCHG8 for backwards compatibility.
   { {""}, CK_None, ~0U, FeatureX87 | FeatureCMPXCHG8B, '\0', false },
@@ -482,13 +483,14 @@
   { {"znver3"}, CK_ZNVER3, FEATURE_AVX2, FeaturesZNVER3, '\0', false },
   { {"znver4"}, CK_ZNVER4, FEATURE_AVX512VBMI2, FeaturesZNVER4, '\0', false },
   // Generic 64-bit processor.
-  { {"x86-64"}, CK_x86_64, ~0U, FeaturesX86_64, '\0', false },
-  { {"x86-64-v2"}, CK_x86_64_v2, ~0U, FeaturesX86_64_V2, '\0', false },
-  { {"x86-64-v3"}, CK_x86_64_v3, ~0U, FeaturesX86_64_V3, '\0', false },
-  { {"x86-64-v4"}, CK_x86_64_v4, ~0U, FeaturesX86_64_V4, '\0', false },
+  { {"x86-64"}, CK_x86_64, FEATURE_SSE2 , FeaturesX86_64, '\0', false },
+  { {"x86-64-v2"}, CK_x86_64_v2, FEATURE_SSE4_2 , FeaturesX86_64_V2, '\0', false },
+  { {"x86-64-v3"}, CK_x86_64_v3, FEATURE_AVX2, FeaturesX86_64_V3, '\0', false },
+  { {"x86-64-v4"}, CK_x86_64_v4, FEATURE_AVX512VL, FeaturesX86_64_V4, '\0', false },
   // Geode processors.
   { {"geode"}, CK_Geode, ~0U, FeaturesGeode, '\0', false },
 };
+// clang-format on
 
 constexpr const char *NoTuneList[] = {"x86-64-v2", "x86-64-v3", "x86-64-v4"};
 
Index: compiler-rt/lib/builtins/cpu_model.c
===
--- compiler-rt/lib/builtins/cpu_model.c
+++ compiler-rt/lib/builtins/cpu_model.c
@@ -158,6 +158,19 @@
   FEATURE_AVX512BITALG,
   FEATURE_AVX512BF16,
   FEATURE_AVX512VP2INTERSECT,
+
+  FEATURE_CMPXCHG16B = 46,
+  FEATURE_F16C = 49,
+  FEATURE_LAHF_LM = 54,
+  FEATURE_LM,
+  FEATURE_WP,
+  FEATURE_LZCNT = 57,
+  FEATURE_MOVBE,
+
+  FEATURE_X86_64_BASELINE = 95,
+  FEATURE_X86_64_V2,
+  FEATURE_X86_64_V3,
+  FEATURE_X86_64_V4,
   CPU_FEATURE_MAX
 };
 
@@ -675,6 +688,7 @@
  unsigned *Features) {
   unsigned EAX = 0, EBX = 0;
 
+#define hasFeature(F) ((Features[F / 32] >> (F % 32)) & 1)
 #define setFeature(F)  \
   Features[F / 32] |= 1U << (F % 32)
 
@@ -695,14 +709,20 @@
 setFeature(FEATURE_SSSE3);
   if ((ECX >> 12) & 1)
 setFeature(FEATURE_FMA);
+  if ((ECX >> 13) & 1)
+setFeature(FEATURE_CMPXCHG16B);
   if ((ECX >> 19) & 1)
 setFeature(FEATURE_SSE4_1);
   if ((ECX >> 20) & 1)
 setFeature(FEATURE_SSE4_2);
+  if ((ECX >> 22) & 1)
+setFeature(FEATURE_MOVBE);
   if ((ECX >> 23) & 1)
 setFeature(FEATURE_POPCNT);
   if ((ECX >> 25) & 1)
 setFeature(FEATURE_AES);
+  if ((ECX >> 29) & 1)
+setFeature(FEATURE_F16C);
 
   // If CPUID indicates support for XSAVE, XRESTORE and AVX, and XGETBV
   // indicates that the AVX registers will be saved and restored on context
@@ -784,12 +804,39 @@
 
   bool HasExtLeaf1 = MaxExtLevel >= 0x8001 &&
  !getX86CpuIDAndInfo(0x8001, , , , );
-  if (HasExtLeaf1 && ((ECX >> 6) & 1))
-setFeature(FEATURE_SSE4_A);
-  if (HasExtLeaf1 && ((ECX >> 11) & 1))
-setFeature(FEATURE_XOP);
-  if (HasExtLeaf1 && ((ECX >> 16) & 1))
-setFeature(FEATURE_FMA4);
+  if (HasExtLeaf1) {
+if (ECX & 1)
+  setFeature(FEATURE_LAHF_LM);
+if ((ECX >> 5) & 1)
+  setFeature(FEATURE_LZCNT);
+if (((ECX >> 6) & 1))
+  setFeature(FEATURE_SSE4_A);
+if (((ECX >> 11) & 1))
+  setFeature(FEATURE_XOP);
+if (((ECX >> 16) & 1))
+  setFeature(FEATURE_FMA4);
+if (((EDX >> 29) & 1))
+  setFeature(FEATURE_LM);
+  }
+
+  if (hasFeature(FEATURE_LM) && hasFeature(FEATURE_SSE2)) {
+setFeature(FEATURE_X86_64_BASELINE);
+if (hasFeature(FEATURE_CMPXCHG16B) && hasFeature(FEATURE_POPCNT) &&
+hasFeature(FEATURE_LAHF_LM) && hasFeature(FEATURE_SSE4_2)) {
+  setFeature(FEATURE_X86_64_V2);
+  if (hasFeature(FEATURE_AVX2) && hasFeature(FEATURE_BMI) &&
+  hasFeature(FEATURE_BMI2) && hasFeature(FEATURE_F16C) &&
+  hasFeature(FEATURE_FMA) && 

[PATCH] D158309: [flang][driver] Mark -Wl as visible in Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158309

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

[PATCH] D158348: [clang][X86] Add __cpuidex function to cpuid.h

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman added reviewers: aaron.ballman, glandium, mstorsjo, craig.topper.
aidengrossman added a comment.

This is an updated version of https://reviews.llvm.org/D150646 that uses 
`__has_builtin` instead of checking for a definition of `_MSC_EXTENSIONS`. This 
fixes the cases that I have received so far and shouldn't cause any other 
issues.

There is one thing that needs some discussion: include order relating to 
`intrin.h`. When using a MSVC target triple with `-fms-extensions` (and maybe 
in other cases), `intrin.h` needs to be included to get the declaration of the 
built-in (or otherwise `__has_builtin` won't detect it and it will error out 
later). This means that the way the patch is setup currently `intrin.h` needs 
to be included before `cpuid.h`. The only way I see to fix this is to use the 
MSVC `__cpuidex` signature which drops the `static` keyword. This makes the 
implementation slightly different from the GCC implementation (where they 
include `static`), but it would allow us to declare the function outside of the 
preprocessor conditional in `cpuid.h` which would make `cpuid.h` 
include-order-agnostic again with respect to `intrin.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158348

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

[PATCH] D158348: [clang][X86] Add __cpuidex function to cpuid.h

[Aiden Grossman via Phabricator via cfe-commits]

aidengrossman created this revision.
Herald added a project: All.
aidengrossman requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, jplehr, sstefan1.
Herald added a project: clang.

MSVC has a __cpuidex function implemented to call the underlying cpuid
instruction which accepts a leaf, subleaf, and data array that the output
data is written into. This patch adds this functionality into clang
under the cpuid.h header. This also makes clang match GCC's behavior.
GCC has had __cpuidex in its cpuid.h since 2020.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158348

Files:
  clang/lib/Headers/cpuid.h
  clang/test/Headers/__cpuidex_conflict.c
  clang/test/Headers/cpuid.c


Index: clang/test/Headers/cpuid.c
===
--- clang/test/Headers/cpuid.c
+++ clang/test/Headers/cpuid.c
@@ -6,14 +6,19 @@
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A 
cpuid\0A xchgq %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
+// CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
 
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, 
i32 %{{[a-z0-9]+}})
+// CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 
"={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, 
i32 %{{[a-z0-9]+}})
 
 unsigned eax0, ebx0, ecx0, edx0;
 unsigned eax1, ebx1, ecx1, edx1;
 
+int cpuid_info[4];
+
 void test_cpuid(unsigned level, unsigned count) {
   __cpuid(level, eax1, ebx1, ecx1, edx1);
   __cpuid_count(level, count, eax0, ebx0, ecx0, edx0);
+  __cpuidex(cpuid_info, level, count);
 }
Index: clang/test/Headers/__cpuidex_conflict.c
===
--- /dev/null
+++ clang/test/Headers/__cpuidex_conflict.c
@@ -0,0 +1,22 @@
+// Make sure that __cpuidex in cpuid.h doesn't conflict with the MS
+// extensions built in by ensuring compilation succeeds:
+// RUN: %clang_cc1 %s -ffreestanding -fms-extensions -fms-compatibility \
+// RUN:  -fms-compatibility-version=19.00 -triple x86_64-pc-windows-msvc 
-emit-llvm -o -
+// %clang_cc1 %s -ffreestanding -triple x86_64-w64-windows-gnu -fms-extensions 
-emit-llvm -o -
+// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+
+typedef __SIZE_TYPE__ size_t;
+
+// We declare __cpuidex here as where the buitlin should be exposed (MSVC), the
+// declaration is in , but  is not available from all the
+// targets that are being tested here.
+void __cpuidex (int[4], int, int);
+
+#include 
+
+int cpuid_info[4];
+
+void test_cpuidex(unsigned level, unsigned count) {
+  __cpuidex(cpuid_info, level, count);
+}
+
Index: clang/lib/Headers/cpuid.h
===
--- clang/lib/Headers/cpuid.h
+++ clang/lib/Headers/cpuid.h
@@ -328,4 +328,14 @@
 return 1;
 }
 
+// In some configurations, __cpuidex is defined as a builtin (primarily
+// -fms-extensions) which will conflict with the __cpuidex definition below.
+#if !(__has_builtin(__cpuidex))
+static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
+{
+  __cpuid_count(__leaf, __subleaf, __cpu_info[0], __cpu_info[1], __cpu_info[2],
+__cpu_info[3]);
+}
+#endif
+
 #endif /* __CPUID_H */


Index: clang/test/Headers/cpuid.c
===
--- clang/test/Headers/cpuid.c
+++ clang/test/Headers/cpuid.c
@@ -6,14 +6,19 @@
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A cpuid\0A xchgq %rbx,${1:q}", "={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}", "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
+// CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}", "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
 
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", "={ax},={bx},={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", 

[PATCH] D158346: [clang-tidy] [readability-container-size-empty] improved check to detect missing usage of .empty() on string_literals Fixes #64547

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 marked 2 inline comments as done.
felix642 added a comment.

Hi @PiotrZSL, thank you for the feedback.

I have addressed most of your comments, but I'm not sure to understand what you 
mean by "Commit/Change description should be updated". Would you be able to 
clarify that for me?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158346

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

[PATCH] D158346: [clang-tidy] [readability-container-size-empty] improved check to detect missing usage of .empty() on string_literals Fixes #64547

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 updated this revision to Diff 551785.
felix642 added a comment.

Fixed tests and addressed comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158346

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -22,6 +22,10 @@
 };
 }
 
+namespace string_literals{
+string operator""s(const char *, size_t);
+}
+
 }
 
 template 
@@ -461,7 +465,7 @@
 constexpr Lazy L;
 static_assert(!L.size(), "");
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: the 'empty' method should be used
-// CHECK-MESSAGES: :90:18: note: method 'Lazy'::empty() defined here
+// CHECK-MESSAGES: :94:18: note: method 'Lazy'::empty() defined here
 // CHECK-FIXES: {{^}}static_assert(L.empty(), "");
 
 struct StructWithLazyNoexcept {
@@ -492,17 +496,17 @@
   if (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   CHECKSIZE(templated_container);
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
 }
 
@@ -575,74 +579,74 @@
   if (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   while (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}while (!templated_container.empty()){{$}}
 
   do {
   }
   while (templated_container.size());
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}while (!templated_container.empty());
 
   for (; templated_container.size();)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}for (; !templated_container.empty();){{$}}
 
   if (true && templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (true && !templated_container.empty()){{$}}
 
   if (true || templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if 

[PATCH] D153924: [OpenMP] Allow exceptions in target regions when offloading to GPUs

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.



Comment at: clang/test/OpenMP/amdgpu_exceptions.cpp:11
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device -fexceptions %s -emit-llvm -S -verify=with 
-Wopenmp-target-exception -o - &> /dev/null
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device %s -emit-llvm -S -verify=with 
-Wopenmp-target-exception -o - &> /dev/null
+

jdoerfert wrote:
> Can we use /dev/null? Do other tests use it? I would expect -analyze or sth 
> instead.
There's other tests, but I think that requires the shell or linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153924

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

[PATCH] D158338: [clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Yes doing some basic calculations would be probably the best, but for that we 
would need to implement some "calculator" or find some exist implementation 
some're in clang.
Even if it would support only basic operations like *


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158338

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

[PATCH] D158338: [clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 added a comment.

Hi @PiotrZSL thank you for taking the time to look at this revision.

I agree with you we should not silence a warning if no other tool can diagnose 
the issue.  I'm guessing that -Winteger-overflow does no trigger any warning on 
unsigned "overflow" since behavior is well defined in the standard :

> 46. This implies that unsigned arithmetic does not overflow because a result 
> that cannot be represented by the resulting unsigned integer type is reduced 
> modulo the number that is one greater than the largest value that can be 
> represented by the resulting unsigned integer type.

But on the other hand I have to agree with DenisYaroshevskiy that it is tedious 
to add a cast to every multiplication of integer literals on a large codebase. 
Especially when we know that those values do not overflow.

Maybe we should add some basic calculations when the operation is composed of 
integer literals, like you previously mentioned, to check if the operation 
actually overflows and print this warning if it does? 
In that case we could also improve the fix-it and suggest to add LU or U 
instead of static_cast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158338

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

[PATCH] D158346: [clang-tidy] [readability-container-size-empty] improved check to detect missing usage of .empty() on string_literals Fixes #64547

[Piotr Zegar via Phabricator via cfe-commits]

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

- Release notes entry is missing.
- Commit/Change description should be updated




Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:171
+  anyOf(stringLiteral(hasSize(0)),
+userDefinedLiteral(hasDescendant(stringLiteral(hasSize(0,
+cxxConstructExpr(isDefaultConstruction()),

Probably best would be to write matcher like this:

```
AST_MATCHER_P(UserDefinedLiteral, hasLiteral, Matcher, InnerMatcher) {
   if (const Expr* CookedLiteral  = Node.getCookedLiteral()) {
  return InnerMatcher.matches(CookedLiteral, Finder, Builder);
   }
   return false;
}
```
and use it instead of hasDescendant
```
userDefinedLiteral(hasLiteral(stringLiteral(hasSize(0,
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp:791
+}
\ No newline at end of file


Add this new line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158346

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

[PATCH] D158346: [clang-tidy] [readability-container-size-empty] improved check to detect missing usage of .empty() on string_literals Fixes #64547

[Félix-Antoine Constantin via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158346

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -22,6 +22,10 @@
 };
 }
 
+namespace string_literals{
+string operator""s(const char *, size_t);
+}
+
 }
 
 template 
@@ -770,3 +774,17 @@
 bool testIgnoredDummyType(const IgnoredDummyType& value) {
   return value == IgnoredDummyType();
 }
+
+bool testStringLiterals(const std::string& s)
+{
+  using namespace std::string_literals;
+  return s == ""s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be 
used
+  // CHECK-FIXES: {{^  }}return s.empty()
+}
+
+bool testNotEmptyStringLiterals(const std::string& s)
+{
+  using namespace std::string_literals;
+  return s == "foo"s;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -166,9 +166,11 @@
   this);
 
   // Comparison to empty string or empty constructor.
-  const auto WrongComparend = anyOf(
-  stringLiteral(hasSize(0)), cxxConstructExpr(isDefaultConstruction()),
-  cxxUnresolvedConstructExpr(argumentCountIs(0)));
+  const auto WrongComparend =
+  anyOf(stringLiteral(hasSize(0)),
+userDefinedLiteral(hasDescendant(stringLiteral(hasSize(0,
+cxxConstructExpr(isDefaultConstruction()),
+cxxUnresolvedConstructExpr(argumentCountIs(0)));
   // Match the object being compared.
   const auto STLArg =
   anyOf(unaryOperator(


Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -22,6 +22,10 @@
 };
 }
 
+namespace string_literals{
+string operator""s(const char *, size_t);
+}
+
 }
 
 template 
@@ -770,3 +774,17 @@
 bool testIgnoredDummyType(const IgnoredDummyType& value) {
   return value == IgnoredDummyType();
 }
+
+bool testStringLiterals(const std::string& s)
+{
+  using namespace std::string_literals;
+  return s == ""s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}return s.empty()
+}
+
+bool testNotEmptyStringLiterals(const std::string& s)
+{
+  using namespace std::string_literals;
+  return s == "foo"s;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -166,9 +166,11 @@
   this);
 
   // Comparison to empty string or empty constructor.
-  const auto WrongComparend = anyOf(
-  stringLiteral(hasSize(0)), cxxConstructExpr(isDefaultConstruction()),
-  cxxUnresolvedConstructExpr(argumentCountIs(0)));
+  const auto WrongComparend =
+  anyOf(stringLiteral(hasSize(0)),
+userDefinedLiteral(hasDescendant(stringLiteral(hasSize(0,
+cxxConstructExpr(isDefaultConstruction()),
+cxxUnresolvedConstructExpr(argumentCountIs(0)));
   // Match the object being compared.
   const auto STLArg =
   anyOf(unaryOperator(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

dexonsmith added inline comments.



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

MaskRay wrote:
> dexonsmith wrote:
> > Why would we want to use the old name here? An alias seems strictly better 
> > to me. 
> Making `overriding-t-option` an alias for `overriding-option` would make 
> `-Wno-overriding-t-option` applies to future overriding option diagnostics, 
> which is exactly what I want to avoid.
> 
I understand that you don't want `-t-` to apply to work on future overriding 
option diagnostics, but I think the platform divergence you're adding here is 
worse.

Having a few Darwin-specific options use `-Woverriding-t-option` (and 
everything else use `-Woverriding-option`) as the canonical spelling is hard to 
reason about for maintainers, and for users.

And might not users on other platforms have `-Woverriding-t-option` hardcoded 
in  build settings? (So @dblaikie's argument that we shouldn't arbitrarily make 
things hard for users would apply to all options?)

IMO, if we're not comfortable removing `-Woverriding-t-option` entirely, then 
it should live on as an alias (easy to reason about), not as 
canonical-in-special-cases (hard to reason about).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158301

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

[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL abandoned this revision.
PiotrZSL marked 4 inline comments as done.
PiotrZSL added a comment.

I do not plan to work on this check anymore, sorry for a wasted time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298

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

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL abandoned this revision.
PiotrZSL added a comment.

I do not plan to work on this check anymore, sorry for a wasted time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148461

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

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

PiotrZSL wrote:
> isuckatcs wrote:
> > isuckatcs wrote:
> > > PiotrZSL wrote:
> > > > isuckatcs wrote:
> > > > > isuckatcs wrote:
> > > > > > Please cover this line with both positive and negative test cases.
> > > > > > 
> > > > > > Also upon looking up both [[ 
> > > > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] 
> > > > > > (C++ 20 draft) and [[ https://github.com/cplusplus/draft/releases | 
> > > > > > N4917]] (C++ 23 draft), they both say for qualification conversion 
> > > > > > that 
> > > > > > 
> > > > > > 
> > > > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > > > 
> > > > > > Why are we not allowing them if the standard is at least C++ 20?
> > > > > Please resolve this thread.
> > > > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > > > throw_derivedfn_catch_basefn.
> > > > Negative is covered by throw_basefn_catch_basefn.
> > > > 
> > > > For members there are tests throw_basem_catch_basem, 
> > > > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > > > this correctly.
> > > > 
> > > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > > standard.
> > > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > > standard.
> > > 
> > > I don't know how to deal with this tbh. In a static analyzer we usually 
> > > want to consider what the standard says and not what a specific compiler 
> > > implementation does, as the compiler can still be buggy, lack the proper 
> > > support for the standard, etc.
> > > 
> > > Maybe we can add a FIXME here that explains, why this check is here and 
> > > that it's the opposite of what the standard says. Then later if it causes 
> > > issues, it will be easy to see why that happens.
> > > 
> > > @xazax.hun do you have a prefered method to deal with these situations in 
> > > the analyzer? If there is one, we could also try it here.
> > @PiotrZSL this is still unanswered. The standard says these are allowed, so 
> > according to the standard we model something wrong here, unless I'm 
> > misunderstanding something.
> If you talk about about '7.3.5 Qualification conversions' then this is only 
> about cv conversion, not about overall conversion from base to derived type, 
> and this is verify by SatisfiesCVRules lambda.
> 
> Without this line, check fails to detect issue in 
> throw_basefn_catch_derivedfn function under C++20.
> 
> Other interesting thing is that even this code:
> ```
>try {
>  throw ::foo;
>} catch(const void(baseMember::*)()) {
>}
> ```
> 
> Causes exception not to be catch, but if you remove const then it's catch.
> Same behavior is in GCC. I do not sure if 7.3.5 even apply to exceptions, and 
> personally I do not care what standard says, but I care how compilers work.
> 
> Right now both GCC and Clang in C++20 and C++17 work in same way, and this 
> ``if`` should be here so check would be align with how compiler works.
> There is
> This is anyway a insignificant scenario that most probably wont be used in 
> field.
> I do not sure if 7.3.5 (7.3.5 Qualification conversions) even apply to 
> exceptions



> [N4917 Post-Summer 2022 C++ working draft] 14.4 Handling an exception
> [...]
> - the handler is of type cv T or const T& where T is a pointer or 
> pointer-to-member type and E is a pointer or pointer-to-member type that can 
> be converted to T by one or more of
>  - a standard pointer conversion (7.3.12) not involving conversions to 
> pointers to private or protected or ambiguous classes
>  - a function pointer conversion (7.3.14)
>  - a qualification conversion (7.3.6), or <-- here it is
> [...]


>  I do not care what standard says, but I care how compilers work.

Then how would you reason about [[ https://godbolt.org/z/oTr3fbG8T | this ]] 
snippet? MSVC detects that the exception won't be caught and reports a warning, 
while gcc and clang fails to do it. In this case different compilers work 
differently. To be fair, the exception should be caught, since the thrown 
object is convertible to the handler (which is probably why gcc and clang don't 
report a warning).

> Without this line, check fails to detect issue in 
> throw_basefn_catch_derivedfn function under C++20.

Then do a proper investigation why that happens instead of adding random lines, 
which contradict what should happen, so that the test can pass. Probably the 
condition is wrong here and we should return `false` if `!isSameP_i` even if 
the standard is C++20 except for some cases with arrays. Look up the C++17 and 
the C++20 standards and compare them.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D153924: [OpenMP] Allow exceptions in target regions when offloading to GPUs

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I think this is basically done, one question though.




Comment at: clang/test/OpenMP/amdgpu_exceptions.cpp:11
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device -fexceptions %s -emit-llvm -S -verify=with 
-Wopenmp-target-exception -o - &> /dev/null
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device %s -emit-llvm -S -verify=with 
-Wopenmp-target-exception -o - &> /dev/null
+

Can we use /dev/null? Do other tests use it? I would expect -analyze or sth 
instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153924

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

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Cant help with the RT/LLVM parts, but the clang parts are pretty much right.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13278
+llvm::Value *
+CodeGenFunction::EmitX86CpuSupports(std::array FeatureMask) {
   Value *Result = Builder.getTrue();

Hmm... I guess size-wise this is on the edge of "const ref vs pass by value".  
I think its fine now, but 'next time' this grows we'll have to think about 
making this a const-ref.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13311
+  llvm::Constant *CpuFeatures2 =
+  CGM.CreateRuntimeVariable(ATy, "__cpu_features2");
+  cast(CpuFeatures2)->setDSOLocal(true);

This won't double-create this if used more than 1x, right?  There doesn't need 
to be something like GetOrCreate... here?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13272
+  uint64_t Mask = llvm::X86::getCpuSupportsMask(FeatureStrs);
+  uint32_t FeaturesMask[4] = {uint32_t(Mask), uint32_t(Mask >> 32), 0, 0};
+  return EmitX86CpuSupports(FeaturesMask);

MaskRay wrote:
> pengfei wrote:
> > Use `Lo_32(Mask)`, `Hi_32(Mask)`?
> Lo_32/Hi_32 are currently unused in clang/. Using this needs MathExtras.h. I 
> am unsure it's worthwhile..
I was previously using Lo_32/Hi_32 here (see the removed parts of 
EmitX86CpuSupports).  Were we using those transitively?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329

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

[PATCH] D158338: [clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In short I'm against simply ignoring literal calculations because this check is 
only thing that currently detect this issue: 
https://github.com/llvm/llvm-project/issues/64828


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158338

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

[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL planned changes to this revision.
PiotrZSL marked 4 inline comments as done.
PiotrZSL added a comment.

TODO: Resolve rest of review comments.




Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:14
 void ExceptionAnalyzer::ExceptionInfo::registerException(
-const Type *ExceptionType) {
+const Type *ExceptionType, const SourceLocation Loc) {
   assert(ExceptionType != nullptr && "Only valid types are accepted");

isuckatcs wrote:
> Is there any particular reason for taking `SourceLocation` by value? A `const 
> SourceLocation &` would avoid an unnecessary copy.
SourceLocation is 4 bytes in size (encoded as enum). In clang is always passed 
by value,



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:21
 void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
-const Throwables ) {
-  if (Exceptions.size() == 0)
+const Throwables , const SourceLocation Loc) {
+  if (Exceptions.empty())

isuckatcs wrote:
> Same question here regarding the argument type.
Same answer, 4 bytes in size.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+  for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
 }

isuckatcs wrote:
> There shouldn't be any invalid location in the container. An exception is 
> thrown by a statement, so we know where in source that happens.
yes and no, this is an safety... and we may still not see an "throw" if we call 
function that explicitly declare that throw some exceptions, but we do not have 
definition.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+(IgnoredTypes.count(TD->getName()) > 0)) {
+  It = ThrownExceptions.erase(It);
+  continue;

isuckatcs wrote:
> You are erasing something from a container on which you're iteration over. 
> Are you sure the iterators are not invalidated, when the internal 
> representation changes?
`erase` returns new valid iterator
https://en.cppreference.com/w/cpp/container/map/erase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298

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

[PATCH] D158338: [clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

I'm not sure if this is right fix, example:

  const std::size_t k1Tb = 1024 * 1024 * 1024 * 1024;

This is detected by -Winteger-overflow, but this:

  const  std::size_t k1Pb =  1024U * 1024U * 1024U * 1024U * 1024U;

is not detect by anything except this check, even that it overflow to 0.
Funny thing that if we change those unsigned to signed, then its detected by 
-Winteger-overflow

For me we shoudn't silent those issues, if someone want they they can always 
put nolint or explicit cast, we could consider adding some basic calculations 
just to verify if there will be no overflow, but still proper way would be to 
provide warning that would say, hey, write this as: "1024LU * 1024LU"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158338

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

[PATCH] D157750: Properly handle -fsplit-machine-functions for fatbinary compilation

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/fsplit-machine-functions-with-cuda-nvptx.c:9
+
+// Check that -fsplit-machine-functions is passed to both x86 and cuda 
compilation and does not cause driver error.
+// MFS2: -fsplit-machine-functions

MaskRay wrote:
> tra wrote:
> > shenhan wrote:
> > > tra wrote:
> > > > shenhan wrote:
> > > > > tra wrote:
> > > > > > We will still see a warning, right? So, for someone compiling with 
> > > > > > `-Werror` that's going to be a problem.
> > > > > > 
> > > > > > Also, if the warning is issued from the top-level driver, we may 
> > > > > > not even be able to suppress it when we disable splitting on GPU 
> > > > > > side with `-Xarch_device -fno-split-machine-functions`.
> > > > > > 
> > > > > > 
> > > > > > We will still see a warning, right?
> > > > > Yes, there still will be a warning. We've discussed it and we think 
> > > > > that pass -fsplit-machine-functions in this case is not a proper 
> > > > > usage and a warning is warranted, and it is not good that skip doing 
> > > > > split silently while uses explicitly ask for it.
> > > > > 
> > > > > > Also, if the warning is issued from the top-level driver
> > > > > The warning will not be issued from the top-level driver, it will be 
> > > > > issued when configuring optimization passes.
> > > > > So:
> > > > > 
> > > > > 
> > > > >   - -fsplit-machine-functions -Xarch_device 
> > > > > -fno-split-machine-functions
> > > > > Will enable MFS for host, disable MFS for gpus and without any 
> > > > > warnings.
> > > > > 
> > > > >   - -Xarch_host -fsplit-machine-functions
> > > > > The same as the above
> > > > > 
> > > > >   - -Xarch_host -fsplit-machine-functions -Xarch_device 
> > > > > -fno-split-machine-functions
> > > > > The same as the above
> > > > > 
> > > > > We've discussed it and we think that pass -fsplit-machine-functions 
> > > > > in this case is not a proper usage and a warning is warranted, and it 
> > > > > is not good that skip doing split silently while uses explicitly ask 
> > > > > for it.
> > > > 
> > > > I would agree with that assertion if we were talking exclusively about 
> > > > CUDA compilation.
> > > > However, a common real world use pattern is that the flags are set 
> > > > globally for all C++ compilations, and then CUDA compilations within 
> > > > the project need to do whatever they need to to keep things working. 
> > > > The original user intent was for the option to affect the host 
> > > > compilation. There's no inherent assumption that it will do anything 
> > > > useful for the GPU.
> > > > 
> > > > In number of similar cases in the past we did settle on silently 
> > > > ignoring some top-level flags that we do expect to encounter in real 
> > > > projects, but which made no sense for the GPU. E.g. sanitizers. If the 
> > > > project is built w/ sanitizer enabled, the idea is to sanitize the host 
> > > > code, The GPU code continues to be built w/o sanitizer enabled. 
> > > > 
> > > > Anyways, as long as we have a way to deal with it it's not a big deal 
> > > > one way or another.
> > > > 
> > > > > -fsplit-machine-functions -Xarch_device -fno-split-machine-functions
> > > > > Will enable MFS for host, disable MFS for gpus and without any 
> > > > > warnings.
> > > > 
> > > > OK. This will work.
> > > > 
> > > > 
> > > > In number of similar cases in the past we did settle on silently 
> > > > ignoring some top-level flags that we do expect to encounter in real 
> > > > projects, but which made no sense for the GPU. E.g. sanitizers. If the 
> > > > project is built w/ sanitizer enabled, the idea is to sanitize the host 
> > > > code, The GPU code continues to be built w/o sanitizer enabled.
> > > 
> > > Can I understand it this way - if the compiler is **only** building for 
> > > CPUs, then silently ignore any optimization flags is not a good behavior. 
> > > If the compiler is building CPUs and GPUs, it is still not a good 
> > > behavior to silently ignore optimization flags for CPUs, but it is 
> > > probably ok to silently ignore optimization flags for GPUs.
> > > 
> > > > OK. This will work.
> > > Thanks for confirming.
> > >  it is probably ok to silently ignore optimization flags for GPUs.
> > 
> > In this case, yes. 
> > 
> > I think the most consistent way to handle the situation is to keep the 
> > warning in place at cc1 compiler level, but change the driver behavior (and 
> > document it) so that it does not pass the splitting options to offloading 
> > sub-compilations. This way we'll do the sensible thing for the most common 
> > use case, yet would still warn if the user tries to enable the splitting 
> > where they should not (e.g. by using `-Xclang -fsplit-machine-functions` 
> > during CUDA compilation)
> > 
> > 
> > 
> > 
> There are excessive spaces before `%clang`. We should keep just one space: 
> `RUN: %clang`
I agree with @tra's analysis. Either do nothing on Clang side and requiring 

[PATCH] D157750: Properly handle -fsplit-machine-functions for fatbinary compilation

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Sorry, but I think this change should be reverted.

(a) `-fsplit-machine-functions` on an unsupported target now emits a warning 
instead of an error. This diverges from the regular expectation for 
target-specific features.

  % fclang --target=riscv64 -fsplit-machine-functions -c a.c
  warning: -fsplit-machine-functions is not valid for riscv64 [-Wbackend-plugin]

`warn_drv_for_elf_only` is not necessary. We typically just use 
`err_drv_unsupported_opt_for_target`.

(b) the test  needs substantial change (D158231 
)

(c) The error reporting should be on the driver side, not in backend. `void 
DiagnosticInfoMachineFunctionSplit::print(DiagnosticPrinter ) const` is not 
necessary. 
At the very least, it should not be in the generic `IR/DiagnosticInfo.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157750

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

[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:542
+   CaughtExceptions)) {
+  CaughtExceptions.emplace(CaughtType, SourceLocation());
   ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),

isuckatcs wrote:
> Here we rethrow something from a `try` block if I understand it correctly.
> 
> ```lang=c++
> void foo() {
> throw 3;
> }
> 
> void bar() {
> try {
> foo();
> } catch(short) {
> }
> }
> ```
> 
> The best way would be to set the `SourceLocation` to the point, where `foo()` 
> is called.
> I think you would need to create a new `struct` called `ThrowableInfo`, which 
> contains, 
> every information that we need to know about the `Throwable` e.g.: type, 
> location, parent
> context, etc. That would also be more extensible.
> 
> If there's no way to deduce this location for some reason, you can still set 
> the location 
> to the `try` block and create messages like `rethrown from try here`, etc. 
> Just don't create
> invalid `SourceLocation`s please, because besides them being incorrect, they 
> are also
> a source of bugs and crashes.
Also, is this branch tested? I don't see any new test case with a `try` block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298

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

[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp:88
+
+  for (auto [ThrowType, ThrowLoc] : AnalyzeResult.getExceptionTypes())
+diag(ThrowLoc, "may throw %0 here", DiagnosticIDs::Note)





Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:14
 void ExceptionAnalyzer::ExceptionInfo::registerException(
-const Type *ExceptionType) {
+const Type *ExceptionType, const SourceLocation Loc) {
   assert(ExceptionType != nullptr && "Only valid types are accepted");

Is there any particular reason for taking `SourceLocation` by value? A `const 
SourceLocation &` would avoid an unnecessary copy.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:21
 void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
-const Throwables ) {
-  if (Exceptions.size() == 0)
+const Throwables , const SourceLocation Loc) {
+  if (Exceptions.empty())

Same question here regarding the argument type.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:25
   Behaviour = State::Throwing;
-  ThrownExceptions.insert(Exceptions.begin(), Exceptions.end());
+  for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);





Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+  for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
 }

There shouldn't be any invalid location in the container. An exception is 
thrown by a statement, so we know where in source that happens.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:352
+
+  auto ShouldRemoveFnt = [HandlerTy, ](const Type *ExceptionTy) {
 CanQualType ExceptionCanTy = ExceptionTy->getCanonicalTypeUnqualified();

For a moment it wasn't entirely clear for me what `Fnt` meant, I suggest using 
naming like `FnType` or `FunctionType`.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:391
+return true;
+  return false;
 }





Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:408
 
-  for (const Type *T : TypesToDelete)
-ThrownExceptions.erase(T);
+CaughtExceptions.emplace(*It);
+It = ThrownExceptions.erase(It);

This introduces a side effect to this function and makes it do 2 things at the 
same time. Besides filtering the caught exceptions, it also collects and 
returns them through a reference parameter.

I don't mind the latter, but in that case I'd like to see the caught exceptions 
returned from the function as a part of the return statement. Also notice that 
if `CaughtExceptions` is not empty, `Result` is `true`, which makes the latter 
redundant. I suggest dropping the `bool` return type and returning the 
`Throwables` instead.





Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+(IgnoredTypes.count(TD->getName()) > 0)) {
+  It = ThrownExceptions.erase(It);
+  continue;

You are erasing something from a container on which you're iteration over. Are 
you sure the iterators are not invalidated, when the internal representation 
changes?



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:542
+   CaughtExceptions)) {
+  CaughtExceptions.emplace(CaughtType, SourceLocation());
   ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),

Here we rethrow something from a `try` block if I understand it correctly.

```lang=c++
void foo() {
throw 3;
}

void bar() {
try {
foo();
} catch(short) {
}
}
```

The best way would be to set the `SourceLocation` to the point, where `foo()` 
is called.
I think you would need to create a new `struct` called `ThrowableInfo`, which 
contains, 
every information that we need to know about the `Throwable` e.g.: type, 
location, parent
context, etc. That would also be more extensible.

If there's no way to deduce this location for some reason, you can still set 
the location 
to the `try` block and create messages like `rethrown from try here`, etc. Just 
don't create
invalid `SourceLocation`s please, because besides them being incorrect, they 
are also
a source of bugs and crashes.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:573
   Excs.getExceptionTypes(), CallStack));
-for (const Type *Throwable : Excs.getExceptionTypes()) {
+for (auto [Throwable, ThrowLoc] : Excs.getExceptionTypes()) {
   if (const auto 

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551776.
PiotrZSL marked an inline comment as done.
PiotrZSL added a comment.

Add throw_basefn_catch_const_basefn test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148461

Files:
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: { \
 // RUN: bugprone-exception-escape.IgnoredExceptions: 'ignored1,ignored2', \
 // RUN: bugprone-exception-escape.FunctionsThatShouldNotThrow: 'enabled1,enabled2,enabled3' \
 // RUN: }}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -420,6 +419,7 @@
 struct baseMember {
 int *iptr;
 virtual void foo(){};
+void boo(){};
 };
 
 struct derivedMember : baseMember {
@@ -441,6 +441,29 @@
   }
 }
 
+void throw_basefn_catch_const_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basefn_catch_const_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(const void(baseMember::*)()) {
+  }
+}
+
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
+void throw_basefn_via_derivedfn_catch_basefn() noexcept {
+  try {
+throw ::boo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' which should not throw exceptions
   try {
@@ -721,28 +744,3 @@
   test_basic_no_throw();
   test_basic_throw();
 }
-
-namespace PR55143 { namespace PR40583 {
-
-struct test_explicit_throw {
-test_explicit_throw() throw(int) { throw 42; }
-test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
-test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
-~test_explicit_throw() throw(int) { throw 42; }
-};
-
-struct test_implicit_throw {
-test_implicit_throw() { throw 42; }
-test_implicit_throw(const test_implicit_throw&) { throw 42; }
-test_implicit_throw(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
-test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
-test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
-~test_implicit_throw() { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions
-};
-
-}}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
@@ -29,3 +29,28 @@
   sub_throws() throw() : super_throws() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions
 };
+
+namespace PR55143 { namespace PR40583 {
+
+struct test_explicit_throw {
+test_explicit_throw() throw(int) { throw 42; }
+test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
+test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
+~test_explicit_throw() throw(int) { throw 42; }
+};
+
+struct test_implicit_throw {
+test_implicit_throw() { throw 42; }
+

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked 2 inline comments as done.
PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

isuckatcs wrote:
> isuckatcs wrote:
> > PiotrZSL wrote:
> > > isuckatcs wrote:
> > > > isuckatcs wrote:
> > > > > Please cover this line with both positive and negative test cases.
> > > > > 
> > > > > Also upon looking up both [[ 
> > > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] 
> > > > > (C++ 20 draft) and [[ https://github.com/cplusplus/draft/releases | 
> > > > > N4917]] (C++ 23 draft), they both say for qualification conversion 
> > > > > that 
> > > > > 
> > > > > 
> > > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > > 
> > > > > Why are we not allowing them if the standard is at least C++ 20?
> > > > Please resolve this thread.
> > > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > > throw_derivedfn_catch_basefn.
> > > Negative is covered by throw_basefn_catch_basefn.
> > > 
> > > For members there are tests throw_basem_catch_basem, 
> > > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > > this correctly.
> > > 
> > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > standard.
> > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > standard.
> > 
> > I don't know how to deal with this tbh. In a static analyzer we usually 
> > want to consider what the standard says and not what a specific compiler 
> > implementation does, as the compiler can still be buggy, lack the proper 
> > support for the standard, etc.
> > 
> > Maybe we can add a FIXME here that explains, why this check is here and 
> > that it's the opposite of what the standard says. Then later if it causes 
> > issues, it will be easy to see why that happens.
> > 
> > @xazax.hun do you have a prefered method to deal with these situations in 
> > the analyzer? If there is one, we could also try it here.
> @PiotrZSL this is still unanswered. The standard says these are allowed, so 
> according to the standard we model something wrong here, unless I'm 
> misunderstanding something.
If you talk about about '7.3.5 Qualification conversions' then this is only 
about cv conversion, not about overall conversion from base to derived type, 
and this is verify by SatisfiesCVRules lambda.

Without this line, check fails to detect issue in throw_basefn_catch_derivedfn 
function under C++20.

Other interesting thing is that even this code:
```
   try {
 throw ::foo;
   } catch(const void(baseMember::*)()) {
   }
```

Causes exception not to be catch, but if you remove const then it's catch.
Same behavior is in GCC. I do not sure if 7.3.5 even apply to exceptions, and 
personally I do not care what standard says, but I care how compilers work.

Right now both GCC and Clang in C++20 and C++17 work in same way, and this 
``if`` should be here so check would be align with how compiler works.
There is
This is anyway a insignificant scenario that most probably wont be used in 
field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148461

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

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13272
+  uint64_t Mask = llvm::X86::getCpuSupportsMask(FeatureStrs);
+  uint32_t FeaturesMask[4] = {uint32_t(Mask), uint32_t(Mask >> 32), 0, 0};
+  return EmitX86CpuSupports(FeaturesMask);

pengfei wrote:
> Use `Lo_32(Mask)`, `Hi_32(Mask)`?
Lo_32/Hi_32 are currently unused in clang/. Using this needs MathExtras.h. I am 
unsure it's worthwhile..



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13318-13320
+Value *Idxs[] = {Builder.getInt32(0), Builder.getInt32(i - 1)};
+Value *Features = Builder.CreateAlignedLoad(
+Int32Ty, Builder.CreateGEP(ATy, CpuFeatures2, Idxs),

pengfei wrote:
> Do we have problem when linking with old version of libgcc?
If we don't use `__cpu_features2[1..3]`, no problem with older libgcc.

The way GCC upgraded `__cpu_features2` to an array is compatible with scalar 
`__cpu_features2`.



Comment at: compiler-rt/lib/builtins/cpu_model.c:808
+  if (HasExtLeaf1) {
+if (ECX & 1)
+  setFeature(FEATURE_LAHF_LM);

pengfei wrote:
> pengfei wrote:
> > Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The 
> > same below.
> Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The 
> same below.
Thanks for the cpuid.h idea, but I think likely no. MSVC does not provide 
cpuid.h. It would be nice if we don't allow MSVC to build the file (we can say 
that `LLVM_ENABLE_PROJECTS='...compiler-rt'` is not allowed and 
LLVM_ENABLE_RUNTIMES or Clang should be used, but I think we are not there yet).

Unfortunately I am also unsure whether anyone uses MSVC to build this file, but 
there are Windows related macros like `FIXME: For MSVC, we should make a 
function pointer global in` in this file...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329

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

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 551773.
MaskRay marked 6 inline comments as done.
MaskRay added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/attr-target-clones.c
  clang/test/CodeGen/builtin-cpu-supports.c
  clang/test/Sema/attr-target-clones.c
  compiler-rt/lib/builtins/cpu_model.c
  llvm/lib/TargetParser/X86TargetParser.cpp

Index: llvm/lib/TargetParser/X86TargetParser.cpp
===
--- llvm/lib/TargetParser/X86TargetParser.cpp
+++ llvm/lib/TargetParser/X86TargetParser.cpp
@@ -317,6 +317,7 @@
 // listed here before, which means it doesn't support -march, -mtune and so on.
 // FIXME: Remove OnlyForCPUDispatchSpecific after all CPUs here support both
 // cpu_dispatch/specific() feature and -march, -mtune, and so on.
+// clang-format off
 constexpr ProcInfo Processors[] = {
  // Empty processor. Include X87 and CMPXCHG8 for backwards compatibility.
   { {""}, CK_None, ~0U, FeatureX87 | FeatureCMPXCHG8B, '\0', false },
@@ -482,13 +483,14 @@
   { {"znver3"}, CK_ZNVER3, FEATURE_AVX2, FeaturesZNVER3, '\0', false },
   { {"znver4"}, CK_ZNVER4, FEATURE_AVX512VBMI2, FeaturesZNVER4, '\0', false },
   // Generic 64-bit processor.
-  { {"x86-64"}, CK_x86_64, ~0U, FeaturesX86_64, '\0', false },
-  { {"x86-64-v2"}, CK_x86_64_v2, ~0U, FeaturesX86_64_V2, '\0', false },
-  { {"x86-64-v3"}, CK_x86_64_v3, ~0U, FeaturesX86_64_V3, '\0', false },
-  { {"x86-64-v4"}, CK_x86_64_v4, ~0U, FeaturesX86_64_V4, '\0', false },
+  { {"x86-64"}, CK_x86_64, FEATURE_SSE2 , FeaturesX86_64, '\0', false },
+  { {"x86-64-v2"}, CK_x86_64_v2, FEATURE_SSE4_2 , FeaturesX86_64_V2, '\0', false },
+  { {"x86-64-v3"}, CK_x86_64_v3, FEATURE_AVX2, FeaturesX86_64_V3, '\0', false },
+  { {"x86-64-v4"}, CK_x86_64_v4, FEATURE_AVX512VL, FeaturesX86_64_V4, '\0', false },
   // Geode processors.
   { {"geode"}, CK_Geode, ~0U, FeaturesGeode, '\0', false },
 };
+// clang-format on
 
 constexpr const char *NoTuneList[] = {"x86-64-v2", "x86-64-v3", "x86-64-v4"};
 
Index: compiler-rt/lib/builtins/cpu_model.c
===
--- compiler-rt/lib/builtins/cpu_model.c
+++ compiler-rt/lib/builtins/cpu_model.c
@@ -158,6 +158,19 @@
   FEATURE_AVX512BITALG,
   FEATURE_AVX512BF16,
   FEATURE_AVX512VP2INTERSECT,
+
+  FEATURE_CMPXCHG16B = 46,
+  FEATURE_F16C = 49,
+  FEATURE_LAHF_LM = 54,
+  FEATURE_LM,
+  FEATURE_WP,
+  FEATURE_LZCNT = 57,
+  FEATURE_MOVBE,
+
+  FEATURE_X86_64_BASELINE = 95,
+  FEATURE_X86_64_V2,
+  FEATURE_X86_64_V3,
+  FEATURE_X86_64_V4,
   CPU_FEATURE_MAX
 };
 
@@ -675,6 +688,7 @@
  unsigned *Features) {
   unsigned EAX = 0, EBX = 0;
 
+#define hasFeature(F) ((Features[F / 32] >> (F % 32)) & 1)
 #define setFeature(F)  \
   Features[F / 32] |= 1U << (F % 32)
 
@@ -695,14 +709,20 @@
 setFeature(FEATURE_SSSE3);
   if ((ECX >> 12) & 1)
 setFeature(FEATURE_FMA);
+  if ((ECX >> 13) & 1)
+setFeature(FEATURE_CMPXCHG16B);
   if ((ECX >> 19) & 1)
 setFeature(FEATURE_SSE4_1);
   if ((ECX >> 20) & 1)
 setFeature(FEATURE_SSE4_2);
+  if ((ECX >> 22) & 1)
+setFeature(FEATURE_MOVBE);
   if ((ECX >> 23) & 1)
 setFeature(FEATURE_POPCNT);
   if ((ECX >> 25) & 1)
 setFeature(FEATURE_AES);
+  if ((ECX >> 29) & 1)
+setFeature(FEATURE_F16C);
 
   // If CPUID indicates support for XSAVE, XRESTORE and AVX, and XGETBV
   // indicates that the AVX registers will be saved and restored on context
@@ -784,12 +804,39 @@
 
   bool HasExtLeaf1 = MaxExtLevel >= 0x8001 &&
  !getX86CpuIDAndInfo(0x8001, , , , );
-  if (HasExtLeaf1 && ((ECX >> 6) & 1))
-setFeature(FEATURE_SSE4_A);
-  if (HasExtLeaf1 && ((ECX >> 11) & 1))
-setFeature(FEATURE_XOP);
-  if (HasExtLeaf1 && ((ECX >> 16) & 1))
-setFeature(FEATURE_FMA4);
+  if (HasExtLeaf1) {
+if (ECX & 1)
+  setFeature(FEATURE_LAHF_LM);
+if ((ECX >> 5) & 1)
+  setFeature(FEATURE_LZCNT);
+if (((ECX >> 6) & 1))
+  setFeature(FEATURE_SSE4_A);
+if (((ECX >> 11) & 1))
+  setFeature(FEATURE_XOP);
+if (((ECX >> 16) & 1))
+  setFeature(FEATURE_FMA4);
+if (((EDX >> 29) & 1))
+  setFeature(FEATURE_LM);
+  }
+
+  if (hasFeature(FEATURE_LM) && hasFeature(FEATURE_SSE2)) {
+setFeature(FEATURE_X86_64_BASELINE);
+if (hasFeature(FEATURE_CMPXCHG16B) && hasFeature(FEATURE_POPCNT) &&
+hasFeature(FEATURE_LAHF_LM) && hasFeature(FEATURE_SSE4_2)) {
+  setFeature(FEATURE_X86_64_V2);
+  if (hasFeature(FEATURE_AVX2) && hasFeature(FEATURE_BMI) &&
+  hasFeature(FEATURE_BMI2) && hasFeature(FEATURE_F16C) &&
+  hasFeature(FEATURE_FMA) && 

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

isuckatcs wrote:
> PiotrZSL wrote:
> > isuckatcs wrote:
> > > isuckatcs wrote:
> > > > Please cover this line with both positive and negative test cases.
> > > > 
> > > > Also upon looking up both [[ 
> > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 
> > > > 20 draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] 
> > > > (C++ 23 draft), they both say for qualification conversion that 
> > > > 
> > > > 
> > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > 
> > > > Why are we not allowing them if the standard is at least C++ 20?
> > > Please resolve this thread.
> > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > throw_derivedfn_catch_basefn.
> > Negative is covered by throw_basefn_catch_basefn.
> > 
> > For members there are tests throw_basem_catch_basem, 
> > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > this correctly.
> > 
> > Tested this with GCC, and behavior is proper and independent to C++ 
> > standard.
> > Tested this with GCC, and behavior is proper and independent to C++ 
> > standard.
> 
> I don't know how to deal with this tbh. In a static analyzer we usually want 
> to consider what the standard says and not what a specific compiler 
> implementation does, as the compiler can still be buggy, lack the proper 
> support for the standard, etc.
> 
> Maybe we can add a FIXME here that explains, why this check is here and that 
> it's the opposite of what the standard says. Then later if it causes issues, 
> it will be easy to see why that happens.
> 
> @xazax.hun do you have a prefered method to deal with these situations in the 
> analyzer? If there is one, we could also try it here.
@PiotrZSL this is still unanswered. The standard says these are allowed, so 
according to the standard we model something wrong here, unless I'm 
misunderstanding something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148461

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



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

dexonsmith wrote:
> Why would we want to use the old name here? An alias seems strictly better to 
> me. 
Making `overriding-t-option` an alias for `overriding-option` would make 
`-Wno-overriding-t-option` applies to future overriding option diagnostics, 
which is exactly what I want to avoid.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158301

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

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

[Yuri Istomin via Phabricator via cfe-commits]

NekoCdr added a comment.

@angelomatnigoogle are you still working on this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141414

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

[PATCH] D158338: [clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 updated this revision to Diff 551771.
felix642 added a comment.

Fixed format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158338

Files:
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -107,10 +107,28 @@
 long n14(int a, int b, int c) {
   return a + b * c;
 }
+
 long n15(int a, int b, int c) {
   return a * b + c;
 }
 
+unsigned long n16()
+{
+  return (1024u) * 1024;
+}
+
+long n17(int a) {
+  return a + 1024 * 1024;
+}
+
+long n18(int a)
+{
+  return (a * 1024);
+  // CHECK-NOTES-ALL: :[[@LINE-1]]:11: warning: performing an implicit 
widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-ALL: :[[@LINE-2]]:11: note: make conversion explicit to 
silence this warning
+  // CHECK-NOTES-ALL: :[[@LINE-3]]:11: note: perform multiplication in a wider 
type
+}
+
 #ifdef __cplusplus
 template 
 T2 template_test(T1 a, T1 b) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@
   `, so that it does not warn
   on macros starting with underscore and lowercase letter.
 
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+  
`
+  check to ignore false-positives with integer literals.
+
 - Improved :doc:`bugprone-lambda-function-name
   ` check by adding option
   `IgnoreMacros` to ignore warnings in macros.
Index: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++ 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -34,6 +34,12 @@
   return BO->getLHS()->IgnoreParens();
 }
 
+static bool hasIntegerLiteralOperators(const Expr *E) {
+  const auto *BO = dyn_cast(E);
+  return isa(BO->getLHS()->IgnoreParenImpCasts()) &&
+ isa(BO->getRHS()->IgnoreParenImpCasts());
+}
+
 ImplicitWideningOfMultiplicationResultCheck::
 ImplicitWideningOfMultiplicationResultCheck(StringRef Name,
 ClangTidyContext *Context)
@@ -89,6 +95,10 @@
   if (!LHS)
 return;
 
+  // Widening multiplication on Integer Literals.
+  if (hasIntegerLiteralOperators(E))
+return;
+
   // Ok, looks like we should diagnose this.
   diag(E->getBeginLoc(), "performing an implicit widening conversion to type "
  "%0 of a multiplication performed in type %1")


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -107,10 +107,28 @@
 long n14(int a, int b, int c) {
   return a + b * c;
 }
+
 long n15(int a, int b, int c) {
   return a * b + c;
 }
 
+unsigned long n16()
+{
+  return (1024u) * 1024;
+}
+
+long n17(int a) {
+  return a + 1024 * 1024;
+}
+
+long n18(int a)
+{
+  return (a * 1024);
+  // CHECK-NOTES-ALL: :[[@LINE-1]]:11: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-ALL: :[[@LINE-2]]:11: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-ALL: :[[@LINE-3]]:11: note: perform multiplication in a wider type
+}
+
 #ifdef __cplusplus
 template 
 T2 template_test(T1 a, T1 b) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@
   `, so that it does not warn
   on macros starting with underscore and lowercase letter.
 
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+  `
+  check to ignore false-positives with integer literals.
+
 - Improved :doc:`bugprone-lambda-function-name
   ` check by adding option
   `IgnoreMacros` to ignore warnings in macros.
Index: 

[PATCH] D158338: [clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[Félix-Antoine Constantin via Phabricator via cfe-commits]

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

The following code is safe and should not trigger the warning
constexpr std::size_t k1Mb = 1024 * 1024;

Fixes #64732


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158338

Files:
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -107,10 +107,28 @@
 long n14(int a, int b, int c) {
   return a + b * c;
 }
+
 long n15(int a, int b, int c) {
   return a * b + c;
 }
 
+unsigned long n16()
+{
+  return (1024u) * 1024;
+}
+
+long n17(int a) {
+  return a + 1024 * 1024;
+}
+
+long n18(int a)
+{
+  return (a * 1024);
+  // CHECK-NOTES-ALL: :[[@LINE-1]]:11: warning: performing an implicit 
widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-ALL: :[[@LINE-2]]:11: note: make conversion explicit to 
silence this warning
+  // CHECK-NOTES-ALL: :[[@LINE-3]]:11: note: perform multiplication in a wider 
type
+}
+
 #ifdef __cplusplus
 template 
 T2 template_test(T1 a, T1 b) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@
   `, so that it does not warn
   on macros starting with underscore and lowercase letter.
 
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+  
`
+  check to ignore false-positives with integer literals.
+
 - Improved :doc:`bugprone-lambda-function-name
   ` check by adding option
   `IgnoreMacros` to ignore warnings in macros.
Index: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++ 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -34,6 +34,13 @@
   return BO->getLHS()->IgnoreParens();
 }
 
+static bool hasIntegerLiteralOperators(const Expr* E)
+{
+  const auto *BO = dyn_cast(E);
+  return isa(BO->getLHS()->IgnoreParenImpCasts()) &&
+  isa(BO->getRHS()->IgnoreParenImpCasts());
+}
+
 ImplicitWideningOfMultiplicationResultCheck::
 ImplicitWideningOfMultiplicationResultCheck(StringRef Name,
 ClangTidyContext *Context)
@@ -89,6 +96,10 @@
   if (!LHS)
 return;
 
+  // Widening multiplication on Integer Literals.
+  if(hasIntegerLiteralOperators(E))
+return;
+
   // Ok, looks like we should diagnose this.
   diag(E->getBeginLoc(), "performing an implicit widening conversion to type "
  "%0 of a multiplication performed in type %1")


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -107,10 +107,28 @@
 long n14(int a, int b, int c) {
   return a + b * c;
 }
+
 long n15(int a, int b, int c) {
   return a * b + c;
 }
 
+unsigned long n16()
+{
+  return (1024u) * 1024;
+}
+
+long n17(int a) {
+  return a + 1024 * 1024;
+}
+
+long n18(int a)
+{
+  return (a * 1024);
+  // CHECK-NOTES-ALL: :[[@LINE-1]]:11: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-ALL: :[[@LINE-2]]:11: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-ALL: :[[@LINE-3]]:11: note: perform multiplication in a wider type
+}
+
 #ifdef __cplusplus
 template 
 T2 template_test(T1 a, T1 b) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@
   `, so that it does not warn
   on macros starting with underscore and lowercase letter.
 
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet marked an inline comment as done.
hazohelet added inline comments.



Comment at: libcxx/include/__type_traits/is_constant_evaluated.h:31
+  return false;
+#endif
 }

philnik wrote:
> Mordante wrote:
> > hazohelet wrote:
> > > Mordante wrote:
> > > > Why is this needed? Does this mean the builtin will fail in C++03 mode?
> > > `__libcpp_is_constant_evaluated` always returns false under C++03 or 
> > > earlier, where constexpr isn't available. This is a fix to run libc++ 
> > > tests without seeing warnings from clang with this patch.
> > > (Live demo: https://godbolt.org/z/oszebM5o5)
> > I see can you add a comment in that regard? To me it looks like the builtin 
> > fails in C++03, where returning false indeed is always the right answer.
> https://godbolt.org/z/bafeeY7b7 shows that 
> `__builtin_is_constant_evaluated()` doesn't always return false in C++03, so 
> this change seems wrong to me.
This is NFC.
The problem is that `__libcpp_is_constant_evaluated()` has different semantics 
from `__builtin_is_constant_evaluated()` in C++03.
The cause of this tautological-falsity here is whether 
`__libcpp_is_constant_evaluated()` is `constexpr`-qualified or not.
`_LIBCPP_CONSTEXPR` macro expands to `constexpr` since C++11, but in pre-C++11 
mode, it expands to nothing.
So in C++03 mode this function is something like `bool 
__libcpp_is_constant_evaluated() { return __builtin_is_constant_evaluated(); 
}`, where it is tautologically-false because it is not constexpr-qualified.

As you mentioned, `__builtin_is_constant_evaluated` does not change its 
semantics depending on C++ versions.
However, `__libcpp_is_constant_evaluated()` always returns false in C++03 as in 
https://godbolt.org/z/oszebM5o5


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

https://reviews.llvm.org/D155064

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[Yurong via Phabricator via cfe-commits]

yronglin added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

yronglin wrote:
> dblaikie wrote:
> > yronglin wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > yronglin wrote:
> > > > > > dblaikie wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > Could/should we add some error checking in the ctor to 
> > > > > > > > > > assert that we don't overflow these longer values/just hit 
> > > > > > > > > > the bug later on?
> > > > > > > > > > 
> > > > > > > > > > (& could we use `unsigned short` here rather than 
> > > > > > > > > > bitfields?)
> > > > > > > > > We've already got them packed in with other bit-fields from 
> > > > > > > > > the expression bits, so I think it's reasonable to continue 
> > > > > > > > > the pattern of using bit-fields (that way we don't 
> > > > > > > > > accidentally end up with padding between the unnamed bits at 
> > > > > > > > > the start and the named bits in this object).
> > > > > > > > > 
> > > > > > > > > I think adding some assertions would not be a bad idea as a 
> > > > > > > > > follow-up.
> > > > > > > > Maybe some unconditional (rather than only in asserts builds) 
> > > > > > > > error handling? (report_fatal_error, if this is low priority 
> > > > > > > > enough to not have an elegant failure mode, but something where 
> > > > > > > > we don't just overflow and carry on would be good... )
> > > > > > > Ping on this? I worry this code has just punted the same bug 
> > > > > > > further down, but not plugged the hole/ensured we don't overflow 
> > > > > > > on novel/larger inputs.
> > > > > > Sorry for the late reply, I was looking through the emails and 
> > > > > > found this. I agree add some assertions to check the value is a 
> > > > > > good idea, It's easy to help people catch bugs, at least with when 
> > > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one 
> > > > > > thing that worries me is that, in ASTReader, we access this field 
> > > > > > directly, not through the constructor or accessor, and we have to 
> > > > > > add assertions everywhere. 
> > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > I don't think we have to add too many assertions. As best I can tell, 
> > > > > we'll need one in each of the `PseudoObjectExpr` constructors and one 
> > > > > in `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only 
> > > > > places we assign a value into the bit-field. Three assertions isn't a 
> > > > > lot, but if we're worried, we could add a setter method that does the 
> > > > > assertion and use the setter in all three places.
> > > > My concern wasn't (well, wasn't entirely) about adding more assertions 
> > > > - but about having a reliable error here. The patch only makes the 
> > > > sizes larger, but doesn't have a hard-stop in case those sizes are 
> > > > exceeded again (which, admittedly, is much harder to do - maybe it's 
> > > > totally unreachable now, for all practical purposes?) 
> > > > 
> > > > I suspect with more carefully constructed recursive inputs could still 
> > > > reach the higher limit & I think it'd be good to fail hard in that case 
> > > > in some way? (it's probably rare enough that a report_fatal_error would 
> > > > be not-the-worst-thing-ever)
> > > > 
> > > > But good assertions would be nice too (the old code only failed when 
> > > > you hit /exactly/ on just the overflow value, and any more than that 
> > > > the wraparound would not crash/fail, but misbehave) - I did add the 
> > > > necessary assertion to ArrayRef (begin <= end) which would've helped 
> > > > detect this more reliably, but some assert checking for overflow in the 
> > > > ctor would be good too (with all the usual nuance/care in checking for 
> > > > overflow) - unless we're going to make that into a fatal or other real 
> > > > error.
> > > Sorry for the very late reply. I have no preference between assertion and 
> > > `llvm_unreachable`, if error then fail fast is looks good. I have a patch 
> > > D158296 to add assertion.
> > Thanks for the assertions - though they still haven't met my main concern 
> > that this should have a hard failure even in a non-assertions build.
> > 
> > I know we don't have a perfect plan/policy for these sort of "run out of 
> > resources/hit a representational limit" issues (at least I don't think we 
> > do... do we, @aaron.ballman ? I know we have some limits (recursion, 
> > template expansion, etc) but they're fairly specific/aren't about every 
> > 

[PATCH] D158152: [clang-tidy]mark record initList as non-const param

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:230
 
+- Improved :doc:`readability-non-const-parameter.cpp
+  ` check to ignore




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158152

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

dblaikie wrote:
> yronglin wrote:
> > dblaikie wrote:
> > > aaron.ballman wrote:
> > > > yronglin wrote:
> > > > > dblaikie wrote:
> > > > > > dblaikie wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > Could/should we add some error checking in the ctor to assert 
> > > > > > > > > that we don't overflow these longer values/just hit the bug 
> > > > > > > > > later on?
> > > > > > > > > 
> > > > > > > > > (& could we use `unsigned short` here rather than bitfields?)
> > > > > > > > We've already got them packed in with other bit-fields from the 
> > > > > > > > expression bits, so I think it's reasonable to continue the 
> > > > > > > > pattern of using bit-fields (that way we don't accidentally end 
> > > > > > > > up with padding between the unnamed bits at the start and the 
> > > > > > > > named bits in this object).
> > > > > > > > 
> > > > > > > > I think adding some assertions would not be a bad idea as a 
> > > > > > > > follow-up.
> > > > > > > Maybe some unconditional (rather than only in asserts builds) 
> > > > > > > error handling? (report_fatal_error, if this is low priority 
> > > > > > > enough to not have an elegant failure mode, but something where 
> > > > > > > we don't just overflow and carry on would be good... )
> > > > > > Ping on this? I worry this code has just punted the same bug 
> > > > > > further down, but not plugged the hole/ensured we don't overflow on 
> > > > > > novel/larger inputs.
> > > > > Sorry for the late reply, I was looking through the emails and found 
> > > > > this. I agree add some assertions to check the value is a good idea, 
> > > > > It's easy to help people catch bugs, at least with when 
> > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one 
> > > > > thing that worries me is that, in ASTReader, we access this field 
> > > > > directly, not through the constructor or accessor, and we have to add 
> > > > > assertions everywhere. 
> > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > I don't think we have to add too many assertions. As best I can tell, 
> > > > we'll need one in each of the `PseudoObjectExpr` constructors and one 
> > > > in `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only 
> > > > places we assign a value into the bit-field. Three assertions isn't a 
> > > > lot, but if we're worried, we could add a setter method that does the 
> > > > assertion and use the setter in all three places.
> > > My concern wasn't (well, wasn't entirely) about adding more assertions - 
> > > but about having a reliable error here. The patch only makes the sizes 
> > > larger, but doesn't have a hard-stop in case those sizes are exceeded 
> > > again (which, admittedly, is much harder to do - maybe it's totally 
> > > unreachable now, for all practical purposes?) 
> > > 
> > > I suspect with more carefully constructed recursive inputs could still 
> > > reach the higher limit & I think it'd be good to fail hard in that case 
> > > in some way? (it's probably rare enough that a report_fatal_error would 
> > > be not-the-worst-thing-ever)
> > > 
> > > But good assertions would be nice too (the old code only failed when you 
> > > hit /exactly/ on just the overflow value, and any more than that the 
> > > wraparound would not crash/fail, but misbehave) - I did add the necessary 
> > > assertion to ArrayRef (begin <= end) which would've helped detect this 
> > > more reliably, but some assert checking for overflow in the ctor would be 
> > > good too (with all the usual nuance/care in checking for overflow) - 
> > > unless we're going to make that into a fatal or other real error.
> > Sorry for the very late reply. I have no preference between assertion and 
> > `llvm_unreachable`, if error then fail fast is looks good. I have a patch 
> > D158296 to add assertion.
> Thanks for the assertions - though they still haven't met my main concern 
> that this should have a hard failure even in a non-assertions build.
> 
> I know we don't have a perfect plan/policy for these sort of "run out of 
> resources/hit a representational limit" issues (at least I don't think we 
> do... do we, @aaron.ballman ? I know we have some limits (recursion, template 
> expansion, etc) but they're fairly specific/aren't about every possible case 
> of integer overflow in some representational element, etc) but we've seen 
> this one is pretty reachable. 
> 
> Here's a test case 

[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551761.
PiotrZSL added a comment.

Rebase + Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-rethrow.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -9,6 +9,7 @@
 struct throwing_destructor {
   ~throwing_destructor() {
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~throwing_destructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
 throw 1;
   }
 };
@@ -16,6 +17,7 @@
 struct throwing_move_constructor {
   throwing_move_constructor(throwing_move_constructor&&) {
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'throwing_move_constructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
 throw 1;
   }
 };
@@ -23,12 +25,14 @@
 struct throwing_move_assignment {
   throwing_move_assignment& operator=(throwing_move_assignment&&) {
 // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
 throw 1;
   }
 };
 
 void throwing_noexcept() noexcept {
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: note: may throw 'int' here
   throw 1;
 }
 
@@ -42,6 +46,7 @@
 
 void throw_and_catch_some(int n) noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch_some' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
   try {
 if (n) throw 1;
 throw 1.1;
@@ -70,6 +75,7 @@
 
 void throw_and_rethrow() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_rethrow' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+2]]:5: note: may throw 'int' here
   try {
 throw 1;
   } catch(int &) {
@@ -79,6 +85,7 @@
 
 void throw_catch_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_throw' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'int' here
   try {
 throw 1;
   } catch(int &) {
@@ -88,6 +95,7 @@
 
 void throw_catch_rethrow_the_rest(int n) noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_rethrow_the_rest' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
   try {
 if (n) throw 1;
 throw 1.1;
@@ -120,6 +128,7 @@
 
 void throw_catch_multi_ptr_1() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_multi_ptr_1' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'char **' here
   try {
 char **p = 0;
 throw p;
@@ -165,6 +174,7 @@
 
 void throw_c_catch_pointer() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
   try {
 int a = 1;
 const int *p = 
@@ -174,6 +184,7 @@
 
 void throw_c_catch_pointer_v() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer_v' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
   try {
 int a = 1;
 const int *p = 
@@ -183,6 +194,7 @@
 
 void throw_v_catch_pointer() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_v_catch_pointer' which should not throw exceptions
+  // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'volatile int *' here
   try {
 int a = 1;
 volatile int *p = 

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

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

dexonsmith added inline comments.



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

Why would we want to use the old name here? An alias seems strictly better to 
me. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158301

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

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551760.
PiotrZSL added a comment.

Rebase + Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148461

Files:
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: { \
 // RUN: bugprone-exception-escape.IgnoredExceptions: 'ignored1,ignored2', \
 // RUN: bugprone-exception-escape.FunctionsThatShouldNotThrow: 'enabled1,enabled2,enabled3' \
 // RUN: }}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -420,6 +419,7 @@
 struct baseMember {
 int *iptr;
 virtual void foo(){};
+void boo(){};
 };
 
 struct derivedMember : baseMember {
@@ -441,6 +441,21 @@
   }
 }
 
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
+void throw_basefn_via_derivedfn_catch_basefn() noexcept {
+  try {
+throw ::boo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' which should not throw exceptions
   try {
@@ -721,28 +736,3 @@
   test_basic_no_throw();
   test_basic_throw();
 }
-
-namespace PR55143 { namespace PR40583 {
-
-struct test_explicit_throw {
-test_explicit_throw() throw(int) { throw 42; }
-test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
-test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
-~test_explicit_throw() throw(int) { throw 42; }
-};
-
-struct test_implicit_throw {
-test_implicit_throw() { throw 42; }
-test_implicit_throw(const test_implicit_throw&) { throw 42; }
-test_implicit_throw(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
-test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
-test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
-~test_implicit_throw() { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions
-};
-
-}}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
@@ -29,3 +29,28 @@
   sub_throws() throw() : super_throws() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions
 };
+
+namespace PR55143 { namespace PR40583 {
+
+struct test_explicit_throw {
+test_explicit_throw() throw(int) { throw 42; }
+test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
+test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
+~test_explicit_throw() throw(int) { throw 42; }
+};
+
+struct test_implicit_throw {
+test_implicit_throw() { throw 42; }
+test_implicit_throw(const test_implicit_throw&) { throw 42; }
+test_implicit_throw(test_implicit_throw&&) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
+test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
+

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Nikolas Klauser via Phabricator via cfe-commits]

philnik added inline comments.



Comment at: libcxx/include/__type_traits/is_constant_evaluated.h:31
+  return false;
+#endif
 }

Mordante wrote:
> hazohelet wrote:
> > Mordante wrote:
> > > Why is this needed? Does this mean the builtin will fail in C++03 mode?
> > `__libcpp_is_constant_evaluated` always returns false under C++03 or 
> > earlier, where constexpr isn't available. This is a fix to run libc++ tests 
> > without seeing warnings from clang with this patch.
> > (Live demo: https://godbolt.org/z/oszebM5o5)
> I see can you add a comment in that regard? To me it looks like the builtin 
> fails in C++03, where returning false indeed is always the right answer.
https://godbolt.org/z/bafeeY7b7 shows that `__builtin_is_constant_evaluated()` 
doesn't always return false in C++03, so this change seems wrong to me.


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

https://reviews.llvm.org/D155064

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

[PATCH] D158008: [AArch64] Add patterns for FMADD, FMSUB

[OverMighty via Phabricator via cfe-commits]

overmighty added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:5393
+  (EXTRACT_SUBREG V128:$Rn, hsub), FPR16:$Rm, FPR16:$Ra)>;
+  }
+

dzhidzhoev wrote:
> BTW, these lines add some patterns for fnmadd. Could you add some tests for 
> them? Or are they already covered by existing ones?
The patterns for `FNMADD` and `FNMSUB` are useless as there are no Neon vector 
equivalents of these instructions. Should I add a template argument to 
`ThreeOperandFPData` to prevent the patterns from being generated for them?

I previously had patterns in AArch64InstrInfo.td for `FMADD` and `FMSUB` only 
and it was suggested to me to move them to `ThreeOperandFPData`: 
https://reviews.llvm.org/D153207?id=532433#inline-1481961. I was asked to split 
that part of my previous patch into a second (this) one.



Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:5418
+  (EXTRACT_SUBREG V128:$Rn, dsub), FPR64:$Rm, FPR64:$Ra)>;
 }
 

dzhidzhoev wrote:
> Is it possible to use extractelt here? Since vector_extract is marked as 
> deprecated in `TargetSelectionDAG.td`
I saw the comment marking it as deprecated but I also saw new commits using 
`vector_extract`. I should have asked for clarification earlier. Is 
`vector_extract` truly deprecated? Should I change patterns I added in previous 
patches to use `extractelt` too?


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

https://reviews.llvm.org/D158008

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

[PATCH] D144429: [clang-tidy] Add bugprone-chained-comparison check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551758.
PiotrZSL added a comment.

Rebase + Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144429

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
@@ -0,0 +1,91 @@
+// RUN: %check_clang_tidy -std=c++98-or-later %s bugprone-chained-comparison %t
+
+void badly_chained_1(int x, int y, int z)
+{
+bool result = x < y < z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_2(int x, int y, int z)
+{
+bool result = x <= y <= z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a <= b <= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_3(int x, int y, int z)
+{
+bool result = x > y > z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a > b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_4(int x, int y, int z)
+{
+bool result = x >= y >= z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a >= b >= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_5(int x, int y, int z)
+{
+bool result = x == y != z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b != c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_6(bool x, bool y, bool z)
+{
+bool result = x != y == z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a != b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_multiple(bool a, bool b, bool c, bool d, bool e, bool f, bool g, bool h)
+{
+bool result = a == b == c == d == e == f == g == h;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_limit(bool v[29])
+{
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h == i == j == k == l == m == n == o == p == q == r == s == t == u == v == w == x == y == z ...' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+bool result = v[0] == v[1] == v[2] == v[3] == v[4] == v[5] == v[6] == v[7] ==
+  v[8] == v[9] == v[10] == v[11] == v[12] == v[13] == v[14] ==
+  v[15] == v[16] == v[17] == v[18] == v[19] == v[20] == v[21] ==
+  v[22] == v[23] == v[24] == v[25] == v[26] == v[27] == v[28];
+
+}
+
+void badly_chained_parens2(int x, int y, int z, int t, int a, int b)
+{
+bool result = (x < y) < (z && t) > (a == b);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_inner(int x, int y, int z, int t, int u)
+{
+bool result = x && y < z < t && u;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void properly_chained_1(int x, int y, int z)
+{
+bool 

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551757.
PiotrZSL added a comment.

Rebase + Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56644

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
  
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -105,9 +105,13 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
@@ -124,11 +128,19 @@
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
+  if ((str + str2).length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
 ;
@@ -140,7 +152,11 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
+  if (wstr.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == L"")
 ;
@@ -149,7 +165,7 @@
   std::vector vect;
   if (vect.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect == std::vector())
 ;
@@ -504,6 +520,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
+  std::basic_string s;
+  if (s.size())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
+  if (s.length())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
 }
 
 void g() {
@@ -753,7 +780,7 @@
   return value.size() == 0U;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :731:8: note: method 'array'::empty() defined here
+// CHECK-MESSAGES: :[[@LINE-25]]:8: note: method 'array'::empty() defined here
 }
 
 bool testArrayCompareToEmpty(const Array& value) {
@@ -764,7 +791,7 @@
   return value == DummyType();
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 

[PATCH] D158008: [AArch64] Add patterns for FMADD, FMSUB

[OverMighty via Phabricator via cfe-commits]

overmighty updated this revision to Diff 551754.
overmighty added a reviewer: dzhidzhoev.
overmighty added a comment.

- Rebase new upstream commits.
- Replace usage of deprecated `vector_extract` with `extractelt` in new 
patterns.
- Add `f16` operators before `EXTRACT_SUBREG`s in new `*Hrrr` instruction 
pattern results to fix crashing tests.


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

https://reviews.llvm.org/D158008

Files:
  clang/test/CodeGen/aarch64-neon-scalar-x-indexed-elem-constrained.c
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/test/CodeGen/AArch64/complex-deinterleaving-f16-mul.ll
  llvm/test/CodeGen/AArch64/fp16_intrinsic_lane.ll
  llvm/test/CodeGen/AArch64/neon-scalar-by-elem-fma.ll

Index: llvm/test/CodeGen/AArch64/neon-scalar-by-elem-fma.ll
===
--- llvm/test/CodeGen/AArch64/neon-scalar-by-elem-fma.ll
+++ llvm/test/CodeGen/AArch64/neon-scalar-by-elem-fma.ll
@@ -7,56 +7,132 @@
 declare float @llvm.experimental.constrained.fma.f32(float, float, float, metadata, metadata)
 declare double @llvm.experimental.constrained.fma.f64(double, double, double, metadata, metadata)
 
-define float @test_fmla_ss4S(float %a, float %b, <4 x float> %v) {
-  ; CHECK-LABEL: test_fmla_ss4S
+define float @test_fmla_ss4S_0(float %a, float %b, <4 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss4S_0
+  ; CHECK: fmadd s0, s1, s2, s0
+  %tmp1 = extractelement <4 x float> %v, i32 0
+  %tmp2 = call float @llvm.fma.f32(float %b, float %tmp1, float %a)
+  ret float %tmp2
+}
+
+define float @test_fmla_ss4S_0_swap(float %a, float %b, <4 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss4S_0_swap
+  ; CHECK: fmadd s0, s2, s1, s0
+  %tmp1 = extractelement <4 x float> %v, i32 0
+  %tmp2 = call float @llvm.fma.f32(float %tmp1, float %b, float %a)
+  ret float %tmp2
+}
+
+define float @test_fmla_ss4S_3(float %a, float %b, <4 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss4S_3
   ; CHECK: fmla {{s[0-9]+}}, {{s[0-9]+}}, {{v[0-9]+}}.s[3]
   %tmp1 = extractelement <4 x float> %v, i32 3
   %tmp2 = call float @llvm.fma.f32(float %b, float %tmp1, float %a)
   ret float %tmp2
 }
 
-define float @test_fmla_ss4S_swap(float %a, float %b, <4 x float> %v) {
-  ; CHECK-LABEL: test_fmla_ss4S_swap
+define float @test_fmla_ss4S_3_swap(float %a, float %b, <4 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss4S_3_swap
   ; CHECK: fmla {{s[0-9]+}}, {{s[0-9]+}}, {{v[0-9]+}}.s[3]
   %tmp1 = extractelement <4 x float> %v, i32 3
   %tmp2 = call float @llvm.fma.f32(float %tmp1, float %a, float %a)
   ret float %tmp2
 }
 
-define float @test_fmla_ss2S(float %a, float %b, <2 x float> %v) {
-  ; CHECK-LABEL: test_fmla_ss2S
+define float @test_fmla_ss2S_0(float %a, float %b, <2 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss2S_0
+  ; CHECK: fmadd s0, s1, s2, s0
+  %tmp1 = extractelement <2 x float> %v, i32 0
+  %tmp2 = call float @llvm.fma.f32(float %b, float %tmp1, float %a)
+  ret float %tmp2
+}
+
+define float @test_fmla_ss2S_0_swap(float %a, float %b, <2 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss2S_0_swap
+  ; CHECK: fmadd s0, s2, s1, s0
+  %tmp1 = extractelement <2 x float> %v, i32 0
+  %tmp2 = call float @llvm.fma.f32(float %tmp1, float %b, float %a)
+  ret float %tmp2
+}
+
+define float @test_fmla_ss2S_1(float %a, float %b, <2 x float> %v) {
+  ; CHECK-LABEL: test_fmla_ss2S_1
   ; CHECK: fmla {{s[0-9]+}}, {{s[0-9]+}}, {{v[0-9]+}}.s[1]
   %tmp1 = extractelement <2 x float> %v, i32 1
   %tmp2 = call float @llvm.fma.f32(float %b, float %tmp1, float %a)
   ret float %tmp2
 }
 
-define double @test_fmla_ddD(double %a, double %b, <1 x double> %v) {
-  ; CHECK-LABEL: test_fmla_ddD
-  ; CHECK: {{fmla d[0-9]+, d[0-9]+, v[0-9]+.d\[0]|fmadd d[0-9]+, d[0-9]+, d[0-9]+, d[0-9]+}}
+define double @test_fmla_ddD_0(double %a, double %b, <1 x double> %v) {
+  ; CHECK-LABEL: test_fmla_ddD_0
+  ; CHECK: fmadd d0, d1, d2, d0
   %tmp1 = extractelement <1 x double> %v, i32 0
   %tmp2 = call double @llvm.fma.f64(double %b, double %tmp1, double %a)
   ret double %tmp2
 }
 
-define double @test_fmla_dd2D(double %a, double %b, <2 x double> %v) {
-  ; CHECK-LABEL: test_fmla_dd2D
+define double @test_fmla_ddD_0_swap(double %a, double %b, <1 x double> %v) {
+  ; CHECK-LABEL: test_fmla_ddD_0_swap
+  ; CHECK: fmadd d0, d2, d1, d0
+  %tmp1 = extractelement <1 x double> %v, i32 0
+  %tmp2 = call double @llvm.fma.f64(double %tmp1, double %b, double %a)
+  ret double %tmp2
+}
+
+define double @test_fmla_dd2D_0(double %a, double %b, <2 x double> %v) {
+  ; CHECK-LABEL: test_fmla_dd2D_0
+  ; CHECK: fmadd d0, d1, d2, d0
+  %tmp1 = extractelement <2 x double> %v, i32 0
+  %tmp2 = call double @llvm.fma.f64(double %b, double %tmp1, double %a)
+  ret double %tmp2
+}
+
+define double @test_fmla_dd2D_0_swap(double %a, double %b, <2 x double> %v) {
+  ; CHECK-LABEL: test_fmla_dd2D_0_swap
+  ; CHECK: fmadd d0, d2, d1, d0
+  %tmp1 = extractelement <2 x double> %v, i32 0
+  %tmp2 = call double @llvm.fma.f64(double 

[PATCH] D158279: clang: Make rewrite-includes-macros.cpp runnable on non-Win

[Nico Weber 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 rG960881a7f32a: clang: Make rewrite-includes-macros.cpp 
runnable on non-Win (authored by thakis).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158279

Files:
  clang/test/Frontend/rewrite-includes-macros.cpp


Index: clang/test/Frontend/rewrite-includes-macros.cpp
===
--- clang/test/Frontend/rewrite-includes-macros.cpp
+++ clang/test/Frontend/rewrite-includes-macros.cpp
@@ -1,7 +1,8 @@
-// REQUIRES: system-windows
-// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c -Xclang 
-verify /Tp -
+// RUN: %clang_cl /E -Xclang -frewrite-includes -- %s | %clang_cl /c -Xclang 
-verify /Tp -
 // expected-no-diagnostics
 
+// This test uses dos-style \r\n line endings.
+// Make sure your editor doesn't rewrite them to unix-style \n line endings.
 int foo();
 int bar();
 #define HELLO \


Index: clang/test/Frontend/rewrite-includes-macros.cpp
===
--- clang/test/Frontend/rewrite-includes-macros.cpp
+++ clang/test/Frontend/rewrite-includes-macros.cpp
@@ -1,7 +1,8 @@
-// REQUIRES: system-windows
-// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c -Xclang -verify /Tp -
+// RUN: %clang_cl /E -Xclang -frewrite-includes -- %s | %clang_cl /c -Xclang -verify /Tp -
 // expected-no-diagnostics
 
+// This test uses dos-style \r\n line endings.
+// Make sure your editor doesn't rewrite them to unix-style \n line endings.
 int foo();
 int bar();
 #define HELLO \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 960881a - clang: Make rewrite-includes-macros.cpp runnable on non-Win

[Nico Weber via cfe-commits]

Author: Nico Weber
Date: 2023-08-19T10:47:37-04:00
New Revision: 960881a7f32abc50fd0e5a349a9981437eba221a

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

LOG: clang: Make rewrite-includes-macros.cpp runnable on non-Win

As far as I can tell, there's nothing Windows-specific about the
test and it passes fine on other platforms.

I found this test when running

rg clang_cl clang/test | rg '%s' | rg -v -- ' -- ' | rg -v not

after 547ee1c81fceaabcb to see if other tests were missing `--`
before `%s` in `%clang_cl` invocations. This was the only one.
Since it used to run only on Windows, it wasn't needed, but as far
as I can tell there's no reason to run it only on Windows.

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

Added: 


Modified: 
clang/test/Frontend/rewrite-includes-macros.cpp

Removed: 




diff  --git a/clang/test/Frontend/rewrite-includes-macros.cpp 
b/clang/test/Frontend/rewrite-includes-macros.cpp
index 4a2de546d45546..1c2bfd440342fe 100644
--- a/clang/test/Frontend/rewrite-includes-macros.cpp
+++ b/clang/test/Frontend/rewrite-includes-macros.cpp
@@ -1,7 +1,8 @@
-// REQUIRES: system-windows
-// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c -Xclang 
-verify /Tp -
+// RUN: %clang_cl /E -Xclang -frewrite-includes -- %s | %clang_cl /c -Xclang 
-verify /Tp -
 // expected-no-diagnostics
 
+// This test uses dos-style \r\n line endings.
+// Make sure your editor doesn't rewrite them to unix-style \n line endings.
 int foo();
 int bar();
 #define HELLO \



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

[PATCH] D157383: [clang][Diagnostics] Provide source range to integer-overflow warnings

[Takuya Shimizu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG985a72b6b3e7: [clang][Diagnostics] Provide source range to 
integer-overflow warnings (authored by hazohelet).

Changed prior to commit:
  https://reviews.llvm.org/D157383?vs=548149=551746#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157383

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.h
  clang/test/Misc/constexpr-source-ranges.cpp


Index: clang/test/Misc/constexpr-source-ranges.cpp
===
--- clang/test/Misc/constexpr-source-ranges.cpp
+++ clang/test/Misc/constexpr-source-ranges.cpp
@@ -34,3 +34,10 @@
 }
 static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
 // CHECK: constexpr-source-ranges.cpp:35:23:{35:23-35:39}
+
+namespace overflow {
+// CHECK:  :{[[@LINE+1]]:9-[[@LINE+1]]:29}:
+int x = -1 + __INT_MAX__ + 2 + 3;
+// CHECK:  :{[[@LINE+1]]:9-[[@LINE+1]]:19}:
+int a = -(1 << 31) + 1;
+}
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -271,7 +271,8 @@
 SmallString<32> Trunc;
 Value.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   } else {
 S.CCEDiag(E, diag::note_constexpr_overflow) << Value << Type;
@@ -478,7 +479,8 @@
 SmallString<32> Trunc;
 NegatedValue.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   }
 
@@ -531,7 +533,8 @@
 SmallString<32> Trunc;
 APResult.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   }
 
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -2798,7 +2798,7 @@
 if (Info.checkingForUndefinedBehavior())
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
-  << toString(Result, 10) << E->getType();
+  << toString(Result, 10) << E->getType() << E->getSourceRange();
 return HandleOverflow(Info, E, Value, E->getType());
   }
   return true;
@@ -13643,7 +13643,7 @@
   if (Info.checkingForUndefinedBehavior())
 Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
  diag::warn_integer_constant_overflow)
-<< toString(Value, 10) << E->getType();
+<< toString(Value, 10) << E->getType() << E->getSourceRange();
 
   if (!HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
   E->getType()))


Index: clang/test/Misc/constexpr-source-ranges.cpp
===
--- clang/test/Misc/constexpr-source-ranges.cpp
+++ clang/test/Misc/constexpr-source-ranges.cpp
@@ -34,3 +34,10 @@
 }
 static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
 // CHECK: constexpr-source-ranges.cpp:35:23:{35:23-35:39}
+
+namespace overflow {
+// CHECK:  :{[[@LINE+1]]:9-[[@LINE+1]]:29}:
+int x = -1 + __INT_MAX__ + 2 + 3;
+// CHECK:  :{[[@LINE+1]]:9-[[@LINE+1]]:19}:
+int a = -(1 << 31) + 1;
+}
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -271,7 +271,8 @@
 SmallString<32> Trunc;
 Value.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   } else {
 S.CCEDiag(E, diag::note_constexpr_overflow) << Value << Type;
@@ -478,7 +479,8 @@
 SmallString<32> Trunc;
 NegatedValue.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   }
 
@@ -531,7 +533,8 @@
 SmallString<32> Trunc;
 

[clang] 985a72b - [clang][Diagnostics] Provide source range to integer-overflow warnings

[Takuya Shimizu via cfe-commits]

Author: Takuya Shimizu
Date: 2023-08-19T22:05:12+09:00
New Revision: 985a72b6b3e74f0d29780b2a97b5817473338ffe

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

LOG: [clang][Diagnostics] Provide source range to integer-overflow warnings

BEFORE:

```
overflow.cpp:1:21: warning: overflow in expression; result is -2147483648 with 
type 'int' [-Winteger-overflow]
1 | int x = __INT_MAX__ + 1 + 3;
  | ^
overflow.cpp:2:9: warning: overflow in expression; result is -2147483648 with 
type 'int' [-Winteger-overflow]
2 | int a = -(1 << 31) + 1;
  | ^
```
AFTER:

```
overflow.cpp:1:21: warning: overflow in expression; result is -2147483648 with 
type 'int' [-Winteger-overflow]
1 | int x = __INT_MAX__ + 1 + 3;
  | ^~~
overflow.cpp:2:9: warning: overflow in expression; result is -2147483648 with 
type 'int' [-Winteger-overflow]
2 | int a = -(1 << 31) + 1;
  | ^~
```

Reviewed By: tbaeder
Differential Revision: https://reviews.llvm.org/D157383

Added: 


Modified: 
clang/lib/AST/ExprConstant.cpp
clang/lib/AST/Interp/Interp.h
clang/test/Misc/constexpr-source-ranges.cpp

Removed: 




diff  --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 9ee9fccfd461f5..9f4c758b81303c 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2798,7 +2798,7 @@ static bool CheckedIntArithmetic(EvalInfo , const 
Expr *E,
 if (Info.checkingForUndefinedBehavior())
   Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
-  << toString(Result, 10) << E->getType();
+  << toString(Result, 10) << E->getType() << E->getSourceRange();
 return HandleOverflow(Info, E, Value, E->getType());
   }
   return true;
@@ -13643,7 +13643,7 @@ bool IntExprEvaluator::VisitUnaryOperator(const 
UnaryOperator *E) {
   if (Info.checkingForUndefinedBehavior())
 Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
  diag::warn_integer_constant_overflow)
-<< toString(Value, 10) << E->getType();
+<< toString(Value, 10) << E->getType() << E->getSourceRange();
 
   if (!HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
   E->getType()))

diff  --git a/clang/lib/AST/Interp/Interp.h b/clang/lib/AST/Interp/Interp.h
index aebab9023a3580..79995b5a74897c 100644
--- a/clang/lib/AST/Interp/Interp.h
+++ b/clang/lib/AST/Interp/Interp.h
@@ -271,7 +271,8 @@ bool AddSubMulHelper(InterpState , CodePtr OpPC, unsigned 
Bits, const T ,
 SmallString<32> Trunc;
 Value.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   } else {
 S.CCEDiag(E, diag::note_constexpr_overflow) << Value << Type;
@@ -478,7 +479,8 @@ bool Neg(InterpState , CodePtr OpPC) {
 SmallString<32> Trunc;
 NegatedValue.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   }
 
@@ -531,7 +533,8 @@ bool IncDecHelper(InterpState , CodePtr OpPC, const 
Pointer ) {
 SmallString<32> Trunc;
 APResult.trunc(Result.bitWidth()).toString(Trunc, 10);
 auto Loc = E->getExprLoc();
-S.report(Loc, diag::warn_integer_constant_overflow) << Trunc << Type;
+S.report(Loc, diag::warn_integer_constant_overflow)
+<< Trunc << Type << E->getSourceRange();
 return true;
   }
 

diff  --git a/clang/test/Misc/constexpr-source-ranges.cpp 
b/clang/test/Misc/constexpr-source-ranges.cpp
index 15fd54847f4762..f21373eff3a95c 100644
--- a/clang/test/Misc/constexpr-source-ranges.cpp
+++ b/clang/test/Misc/constexpr-source-ranges.cpp
@@ -34,3 +34,10 @@ constexpr int ints(int a, int b, int c, int d) {
 }
 static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
 // CHECK: constexpr-source-ranges.cpp:35:23:{35:23-35:39}
+
+namespace overflow {
+// CHECK:  :{[[@LINE+1]]:9-[[@LINE+1]]:29}:
+int x = -1 + __INT_MAX__ + 2 + 3;
+// CHECK:  :{[[@LINE+1]]:9-[[@LINE+1]]:19}:
+int a = -(1 << 31) + 1;
+}



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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/SemaCXX/offsetof.cpp:106
+int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}
+int x4[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+

yichi170 wrote:
> cor3ntin wrote:
> > yichi170 wrote:
> > > hubert.reinterpretcast wrote:
> > > > aaron.ballman wrote:
> > > > > yichi170 wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > There's one more test I'd like to see:
> > > > > > > ```
> > > > > > > struct S {
> > > > > > >   int Foo;
> > > > > > > };
> > > > > > > 
> > > > > > > template 
> > > > > > > void func() {
> > > > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > > > }
> > > > > > > 
> > > > > > > void inst() {
> > > > > > >   func();
> > > > > > > }
> > > > > > > ```
> > > > > > It would get the compile error in the current patch, but I think it 
> > > > > > should be compiled without any error, right?
> > > > > Correct, that should be accepted: https://godbolt.org/z/1f6a9Yaxa
> > > > Should expect this to pass too:
> > > > ```
> > > > template 
> > > > struct Z {
> > > >   static_assert(!__builtin_offsetof(T, template Q::x));
> > > > };
> > > > 
> > > > struct A {
> > > >   template  using Q = T;
> > > >   int x;
> > > > };
> > > > 
> > > > Z za;
> > > > ```
> > > Wow. Does it mean we cannot simply parse the identifier, `::`, `.` and 
> > > brackets in `__builtin_offsetof`?
> > GCC seems to support that. 
> > 
> > We probably want to call `ParseOptionalCXXScopeSpecifier` and store the 
> > `NestedNameSpecifierLoc` we'd get from it (and then parse the (sequence of) 
> > identifier(s) corresponding to the member) as we do now.
> > 
> > The documentation of 
> > https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Offsetof.html#index-_005f_005fbuiltin_005foffsetof
> >  
> > seems inaccurate,
> > 
> > it seems to be
> > 
> > `"__builtin_offsetof" "(" typename ","  nested-name-specifier 
> > offsetof_member_designator ")"`
> > 
> > 
> > Note that you will have to take care of transforming the nested name in 
> > TreeTransform and handle type dependencies. Let me know if you have further 
> > questions,
> > it's more involved than what you signed for. Sorry for not spotting that 
> > earlier (Thanks @hubert.reinterpretcast !)
> Thank you for all the help! I'll take a look at it!
I was wrong, we need another approach.

I think the grammar is actually
```
member-designator:
  qualified-id
  member-designator.qualified-id
  member-designator.qualified-id
```
IE, we should support https://godbolt.org/z/eEq8snMc8

Unfortunately, this looks like a much bigger change that we envisioned when we 
tagged this as a good first issue, to the extent I'm not sure what is actually 
the right design is would be.

For each component I imagine we want to store
`NestedNameSpecifierLoc + DeclarationNameInfo`

The parser would have to produce a CXXScopeSpec + UnqualifiedId pair for each 
component.

The expression is dependent if any of the component is type dependent,

`OffsetOfNode` would have to change, but i think we can get away
Only changing the identifier case (ie the dependent case)  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

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

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 551744.
hazohelet marked 2 inline comments as done.
hazohelet added a comment.

Address comments from Mark

- Added comment about the need of macro use in libc++ include file.


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

https://reviews.llvm.org/D155064

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/Interp/builtins.cpp
  clang/test/AST/Interp/if.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/CXX/expr/expr.const/p2-0x.cpp
  clang/test/CXX/expr/expr.const/p6-2a.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p3.cpp
  clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p4.cpp
  clang/test/Parser/pragma-fenv_access.c
  clang/test/SemaCXX/constant-conversion.cpp
  clang/test/SemaCXX/constant-expression-cxx11.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp
  clang/test/SemaCXX/cxx2b-consteval-if.cpp
  clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
  clang/test/SemaCXX/fixit-tautological-meta-constant.cpp
  clang/test/SemaCXX/vartemplate-lambda.cpp
  clang/test/SemaCXX/warn-constant-evaluated-constexpr.cpp
  clang/test/SemaCXX/warn-tautological-meta-constant.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/unittests/Support/TimeProfilerTest.cpp
  libcxx/include/__type_traits/is_constant_evaluated.h
  
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp
  
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp

Index: libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
===
--- libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
+++ libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
@@ -24,7 +24,7 @@
 #else
   // expected-error-re@+1 (static_assert|static assertion)}} failed}}
   static_assert(!std::is_constant_evaluated(), "");
-  // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression}}
+  // expected-warning-re@-1 0-1 {{'std::is_constant_evaluated' will always evaluate to {{('true' in a manifestly constant-evaluated expression|true in this context)
 #endif
   return 0;
 }
Index: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp
===
--- libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp
+++ libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp
@@ -93,12 +93,10 @@
 }
 
 int main(int, char**) {
-  if (!std::is_constant_evaluated()) {
-test_containers, std::deque>();
-test_containers, std::vector>();
-test_containers, std::deque>();
-test_containers, std::vector>();
-  }
+  test_containers, std::deque>();
+  test_containers, std::vector>();
+  test_containers, std::deque>();
+  test_containers, std::vector>();
 
   types::for_each(types::forward_iterator_list{}, [] {
 test_join_view();
Index: libcxx/include/__type_traits/is_constant_evaluated.h
===
--- libcxx/include/__type_traits/is_constant_evaluated.h
+++ libcxx/include/__type_traits/is_constant_evaluated.h
@@ -24,7 +24,14 @@
 #endif
 
 _LIBCPP_HIDE_FROM_ABI inline _LIBCPP_CONSTEXPR bool __libcpp_is_constant_evaluated() _NOEXCEPT {
+// __builtin_is_constant_evaluated() in this function always evaluates to false in pre-C++11 mode
+// because this function is not constexpr-qualified.
+// The following macro use clarifies this and avoids warnings from compilers.
+#ifndef _LIBCPP_CXX03_LANG
   return __builtin_is_constant_evaluated();
+#else
+  return false;
+#endif
 }
 
 _LIBCPP_END_NAMESPACE_STD
Index: clang/unittests/Support/TimeProfilerTest.cpp
===
--- clang/unittests/Support/TimeProfilerTest.cpp
+++ clang/unittests/Support/TimeProfilerTest.cpp
@@ -188,7 +188,6 @@
 | EvaluateAsBooleanCondition ()
 | | EvaluateAsRValue ()
 | EvaluateAsInitializer (slow_value)
-| EvaluateAsConstantExpr ()
 | EvaluateAsConstantExpr ()
 | EvaluateAsRValue ()
 | EvaluateAsInitializer (slow_init_list)
Index: clang/test/SemaTemplate/concepts.cpp

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: dblaikie.
aaron.ballman added a comment.

This seems like a reasonable approach to me. CC @dblaikie and @dexonsmith to 
make sure this addresses their concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158301

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

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Mark de Wever via Phabricator via cfe-commits]

Mordante added inline comments.



Comment at: libcxx/include/__type_traits/is_constant_evaluated.h:31
+  return false;
+#endif
 }

hazohelet wrote:
> Mordante wrote:
> > Why is this needed? Does this mean the builtin will fail in C++03 mode?
> `__libcpp_is_constant_evaluated` always returns false under C++03 or earlier, 
> where constexpr isn't available. This is a fix to run libc++ tests without 
> seeing warnings from clang with this patch.
> (Live demo: https://godbolt.org/z/oszebM5o5)
I see can you add a comment in that regard? To me it looks like the builtin 
fails in C++03, where returning false indeed is always the right answer.


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

https://reviews.llvm.org/D155064

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

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added inline comments.



Comment at: libcxx/include/__type_traits/is_constant_evaluated.h:31
+  return false;
+#endif
 }

Mordante wrote:
> Why is this needed? Does this mean the builtin will fail in C++03 mode?
`__libcpp_is_constant_evaluated` always returns false under C++03 or earlier, 
where constexpr isn't available. This is a fix to run libc++ tests without 
seeing warnings from clang with this patch.
(Live demo: https://godbolt.org/z/oszebM5o5)


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

https://reviews.llvm.org/D155064

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

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Mark de Wever via Phabricator via cfe-commits]

Mordante added inline comments.



Comment at: libcxx/include/__type_traits/is_constant_evaluated.h:31
+  return false;
+#endif
 }

Why is this needed? Does this mean the builtin will fail in C++03 mode?



Comment at: 
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp:27
   static_assert(!std::is_constant_evaluated(), "");
-  // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always 
evaluate to 'true' in a manifestly constant-evaluated expression}}
+  // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always 
evaluate to true in this context}}
 #endif

philnik wrote:
> cor3ntin wrote:
> > Mordante wrote:
> > > hazohelet wrote:
> > > > philnik wrote:
> > > > > Mordante wrote:
> > > > > > Since libc++ support the latest ToT Clang and the last two official 
> > > > > > releases this wont work. The `expected-warning` needs to be a 
> > > > > > `expected-warning-re` that works for both the new and old diagnostic
> > > > > You can also just shorten it to `'std::is_constant_evaluated' will 
> > > > > always evaluate to`. Seems good enough to me.
> > > > Thanks!
> > > I really would like a regex. To me the current message misses an 
> > > important piece of information; the `true` part. I care less about the 
> > > rest of the message, but stripping the `true` means a warning like 
> > > `std::is_constant_evaluated' will always evaluate to FALSE` would be 
> > > valid too.
> > Agreed with Mordante
> We're not in the business of testing the compiler though. Taking a closer 
> look, I'm not actually sure why this test exists at all. It doesn't seem like 
> it tests anything useful w.r.t. the library. This has been added in 2fc5a78, 
> but there the warning isn't checked, so that was clearly not the original 
> intention.
> We're not in the business of testing the compiler though. Taking a closer 
> look, I'm not actually sure why this test exists at all. It doesn't seem like 
> it tests anything useful w.r.t. the library. This has been added in 2fc5a78, 
> but there the warning isn't checked, so that was clearly not the original 
> intention.

I agree. But to me this test tests whether 


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

https://reviews.llvm.org/D155064

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

[clang] 69a8636 - [clang][NFC] Remove redundant whitespaces

[Younan Zhang via cfe-commits]

Author: Younan Zhang
Date: 2023-08-19T19:25:32+08:00
New Revision: 69a8636c4dec2c9e561224e603ea9864a06bee3a

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

LOG: [clang][NFC] Remove redundant whitespaces

This breaks the clang check-format on CI.

+ grep -rnI '[[:blank:]]$' clang/lib clang/include clang/docs
clang/lib/Analysis/UnsafeBufferUsage.cpp:2277:#endif

Added: 


Modified: 
clang/lib/Analysis/UnsafeBufferUsage.cpp

Removed: 




diff  --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp 
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 21d3f6f2faaa68..29543131dae230 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2274,7 +2274,7 @@ getFixIts(FixableGadgetSets , const 
Strategy ,
   VD, F->getBaseStmt()->getBeginLoc(),
   ("gadget '" + F->getDebugName() + "' refused to produce a fix")
   .str());
-#endif  
+#endif
   FixItsForVariable.erase(VD);
   break;
 }



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

[PATCH] D158302: [clang] Add simple utils to ensure static assert on bit count of DeclContext

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 551738.
danix800 added a comment.

Apply git-clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158302

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Support/BitFieldReflection.h
  clang/lib/Serialization/ASTWriterDecl.cpp

Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -433,10 +433,6 @@
 }
 
 void ASTDeclWriter::VisitTagDecl(TagDecl *D) {
-  static_assert(DeclContext::NumTagDeclBits == 10,
-"You need to update the serializer after you change the "
-"TagDeclBits");
-
   VisitRedeclarable(D);
   VisitTypeDecl(D);
   Record.push_back(D->getIdentifierNamespace());
@@ -461,10 +457,6 @@
 }
 
 void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
-  static_assert(DeclContext::NumEnumDeclBits == 20,
-"You need to update the serializer after you change the "
-"EnumDeclBits");
-
   VisitTagDecl(D);
   Record.AddTypeSourceInfo(D->getIntegerTypeSourceInfo());
   if (!D->getIntegerTypeSourceInfo())
@@ -508,10 +500,6 @@
 }
 
 void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) {
-  static_assert(DeclContext::NumRecordDeclBits == 41,
-"You need to update the serializer after you change the "
-"RecordDeclBits");
-
   VisitTagDecl(D);
   Record.push_back(D->hasFlexibleArrayMember());
   Record.push_back(D->isAnonymousStructOrUnion());
@@ -580,10 +568,6 @@
 }
 
 void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
-  static_assert(DeclContext::NumFunctionDeclBits == 31,
-"You need to update the serializer after you change the "
-"FunctionDeclBits");
-
   VisitRedeclarable(D);
 
   Record.push_back(D->getTemplatedKind());
@@ -735,10 +719,6 @@
 }
 
 void ASTDeclWriter::VisitObjCMethodDecl(ObjCMethodDecl *D) {
-  static_assert(DeclContext::NumObjCMethodDeclBits == 24,
-"You need to update the serializer after you change the "
-"ObjCMethodDeclBits");
-
   VisitNamedDecl(D);
   // FIXME: convert to LazyStmtPtr?
   // Unlike C/C++, method bodies will never be in header files.
@@ -797,10 +777,6 @@
 }
 
 void ASTDeclWriter::VisitObjCContainerDecl(ObjCContainerDecl *D) {
-  static_assert(DeclContext::NumObjCContainerDeclBits == 51,
-"You need to update the serializer after you change the "
-"ObjCContainerDeclBits");
-
   VisitNamedDecl(D);
   Record.AddSourceLocation(D->getAtStartLoc());
   Record.AddSourceRange(D->getAtEndRange());
@@ -1284,10 +1260,6 @@
 }
 
 void ASTDeclWriter::VisitLinkageSpecDecl(LinkageSpecDecl *D) {
-  static_assert(DeclContext::NumLinkageSpecDeclBits == 4,
-"You need to update the serializer after you change the"
-"LinkageSpecDeclBits");
-
   VisitDecl(D);
   Record.push_back(D->getLanguage());
   Record.AddSourceLocation(D->getExternLoc());
@@ -1495,10 +1467,6 @@
 }
 
 void ASTDeclWriter::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
-  static_assert(DeclContext::NumCXXConstructorDeclBits == 20,
-"You need to update the serializer after you change the "
-"CXXConstructorDeclBits");
-
   Record.push_back(D->getTrailingAllocKind());
   addExplicitSpecifier(D->getExplicitSpecifier(), Record);
   if (auto Inherited = D->getInheritedConstructor()) {
@@ -1875,10 +1843,6 @@
 
 /// Emit the DeclContext part of a declaration context decl.
 void ASTDeclWriter::VisitDeclContext(DeclContext *DC) {
-  static_assert(DeclContext::NumDeclContextBits == 13,
-"You need to update the serializer after you change the "
-"DeclContextBits");
-
   Record.AddOffset(Writer.WriteDeclContextLexicalBlock(Context, DC));
   Record.AddOffset(Writer.WriteDeclContextVisibleBlock(Context, DC));
 }
@@ -1989,10 +1953,6 @@
 }
 
 void ASTDeclWriter::VisitOMPDeclareReductionDecl(OMPDeclareReductionDecl *D) {
-  static_assert(DeclContext::NumOMPDeclareReductionDeclBits == 2,
-"You need to update the serializer after you change the "
-"NumOMPDeclareReductionDeclBits");
-
   VisitValueDecl(D);
   Record.AddSourceLocation(D->getBeginLoc());
   Record.AddStmt(D->getCombinerIn());
Index: clang/include/clang/Support/BitFieldReflection.h
===
--- /dev/null
+++ clang/include/clang/Support/BitFieldReflection.h
@@ -0,0 +1,104 @@
+//===--- BitFieldReflection.h - BitField Reflection Utils ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//

[PATCH] D158302: [clang] Add simple utils to ensure static assert on bit count of DeclContext

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 551737.
danix800 added a comment.

Remove ununsed code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158302

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Support/BitFieldReflection.h
  clang/lib/Serialization/ASTWriterDecl.cpp

Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -433,10 +433,6 @@
 }
 
 void ASTDeclWriter::VisitTagDecl(TagDecl *D) {
-  static_assert(DeclContext::NumTagDeclBits == 10,
-"You need to update the serializer after you change the "
-"TagDeclBits");
-
   VisitRedeclarable(D);
   VisitTypeDecl(D);
   Record.push_back(D->getIdentifierNamespace());
@@ -461,10 +457,6 @@
 }
 
 void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
-  static_assert(DeclContext::NumEnumDeclBits == 20,
-"You need to update the serializer after you change the "
-"EnumDeclBits");
-
   VisitTagDecl(D);
   Record.AddTypeSourceInfo(D->getIntegerTypeSourceInfo());
   if (!D->getIntegerTypeSourceInfo())
@@ -508,10 +500,6 @@
 }
 
 void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) {
-  static_assert(DeclContext::NumRecordDeclBits == 41,
-"You need to update the serializer after you change the "
-"RecordDeclBits");
-
   VisitTagDecl(D);
   Record.push_back(D->hasFlexibleArrayMember());
   Record.push_back(D->isAnonymousStructOrUnion());
@@ -580,10 +568,6 @@
 }
 
 void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
-  static_assert(DeclContext::NumFunctionDeclBits == 31,
-"You need to update the serializer after you change the "
-"FunctionDeclBits");
-
   VisitRedeclarable(D);
 
   Record.push_back(D->getTemplatedKind());
@@ -735,10 +719,6 @@
 }
 
 void ASTDeclWriter::VisitObjCMethodDecl(ObjCMethodDecl *D) {
-  static_assert(DeclContext::NumObjCMethodDeclBits == 24,
-"You need to update the serializer after you change the "
-"ObjCMethodDeclBits");
-
   VisitNamedDecl(D);
   // FIXME: convert to LazyStmtPtr?
   // Unlike C/C++, method bodies will never be in header files.
@@ -797,10 +777,6 @@
 }
 
 void ASTDeclWriter::VisitObjCContainerDecl(ObjCContainerDecl *D) {
-  static_assert(DeclContext::NumObjCContainerDeclBits == 51,
-"You need to update the serializer after you change the "
-"ObjCContainerDeclBits");
-
   VisitNamedDecl(D);
   Record.AddSourceLocation(D->getAtStartLoc());
   Record.AddSourceRange(D->getAtEndRange());
@@ -1284,10 +1260,6 @@
 }
 
 void ASTDeclWriter::VisitLinkageSpecDecl(LinkageSpecDecl *D) {
-  static_assert(DeclContext::NumLinkageSpecDeclBits == 4,
-"You need to update the serializer after you change the"
-"LinkageSpecDeclBits");
-
   VisitDecl(D);
   Record.push_back(D->getLanguage());
   Record.AddSourceLocation(D->getExternLoc());
@@ -1495,10 +1467,6 @@
 }
 
 void ASTDeclWriter::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
-  static_assert(DeclContext::NumCXXConstructorDeclBits == 20,
-"You need to update the serializer after you change the "
-"CXXConstructorDeclBits");
-
   Record.push_back(D->getTrailingAllocKind());
   addExplicitSpecifier(D->getExplicitSpecifier(), Record);
   if (auto Inherited = D->getInheritedConstructor()) {
@@ -1875,10 +1843,6 @@
 
 /// Emit the DeclContext part of a declaration context decl.
 void ASTDeclWriter::VisitDeclContext(DeclContext *DC) {
-  static_assert(DeclContext::NumDeclContextBits == 13,
-"You need to update the serializer after you change the "
-"DeclContextBits");
-
   Record.AddOffset(Writer.WriteDeclContextLexicalBlock(Context, DC));
   Record.AddOffset(Writer.WriteDeclContextVisibleBlock(Context, DC));
 }
@@ -1989,10 +1953,6 @@
 }
 
 void ASTDeclWriter::VisitOMPDeclareReductionDecl(OMPDeclareReductionDecl *D) {
-  static_assert(DeclContext::NumOMPDeclareReductionDeclBits == 2,
-"You need to update the serializer after you change the "
-"NumOMPDeclareReductionDeclBits");
-
   VisitValueDecl(D);
   Record.AddSourceLocation(D->getBeginLoc());
   Record.AddStmt(D->getCombinerIn());
Index: clang/include/clang/Support/BitFieldReflection.h
===
--- /dev/null
+++ clang/include/clang/Support/BitFieldReflection.h
@@ -0,0 +1,104 @@
+//===--- BitFieldReflection.h - BitField Reflection Utils ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 551736.
zyounan added a comment.

Sorry, forgot to update tests :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -60,7 +60,10 @@
 for (unsigned I = 0; I < NumResults; ++I) {
   auto R = Results[I];
   if (R.Kind == CodeCompletionResult::RK_Declaration) {
-if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+auto *ND = R.getDeclaration();
+if (auto *Template = llvm::dyn_cast(ND))
+  ND = Template->getTemplatedDecl();
+if (const auto *FD = llvm::dyn_cast(ND)) {
   CompletedFunctionDecl D;
   D.Name = FD->getNameAsString();
   D.CanBeCall = R.FunctionCanBeCall;
@@ -191,6 +194,10 @@
 struct Foo {
   static int staticMethod();
   int method() const;
+  template 
+  void generic(U);
+  template 
+  static T staticGeneric();
   Foo() {
 this->$canBeCall^
 $canBeCall^
@@ -199,6 +206,8 @@
 };
 
 struct Derived : Foo {
+  using Foo::method;
+  using Foo::generic;
   Derived() {
 Foo::$canBeCall^
   }
@@ -207,15 +216,29 @@
 struct OtherClass {
   OtherClass() {
 Foo f;
+Derived d;
 f.$canBeCall^
+; // Prevent parsing as 'f.f'
+f.Foo::$canBeCall^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$cannotBeCall^
+  ;
+  d.Foo::$canBeCall^
+  ;
+  d.Derived::$canBeCall^
 }
 )cpp");
 
@@ -223,12 +246,16 @@
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
 canBeCall(true;
+EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+canBeCall(true;
   }
 
   for (const auto  : Code.points("cannotBeCall")) {
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
 canBeCall(false;
+EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+canBeCall(false;
   }
 
   // static method can always be a call
@@ -236,6 +263,8 @@
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("staticMethod"), isStatic(true),
 canBeCall(true;
+EXPECT_THAT(Results, Contains(AllOf(named("staticGeneric"), isStatic(true),
+canBeCall(true;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ clang/test/CodeCompletion/member-access.cpp
@@ -341,3 +341,14 @@
   // RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:339:10 %s -o - | FileCheck -check-prefix=CHECK-FIELD-DECLARED-VIA-USING %s
   // CHECK-FIELD-DECLARED-VIA-USING: [#int#]field (requires fix-it: {339:8-339:9} to "->")
 }
+
+namespace function_can_be_call {
+  struct S {
+template 
+V foo(T, U);
+  };
+
+  ::f
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:351:7 %s -o - | FileCheck -check-prefix=CHECK_FUNCTION_CAN_BE_CALL %s
+  // CHECK_FUNCTION_CAN_BE_CALL: COMPLETION: foo : [#V#]foo<<#typename T#>, <#typename U#>{#, <#typename V#>#}>(<#T#>, <#U#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -338,8 +338,11 @@
   ///
   /// \param InBaseClass whether the result was found in a base
   /// class of the searched context.
+  ///
+  /// \param BaseExprType the type of expression that precedes the "." or "->"
+  /// in a member access expression.
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
- bool InBaseClass);
+   

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks tests on windows: http://45.33.8.238/win/82918/step_7.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157565

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

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 551730.
zyounan added a comment.

Fix the bool expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -60,7 +60,10 @@
 for (unsigned I = 0; I < NumResults; ++I) {
   auto R = Results[I];
   if (R.Kind == CodeCompletionResult::RK_Declaration) {
-if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+auto *ND = R.getDeclaration();
+if (auto *Template = llvm::dyn_cast(ND))
+  ND = Template->getTemplatedDecl();
+if (const auto *FD = llvm::dyn_cast(ND)) {
   CompletedFunctionDecl D;
   D.Name = FD->getNameAsString();
   D.CanBeCall = R.FunctionCanBeCall;
@@ -191,6 +194,10 @@
 struct Foo {
   static int staticMethod();
   int method() const;
+  template 
+  void generic(U);
+  template 
+  static T staticGeneric();
   Foo() {
 this->$canBeCall^
 $canBeCall^
@@ -199,6 +206,8 @@
 };
 
 struct Derived : Foo {
+  using Foo::method;
+  using Foo::generic;
   Derived() {
 Foo::$canBeCall^
   }
@@ -207,15 +216,29 @@
 struct OtherClass {
   OtherClass() {
 Foo f;
+Derived d;
 f.$canBeCall^
+; // Prevent parsing as 'f.f'
+f.Foo::$canBeCall^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$cannotBeCall^
+  ;
+  d.Foo::$canBeCall^
+  ;
+  d.Derived::$canBeCall^
 }
 )cpp");
 
@@ -223,12 +246,16 @@
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
 canBeCall(true;
+EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+canBeCall(true;
   }
 
   for (const auto  : Code.points("cannotBeCall")) {
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
 canBeCall(false;
+EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+canBeCall(false;
   }
 
   // static method can always be a call
@@ -236,6 +263,8 @@
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("staticMethod"), isStatic(true),
 canBeCall(true;
+EXPECT_THAT(Results, Contains(AllOf(named("staticGeneric"), isStatic(true),
+canBeCall(true;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ clang/test/CodeCompletion/member-access.cpp
@@ -341,3 +341,14 @@
   // RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:339:10 %s -o - | FileCheck -check-prefix=CHECK-FIELD-DECLARED-VIA-USING %s
   // CHECK-FIELD-DECLARED-VIA-USING: [#int#]field (requires fix-it: {339:8-339:9} to "->")
 }
+
+namespace function_can_be_call {
+  struct S {
+template 
+V foo(T, U);
+  };
+
+  ::f
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:351:7 %s -o - | FileCheck -check-prefix=CHECK_FUNCTION_CAN_BE_CALL %s
+  // CHECK_FUNCTION_CAN_BE_CALL: COMPLETION: foo : [#V#]foo<<#typename T#>, <#typename U#>{#, <#typename V#>#}>(<#T#>, <#U#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -338,8 +338,11 @@
   ///
   /// \param InBaseClass whether the result was found in a base
   /// class of the searched context.
+  ///
+  /// \param BaseExprType the type of expression that precedes the "." or "->"
+  /// in a member access expression.
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
- bool InBaseClass);
+ 

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13272
+  uint64_t Mask = llvm::X86::getCpuSupportsMask(FeatureStrs);
+  uint32_t FeaturesMask[4] = {uint32_t(Mask), uint32_t(Mask >> 32), 0, 0};
+  return EmitX86CpuSupports(FeaturesMask);

Use `Lo_32(Mask)`, `Hi_32(Mask)`?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13318-13320
+Value *Idxs[] = {Builder.getInt32(0), Builder.getInt32(i - 1)};
+Value *Features = Builder.CreateAlignedLoad(
+Int32Ty, Builder.CreateGEP(ATy, CpuFeatures2, Idxs),

Do we have problem when linking with old version of libgcc?



Comment at: compiler-rt/lib/builtins/cpu_model.c:808
+  if (HasExtLeaf1) {
+if (ECX & 1)
+  setFeature(FEATURE_LAHF_LM);

Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The 
same below.



Comment at: compiler-rt/lib/builtins/cpu_model.c:808
+  if (HasExtLeaf1) {
+if (ECX & 1)
+  setFeature(FEATURE_LAHF_LM);

pengfei wrote:
> Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The 
> same below.
Can we reuse bit defination in cpuid.h, e.g., `if (ECX & bit_LAHF_LM)`? The 
same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158329

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

[PATCH] D158061: [clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 551728.
zyounan added a comment.

Rebase and Remove the stray status


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158061

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ExprConcepts.h
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
  clang/test/SemaCXX/concept-fatal-error.cpp

Index: clang/test/SemaCXX/concept-fatal-error.cpp
===
--- clang/test/SemaCXX/concept-fatal-error.cpp
+++ clang/test/SemaCXX/concept-fatal-error.cpp
@@ -1,4 +1,4 @@
-// RUN: not %clang_cc1 -fsyntax-only -std=c++20 -ferror-limit 1 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -ferror-limit 1 -verify %s
 
 template 
 concept f = requires { 42; };
@@ -6,5 +6,5 @@
   // The missing semicolon will trigger an error and -ferror-limit=1 will make it fatal
   // We test that we do not crash in such cases (#55401)
   int i = requires { { i } f } // expected-error {{expected ';' at end of declaration list}}
-   // expected-error@* {{too many errros emitted}}
+   // expected-error@* {{too many errors emitted}}
 };
Index: clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+
+template  class normal_iterator {};
+
+template  struct is_convertible {};
+
+template 
+inline constexpr bool is_convertible_v = is_convertible::value; // #1
+
+// expected-error@#1 {{no member named 'value' in 'is_convertible'}}
+
+template 
+concept convertible_to = is_convertible_v; // expected-note 0+{{}}
+template 
+  requires requires(IteratorL lhs, IteratorR rhs) { // expected-note 0+{{}}
+{ lhs == rhs } -> convertible_to; // #2
+  }
+constexpr bool compare(normal_iterator lhs, normal_iterator rhs) {
+  return false;
+}
+
+// We don't know exactly the substituted type for `lhs == rhs`, thus a placeholder 'expr-type' is emitted.
+// expected-note@#2 {{'convertible_to'}}
+
+// Consume remaining notes/errors.
+// expected-note@* 0+{{}}
+// expected-error@* 0+{{}}
+class Object;
+
+void function() {
+  normal_iterator begin, end;
+  compare(begin, end);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2271,9 +2271,9 @@
   getPackIndex(Pack), Arg, TL.getNameLoc());
 }
 
-template
 static concepts::Requirement::SubstitutionDiagnostic *
-createSubstDiag(Sema , TemplateDeductionInfo , EntityPrinter Printer) {
+createSubstDiag(Sema , TemplateDeductionInfo ,
+concepts::EntityPrinter Printer) {
   SmallString<128> Message;
   SourceLocation ErrorLoc;
   if (Info.hasSFINAEDiagnostic()) {
@@ -2297,6 +2297,19 @@
   StringRef(MessageBuf, Message.size())};
 }
 
+concepts::Requirement::SubstitutionDiagnostic *
+concepts::createSubstDiagAt(Sema , SourceLocation Location,
+EntityPrinter Printer) {
+  SmallString<128> Entity;
+  llvm::raw_svector_ostream OS(Entity);
+  Printer(OS);
+  char *EntityBuf = new (S.Context) char[Entity.size()];
+  llvm::copy(Entity, EntityBuf);
+  return new (S.Context) concepts::Requirement::SubstitutionDiagnostic{
+  /*SubstitutedEntity=*/StringRef(EntityBuf, Entity.size()),
+  /*DiagLoc=*/Location, /*DiagMessage=*/StringRef()};
+}
+
 ExprResult TemplateInstantiator::TransformRequiresTypeParams(
 SourceLocation KWLoc, SourceLocation RBraceLoc, const RequiresExpr *RE,
 RequiresExprBodyDecl *Body, ArrayRef Params,
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
@@ -9074,16 +9075,22 @@
 MLTAL.addOuterRetainedLevels(TPL->getDepth());
 const TypeConstraint *TC = Param->getTypeConstraint();
 assert(TC && "Type Constraint cannot be null here");
-ExprResult Constraint =
-SubstExpr(TC->getImmediatelyDeclaredConstraint(), MLTAL);
+auto *IDC = TC->getImmediatelyDeclaredConstraint();
+assert(IDC && "ImmediatelyDeclaredConstraint can't be null here.");
+ExprResult Constraint = SubstExpr(IDC, MLTAL);
 if (Constraint.isInvalid()) {
-  Status = concepts::ExprRequirement::SS_ExprSubstitutionFailure;
-} else {
-  SubstitutedConstraintExpr =
-  

[PATCH] D158061: [clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 551726.
zyounan added a comment.

Add release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158061

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ExprConcepts.h
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
  clang/test/SemaCXX/concept-fatal-error.cpp

Index: clang/test/SemaCXX/concept-fatal-error.cpp
===
--- clang/test/SemaCXX/concept-fatal-error.cpp
+++ clang/test/SemaCXX/concept-fatal-error.cpp
@@ -1,4 +1,4 @@
-// RUN: not %clang_cc1 -fsyntax-only -std=c++20 -ferror-limit 1 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -ferror-limit 1 -verify %s
 
 template 
 concept f = requires { 42; };
@@ -6,5 +6,5 @@
   // The missing semicolon will trigger an error and -ferror-limit=1 will make it fatal
   // We test that we do not crash in such cases (#55401)
   int i = requires { { i } f } // expected-error {{expected ';' at end of declaration list}}
-   // expected-error@* {{too many errros emitted}}
+   // expected-error@* {{too many errors emitted}}
 };
Index: clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+
+template  class normal_iterator {};
+
+template  struct is_convertible {};
+
+template 
+inline constexpr bool is_convertible_v = is_convertible::value; // #1
+
+// expected-error@#1 {{no member named 'value' in 'is_convertible'}}
+
+template 
+concept convertible_to = is_convertible_v; // expected-note 0+{{}}
+template 
+  requires requires(IteratorL lhs, IteratorR rhs) { // expected-note 0+{{}}
+{ lhs == rhs } -> convertible_to; // #2
+  }
+constexpr bool compare(normal_iterator lhs, normal_iterator rhs) {
+  return false;
+}
+
+// We don't know exactly the substituted type for `lhs == rhs`, thus a placeholder 'expr-type' is emitted.
+// expected-note@#2 {{'convertible_to'}}
+
+// Consume remaining notes/errors.
+// expected-note@* 0+{{}}
+// expected-error@* 0+{{}}
+class Object;
+
+void function() {
+  normal_iterator begin, end;
+  compare(begin, end);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2271,9 +2271,9 @@
   getPackIndex(Pack), Arg, TL.getNameLoc());
 }
 
-template
 static concepts::Requirement::SubstitutionDiagnostic *
-createSubstDiag(Sema , TemplateDeductionInfo , EntityPrinter Printer) {
+createSubstDiag(Sema , TemplateDeductionInfo ,
+concepts::EntityPrinter Printer) {
   SmallString<128> Message;
   SourceLocation ErrorLoc;
   if (Info.hasSFINAEDiagnostic()) {
@@ -2297,6 +2297,19 @@
   StringRef(MessageBuf, Message.size())};
 }
 
+concepts::Requirement::SubstitutionDiagnostic *
+concepts::createSubstDiagAt(Sema , SourceLocation Location,
+EntityPrinter Printer) {
+  SmallString<128> Entity;
+  llvm::raw_svector_ostream OS(Entity);
+  Printer(OS);
+  char *EntityBuf = new (S.Context) char[Entity.size()];
+  llvm::copy(Entity, EntityBuf);
+  return new (S.Context) concepts::Requirement::SubstitutionDiagnostic{
+  /*SubstitutedEntity=*/StringRef(EntityBuf, Entity.size()),
+  /*DiagLoc=*/Location, /*DiagMessage=*/StringRef()};
+}
+
 ExprResult TemplateInstantiator::TransformRequiresTypeParams(
 SourceLocation KWLoc, SourceLocation RBraceLoc, const RequiresExpr *RE,
 RequiresExprBodyDecl *Body, ArrayRef Params,
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprConcepts.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
@@ -9074,16 +9075,23 @@
 MLTAL.addOuterRetainedLevels(TPL->getDepth());
 const TypeConstraint *TC = Param->getTypeConstraint();
 assert(TC && "Type Constraint cannot be null here");
-ExprResult Constraint =
-SubstExpr(TC->getImmediatelyDeclaredConstraint(), MLTAL);
+auto *IDC = TC->getImmediatelyDeclaredConstraint();
+assert(IDC && "ImmediatelyDeclaredConstraint can't be null here.");
+ExprResult Constraint = SubstExpr(IDC, MLTAL);
 if (Constraint.isInvalid()) {
   Status = concepts::ExprRequirement::SS_ExprSubstitutionFailure;
-} else {
-  SubstitutedConstraintExpr =
-  

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thanks for Sam and Nathan's thorough review! I've updated, and please take 
another look.
(Apologize for churning on weekends, but I don't have chunk time during the 
week.)




Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
 case CodeCompletionString::CK_RightParen:
+  if (DropFunctionArguments &&
+  ResultKind == CodeCompletionResult::RK_Declaration)

sammccall wrote:
> nridge wrote:
> > It looks like we are assuming two things:
> > 
> >  1. Any LeftParen in an RK_Declaration starts a function argument list
> >  2. Everything that comes after the function argument list can be discarded
> > 
> > I think these assumptions are fine to make, but let's be explicit about 
> > them in a comment
> Agree, but also I think the code could reflect this more directly:
>  - this should trigger only for CK_LeftParen, not CK_RightParen
> 
> Rather than redirecting output to a different string by reassigning the 
> param, I think it would be a bit more direct to have
> 
> ```
> optional TruncateSnippetAt;
> ...
> case CK_LeftBracket:
>   if (DropFunctionArguments && ... && !TruncateSnippetAt)
> TruncateSnippetAt = Snippet->size();
> ...
> if (TruncateSnippetAt)
>   Snippet->resize(*TruncateSnippetAt);
> }
> ```
> 
> (though still not totally clear)
> It looks like we are assuming two things

Honestly, I don't quite prefer this assumption. However, we have lost some of 
the semantics e.g., the structure of a call expression within this function, 
and unfortunately it's not trivial to pass these out from clang. :-(

> (though still not totally clear)

Yeah. I imagine a clearer way would be separating the calculation for 
`Signature` and `Snippet`, and we could bail out early for `Snippet`. But I'm 
afraid that making so many adjustments to `getSignature` for such a small 
feature is inappropriate.

Assuming the left parenthesis is the beginning of a call might be sufficient 
for the time being, and let's leave the refactoring to subsequent patches.




Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:580
 auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
 EXPECT_THAT(Results.Completions,
 Contains(AllOf(named("method"), snippetSuffix("";

nridge wrote:
> Since you're updating this test anyways, could you add `signature()` matchers 
> for the non-template cases as well please?
Of course!



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:345
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
- bool InBaseClass);
+ bool InBaseClass, QualType BaseType);
 

nridge wrote:
> The word "base" is a bit overloaded in this signature; in the parameter 
> `InBaseClass` it refers to inheritance, but in `BaseType` it refers to the 
> base expression of a `MemberExpr`.
> 
> Maybe we can name the new parameter `BaseExprType` to avoid confusion?
Makes sense to me. Thanks for the correction.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1285
 Result.ShadowDecl = Using;
 AddResult(Result, CurContext, Hiding);
 return;

nridge wrote:
> Should we propagate `BaseType` into this recursive call?
Yes. And thanks for spotting this. I added another test case reflecting it.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // When completing a non-static member function (and not via
-  // dot/arrow member access) and we're not inside that class' scope,
-  // it can't be a call.
+  // Decide whether or not a non-static member function can be a call.
   if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) 
{

sammccall wrote:
> This is confusing: the `CCC_Symbol` test is part of the specific heuristics 
> being used (it's the "not via dot/arrow member access" part, right?) but 
> you've moved the comment explaining what it does.
> 
> Also this function is just getting too long, and we're inlining more 
> complicated control flow here.
> Can we extract a function?
> 
> ```
> const auto *Method = ...;
> if (Method & !Method->isStatic()) {
>   R.FunctionCanBeCall = canMethodBeCalled(...);
> }
> ```
> it's the "not via dot/arrow member access" part

(Sorry for being unaware of the historical context). But I think `CCC_Symbol` 
should mean "we're completing a name such as a function or type name" per its 
comment. The heuristic for dot/arrow member access actually lies on the next 
line, i.e., if the completing decl is a CXXMethodDecl.

> Can we extract a function?

Sure. 



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1425
+R.FunctionCanBeCall =
+MaybeDerived == MaybeBase || 
MaybeDerived->isDerivedFrom(MaybeBase);
+  }

nridge wrote:
> Is there any situation where `MaybeDerived == MaybeBase || 
> 

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 551723.
zyounan marked 14 inline comments as done.
zyounan added a comment.

Address many comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -60,7 +60,10 @@
 for (unsigned I = 0; I < NumResults; ++I) {
   auto R = Results[I];
   if (R.Kind == CodeCompletionResult::RK_Declaration) {
-if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+auto *ND = R.getDeclaration();
+if (auto *Template = llvm::dyn_cast(ND))
+  ND = Template->getTemplatedDecl();
+if (const auto *FD = llvm::dyn_cast(ND)) {
   CompletedFunctionDecl D;
   D.Name = FD->getNameAsString();
   D.CanBeCall = R.FunctionCanBeCall;
@@ -191,6 +194,10 @@
 struct Foo {
   static int staticMethod();
   int method() const;
+  template 
+  void generic(U);
+  template 
+  static T staticGeneric();
   Foo() {
 this->$canBeCall^
 $canBeCall^
@@ -199,6 +206,8 @@
 };
 
 struct Derived : Foo {
+  using Foo::method;
+  using Foo::generic;
   Derived() {
 Foo::$canBeCall^
   }
@@ -207,15 +216,29 @@
 struct OtherClass {
   OtherClass() {
 Foo f;
+Derived d;
 f.$canBeCall^
+; // Prevent parsing as 'f.f'
+f.Foo::$canBeCall^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$cannotBeCall^
+  ;
+  d.Foo::$canBeCall^
+  ;
+  d.Derived::$canBeCall^
 }
 )cpp");
 
@@ -223,12 +246,16 @@
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
 canBeCall(true;
+EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+canBeCall(true;
   }
 
   for (const auto  : Code.points("cannotBeCall")) {
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
 canBeCall(false;
+EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false),
+canBeCall(false;
   }
 
   // static method can always be a call
@@ -236,6 +263,8 @@
 auto Results = CollectCompletedFunctions(Code.code(), P);
 EXPECT_THAT(Results, Contains(AllOf(named("staticMethod"), isStatic(true),
 canBeCall(true;
+EXPECT_THAT(Results, Contains(AllOf(named("staticGeneric"), isStatic(true),
+canBeCall(true;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ clang/test/CodeCompletion/member-access.cpp
@@ -341,3 +341,14 @@
   // RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:339:10 %s -o - | FileCheck -check-prefix=CHECK-FIELD-DECLARED-VIA-USING %s
   // CHECK-FIELD-DECLARED-VIA-USING: [#int#]field (requires fix-it: {339:8-339:9} to "->")
 }
+
+namespace function_can_be_call {
+  struct S {
+template 
+V foo(T, U);
+  };
+
+  ::f
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:351:7 %s -o - | FileCheck -check-prefix=CHECK_FUNCTION_CAN_BE_CALL %s
+  // CHECK_FUNCTION_CAN_BE_CALL: COMPLETION: foo : [#V#]foo<<#typename T#>, <#typename U#>{#, <#typename V#>#}>(<#T#>, <#U#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -338,8 +338,11 @@
   ///
   /// \param InBaseClass whether the result was found in a base
   /// class of the searched context.
+  ///
+  /// \param BaseExprType the type of expression that precedes the "." or "->"
+  /// in a member access expression.
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
-   

[clang] d188916 - [Driver][DXC] Accept -f{no-, }discard-value-names in the DXC driver

[Justin Bogner via cfe-commits]

Author: Justin Bogner
Date: 2023-08-19T01:47:16-07:00
New Revision: d1889167b10a68bf25c284b040ee1ac4df844000

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

LOG: [Driver][DXC] Accept -f{no-,}discard-value-names in the DXC driver

Added: 
clang/test/Driver/dxc_valuenames.hlsl

Modified: 
clang/include/clang/Driver/Options.td

Removed: 




diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index f7c9bb854eb740..19a2abf494363c 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1870,9 +1870,11 @@ defm safe_buffer_usage_suggestions : 
BoolFOption<"safe-buffer-usage-suggestions"
   PosFlag,
   NegFlag>;
-def fdiscard_value_names : Flag<["-"], "fdiscard-value-names">, 
Group,
+def fdiscard_value_names : Flag<["-"], "fdiscard-value-names">,
+  Group, Visibility<[ClangOption, DXCOption]>,
   HelpText<"Discard value names in LLVM IR">, Flags<[NoXarchOption]>;
-def fno_discard_value_names : Flag<["-"], "fno-discard-value-names">, 
Group,
+def fno_discard_value_names : Flag<["-"], "fno-discard-value-names">,
+  Group, Visibility<[ClangOption, DXCOption]>,
   HelpText<"Do not discard value names in LLVM IR">, Flags<[NoXarchOption]>;
 defm dollars_in_identifiers : BoolFOption<"dollars-in-identifiers",
   LangOpts<"DollarIdents">, Default,

diff  --git a/clang/test/Driver/dxc_valuenames.hlsl 
b/clang/test/Driver/dxc_valuenames.hlsl
new file mode 100644
index 00..29ee912e540468
--- /dev/null
+++ b/clang/test/Driver/dxc_valuenames.hlsl
@@ -0,0 +1,4 @@
+// RUN: %clang_dxc -### -T lib_6_6 -Fc - -fdiscard-value-names %s 2>&1 | 
FileCheck -check-prefix=CHECK-DISCARD-NAMES %s
+// RUN: %clang_dxc -### -T lib_6_6 -Fc - -fno-discard-value-names %s 2>&1 | 
FileCheck -check-prefix=CHECK-NO-DISCARD-NAMES %s
+// CHECK-DISCARD-NAMES: "-discard-value-names"
+// CHECK-NO-DISCARD-NAMES-NOT: "-discard-value-names"



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

[PATCH] D154382: [ClangRepl] support code completion at a REPL

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev accepted this revision.
v.g.vassilev added a comment.

Thanks for working on this @capfredf. Let me know if I should land that for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154382

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

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

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

steakhal requested changes to this revision.
steakhal added subscribers: tbaeder, ABataev.
steakhal added a comment.
This revision now requires changes to proceed.

I think we have two 2 TPs, that were interesting to see. I've CCd the code 
owners of the affected parts to confirm this.

As a general note,this patch is not NFC, thus it's expected to demonstrate the 
behavior difference by a test.
Without tests, at least in the Static Analyzer, we generally don't accept 
patches.

---

I've seen a few similar patches from you @Manna and probably some related folks.
Could you please clarify the motivation and the expectations? Or feel free to 
point me to the Discuss post around the subject if I happened to miss it.




Comment at: clang/lib/AST/Interp/InterpBlock.cpp:94-95
 
 DeadBlock::DeadBlock(DeadBlock *, Block *Blk)
-: Root(Root), B(Blk->Desc, Blk->IsStatic, Blk->IsExtern, /*isDead=*/true) {
+: Root(Root), B(Blk->Desc, Blk->IsExtern, Blk->IsStatic, /*isDead=*/true) {
   // Add the block to the chain of dead blocks.

I think this is a TP.
I find this unfortunate, that while all the ˙Block` ctors accepts the 
parameters in the order how the backing fields are defined - except for the 
`isDead` ctor overload, where the `IsExtern` and `IsStatic`.

To address this, I'd recommend not swapping the parameters at the callsite, but 
rather fix the ctor overload to expect the parameters in the right order as the 
rest of the ctors do. And of course, check all the callsites to this weird 
constructor to fix them as well.

WDYT @tbaeder



Comment at: clang/lib/Sema/SemaOpenMP.cpp:18098-18101
   case OMPC_allocate:
 Res = ActOnOpenMPAllocateClause(Data.DepModOrTailExpr, VarList, StartLoc,
-LParenLoc, ColonLoc, EndLoc);
+ColonLoc, LParenLoc, EndLoc);
 break;

Looks like another TP.
WDYT @ABataev



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1468-1477
   std::optional getRangeForNegatedSymSym(const SymSymExpr *SSE) {
 return getRangeForNegatedExpr(
 [SSE, State = this->State]() -> SymbolRef {
   if (SSE->getOpcode() == BO_Sub)
 return State->getSymbolManager().getSymSymExpr(
-SSE->getRHS(), BO_Sub, SSE->getLHS(), SSE->getType());
+SSE->getLHS(), BO_Sub, SSE->getRHS(), SSE->getType());
   return nullptr;

Now this is within my realm.
This code applies the following transformation: `-(X - Y)  =>  (Y - X)` .
Consequently, this is intentional.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1529-1536
   // If ranges were not previously found,
   // try to find a reversed expression (y > x).
   if (!QueriedRangeSet) {
 const BinaryOperatorKind ROP =
 BinaryOperator::reverseComparisonOp(QueriedOP);
-SymSym = SymMgr.getSymSymExpr(RHS, ROP, LHS, T);
+SymSym = SymMgr.getSymSymExpr(LHS, ROP, RHS, T);
 QueriedRangeSet = getConstraint(State, SymSym);

If you have recommendations on improving the comments of the surrounding 
context, let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285

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

[clang] b627bde - [Interp] Modernize State (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-19T00:27:12-07:00
New Revision: b627bde4d5cdbdb8c72c6a94f39e145f5cb28887

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

LOG: [Interp] Modernize State (NFC)

Added: 


Modified: 
clang/lib/AST/Interp/State.h

Removed: 




diff  --git a/clang/lib/AST/Interp/State.h b/clang/lib/AST/Interp/State.h
index d897b7c2027561..f1e8e3618f34fe 100644
--- a/clang/lib/AST/Interp/State.h
+++ b/clang/lib/AST/Interp/State.h
@@ -71,7 +71,7 @@ class State {
   virtual unsigned getCallStackDepth() = 0;
 
 public:
-  State() : InConstantContext(false) {}
+  State() = default;
   /// Diagnose that the evaluation could not be folded (FF => FoldFailure)
   OptionalDiagnostic
   FFDiag(SourceLocation Loc,
@@ -121,7 +121,7 @@ class State {
 
   /// Whether or not we're in a context where the front end requires a
   /// constant value.
-  bool InConstantContext;
+  bool InConstantContext = false;
 
 private:
   void addCallStack(unsigned Limit);



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

[clang] fbaf5cb - [StaticAnalyzer] Modernize EventDispatcher (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-19T00:27:11-07:00
New Revision: fbaf5cbb734fb1ae1f905a3d3657a09ccbeba86a

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

LOG: [StaticAnalyzer] Modernize EventDispatcher (NFC)

Added: 


Modified: 
clang/include/clang/StaticAnalyzer/Core/Checker.h

Removed: 




diff  --git a/clang/include/clang/StaticAnalyzer/Core/Checker.h 
b/clang/include/clang/StaticAnalyzer/Core/Checker.h
index b92f0e1e9f0f47..8a46282a595eae 100644
--- a/clang/include/clang/StaticAnalyzer/Core/Checker.h
+++ b/clang/include/clang/StaticAnalyzer/Core/Checker.h
@@ -533,9 +533,9 @@ class Checker : public CHECK1, public CheckerBase {
 
 template 
 class EventDispatcher {
-  CheckerManager *Mgr;
+  CheckerManager *Mgr = nullptr;
 public:
-  EventDispatcher() : Mgr(nullptr) { }
+  EventDispatcher() = default;
 
   template 
   static void _register(CHECKER *checker, CheckerManager ) {



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

[clang] 519ea98 - [Sema] Modernize VirtSpecifiers (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-19T00:27:09-07:00
New Revision: 519ea98044127a0abd8f0648404ae55ce2c05c70

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

LOG: [Sema] Modernize VirtSpecifiers (NFC)

Added: 


Modified: 
clang/include/clang/Sema/DeclSpec.h

Removed: 




diff  --git a/clang/include/clang/Sema/DeclSpec.h 
b/clang/include/clang/Sema/DeclSpec.h
index c63378c732908d..4a5d7145803979 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -2705,7 +2705,7 @@ class VirtSpecifiers {
 VS_Abstract = 16
   };
 
-  VirtSpecifiers() : Specifiers(0), LastSpecifier(VS_None) { }
+  VirtSpecifiers() = default;
 
   bool SetSpecifier(Specifier VS, SourceLocation Loc,
 const char *);
@@ -2729,8 +2729,8 @@ class VirtSpecifiers {
   Specifier getLastSpecifier() const { return LastSpecifier; }
 
 private:
-  unsigned Specifiers;
-  Specifier LastSpecifier;
+  unsigned Specifiers = 0;
+  Specifier LastSpecifier = VS_None;
 
   SourceLocation VS_overrideLoc, VS_finalLoc, VS_abstractLoc;
   SourceLocation FirstLocation;



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

[clang-tools-extra] 2fbbb7e - [clangd] Modernize SymbolLocation::Position (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-19T00:27:08-07:00
New Revision: 2fbbb7efe0d23c69029f175b6b0555b45fcf9c0e

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

LOG: [clangd] Modernize SymbolLocation::Position (NFC)

Added: 


Modified: 
clang-tools-extra/clangd/index/SymbolLocation.h

Removed: 




diff  --git a/clang-tools-extra/clangd/index/SymbolLocation.h 
b/clang-tools-extra/clangd/index/SymbolLocation.h
index eadf211bb3bf6a..ea7d605172e477 100644
--- a/clang-tools-extra/clangd/index/SymbolLocation.h
+++ b/clang-tools-extra/clangd/index/SymbolLocation.h
@@ -30,7 +30,7 @@ struct SymbolLocation {
   // Position is encoded into 32 bits to save space.
   // If Line/Column overflow, the value will be their maximum value.
   struct Position {
-Position() : LineColumnPacked(0) {}
+Position() = default;
 void setLine(uint32_t Line);
 uint32_t line() const { return LineColumnPacked >> ColumnBits; }
 void setColumn(uint32_t Column);
@@ -46,7 +46,7 @@ struct SymbolLocation {
 static constexpr uint32_t MaxColumn = (1 << ColumnBits) - 1;
 
   private:
-uint32_t LineColumnPacked; // Top 20 bit line, bottom 12 bits column.
+uint32_t LineColumnPacked = 0; // Top 20 bit line, bottom 12 bits column.
   };
 
   /// The symbol range, using half-open range [Start, End).



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

[clang] f237da1 - [AST] Use DenseMap::lookup (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-18T23:52:57-07:00
New Revision: f237da1a2f6b89d8a832d37d64fb57795bfa7b9a

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

LOG: [AST] Use DenseMap::lookup (NFC)

Added: 


Modified: 
clang/include/clang/AST/VTableBuilder.h

Removed: 




diff  --git a/clang/include/clang/AST/VTableBuilder.h 
b/clang/include/clang/AST/VTableBuilder.h
index 1bf7d0467aa3fb..fbf6c041a1ec11 100644
--- a/clang/include/clang/AST/VTableBuilder.h
+++ b/clang/include/clang/AST/VTableBuilder.h
@@ -279,7 +279,7 @@ class VTableLayout {
 
   AddressPointLocation getAddressPoint(BaseSubobject Base) const {
 assert(AddressPoints.count(Base) && "Did not find address point!");
-return AddressPoints.find(Base)->second;
+return AddressPoints.lookup(Base);
   }
 
   const AddressPointsMapTy () const {



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

[clang-tools-extra] 11e2975 - Fx typos in documentation

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-18T23:36:04-07:00
New Revision: 11e2975810acd6abde9071818e03634d99492b54

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

LOG: Fx typos in documentation

Added: 


Modified: 
clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
clang/docs/ClangFormatStyleOptions.rst
clang/docs/DebuggingCoroutines.rst
clang/docs/LanguageExtensions.rst
clang/docs/OpenCLSupport.rst
clang/docs/ReleaseNotes.rst
libc/docs/porting.rst
libcxx/docs/DesignDocs/FileTimeType.rst
lldb/source/Plugins/TraceExporter/docs/htr.rst
llvm/docs/ScudoHardenedAllocator.rst
openmp/docs/CommandLineArgumentReference.rst
openmp/docs/design/Runtimes.rst

Removed: 




diff  --git a/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst 
b/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
index b7a3e76401c468..09f5ef01c10c38 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
@@ -7,5 +7,5 @@ Finds improper usages of `XCTAssertEqual` and 
`XCTAssertNotEqual` and replaces
 them with `XCTAssertEqualObjects` or `XCTAssertNotEqualObjects`.
 
 This makes tests less fragile, as many improperly rely on pointer equality for
-strings that have equal values.  This assumption is not guarantted by the
+strings that have equal values.  This assumption is not guaranteed by the
 language.

diff  --git a/clang/docs/ClangFormatStyleOptions.rst 
b/clang/docs/ClangFormatStyleOptions.rst
index 651fa4436dfe78..3b3f6f2860906a 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -5097,7 +5097,7 @@ the configuration (without a prefix: ``Auto``).
   AfterControlStatements: true
   AfterFunctionDefinitionName: true
 
-  * ``bool AfterControlStatements`` If ``true``, put space betwee control 
statement keywords
+  * ``bool AfterControlStatements`` If ``true``, put space between control 
statement keywords
 (for/if/while...) and opening parentheses.
 
 .. code-block:: c++

diff  --git a/clang/docs/DebuggingCoroutines.rst 
b/clang/docs/DebuggingCoroutines.rst
index 842f7645f967bf..53bdd08fdbc02f 100644
--- a/clang/docs/DebuggingCoroutines.rst
+++ b/clang/docs/DebuggingCoroutines.rst
@@ -501,7 +501,7 @@ So we can use the ``continuation`` field to construct the 
asynchronous stack:
   # In the example, the continuation is the first field member of the 
promise_type.
   # So they have the same addresses.
   # If we want to generalize the scripts to other coroutine types, we 
need to be sure
-  # the continuation field is the first memeber of promise_type.
+  # the continuation field is the first member of promise_type.
   self.continuation_addr = self.promise_addr
 
   def next_task_addr(self):

diff  --git a/clang/docs/LanguageExtensions.rst 
b/clang/docs/LanguageExtensions.rst
index c6f781dfe35e89..a51dea03fcb655 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -2410,7 +2410,7 @@ this always needs to be called to grow the table.
 It takes three arguments. The first argument is the WebAssembly table
 to grow. The second argument is the reference typed value to store in
 the new table entries (the initialization value), and the third argument
-is the amound to grow the table by. It returns the previous table size
+is the amount to grow the table by. It returns the previous table size
 or -1. It will return -1 if not enough space could be allocated.
 
 .. code-block:: c++

diff  --git a/clang/docs/OpenCLSupport.rst b/clang/docs/OpenCLSupport.rst
index 43c30970d113bf..f9b6564a9ebaac 100644
--- a/clang/docs/OpenCLSupport.rst
+++ b/clang/docs/OpenCLSupport.rst
@@ -333,7 +333,7 @@ Missing features or with limited support
 - IR generation for non-trivial global destructors is incomplete (See:
   `PR48047 `_).
 
-- Support of `destrutors with non-default address spaces
+- Support of `destructors with non-default address spaces
   
`_
   is incomplete (See: `D109609 `_).
 

diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 054c06ffa0f5c9..d76005692aa809 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,7 +173,7 @@ Bug Fixes to C++ Support
   a Unicode character whose name contains a ``-``.
   (Fixes `#64161 `_)
 
-- Fix cases where we ignore ambiguous name lookup when looking up memebers.
+- Fix cases where we ignore ambiguous name 

[clang] 11e2975 - Fx typos in documentation

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-08-18T23:36:04-07:00
New Revision: 11e2975810acd6abde9071818e03634d99492b54

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

LOG: Fx typos in documentation

Added: 


Modified: 
clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
clang/docs/ClangFormatStyleOptions.rst
clang/docs/DebuggingCoroutines.rst
clang/docs/LanguageExtensions.rst
clang/docs/OpenCLSupport.rst
clang/docs/ReleaseNotes.rst
libc/docs/porting.rst
libcxx/docs/DesignDocs/FileTimeType.rst
lldb/source/Plugins/TraceExporter/docs/htr.rst
llvm/docs/ScudoHardenedAllocator.rst
openmp/docs/CommandLineArgumentReference.rst
openmp/docs/design/Runtimes.rst

Removed: 




diff  --git a/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst 
b/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
index b7a3e76401c468..09f5ef01c10c38 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/objc/assert-equals.rst
@@ -7,5 +7,5 @@ Finds improper usages of `XCTAssertEqual` and 
`XCTAssertNotEqual` and replaces
 them with `XCTAssertEqualObjects` or `XCTAssertNotEqualObjects`.
 
 This makes tests less fragile, as many improperly rely on pointer equality for
-strings that have equal values.  This assumption is not guarantted by the
+strings that have equal values.  This assumption is not guaranteed by the
 language.

diff  --git a/clang/docs/ClangFormatStyleOptions.rst 
b/clang/docs/ClangFormatStyleOptions.rst
index 651fa4436dfe78..3b3f6f2860906a 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -5097,7 +5097,7 @@ the configuration (without a prefix: ``Auto``).
   AfterControlStatements: true
   AfterFunctionDefinitionName: true
 
-  * ``bool AfterControlStatements`` If ``true``, put space betwee control 
statement keywords
+  * ``bool AfterControlStatements`` If ``true``, put space between control 
statement keywords
 (for/if/while...) and opening parentheses.
 
 .. code-block:: c++

diff  --git a/clang/docs/DebuggingCoroutines.rst 
b/clang/docs/DebuggingCoroutines.rst
index 842f7645f967bf..53bdd08fdbc02f 100644
--- a/clang/docs/DebuggingCoroutines.rst
+++ b/clang/docs/DebuggingCoroutines.rst
@@ -501,7 +501,7 @@ So we can use the ``continuation`` field to construct the 
asynchronous stack:
   # In the example, the continuation is the first field member of the 
promise_type.
   # So they have the same addresses.
   # If we want to generalize the scripts to other coroutine types, we 
need to be sure
-  # the continuation field is the first memeber of promise_type.
+  # the continuation field is the first member of promise_type.
   self.continuation_addr = self.promise_addr
 
   def next_task_addr(self):

diff  --git a/clang/docs/LanguageExtensions.rst 
b/clang/docs/LanguageExtensions.rst
index c6f781dfe35e89..a51dea03fcb655 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -2410,7 +2410,7 @@ this always needs to be called to grow the table.
 It takes three arguments. The first argument is the WebAssembly table
 to grow. The second argument is the reference typed value to store in
 the new table entries (the initialization value), and the third argument
-is the amound to grow the table by. It returns the previous table size
+is the amount to grow the table by. It returns the previous table size
 or -1. It will return -1 if not enough space could be allocated.
 
 .. code-block:: c++

diff  --git a/clang/docs/OpenCLSupport.rst b/clang/docs/OpenCLSupport.rst
index 43c30970d113bf..f9b6564a9ebaac 100644
--- a/clang/docs/OpenCLSupport.rst
+++ b/clang/docs/OpenCLSupport.rst
@@ -333,7 +333,7 @@ Missing features or with limited support
 - IR generation for non-trivial global destructors is incomplete (See:
   `PR48047 `_).
 
-- Support of `destrutors with non-default address spaces
+- Support of `destructors with non-default address spaces
   
`_
   is incomplete (See: `D109609 `_).
 

diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 054c06ffa0f5c9..d76005692aa809 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,7 +173,7 @@ Bug Fixes to C++ Support
   a Unicode character whose name contains a ``-``.
   (Fixes `#64161 `_)
 
-- Fix cases where we ignore ambiguous name lookup when looking up memebers.
+- Fix cases where we ignore ambiguous name