[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

This looks great to me, thanks. @rjmccall should sign off on it though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I'd vote for something like `-fassume-nothrow-exception-dtor` or 
`-fenforce-nothrow-exception-dtor` depending on if the patch will also 
implement the enforcement mentioned in 
https://github.com/llvm/llvm-project/issues/57375#issuecomment-1230590204, but 
we can bikeshed that on the patch :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D108905#4654403 , @ChuanqiXu wrote:

> In D108905#4654393 , @smeenai wrote:
>
>> In D108905#2975712 , @rsmith wrote:
>>
>>> No decision as yet, but so far it looks very likely that we'll settle on 
>>> the rule that exceptions cannot have potentially-throwing destructors, and 
>>> that we should reject `throw`s of such types. I don't think that should be 
>>> applied retroactively to C++98, though, because destructors were not 
>>> implicitly non-throwing back then.
>>
>> Were there any updates here? This seems like a useful change for us, and I 
>> was wondering if it could be unblocked now.
>
> What @rsmith talked about is in the standard-wise. I mean, if you feel this 
> is useful, it is possible to add an compiler option to enable this. I've 
> filed an issue to track this: 
> https://github.com/llvm/llvm-project/issues/57375. My plan was to make this 
> happen this year.

Yup, an option to enable this would be great for us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.
Herald added subscribers: pmatos, asb.
Herald added a project: All.

In D108905#2975712 , @rsmith wrote:

> No decision as yet, but so far it looks very likely that we'll settle on the 
> rule that exceptions cannot have potentially-throwing destructors, and that 
> we should reject `throw`s of such types. I don't think that should be applied 
> retroactively to C++98, though, because destructors were not implicitly 
> non-throwing back then.

Were there any updates here? This seems like a useful change for us, and I was 
wondering if it could be unblocked now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905

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

[PATCH] D140925: [CMake] Use Clang to infer the target triple

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: runtimes/CMakeLists.txt:167
 
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+  set(CXX_TARGET_TRIPLE ${CMAKE_CXX_COMPILER} --target=${LLVM_RUNTIME_TRIPLE} 
-print-target-triple)

ldionne wrote:
> Is there any reason why you're carving out Apple here? Is it because 
> `-print-target-triple` doesn't work properly on that platform (I think this 
> rings a bell).
> 
> Anyway, this is non-blocking.
I think it's cos `LLVM_PER_TARGET_RUNTIME_DIR` is not supported for Apple 
platforms in general. I see a bunch of similar conditionals in other parts of 
compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140925

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158476#4647652 , @MaskRay wrote:

> nits

Sorry, this came in right as I committed the diff. I pushed 
rG915ebb07dfc53486eccf0dc09b6411929a463e98 
 to 
address it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG58288c6c1214: [driver] Search for compatible Android runtime 
directories (authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android23/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android29/libc++.so
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android23/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android29/libclang_rt.builtins.a
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/android-unversioned-fallback-warning.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -17,12 +17,25 @@
 // RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
-// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
 
 // RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
-// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android26 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+
+// RUN: %clang --target=aarch64-unknown-linux-android29 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
+// CHECK-LIBCXX-ANDROID29: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android31 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
 
 // RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
@@ -45,13 +58,29 @@
 // RUN: --target=aarch64-unknown-linux-android21 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
-// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android23 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
-// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android26 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android29 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
+// CHECK-FILE-NAME-ANDROID29: lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libclang_rt.builtins.a
+
+// RUN: 

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb1e3cd1d7944: [driver] Conditionally include installed 
libc++ headers for Android (authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159292

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/android-ndk-standalone.cpp
  clang/test/Driver/android-no-installed-libcxx.cpp
  clang/test/Driver/linux-header-search.cpp
  clang/test/Driver/linux-musl-header-search.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -9,7 +9,7 @@
 // CHECK-PER-TARGET-RUNTIME: "-cc1"
 // CHECK-PER-TARGET-RUNTIME: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-PER-TARGET-RUNTIME: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}/../include/c++/v1"
+// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}{{/|}}..{{/|}}include{{/|}}c++{{/|}}v1"
 // CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
Index: clang/test/Driver/linux-musl-header-search.cpp
===
--- clang/test/Driver/linux-musl-header-search.cpp
+++ clang/test/Driver/linux-musl-header-search.cpp
@@ -9,7 +9,7 @@
 // This is different from a glibc-based distribution.
 // CHECK-X86-64-LIBCXX: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // CHECK-X86-64-LIBCXX: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]{{/|}}usr{{/|}}include{{/|}}c++{{/|}}v1"
 // CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-X86-64-LIBCXX: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
 // CHECK-X86-64-LIBCXX: "-internal-isystem" "[[RESOURCE_DIR]]{{/|}}include"
Index: clang/test/Driver/linux-header-search.cpp
===
--- clang/test/Driver/linux-header-search.cpp
+++ clang/test/Driver/linux-header-search.cpp
@@ -13,8 +13,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-SYSROOT %s
 // CHECK-BASIC-LIBCXX-SYSROOT: "-cc1"
 // CHECK-BASIC-LIBCXX-SYSROOT: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]][[SEP:/|]]usr[[SEP]]include[[SEP]]x86_64-unknown-linux-gnu[[SEP]]c++[[SEP]]v1"
+// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]][[SEP]]usr[[SEP]]include[[SEP]]c++[[SEP]]v1"
 // CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 
 // Test include paths when the sysroot path ends with `/`.
@@ -28,8 +28,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-SYSROOT-SLASH %s
 // CHECK-BASIC-LIBCXX-SYSROOT-SLASH: "-cc1"
 // CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-isysroot" "[[SYSROOT:[^"]+/]]"
-// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/c++/v1"
+// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr[[SEP:/|]]include[[SEP]]x86_64-unknown-linux-gnu[[SEP]]c++[[SEP]]v1"
+// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr[[SEP]]include[[SEP]]c++[[SEP]]v1"
 // CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/local/include"
 
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
@@ -42,8 +42,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-INSTALL %s
 // CHECK-BASIC-LIBCXX-INSTALL: "-cc1"
 // CHECK-BASIC-LIBCXX-INSTALL: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin/../include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin/../include/c++/v1"
+// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin[[SEP:/|]]..[[SEP]]include[[SEP]]x86_64-unknown-linux-gnu[[SEP]]c++[[SEP]]v1"
+// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin[[SEP]]..[[SEP]]include[[SEP]]c++[[SEP]]v1"
 // CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 //
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
@@ -56,8 

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1212d1b51125: [driver] Perform fallback target searches for 
stdlib (authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159293

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/bin/.keep
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -14,6 +14,21 @@
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
 
+// RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID %s
+// CHECK-LIBCXX-ANDROID: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
 // RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name 2>&1 \
 // RUN: --target=x86_64-unknown-linux-gnu \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
Index: clang/test/Driver/android-installed-libcxx.cpp
===
--- clang/test/Driver/android-installed-libcxx.cpp
+++ clang/test/Driver/android-installed-libcxx.cpp
@@ -18,5 +18,9 @@
 // RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
 // RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
 
+// RUN: %clang -target aarch64-none-linux-android21 -ccc-install-dir %/t2/bin \
+// RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
+// RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
+
 // ANDROID-DIR: "-internal-isystem" "[[DIR]][[SEP:/|]]..[[SEP]]include[[SEP]]aarch64-none-linux-android[[SEP]]c++[[SEP]]v1"
 // ANDROID-DIR-SAME: "-internal-isystem" "[[DIR]][[SEP]]..[[SEP]]include[[SEP]]c++[[SEP]]v1"
Index: clang/lib/Driver/ToolChains/VEToolchain.cpp
===
--- clang/lib/Driver/ToolChains/VEToolchain.cpp
+++ clang/lib/Driver/ToolChains/VEToolchain.cpp
@@ -45,11 +45,11 @@
   getFilePaths().clear();
 
   // Add library directories:
-  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPaths)
+  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPath)
   //   ${RESOURCEDIR}/lib/linux/ve, (== getArchSpecificLibPaths)
   //   ${SYSROOT}/opt/nec/ve/lib,
-  for (auto  : getStdlibPaths())
-getFilePaths().push_back(std::move(Path));
+  if (std::optional Path = getStdlibPath())
+getFilePaths().push_back(std::move(*Path));
   for (const auto  : getArchSpecificLibPaths())
 getFilePaths().push_back(Path);
   getFilePaths().push_back(computeSysRoot() + "/opt/nec/ve/lib");
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -3100,7 +3100,6 @@
llvm::opt::ArgStringList ) const {
   const Driver  = getDriver();
   std::string SysRoot = computeSysRoot();
-  std::string Target = getTripleString();
 
   auto AddIncludePath = [&](StringRef Path, bool TargetDirRequired = false) {
 std::string Version = detectLibcxxVersion(Path);
@@ -3108,11 +3107,17 @@
   return false;
 
 // First add the per-target include path if it exists.
-SmallString<128> TargetDir(Path);
-llvm::sys::path::append(TargetDir, Target, "c++", Version);
-if (D.getVFS().exists(TargetDir))
-  

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D159292#4646811 , @rprichard wrote:

> In D159292#4646096 , @smeenai wrote:
>
>> Ping.
>
> I asked a couple of other people at Google to take a look at the patches. 
> Someone should get to it next week I think?

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159292

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159292

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

[PATCH] D159486: Fix build error in CI.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I don't think pre-merge testing covers Mac, only Linux and Windows (which is 
presumably how the original breakage got in). If the test passes for you 
locally I'd just push it and monitor the bots afterwards. (That isn't general 
advice, but in this specific case, since we already have broken bots, I think 
it's worth prioritizing a potential fix for them over waiting for previously 
known-good configurations.)


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

https://reviews.llvm.org/D159486

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

[PATCH] D159486: Fix build error in CI.

[Shoaib Meenai via Phabricator via cfe-commits]

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

The `-triple x86_64-unknown-unknown` shouldn't be required after the regex 
change, but I'm fine either way. I'd recommend pushing this to fix the bots, 
and Aaron/Nico can do a post-commit review if they have any other comments.


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

https://reviews.llvm.org/D159486

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

[PATCH] D159486: Fix build error in CI.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Generally it's preferred to either push bot failure fixes directly or revert 
the original commit (also without review), to get bots unblocked as soon as 
possible :)

From what I can see, the test failure is because `dso_local` isn't emitted on 
Mac. That seems incidental to the point of your test, so using a regex match 
instead might be preferable.


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

https://reviews.llvm.org/D159486

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Ping. I added the warning and release note. I haven't made search paths be 
dumped in `-v` mode because I thought it could be pretty noisy (especially 
since you'll be doing three searches, for resource dir libraries, libc++ 
per-target headers, and non-resource dir libraries), but I'm happy to do so if 
people think it'll be useful (or add it under a different flag).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D159292#4632389 , @rprichard wrote:

> This change looks like an improvement to me. The NDK currently puts the 
> libc++ headers into the sysroot, but putting it in the usual toolchain 
> include location seems better.

Are you comfortable accepting this, or should I ping other reviewers? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159292

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

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 555247.
smeenai added a comment.

Rebase to avoid pre-existing CI failure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159293

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/bin/.keep
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -14,6 +14,21 @@
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
 
+// RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID %s
+// CHECK-LIBCXX-ANDROID: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
 // RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name 2>&1 \
 // RUN: --target=x86_64-unknown-linux-gnu \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
Index: clang/test/Driver/android-installed-libcxx.cpp
===
--- clang/test/Driver/android-installed-libcxx.cpp
+++ clang/test/Driver/android-installed-libcxx.cpp
@@ -18,5 +18,9 @@
 // RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
 // RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
 
+// RUN: %clang -target aarch64-none-linux-android21 -ccc-install-dir %/t2/bin \
+// RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
+// RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
+
 // ANDROID-DIR: "-internal-isystem" "[[DIR]][[SEP:/|]]..[[SEP]]include[[SEP]]aarch64-none-linux-android[[SEP]]c++[[SEP]]v1"
 // ANDROID-DIR-SAME: "-internal-isystem" "[[DIR]][[SEP]]..[[SEP]]include[[SEP]]c++[[SEP]]v1"
Index: clang/lib/Driver/ToolChains/VEToolchain.cpp
===
--- clang/lib/Driver/ToolChains/VEToolchain.cpp
+++ clang/lib/Driver/ToolChains/VEToolchain.cpp
@@ -45,11 +45,11 @@
   getFilePaths().clear();
 
   // Add library directories:
-  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPaths)
+  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPath)
   //   ${RESOURCEDIR}/lib/linux/ve, (== getArchSpecificLibPaths)
   //   ${SYSROOT}/opt/nec/ve/lib,
-  for (auto  : getStdlibPaths())
-getFilePaths().push_back(std::move(Path));
+  if (std::optional Path = getStdlibPath())
+getFilePaths().push_back(std::move(*Path));
   for (const auto  : getArchSpecificLibPaths())
 getFilePaths().push_back(Path);
   getFilePaths().push_back(computeSysRoot() + "/opt/nec/ve/lib");
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -3096,7 +3096,6 @@
llvm::opt::ArgStringList ) const {
   const Driver  = getDriver();
   std::string SysRoot = computeSysRoot();
-  std::string Target = getTripleString();
 
   auto AddIncludePath = [&](StringRef Path, bool TargetDirRequired = false) {
 std::string Version = detectLibcxxVersion(Path);
@@ -3104,11 +3103,17 @@
   return false;
 
 // First add the per-target include path if it exists.
-SmallString<128> TargetDir(Path);
-llvm::sys::path::append(TargetDir, Target, "c++", Version);
-if (D.getVFS().exists(TargetDir))
-  addSystemInclude(DriverArgs, CC1Args, TargetDir);
-else if (TargetDirRequired)
+

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 555246.
smeenai added a comment.

Rebase to avoid pre-existing CI failure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159292

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/android-ndk-standalone.cpp
  clang/test/Driver/android-no-installed-libcxx.cpp
  clang/test/Driver/linux-header-search.cpp
  clang/test/Driver/linux-musl-header-search.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -9,7 +9,7 @@
 // CHECK-PER-TARGET-RUNTIME: "-cc1"
 // CHECK-PER-TARGET-RUNTIME: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-PER-TARGET-RUNTIME: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}/../include/c++/v1"
+// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}{{/|}}..{{/|}}include{{/|}}c++{{/|}}v1"
 // CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
Index: clang/test/Driver/linux-musl-header-search.cpp
===
--- clang/test/Driver/linux-musl-header-search.cpp
+++ clang/test/Driver/linux-musl-header-search.cpp
@@ -9,7 +9,7 @@
 // This is different from a glibc-based distribution.
 // CHECK-X86-64-LIBCXX: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // CHECK-X86-64-LIBCXX: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]{{/|}}usr{{/|}}include{{/|}}c++{{/|}}v1"
 // CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-X86-64-LIBCXX: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
 // CHECK-X86-64-LIBCXX: "-internal-isystem" "[[RESOURCE_DIR]]{{/|}}include"
Index: clang/test/Driver/linux-header-search.cpp
===
--- clang/test/Driver/linux-header-search.cpp
+++ clang/test/Driver/linux-header-search.cpp
@@ -13,8 +13,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-SYSROOT %s
 // CHECK-BASIC-LIBCXX-SYSROOT: "-cc1"
 // CHECK-BASIC-LIBCXX-SYSROOT: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]][[SEP:/|]]usr[[SEP]]include[[SEP]]x86_64-unknown-linux-gnu[[SEP]]c++[[SEP]]v1"
+// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]][[SEP]]usr[[SEP]]include[[SEP]]c++[[SEP]]v1"
 // CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 
 // Test include paths when the sysroot path ends with `/`.
@@ -28,8 +28,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-SYSROOT-SLASH %s
 // CHECK-BASIC-LIBCXX-SYSROOT-SLASH: "-cc1"
 // CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-isysroot" "[[SYSROOT:[^"]+/]]"
-// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/c++/v1"
+// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr[[SEP:/|]]include[[SEP]]x86_64-unknown-linux-gnu[[SEP]]c++[[SEP]]v1"
+// CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr[[SEP]]include[[SEP]]c++[[SEP]]v1"
 // CHECK-BASIC-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/local/include"
 
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
@@ -42,8 +42,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-INSTALL %s
 // CHECK-BASIC-LIBCXX-INSTALL: "-cc1"
 // CHECK-BASIC-LIBCXX-INSTALL: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin/../include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin/../include/c++/v1"
+// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin[[SEP:/|]]..[[SEP]]include[[SEP]]x86_64-unknown-linux-gnu[[SEP]]c++[[SEP]]v1"
+// CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/bin[[SEP]]..[[SEP]]include[[SEP]]c++[[SEP]]v1"
 // CHECK-BASIC-LIBCXX-INSTALL: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 //
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
@@ -56,8 +56,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXXV2-SYSROOT %s
 // 

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 555213.
smeenai added a comment.

Add warning and release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android23/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android29/libc++.so
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android23/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android29/libclang_rt.builtins.a
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/android-unversioned-fallback-warning.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -17,12 +17,25 @@
 // RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
-// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
 
 // RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
-// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android26 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+
+// RUN: %clang --target=aarch64-unknown-linux-android29 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
+// CHECK-LIBCXX-ANDROID29: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android31 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
 
 // RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
@@ -45,13 +58,29 @@
 // RUN: --target=aarch64-unknown-linux-android21 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
-// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android23 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
-// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android26 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android29 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
+// CHECK-FILE-NAME-ANDROID29: lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: 

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158476#4632188 , @thakis wrote:

> Works for me. How do you imagine the transition happening? Should we emit 
> some kind of warning if the old fallback is hit?

That's a good call. I'll add that plus a release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:3108
+SmallString<128> TargetDir(Path);
+llvm::sys::path::append(TargetDir, Target, "c++", Version);
 if (D.getVFS().exists(TargetDir))

rprichard wrote:
> Will the `Target` here have an Android API level suffix?
> 
Yup, it will. D159293 and D158476 are the follow-ups to address that. The first 
one makes this call-site use the current fallback logic in the driver, and the 
second one extends that fallback logic to look for the next newest available 
API level if you don't have headers/libraries for the current API level.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159292

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a reviewer: glandium.
smeenai added a comment.

@thakis I'm inclined to leave the fallback for a triple without any version in 
place for now, since it seems like Mozilla might be relying on it too, and we 
can remove it once everyone's moved over to the new fallback mechanism. What do 
you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 555086.
smeenai added a comment.

Kick off pre-merge checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android23/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android29/libc++.so
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android23/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android29/libclang_rt.builtins.a
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -17,12 +17,25 @@
 // RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
-// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
 
 // RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
-// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android26 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+
+// RUN: %clang --target=aarch64-unknown-linux-android29 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
+// CHECK-LIBCXX-ANDROID29: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android31 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
 
 // RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
@@ -45,13 +58,29 @@
 // RUN: --target=aarch64-unknown-linux-android21 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
-// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android23 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
-// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android26 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android29 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
+// CHECK-FILE-NAME-ANDROID29: lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android31 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck 

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 555084.
smeenai added a comment.

Kick off pre-merge checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159293

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/bin/.keep
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -14,6 +14,21 @@
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
 
+// RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID %s
+// CHECK-LIBCXX-ANDROID: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
 // RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name 2>&1 \
 // RUN: --target=x86_64-unknown-linux-gnu \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
Index: clang/test/Driver/android-installed-libcxx.cpp
===
--- clang/test/Driver/android-installed-libcxx.cpp
+++ clang/test/Driver/android-installed-libcxx.cpp
@@ -18,5 +18,9 @@
 // RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
 // RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
 
+// RUN: %clang -target aarch64-none-linux-android21 -ccc-install-dir %/t2/bin \
+// RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
+// RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
+
 // ANDROID-DIR: "-internal-isystem" "[[DIR]][[SEP:/|]]..[[SEP]]include[[SEP]]aarch64-none-linux-android[[SEP]]c++[[SEP]]v1"
 // ANDROID-DIR-SAME: "-internal-isystem" "[[DIR]][[SEP]]..[[SEP]]include[[SEP]]c++[[SEP]]v1"
Index: clang/lib/Driver/ToolChains/VEToolchain.cpp
===
--- clang/lib/Driver/ToolChains/VEToolchain.cpp
+++ clang/lib/Driver/ToolChains/VEToolchain.cpp
@@ -45,11 +45,11 @@
   getFilePaths().clear();
 
   // Add library directories:
-  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPaths)
+  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPath)
   //   ${RESOURCEDIR}/lib/linux/ve, (== getArchSpecificLibPaths)
   //   ${SYSROOT}/opt/nec/ve/lib,
-  for (auto  : getStdlibPaths())
-getFilePaths().push_back(std::move(Path));
+  if (std::optional Path = getStdlibPath())
+getFilePaths().push_back(std::move(*Path));
   for (const auto  : getArchSpecificLibPaths())
 getFilePaths().push_back(Path);
   getFilePaths().push_back(computeSysRoot() + "/opt/nec/ve/lib");
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -3096,7 +3096,6 @@
llvm::opt::ArgStringList ) const {
   const Driver  = getDriver();
   std::string SysRoot = computeSysRoot();
-  std::string Target = getTripleString();
 
   auto AddIncludePath = [&](StringRef Path, bool TargetDirRequired = false) {
 std::string Version = detectLibcxxVersion(Path);
@@ -3104,11 +3103,17 @@
   return false;
 
 // First add the per-target include path if it exists.
-SmallString<128> TargetDir(Path);
-llvm::sys::path::append(TargetDir, Target, "c++", Version);
-if (D.getVFS().exists(TargetDir))
-  addSystemInclude(DriverArgs, CC1Args, TargetDir);
-else if (TargetDirRequired)
+bool 

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158475#4626856 , @glandium wrote:

> In D158475#4626636 , @smeenai wrote:
>
>> I'm halfway through an implementation of this, but I just realized that on 
>> Android, Clang never searches for C++ headers installed alongside the 
>> driver: 
>> https://github.com/llvm/llvm-project/blob/7af0eff5405bb88dc96c0b19892da0fbb44db433/clang/lib/Driver/ToolChains/Gnu.cpp#L3116-L3118.
>>  Are you using the C++ headers from the NDK but a library that you built 
>> locally, or are you manually specifying the path to your C++ headers?
>
> At the moment, we're only really using libunwind from the clang stdlib 
> directory. So we don't really have the problem with headers. It will 
> eventually be a problem when we get to use libc++, which we'll want at some 
> point.

I decided to just fix the whole thing properly: D159292 
 and D159293 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158475

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 555082.
smeenai added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android23/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android29/libc++.so
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android23/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android29/libclang_rt.builtins.a
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -17,12 +17,25 @@
 // RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
-// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
 
 // RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
 // RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
-// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android26 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+
+// RUN: %clang --target=aarch64-unknown-linux-android29 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
+// CHECK-LIBCXX-ANDROID29: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android31 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID29 %s
 
 // RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
 // RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
@@ -45,13 +58,29 @@
 // RUN: --target=aarch64-unknown-linux-android21 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
-// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android23 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
-// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android26 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android29 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
+// CHECK-FILE-NAME-ANDROID29: lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android31 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck 

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: MaskRay, phosek, thakis, glandium.
Herald added a subscriber: abrachet.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Searching for target-specific standard library header and library paths
should perform fallback searches for targets, the same way searching for
the runtime libraries does. It's important for the header and library
searches to be consistent, otherwise we could end up using mismatched
headers and libraries. (See also https://reviews.llvm.org/D146664.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159293

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/bin/.keep
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android/libc++.so
  
clang/test/Driver/Inputs/basic_android_libcxx_tree/usr/lib/aarch64-unknown-linux-android21/libc++.so
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -14,6 +14,21 @@
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
 
+// RUN: %clang --target=aarch64-unknown-linux-android21 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID21 %s
+// CHECK-LIBCXX-ANDROID21: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android23 -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID23 %s
+// CHECK-LIBCXX-ANDROID23: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
+// RUN: %clang --target=aarch64-unknown-linux-android -print-file-name=libc++.so 2>&1 \
+// RUN: -ccc-install-dir %S/Inputs/basic_android_libcxx_tree/usr/bin \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-ANDROID %s
+// CHECK-LIBCXX-ANDROID: ..{{/|\\}}lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libc++.so
+
 // RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name 2>&1 \
 // RUN: --target=x86_64-unknown-linux-gnu \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
Index: clang/test/Driver/android-installed-libcxx.cpp
===
--- clang/test/Driver/android-installed-libcxx.cpp
+++ clang/test/Driver/android-installed-libcxx.cpp
@@ -18,5 +18,9 @@
 // RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
 // RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
 
+// RUN: %clang -target aarch64-none-linux-android21 -ccc-install-dir %/t2/bin \
+// RUN:   --sysroot=%t2/sysroot -stdlib=libc++ -fsyntax-only \
+// RUN:   %s -### 2>&1 | FileCheck --check-prefix=ANDROID-DIR -DDIR=%/t2/bin %s
+
 // ANDROID-DIR: "-internal-isystem" "[[DIR]][[SEP:/|]]..[[SEP]]include[[SEP]]aarch64-none-linux-android[[SEP]]c++[[SEP]]v1"
 // ANDROID-DIR-SAME: "-internal-isystem" "[[DIR]][[SEP]]..[[SEP]]include[[SEP]]c++[[SEP]]v1"
Index: clang/lib/Driver/ToolChains/VEToolchain.cpp
===
--- clang/lib/Driver/ToolChains/VEToolchain.cpp
+++ clang/lib/Driver/ToolChains/VEToolchain.cpp
@@ -45,11 +45,11 @@
   getFilePaths().clear();
 
   // Add library directories:
-  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPaths)
+  //   ${BINPATH}/../lib/ve-unknown-linux-gnu, (== getStdlibPath)
   //   ${RESOURCEDIR}/lib/linux/ve, (== getArchSpecificLibPaths)
   //   ${SYSROOT}/opt/nec/ve/lib,
-  for (auto  : getStdlibPaths())
-getFilePaths().push_back(std::move(Path));
+  if (std::optional Path = getStdlibPath())
+getFilePaths().push_back(std::move(*Path));
   for (const auto  : getArchSpecificLibPaths())
 getFilePaths().push_back(Path);
   getFilePaths().push_back(computeSysRoot() + "/opt/nec/ve/lib");
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -3096,7 +3096,6 @@
llvm::opt::ArgStringList ) const {
   const Driver  = getDriver();
   std::string SysRoot = computeSysRoot();
-  std::string Target = getTripleString();
 
   auto AddIncludePath = 

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: danalbert, srhines, pcc, phosek, thakis, MaskRay, 
glandium.
Herald added a subscriber: danielkiss.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

https://reviews.llvm.org/D71154 prevented Clang from search for libc++
headers installed alongside the driver when targeting Android. The
motivation was the NDK's use of a different libc++ inline namespace
(`__ndk1` instead of the standard `__1`), which made regular libc++
headers incompatible with the NDK's libc++ library.

Since then, libc++ has gained the ability to install its `__config_site`
header (which controls the inline namespace, among other things) to a
per-target include directory, which enables per-target customizations.
If this directory is present, the user has expressly built libc++ for
Android, and we should use those headers.

The motivation is that, with the current setup, if a user builds their
own libc++ for Android, they'll use the library they built themselves
but the NDK's headers instead of their own, which is surprising at best
and can cause all sorts of problems (e.g. if you built your own libc++
with a different ABI configuration). It's important to match the headers
and libraries in that scenario, and checking for an Android per-target
include directory lets us do so without regressing the original scenario
which https://reviews.llvm.org/D71154 was addressing.

While I'm here, switch to using sys::path::append instead of slashes
directly, to get system path separators on Windows, which is consistent
with how library paths are constructed (and that consistency will be
important in a follow-up, where we use a common search function for the
include and library path construction).

(As an aside, one of the motivations for https://reviews.llvm.org/D71154
was to support targeting both Android and Apple platforms, which
expected libc++ headers to be provided by the toolcain at the time.
Apple has since switched to including libc++ headers in the platform SDK
instead of in the toolchain, so that specific motivation no longer
applies either.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159292

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/android-installed-libcxx.cpp
  clang/test/Driver/android-ndk-standalone.cpp
  clang/test/Driver/android-no-installed-libcxx.cpp
  clang/test/Driver/linux-header-search.cpp
  clang/test/Driver/linux-musl-header-search.cpp
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -9,7 +9,7 @@
 // CHECK-PER-TARGET-RUNTIME: "-cc1"
 // CHECK-PER-TARGET-RUNTIME: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-PER-TARGET-RUNTIME: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}/../include/c++/v1"
+// CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "{{.*}}{{/|}}..{{/|}}include{{/|}}c++{{/|}}v1"
 // CHECK-PER-TARGET-RUNTIME: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-PER-TARGET-RUNTIME: "--sysroot=[[SYSROOT]]"
 // CHECK-PER-TARGET-RUNTIME: "-L{{.*}}{{/|}}..{{/|}}lib{{/|}}x86_64-unknown-linux-gnu"
Index: clang/test/Driver/linux-musl-header-search.cpp
===
--- clang/test/Driver/linux-musl-header-search.cpp
+++ clang/test/Driver/linux-musl-header-search.cpp
@@ -9,7 +9,7 @@
 // This is different from a glibc-based distribution.
 // CHECK-X86-64-LIBCXX: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // CHECK-X86-64-LIBCXX: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]{{/|}}usr{{/|}}include{{/|}}c++{{/|}}v1"
 // CHECK-X86-64-LIBCXX: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
 // CHECK-X86-64-LIBCXX: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
 // CHECK-X86-64-LIBCXX: "-internal-isystem" "[[RESOURCE_DIR]]{{/|}}include"
Index: clang/test/Driver/linux-header-search.cpp
===
--- clang/test/Driver/linux-header-search.cpp
+++ clang/test/Driver/linux-header-search.cpp
@@ -13,8 +13,8 @@
 // RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXX-SYSROOT %s
 // CHECK-BASIC-LIBCXX-SYSROOT: "-cc1"
 // CHECK-BASIC-LIBCXX-SYSROOT: "-isysroot" "[[SYSROOT:[^"]+]]"
-// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/x86_64-unknown-linux-gnu/c++/v1"
-// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-BASIC-LIBCXX-SYSROOT: "-internal-isystem" 

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D152495#4628877 , @hans wrote:

> In D152495#4628870 , @goncharov 
> wrote:
>
>> due to this change we have a enourmous number of new warnings, on the other 
>> hand -Wunused-variable is a valuable warning. I am not sure what is the 
>> policy and best practices for warnings but maybe there is a possiblity to 
>> make a transition period for enabling this type of warning separetely and to 
>> allow updating existing code?
>
> The usual policy is to put new warnings behind new flags so users can disable 
> them selectively. It gets trickier when it's existing warnings getting 
> enhanced like this. Would it be possible to put this new functionality behind 
> a flag?

Yeah, this has been a recurring issue when existing warnings get enhanced. It's 
often not feasible to fix all new instances right away, but you don't want to 
disable or `-Wno-error` the warning either cos then new issues can start 
creeping in, leaving you with no good options. Maybe we should have a policy 
around even enhancements to existing warnings always going in their own 
subgroup, so that we can disable them selectively while not regressing 
anything? @aaron.ballman, what are your thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D152495#4628028 , @goncharov wrote:

> there is a number of unused vaiables in conditional loops that are firing 
> now, I have submitted  
> https://github.com/llvm/llvm-project/commit/74f4daef0412be33002bd4e24a30cb47d0187ecf
>  but I suspect it's just a start.
> How did you checked the project code for that?

Per rG01b88dd66d9d52d3f531a7d490e18c549f36f445 
, unused 
`dyn_cast` results should turn into `isa`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158475#4623852 , @glandium wrote:

> In D158475#4623842 , @smeenai wrote:
>
>> In D158475#4623471 , @glandium 
>> wrote:
>>
>>> This conflicts with D146664 
>>
>> It sounds like you want the same logic as D158476 
>>  to apply to the stdlib search as well as 
>> the runtimes search, right?
>
> Sounds about right.
>
>> I can make both be consistent.
>
> That would be much appreciated.

I'm halfway through an implementation of this, but I just realized that on 
Android, Clang never searches for C++ headers installed alongside the driver: 
https://github.com/llvm/llvm-project/blob/7af0eff5405bb88dc96c0b19892da0fbb44db433/clang/lib/Driver/ToolChains/Gnu.cpp#L3116-L3118.
 Are you using the C++ headers from the NDK but a library that you built 
locally, or are you manually specifying the path to your C++ headers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158475

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

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158475#4623471 , @glandium wrote:

> This conflicts with D146664 

It sounds like you want the same logic as D158476 
 to apply to the stdlib search as well as the 
runtimes search, right? I can make both be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158475

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

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

[Shoaib Meenai 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 rGb6a1473f97d3: [driver] Refactor getRuntimePaths. NFC 
(authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158475

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp

Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -86,8 +86,8 @@
   List.push_back(Path);
   };
 
-  for (const auto  : getRuntimePaths())
-addIfExists(getLibraryPaths(), Path);
+  if (std::optional Path = getRuntimePath())
+getLibraryPaths().push_back(*Path);
   for (const auto  : getStdlibPaths())
 addIfExists(getFilePaths(), Path);
   for (const auto  : getArchSpecificLibPaths())
@@ -677,15 +677,18 @@
   return Args.MakeArgString(getCompilerRT(Args, Component, Type));
 }
 
-ToolChain::path_list ToolChain::getRuntimePaths() const {
-  path_list Paths;
-  auto addPathForTriple = [this, ](const llvm::Triple ) {
+std::optional ToolChain::getRuntimePath() const {
+  auto getPathForTriple =
+  [this](const llvm::Triple ) -> std::optional {
 SmallString<128> P(D.ResourceDir);
 llvm::sys::path::append(P, "lib", Triple.str());
-Paths.push_back(std::string(P.str()));
+if (getVFS().exists(P))
+  return std::string(P);
+return {};
   };
 
-  addPathForTriple(getTriple());
+  if (auto Path = getPathForTriple(getTriple()))
+return *Path;
 
   // When building with per target runtime directories, various ways of naming
   // the Arm architecture may have been normalised to simply "arm".
@@ -705,7 +708,8 @@
   if (getTriple().getArch() == Triple::arm && !getTriple().isArmMClass()) {
 llvm::Triple ArmTriple = getTriple();
 ArmTriple.setArch(Triple::arm);
-addPathForTriple(ArmTriple);
+if (auto Path = getPathForTriple(ArmTriple))
+  return *Path;
   }
 
   // Android targets may include an API level at the end. We still want to fall
@@ -714,10 +718,11 @@
   getTriple().getEnvironmentName() != "android") {
 llvm::Triple TripleWithoutLevel = getTriple();
 TripleWithoutLevel.setEnvironmentName("android");
-addPathForTriple(TripleWithoutLevel);
+if (auto Path = getPathForTriple(TripleWithoutLevel))
+  return *Path;
   }
 
-  return Paths;
+  return {};
 }
 
 ToolChain::path_list ToolChain::getStdlibPaths() const {
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2163,16 +2163,8 @@
   }
 
   if (C.getArgs().hasArg(options::OPT_print_runtime_dir)) {
-std::string RuntimePath;
-// Get the first existing path, if any.
-for (auto Path : TC.getRuntimePaths()) {
-  if (getVFS().exists(Path)) {
-RuntimePath = Path;
-break;
-  }
-}
-if (!RuntimePath.empty())
-  llvm::outs() << RuntimePath << '\n';
+if (std::optional RuntimePath = TC.getRuntimePath())
+  llvm::outs() << *RuntimePath << '\n';
 else
   llvm::outs() << TC.getCompilerRTPath() << '\n';
 return false;
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -500,8 +501,8 @@
 StringRef Component,
 FileType Type = ToolChain::FT_Static) const;
 
-  // Returns target specific runtime paths.
-  path_list getRuntimePaths() const;
+  // Returns the target specific runtime path if it exists.
+  std::optional getRuntimePath() const;
 
   // Returns target specific standard library paths.
   path_list getStdlibPaths() const;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

D158798  demonstrates a working Android 
runtimes setup with per-target runtime directories once this change is in 
place. It'll be nicer once D140925  lands, 
but it's workable even without that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

[PATCH] D158798: [RFC] Android runtimes build demonstration

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

The manual `androideabi` normalization is only needed when building the 
runtimes, not when using them:

  $ for i in {21..32}; do printf '%s ' $i; bin/clang 
--print-file-name=libclang_rt.builtins.a -target 
armv7-none-linux-androideabi$i; done
  21 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  22 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  23 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  24 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  25 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  26 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  27 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  28 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android21/libclang_rt.builtins.a
  29 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android29/libclang_rt.builtins.a
  30 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android29/libclang_rt.builtins.a
  31 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android29/libclang_rt.builtins.a
  32 
/data/users/smeenai/llvm-project/build/Release/lib/clang/18/lib/armv7-none-linux-android29/libclang_rt.builtins.a


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158798

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

[PATCH] D158798: [RFC] Android runtimes build demonstration

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: danalbert, rprichard, srhines.
Herald added a subscriber: danielkiss.
Herald added a project: All.
smeenai requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

This demonstrates a possible Android runtimes build setup once
https://reviews.llvm.org/D158476 lands and runtimes library searching
accounts for versioned Android triples. Meta uses a similar setup
internally (except for using a standalone CMake toolchain file instead
of wrapping the NDK's; I'm not actually sure which one I prefer.)

I'm putting this up as a demonstration of aspects of the runtime build
(in particular delegating to separate toolchain and cache files, which I
find to be much more manageable than putting everything in the runtimes
cache file). If the Android toolchain maintainers think it could be
generally useful, I'd be happy for a maintainer to commandeer it and
change it as they see fit, or make any suggested changes myself.

There's some existing Android cache and toolchain files:

- clang/cmake/caches/Android.cmake
- clang/cmake/caches/Android-stage2.cmake
- llvm/cmake/platforms/Android.cmake

As far as I can tell the toolchain file is pretty bit-rotted and the
cache files don't reflect the current NDK runtimes configuration (e.g.
the use of the `__ndk1` inline namespace). I'm not changing them here,
but it'd be great if a maintainer could take a look and decide what to
do with them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158798

Files:
  clang/cmake/caches/Android-libcxx.cmake
  clang/cmake/caches/Android-runtimes.cmake
  llvm/cmake/platforms/Android-runtimes.cmake

Index: llvm/cmake/platforms/Android-runtimes.cmake
===
--- /dev/null
+++ llvm/cmake/platforms/Android-runtimes.cmake
@@ -0,0 +1,48 @@
+# A wrapper around the NDK's CMake toolchain that uses the just-built
+# compiler instead of the NDK's and tweaks flags to work with a
+# partially built toolchain, for the runtimes build.
+
+if(NOT NDK_PATH)
+  message(FATAL_ERROR "NDK_PATH must point to NDK path")
+endif()
+
+# These are passed down from the runtimes build.
+set(ORIGINAL_C_COMPILER "${CMAKE_C_COMPILER}")
+set(ORIGINAL_CXX_COMPILER "${CMAKE_CXX_COMPILER}")
+
+include("${NDK_PATH}/build/cmake/android.toolchain.cmake")
+
+set(CMAKE_C_COMPILER "${ORIGINAL_C_COMPILER}")
+set(CMAKE_CXX_COMPILER "${ORIGINAL_CXX_COMPILER}")
+
+list(APPEND CMAKE_TRY_COMPILE_PLATFORM_VARIABLES NDK_PATH)
+
+# We won't have libunwind available since we're building it. CMake's ABI
+# checks fail without this.
+# FIXME: compiler-rt uses --unwindlib=none already; does that just need
+# to be more comprehensive, or is the "Detecting compiler ABI info"
+# check ingrained enough in CMake that only the toolchain file can
+# influence it?
+string(APPEND CMAKE_EXE_LINKER_FLAGS " --unwindlib=none")
+string(APPEND CMAKE_MODULE_LINKER_FLAGS " --unwindlib=none")
+string(APPEND CMAKE_SHARED_LINKER_FLAGS " --unwindlib=none")
+
+# libatomic is provided by compiler-rt and our just-built compiler
+# doesn't have the NDK's dummy libatomic available.
+string(REPLACE "-latomic" "" CMAKE_C_STANDARD_LIBRARIES_INIT "${CMAKE_C_STANDARD_LIBRARIES_INIT}")
+string(REPLACE "-latomic" "" CMAKE_CXX_STANDARD_LIBRARIES_INIT "${CMAKE_CXX_STANDARD_LIBRARIES_INIT}")
+
+# Fortify doesn't play well with -ffreestanding, which is used heavily
+# by sanitizers, because e.g. it prevents Clang's built-in limits.h from
+# reaching out to the NDK's and supplying SSIZE_MAX.
+# FIXME: should this be in compiler-rt instead?
+string(REPLACE "-D_FORTIFY_SOURCE=2" "" CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS}")
+string(REPLACE "-D_FORTIFY_SOURCE=2" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
+string(REPLACE "-D_FORTIFY_SOURCE=2" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
+
+# Clang canonicalizes androideabi as just android, so we need to mimic
+# that to get the correct per-target installation directory.
+# FIXME: remove this once https://reviews.llvm.org/D140925 lands
+string(REPLACE "androideabi" "android" CMAKE_ASM_COMPILER_TARGET "${CMAKE_ASM_COMPILER_TARGET}")
+string(REPLACE "androideabi" "android" CMAKE_C_COMPILER_TARGET "${CMAKE_C_COMPILER_TARGET}")
+string(REPLACE "androideabi" "android" CMAKE_CXX_COMPILER_TARGET "${CMAKE_CXX_COMPILER_TARGET}")
Index: clang/cmake/caches/Android-runtimes.cmake
===
--- /dev/null
+++ clang/cmake/caches/Android-runtimes.cmake
@@ -0,0 +1,58 @@
+if(NOT NDK_PATH)
+  message(FATAL_ERROR "NDK_PATH must point to NDK path")
+endif()
+
+set(LLVM_ENABLE_PROJECTS
+  clang
+  lld
+  CACHE STRING "")
+
+set(LLVM_ENABLE_RUNTIMES
+  compiler-rt
+  libunwind
+  libcxxabi
+  libcxx
+  CACHE STRING "")
+
+function(android_runtime arch api_level target)
+  if(arch STREQUAL armv7)
+set(abi armeabi-v7a)
+  elseif(arch STREQUAL aarch64)
+set(abi 

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 553291.
smeenai added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android23/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android29/libclang_rt.builtins.a
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -30,13 +30,29 @@
 // RUN: --target=aarch64-unknown-linux-android21 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
-// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android23 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
-// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android26 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android29 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
+// CHECK-FILE-NAME-ANDROID29: lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android31 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android \
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -677,6 +677,46 @@
   return Args.MakeArgString(getCompilerRT(Args, Component, Type));
 }
 
+// Android target triples contain a target version. If we don't have libraries
+// for the exact target version, we should fall back to the next newest version
+// or a versionless path, if any.
+std::optional ToolChain::getFallbackAndroidRuntimePath() const {
+  llvm::Triple TripleWithoutLevel(getTriple());
+  TripleWithoutLevel.setEnvironmentName("android"); // remove any version number
+  const std::string  = TripleWithoutLevel.str();
+  unsigned TripleVersion = getTriple().getEnvironmentVersion().getMajor();
+  unsigned BestVersion = 0;
+
+  SmallString<128> LibDir(D.ResourceDir);
+  llvm::sys::path::append(LibDir, "lib");
+  SmallString<32> TripleDir;
+  std::error_code EC;
+  for (llvm::vfs::directory_iterator LI = getVFS().dir_begin(LibDir, EC), LE;
+   !EC && LI != LE; LI = LI.increment(EC)) {
+StringRef DirName = llvm::sys::path::filename(LI->path());
+StringRef DirNameSuffix = DirName;
+if (DirNameSuffix.consume_front(TripleWithoutLevelStr)) {
+  if (DirNameSuffix.empty() && TripleDir.empty()) {
+TripleDir = DirName;
+  } else {
+unsigned Version;
+if (!DirNameSuffix.getAsInteger(10, Version) && Version > BestVersion &&
+Version < TripleVersion) {
+  BestVersion = Version;
+  TripleDir = DirName;
+}
+  }
+}
+  }
+
+  if (!TripleDir.empty()) {
+llvm::sys::path::append(LibDir, TripleDir);
+return std::string(LibDir);
+  }
+
+  return {};
+}
+
 std::optional ToolChain::getRuntimePath() const {
   auto getPathForTriple =
   [this](const llvm::Triple ) -> std::optional {
@@ -712,15 +752,8 @@
   return *Path;
   }
 
-  // Android targets may include an API level at the end. We still want to fall
-  // back on a path without the API 

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 553290.
smeenai added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158475

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp

Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -86,8 +86,8 @@
   List.push_back(Path);
   };
 
-  for (const auto  : getRuntimePaths())
-addIfExists(getLibraryPaths(), Path);
+  if (std::optional Path = getRuntimePath())
+getLibraryPaths().push_back(*Path);
   for (const auto  : getStdlibPaths())
 addIfExists(getFilePaths(), Path);
   for (const auto  : getArchSpecificLibPaths())
@@ -677,15 +677,18 @@
   return Args.MakeArgString(getCompilerRT(Args, Component, Type));
 }
 
-ToolChain::path_list ToolChain::getRuntimePaths() const {
-  path_list Paths;
-  auto addPathForTriple = [this, ](const llvm::Triple ) {
+std::optional ToolChain::getRuntimePath() const {
+  auto getPathForTriple =
+  [this](const llvm::Triple ) -> std::optional {
 SmallString<128> P(D.ResourceDir);
 llvm::sys::path::append(P, "lib", Triple.str());
-Paths.push_back(std::string(P.str()));
+if (getVFS().exists(P))
+  return std::string(P);
+return {};
   };
 
-  addPathForTriple(getTriple());
+  if (auto Path = getPathForTriple(getTriple()))
+return *Path;
 
   // When building with per target runtime directories, various ways of naming
   // the Arm architecture may have been normalised to simply "arm".
@@ -705,7 +708,8 @@
   if (getTriple().getArch() == Triple::arm && !getTriple().isArmMClass()) {
 llvm::Triple ArmTriple = getTriple();
 ArmTriple.setArch(Triple::arm);
-addPathForTriple(ArmTriple);
+if (auto Path = getPathForTriple(ArmTriple))
+  return *Path;
   }
 
   // Android targets may include an API level at the end. We still want to fall
@@ -714,10 +718,11 @@
   getTriple().getEnvironmentName() != "android") {
 llvm::Triple TripleWithoutLevel = getTriple();
 TripleWithoutLevel.setEnvironmentName("android");
-addPathForTriple(TripleWithoutLevel);
+if (auto Path = getPathForTriple(TripleWithoutLevel))
+  return *Path;
   }
 
-  return Paths;
+  return {};
 }
 
 ToolChain::path_list ToolChain::getStdlibPaths() const {
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2166,16 +2166,8 @@
   }
 
   if (C.getArgs().hasArg(options::OPT_print_runtime_dir)) {
-std::string RuntimePath;
-// Get the first existing path, if any.
-for (auto Path : TC.getRuntimePaths()) {
-  if (getVFS().exists(Path)) {
-RuntimePath = Path;
-break;
-  }
-}
-if (!RuntimePath.empty())
-  llvm::outs() << RuntimePath << '\n';
+if (std::optional RuntimePath = TC.getRuntimePath())
+  llvm::outs() << *RuntimePath << '\n';
 else
   llvm::outs() << TC.getCompilerRTPath() << '\n';
 return false;
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -500,8 +501,8 @@
 StringRef Component,
 FileType Type = ToolChain::FT_Static) const;
 
-  // Returns target specific runtime paths.
-  path_list getRuntimePaths() const;
+  // Returns the target specific runtime path if it exists.
+  std::optional getRuntimePath() const;
 
   // Returns target specific standard library paths.
   path_list getStdlibPaths() const;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158476#4609243 , @srhines wrote:

> This approach LGTM, assuming that https://reviews.llvm.org/D140925 lands as 
> well to ensure that the triple does use `androideabi`, since it would prevent 
> custom build setups from needing additional help.

@phosek is working on that one, but I think this is useful even without it, 
because you just need to use `android` in your triple when building the 
runtimes, and can use `androideabi` everywhere else. I'll put up an example 
runtimes build setup with that to demonstrate.

>> (Aside: why do we need to produce runtime libraries for different versions 
>> in the first place? We want one for the minimum OS version we support, of 
>> course, but then there's important additions like version 29 adding ELF TLS 
>> support and version 31 adding a thread properties API used by LSAN, so 
>> building libraries targeting those important versions and having them be 
>> dynamically selected based on your target OS version is super useful. We 
>> could build the runtime libraries for every single OS version we wanted to 
>> support, but that's a lot of versions (at least 21 through 31, times four 
>> architectures), which is wasteful for both build times and toolchain 
>> distribution size.)
>
> I don't think we need all the versions, but today we really only use 21 
> (lowest level), and 30 (a conservative level for the platform that supports 
> all our apexes). Ideally we'd also pick up the latest platform version, and 
> we probably should explicitly call out 29 for ELF TLS, so that would be ~4 
> supported levels, but maybe we can use 29 instead of 30 to cut it to ~3 for 
> most architectures.

Ah, yeah, https://reviews.llvm.org/D85927 dynamically detects the thread 
properties API, so we don't need an explicit build for 31. An explicit build 
with ELF TLS is still required though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158476#4608428 , @thakis wrote:

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

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D158476#4605360 , @MaskRay wrote:

>> // Android target triples contain a target version. If we don't have 
>> libraries for the exact target version, we should fall back to the next 
>> newest version or a versionless path, if any.
>
> An Android maintainer can override my opinion but my feeling is that this 
> ad-hoc rule probably should be the responsibility of the file hierarchy 
> instead of clangDriver. This really easy to cause mix-and-match situations 
> where the problem is on the user side but the user may blame Clang.
>
> This can be implemented by adding symlinks in the file hierarchy.

That was one of the alternatives I brought up in 
https://discourse.llvm.org/t/handling-version-numbers-in-per-target-runtime-directories/62717,
 but symlinks are complicated on Windows, and you'd also have to add build 
system code to create those symlinks, so I don't think it'll be a clear win.

Are you concerned with falling back to the next newest version or the 
versionless path (or both)? I think the next newest version is pretty inline 
with general OS compatibility: if you don't have version N, look for version N 
- 1, N - 2, etc (since they're forward compatible, see also 
https://discourse.llvm.org/t/handling-version-numbers-in-per-target-runtime-directories/62717/13).
 The versionless path is a little less principled IMO (though I guess you could 
see it as implicitly being compatible with any version) ... maybe we could get 
rid of that if Chrome is okay moving to a versioned directory.

(Aside: why do we need to produce runtime libraries for different versions in 
the first place? We want one for the minimum OS version we support, of course, 
but then there's important additions like version 29 adding ELF TLS support and 
version 31 adding a thread properties API used by LSAN, so building libraries 
targeting those important versions and having them be dynamically selected 
based on your target OS version is super useful. We could build the runtime 
libraries for every single OS version we wanted to support, but that's a lot of 
versions (at least 21 through 31, times four architectures), which is wasteful 
for both build times and toolchain distribution size.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158476

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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: collinbaker, thakis, MaskRay, phosek, danalbert, 
srhines.
Herald added subscribers: danielkiss, kristof.beyls.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Android triples include a version number, which makes direct triple
comparisons for per-target runtime directory searching not always work.
Instead, look for the triple with the highest compatible version number
and use that per-target runtime directory instead. This maintains the
existing fallback to a triple without any version number, but I'm hoping
we can remove that in the future. https://discourse.llvm.org/t/62717
discusses this further.

The one remaining triple mismatch after this is that Android armv7
triples usually have an environment of `androideabi`, which Clang
normalizes to `android`. If you use the `androideabi` triple when
building the runtimes with a per-target runtimes dir, the directory will
get created with `androideabi` in its name, but Clang's triple search
uses the normalized triple and will look for an `android` directory
instead. https://reviews.llvm.org/D140925 will fix that by normalizing
triples when creating the per-target runtimes directories as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158476

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android21/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android23/libclang_rt.builtins.a
  
clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/aarch64-unknown-linux-android29/libclang_rt.builtins.a
  clang/test/Driver/linux-per-target-runtime-dir.c

Index: clang/test/Driver/linux-per-target-runtime-dir.c
===
--- clang/test/Driver/linux-per-target-runtime-dir.c
+++ clang/test/Driver/linux-per-target-runtime-dir.c
@@ -30,13 +30,29 @@
 // RUN: --target=aarch64-unknown-linux-android21 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID21 %s
-// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android21{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID21: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android23 \
 // RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
 // RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
-// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android{{/|\\}}libclang_rt.builtins.a
+// CHECK-FILE-NAME-ANDROID23: lib{{/|\\}}aarch64-unknown-linux-android23{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android26 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID23 %s
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android29 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
+// CHECK-FILE-NAME-ANDROID29: lib{{/|\\}}aarch64-unknown-linux-android29{{/|\\}}libclang_rt.builtins.a
+
+// RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
+// RUN: --target=aarch64-unknown-linux-android31 \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN:   | FileCheck --check-prefix=CHECK-FILE-NAME-ANDROID29 %s
 
 // RUN: %clang -rtlib=compiler-rt -print-file-name=libclang_rt.builtins.a 2>&1 \
 // RUN: --target=aarch64-unknown-linux-android \
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -677,6 +677,46 @@
   return Args.MakeArgString(getCompilerRT(Args, Component, Type));
 }
 
+// Android target triples contain a target version. If we don't have libraries
+// for the exact target version, we should fall back to the next newest version
+// or a versionless path, if any.
+std::optional ToolChain::getFallbackAndroidRuntimePath() const {
+  llvm::Triple TripleWithoutLevel(getTriple());
+  TripleWithoutLevel.setEnvironmentName("android"); // remove any version number
+  const std::string  = TripleWithoutLevel.str();
+  unsigned TripleVersion = getTriple().getEnvironmentVersion().getMajor();
+  unsigned BestVersion = 0;
+
+  SmallString<128> LibDir(D.ResourceDir);
+  

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: collinbaker, thakis, phosek, MaskRay.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This used to be getRuntimePath till https://reviews.llvm.org/D115049
added a fallback search path for Android. As far as I can tell, the
intent has always been to use the first existing path though instead of
actually supporting multiple runtime paths. We can move the existence
checks into getRuntimePath and have it return std::optional, which also
makes the `--print-runtime-dir` behavior much cleaner.

The motivation is a follow-up change to Android runtime path searches,
which is much nicer with this in place.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158475

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp

Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -86,8 +86,8 @@
   List.push_back(Path);
   };
 
-  for (const auto  : getRuntimePaths())
-addIfExists(getLibraryPaths(), Path);
+  if (std::optional Path = getRuntimePath())
+getLibraryPaths().push_back(*Path);
   for (const auto  : getStdlibPaths())
 addIfExists(getFilePaths(), Path);
   for (const auto  : getArchSpecificLibPaths())
@@ -677,15 +677,18 @@
   return Args.MakeArgString(getCompilerRT(Args, Component, Type));
 }
 
-ToolChain::path_list ToolChain::getRuntimePaths() const {
-  path_list Paths;
-  auto addPathForTriple = [this, ](const llvm::Triple ) {
+std::optional ToolChain::getRuntimePath() const {
+  auto getPathForTriple =
+  [this](const llvm::Triple ) -> std::optional {
 SmallString<128> P(D.ResourceDir);
 llvm::sys::path::append(P, "lib", Triple.str());
-Paths.push_back(std::string(P.str()));
+if (getVFS().exists(P))
+  return std::string(P);
+return {};
   };
 
-  addPathForTriple(getTriple());
+  if (auto Path = getPathForTriple(getTriple()))
+return *Path;
 
   // When building with per target runtime directories, various ways of naming
   // the Arm architecture may have been normalised to simply "arm".
@@ -705,7 +708,8 @@
   if (getTriple().getArch() == Triple::arm && !getTriple().isArmMClass()) {
 llvm::Triple ArmTriple = getTriple();
 ArmTriple.setArch(Triple::arm);
-addPathForTriple(ArmTriple);
+if (auto Path = getPathForTriple(ArmTriple))
+  return *Path;
   }
 
   // Android targets may include an API level at the end. We still want to fall
@@ -714,10 +718,11 @@
   getTriple().getEnvironmentName() != "android") {
 llvm::Triple TripleWithoutLevel = getTriple();
 TripleWithoutLevel.setEnvironmentName("android");
-addPathForTriple(TripleWithoutLevel);
+if (auto Path = getPathForTriple(TripleWithoutLevel))
+  return *Path;
   }
 
-  return Paths;
+  return {};
 }
 
 ToolChain::path_list ToolChain::getStdlibPaths() const {
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2168,16 +2168,8 @@
   }
 
   if (C.getArgs().hasArg(options::OPT_print_runtime_dir)) {
-std::string RuntimePath;
-// Get the first existing path, if any.
-for (auto Path : TC.getRuntimePaths()) {
-  if (getVFS().exists(Path)) {
-RuntimePath = Path;
-break;
-  }
-}
-if (!RuntimePath.empty())
-  llvm::outs() << RuntimePath << '\n';
+if (std::optional RuntimePath = TC.getRuntimePath())
+  llvm::outs() << *RuntimePath << '\n';
 else
   llvm::outs() << TC.getCompilerRTPath() << '\n';
 return false;
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -500,8 +501,8 @@
 StringRef Component,
 FileType Type = ToolChain::FT_Static) const;
 
-  // Returns target specific runtime paths.
-  path_list getRuntimePaths() const;
+  // Returns the target specific runtime path if it exists.
+  std::optional getRuntimePath() const;
 
   // Returns target specific standard library paths.
   path_list getStdlibPaths() const;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140925: [CMake] Use Clang to infer the target triple

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.
Herald added a subscriber: ekilmer.
Herald added a project: libunwind.
Herald added a reviewer: libunwind.

I think we should land this. Clang has grown workarounds in the meantime, e.g. 
https://github.com/llvm/llvm-project/blob/4001ae175cbe351d496a6cd5481a554b346f706d/clang/lib/Driver/ToolChain.cpp#L690-L709,
 and I think this is much cleaner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140925

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

[PATCH] D158218: [CMake] Deprecate DEFAULT_SYSROOT and GCC_INSTALL_PREFIX

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Conceptually this seems like a great direction to go in. We should just leave 
it up for a while so that anyone using these can comment on any potential 
issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158218

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

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D141918#4577930 , @jyknight wrote:

> In D141918#4566838 , @smeenai wrote:
>
>> $ clang -std=c++20 -O2 -c crash.cpp
>> cannot use musttail with unwindabort
>
> Thanks for the report. We were missing a check of `CI.isUnwindAbort()` in 
> CoroSplit.cpp's `shouldBeMustTail()` function -- fixed for next version of 
> the series. (It's not a regression in tail-callability for coroutines, since 
> that `call unwindabort` would've previously been an `invoke`, which also 
> cannot be musttail'd.)

Just to confirm, this hasn't been uploaded yet, right? I wanted to try it out 
once it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141918

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

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a subscriber: mstorsjo.
smeenai added inline comments.



Comment at: clang/test/Driver/cl-options.c:567
 
-// This test was super sneaky: "/Z7" means "line-tables", but "-gdwarf" occurs
-// later on the command line, so it should win. Interestingly the cc1 arguments
-// came out right, but had wrong semantics, because an invariant assumed by
-// CompilerInvocation was violated: it expects that at most one of {gdwarfN,
-// line-tables-only} appear. If you assume that, then you can safely use
-// Args.hasArg to test whether a boolean flag is present without caring
-// where it appeared. And for this test, it appeared to the left of -gdwarf
-// which made it "win". This test could not detect that bug.
+// If We specify both /Z7 and -gdwarf we should get dwarf, not codeview.
 // RUN: %clang_cl /Z7 -gdwarf /c -### -- %s 2>&1 | FileCheck 
-check-prefix=Z7_gdwarf %s

bogner wrote:
> smeenai wrote:
> > bogner wrote:
> > > rnk wrote:
> > > > I think Meta folks depend on a mode that emits both codeview and DWARF. 
> > > > +@smeenai
> > > Hopefully if that's the case there's a better test somewhere than this 
> > > one that discusses in detail how `/Z7` is an alias for 
> > > `-gline-tables-only`, which hasn't been true since 2017.
> > Thanks for the headers up :) We don't depend on that mode any more though, 
> > so no problems on our end.
> Also note that this change does not affect what happens if we specify both 
> `-gcodeview` and `-gdwarf` together. That continues to pass both `-gcodeview` 
> and `-dwarf-version=...` to `clang -cc1`. However, I don't see any tests that 
> actually check that behaviour, so if that is a mode that's useful to keep 
> working we definitely have some gaps there.
> 
> This change means that if you want to ask `-cc1` for dwarf *and* codeview 
> with `/Z7` or `/Zi`, you'll need to specify `-gdwarf` and `-gcodeview` now. 
> This matches how you would get both sets of options to render with `-g`, 
> which I think makes sense.
Yeah, that sounds good. IIRC when we were using the joint codeview + DWARF mode 
it was with the gcc-style Clang driver and not clang-cl, and we passed `-gdwarf 
-gcodeview` or similar to enable it anyway. We don't need that anymore though; 
@mstorsjo would know if there's anything on the MinGW side that cares for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157794

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

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: clang/test/Driver/cl-options.c:567
 
-// This test was super sneaky: "/Z7" means "line-tables", but "-gdwarf" occurs
-// later on the command line, so it should win. Interestingly the cc1 arguments
-// came out right, but had wrong semantics, because an invariant assumed by
-// CompilerInvocation was violated: it expects that at most one of {gdwarfN,
-// line-tables-only} appear. If you assume that, then you can safely use
-// Args.hasArg to test whether a boolean flag is present without caring
-// where it appeared. And for this test, it appeared to the left of -gdwarf
-// which made it "win". This test could not detect that bug.
+// If We specify both /Z7 and -gdwarf we should get dwarf, not codeview.
 // RUN: %clang_cl /Z7 -gdwarf /c -### -- %s 2>&1 | FileCheck 
-check-prefix=Z7_gdwarf %s

bogner wrote:
> rnk wrote:
> > I think Meta folks depend on a mode that emits both codeview and DWARF. 
> > +@smeenai
> Hopefully if that's the case there's a better test somewhere than this one 
> that discusses in detail how `/Z7` is an alias for `-gline-tables-only`, 
> which hasn't been true since 2017.
Thanks for the headers up :) We don't depend on that mode any more though, so 
no problems on our end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157794

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

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Thanks for the rebase!

We're eager to try this for Meta's Android apps, where size is a priority and 
better `noexcept` codegen would be really helpful. I ran into a crash with 
coroutines though, which blocks me from fully evaluating the size difference 
made by these changes. The reduced test case is in P8313 
, and you can repro the crash by running:

  $ clang -std=c++20 -O2 -c crash.cpp
  cannot use musttail with unwindabort
musttail call unwindabort fastcc void %10(ptr %call57)
  in function _Z5startv.resume
  fatal error: error in backend: Broken function found, compilation aborted!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141918

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

[PATCH] D153623: [clang][Sema] Add fixit for scoped enum format error

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Sorry for the delayed comment here.

The fix-it is convenient, but is it the best suggestion? It'll end up 
suggesting truncating the enum value instead of using the proper format 
specifier in https://godbolt.org/z/xdhrefG95, for example. More insidiously, 
the `static_cast` might work fine for all enum uses today but silently cause 
issues for future enum values.

Using `std::to_underyling` and matching the format specifier to the underlying 
type would be the best suggestion IMO, but that only works in C++23 and above 
:( I guess `std::underlying_type_t` isn't super ugly either for C++14 and 
above, but bare `std::underlying_type` is pretty rough to use in a cast. All of 
those require the addition of a header though, and I'm not sure if that's 
acceptable for a fix-it.

Would we at least consider suggesting a format specifier and cast based on the 
underlying type of the enum, instead of just casting to whatever type the 
format specifier was using? That won't guard against future changes to the 
enum, but it's better than the status quo IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153623

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

[PATCH] D155081: Specify the developer policy around links to external resources

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

aaron.ballman wrote:
> arsenm wrote:
> > I haven't quite figured out what the exact syntaxes which are automatically 
> > recognized. It seems to recognize "Fixes #Nxyz"
> Yup, it does support that form as well. I had heard more than once during 
> code review that folks seem to prefer the full link because it's easier to 
> click on that from the commit message than it is to navigate to the fix from 
> the number alone. That seemed like a pretty good reason to recommend the full 
> form, but I don't have strong opinions.
+1 for encouraging the full link


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

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

[PATCH] D153989: [compiler-rt] Move crt into builtins

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D153989#4465907 , @phosek wrote:

> In D153989#4465759 , @smeenai wrote:
>
>> Sorry, it's been a week :D I assume that crt doesn't need the builtins to be 
>> available for its configure (the way the rest of compiler-rt does)? If so, 
>> this LGTM.
>
> Great question, I checked and crt doesn't need builtins, just Clang headers, 
> same as builtins.

Cool, this does seem like the simplest and best way to go then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153989

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

[PATCH] D153989: [compiler-rt] Move crt into builtins

[Shoaib Meenai via Phabricator via cfe-commits]

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

Sorry, it's been a week :D I assume that crt doesn't need the builtins to be 
available for its configure (the way the rest of compiler-rt does)? If so, this 
LGTM.




Comment at: compiler-rt/lib/builtins/CMakeLists.txt:57
 
-if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
+if(${OS_NAME} MATCHES "AIX")
   include(CompilerRTAIXUtils)

While you're changing all these (is that important here or just a drive-by fix, 
btw?), could you either quote them or drop the `${}` to prevent weird things 
from happening if we have have e.g. an `AIX` variable defined? 
(https://cmake.org/cmake/help/latest/command/if.html#variable-expansion for 
context for anyone not familiar with the issue here.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153989

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

[PATCH] D153176: [Frontend] Remove ShowIncludesPretendHeader

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1df10f15807f: [Frontend] Remove ShowIncludesPretendHeader 
(authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153176

Files:
  clang/include/clang/Frontend/DependencyOutputOptions.h
  clang/lib/Frontend/HeaderIncludeGen.cpp


Index: clang/lib/Frontend/HeaderIncludeGen.cpp
===
--- clang/lib/Frontend/HeaderIncludeGen.cpp
+++ clang/lib/Frontend/HeaderIncludeGen.cpp
@@ -214,13 +214,8 @@
 
 // We track when we are done with the predefines by watching for the first
 // place where we drop back to a nesting depth of 1.
-if (CurrentIncludeDepth == 1 && !HasProcessedPredefines) {
-  if (!DepOpts.ShowIncludesPretendHeader.empty()) {
-PrintHeaderInfo(OutputFile, DepOpts.ShowIncludesPretendHeader,
-ShowDepth, 2, MSStyle);
-  }
+if (CurrentIncludeDepth == 1 && !HasProcessedPredefines)
   HasProcessedPredefines = true;
-}
 
 return;
   } else {
@@ -233,8 +228,6 @@
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
 --IncludeDepth; // Ignore indent from .
-  else if (!DepOpts.ShowIncludesPretendHeader.empty())
-++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "" and "" in a bunch of places.
Index: clang/include/clang/Frontend/DependencyOutputOptions.h
===
--- clang/include/clang/Frontend/DependencyOutputOptions.h
+++ clang/include/clang/Frontend/DependencyOutputOptions.h
@@ -76,9 +76,6 @@
   /// target.
   std::vector> ExtraDeps;
 
-  /// In /showIncludes mode, pretend the main TU is a header with this name.
-  std::string ShowIncludesPretendHeader;
-
   /// The file to write GraphViz-formatted header dependencies to.
   std::string DOTOutputFile;
 


Index: clang/lib/Frontend/HeaderIncludeGen.cpp
===
--- clang/lib/Frontend/HeaderIncludeGen.cpp
+++ clang/lib/Frontend/HeaderIncludeGen.cpp
@@ -214,13 +214,8 @@
 
 // We track when we are done with the predefines by watching for the first
 // place where we drop back to a nesting depth of 1.
-if (CurrentIncludeDepth == 1 && !HasProcessedPredefines) {
-  if (!DepOpts.ShowIncludesPretendHeader.empty()) {
-PrintHeaderInfo(OutputFile, DepOpts.ShowIncludesPretendHeader,
-ShowDepth, 2, MSStyle);
-  }
+if (CurrentIncludeDepth == 1 && !HasProcessedPredefines)
   HasProcessedPredefines = true;
-}
 
 return;
   } else {
@@ -233,8 +228,6 @@
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
 --IncludeDepth; // Ignore indent from .
-  else if (!DepOpts.ShowIncludesPretendHeader.empty())
-++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "" and "" in a bunch of places.
Index: clang/include/clang/Frontend/DependencyOutputOptions.h
===
--- clang/include/clang/Frontend/DependencyOutputOptions.h
+++ clang/include/clang/Frontend/DependencyOutputOptions.h
@@ -76,9 +76,6 @@
   /// target.
   std::vector> ExtraDeps;
 
-  /// In /showIncludes mode, pretend the main TU is a header with this name.
-  std::string ShowIncludesPretendHeader;
-
   /// The file to write GraphViz-formatted header dependencies to.
   std::string DOTOutputFile;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG67a11290df64: [Frontend] Dont output skipped includes 
from predefines (authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153175

Files:
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/test/Frontend/print-header-includes.c

Index: clang/test/Frontend/print-header-includes.c
===
--- clang/test/Frontend/print-header-includes.c
+++ clang/test/Frontend/print-header-includes.c
@@ -17,6 +17,29 @@
 // SKIPPED: .. {{.*test2.h}}
 // SKIPPED: .. {{.*test2.h}}
 
+// RUN: %clang_cc1 -isystem %S -isystem %S/Inputs/SystemHeaderPrefix \
+// RUN: -E -H -fshow-skipped-includes -sys-header-deps -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-SYS < %t.stderr %s
+
+// SKIPPED-SYS: . {{.*noline.h}}
+// SKIPPED-SYS: . {{.*test.h}}
+// SKIPPED-SYS: .. {{.*test2.h}}
+// SKIPPED-SYS: .. {{.*test2.h}}
+
+// RUN: %clang_cc1 -I%S -isystem %S/Inputs/SystemHeaderPrefix -include Inputs/test.h \
+// RUN: -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-PREDEFINES < %t.stderr %s
+
+// The skipped include of test2.h from the -include test.h shouldn't be printed.
+// SKIPPED-PREDEFINES-NOT: {{.*test2.h}}
+// SKIPPED-PREDEFINES: . {{.*test.h}}
+
+// RUN: %clang_cc1 -isystem %S -isystem %S/Inputs/SystemHeaderPrefix \
+// RUN: -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-NO-SYS --allow-empty < %t.stderr %s
+
+// SKIPPED-NO-SYS-NOT: .
+
 // RUN: %clang_cc1 -I%S -include Inputs/test3.h -isystem %S/Inputs/SystemHeaderPrefix \
 // RUN: -E -H -sys-header-deps -o /dev/null %s 2> %t.stderr
 // RUN: FileCheck --check-prefix SYSHEADERS < %t.stderr %s
@@ -58,4 +81,4 @@
 // MS-IGNORELIST-NOT: Note
 
 #include 
-#include "Inputs/test.h"
+#include 
Index: clang/lib/Frontend/HeaderIncludeGen.cpp
===
--- clang/lib/Frontend/HeaderIncludeGen.cpp
+++ clang/lib/Frontend/HeaderIncludeGen.cpp
@@ -49,6 +49,18 @@
 
   void FileSkipped(const FileEntryRef , const Token ,
SrcMgr::CharacteristicKind FileType) override;
+
+private:
+  bool ShouldShowHeader(SrcMgr::CharacteristicKind HeaderType) {
+if (!DepOpts.IncludeSystemHeaders && isSystem(HeaderType))
+  return false;
+
+// Show the current header if we are (a) past the predefines, or (b) showing
+// all headers and in the predefines at a depth past the initial file and
+// command line buffers.
+return (HasProcessedPredefines ||
+(ShowAllHeaders && CurrentIncludeDepth > 2));
+  }
 };
 
 /// A callback for emitting header usage information to a file in JSON. Each
@@ -211,29 +223,22 @@
 }
 
 return;
-  } else
+  } else {
+return;
+  }
+
+  if (!ShouldShowHeader(NewFileType))
 return;
 
-  // Show the header if we are (a) past the predefines, or (b) showing all
-  // headers and in the predefines at a depth past the initial file and command
-  // line buffers.
-  bool ShowHeader = (HasProcessedPredefines ||
- (ShowAllHeaders && CurrentIncludeDepth > 2));
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
 --IncludeDepth; // Ignore indent from .
   else if (!DepOpts.ShowIncludesPretendHeader.empty())
 ++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
-  if (!DepOpts.IncludeSystemHeaders && isSystem(NewFileType))
-ShowHeader = false;
-
-  // Dump the header include information we are past the predefines buffer or
-  // are showing all headers and this isn't the magic implicit 
-  // header.
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "" and "" in a bunch of places.
-  if (ShowHeader && Reason == PPCallbacks::EnterFile &&
+  if (Reason == PPCallbacks::EnterFile &&
   UserLoc.getFilename() != StringRef("")) {
 PrintHeaderInfo(OutputFile, UserLoc.getFilename(), ShowDepth, IncludeDepth,
 MSStyle);
@@ -246,7 +251,7 @@
   if (!DepOpts.ShowSkippedHeaderIncludes)
 return;
 
-  if (!DepOpts.IncludeSystemHeaders && isSystem(FileType))
+  if (!ShouldShowHeader(FileType))
 return;
 
   PrintHeaderInfo(OutputFile, SkippedFile.getName(), ShowDepth,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D153175#4431923 , @hans wrote:

>> -H is supposed to skip outputting headers from -include command line
>> arguments, but -fshow-skipped-includes was outputting any skipped
>> includes encountered via -include.
>
> I was thrown off by the "-H is supposed to skip ... but 
> -fshow-skipped-includes was outputting ..."
>
> I suppose the point is that we do want `-fshow-skipped-includes` to show 
> skipped includes, but not the ones from `-include` or system headers (unless 
> `-sys-header-deps`)?
>
> lgtm if I got that right.

Yup, my description was kinda terrible :D Lemme try to clarify it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153175

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

[PATCH] D153176: [Frontend] Remove ShowIncludesPretendHeader

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: hans, rnk, thakis.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

It hasn't been written to since https://reviews.llvm.org/D46652, so it
was always empty. I don't have enough context on that diff to know if
the removal of the write to ShowIncludesPretendHeader in that diff was
intentional, but no one's complained about it for five years, so I
assume we're okay to just get rid of it entirely.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153176

Files:
  clang/include/clang/Frontend/DependencyOutputOptions.h
  clang/lib/Frontend/HeaderIncludeGen.cpp


Index: clang/lib/Frontend/HeaderIncludeGen.cpp
===
--- clang/lib/Frontend/HeaderIncludeGen.cpp
+++ clang/lib/Frontend/HeaderIncludeGen.cpp
@@ -214,13 +214,8 @@
 
 // We track when we are done with the predefines by watching for the first
 // place where we drop back to a nesting depth of 1.
-if (CurrentIncludeDepth == 1 && !HasProcessedPredefines) {
-  if (!DepOpts.ShowIncludesPretendHeader.empty()) {
-PrintHeaderInfo(OutputFile, DepOpts.ShowIncludesPretendHeader,
-ShowDepth, 2, MSStyle);
-  }
+if (CurrentIncludeDepth == 1 && !HasProcessedPredefines)
   HasProcessedPredefines = true;
-}
 
 return;
   } else {
@@ -233,8 +228,6 @@
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
 --IncludeDepth; // Ignore indent from .
-  else if (!DepOpts.ShowIncludesPretendHeader.empty())
-++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "" and "" in a bunch of places.
Index: clang/include/clang/Frontend/DependencyOutputOptions.h
===
--- clang/include/clang/Frontend/DependencyOutputOptions.h
+++ clang/include/clang/Frontend/DependencyOutputOptions.h
@@ -76,9 +76,6 @@
   /// target.
   std::vector> ExtraDeps;
 
-  /// In /showIncludes mode, pretend the main TU is a header with this name.
-  std::string ShowIncludesPretendHeader;
-
   /// The file to write GraphViz-formatted header dependencies to.
   std::string DOTOutputFile;
 


Index: clang/lib/Frontend/HeaderIncludeGen.cpp
===
--- clang/lib/Frontend/HeaderIncludeGen.cpp
+++ clang/lib/Frontend/HeaderIncludeGen.cpp
@@ -214,13 +214,8 @@
 
 // We track when we are done with the predefines by watching for the first
 // place where we drop back to a nesting depth of 1.
-if (CurrentIncludeDepth == 1 && !HasProcessedPredefines) {
-  if (!DepOpts.ShowIncludesPretendHeader.empty()) {
-PrintHeaderInfo(OutputFile, DepOpts.ShowIncludesPretendHeader,
-ShowDepth, 2, MSStyle);
-  }
+if (CurrentIncludeDepth == 1 && !HasProcessedPredefines)
   HasProcessedPredefines = true;
-}
 
 return;
   } else {
@@ -233,8 +228,6 @@
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
 --IncludeDepth; // Ignore indent from .
-  else if (!DepOpts.ShowIncludesPretendHeader.empty())
-++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "" and "" in a bunch of places.
Index: clang/include/clang/Frontend/DependencyOutputOptions.h
===
--- clang/include/clang/Frontend/DependencyOutputOptions.h
+++ clang/include/clang/Frontend/DependencyOutputOptions.h
@@ -76,9 +76,6 @@
   /// target.
   std::vector> ExtraDeps;
 
-  /// In /showIncludes mode, pretend the main TU is a header with this name.
-  std::string ShowIncludesPretendHeader;
-
   /// The file to write GraphViz-formatted header dependencies to.
   std::string DOTOutputFile;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: hans, rnk, thakis.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

`-H` is supposed to skip outputting headers from `-include` command line
arguments, but `-fshow-skipped-includes` was outputting any skipped
includes encountered via `-include`. Add a check to prevent this, and
add tests for the interaction between `-fshow-skipped-includes` and
`-sys-header-deps` while I'm here.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153175

Files:
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/test/Frontend/print-header-includes.c

Index: clang/test/Frontend/print-header-includes.c
===
--- clang/test/Frontend/print-header-includes.c
+++ clang/test/Frontend/print-header-includes.c
@@ -17,6 +17,29 @@
 // SKIPPED: .. {{.*test2.h}}
 // SKIPPED: .. {{.*test2.h}}
 
+// RUN: %clang_cc1 -isystem %S -isystem %S/Inputs/SystemHeaderPrefix \
+// RUN: -E -H -fshow-skipped-includes -sys-header-deps -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-SYS < %t.stderr %s
+
+// SKIPPED-SYS: . {{.*noline.h}}
+// SKIPPED-SYS: . {{.*test.h}}
+// SKIPPED-SYS: .. {{.*test2.h}}
+// SKIPPED-SYS: .. {{.*test2.h}}
+
+// RUN: %clang_cc1 -I%S -isystem %S/Inputs/SystemHeaderPrefix -include Inputs/test.h \
+// RUN: -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-PREDEFINES < %t.stderr %s
+
+// The skipped include of test2.h from the -include test.h shouldn't be printed.
+// SKIPPED-PREDEFINES-NOT: {{.*test2.h}}
+// SKIPPED-PREDEFINES: . {{.*test.h}}
+
+// RUN: %clang_cc1 -isystem %S -isystem %S/Inputs/SystemHeaderPrefix \
+// RUN: -E -H -fshow-skipped-includes -o /dev/null %s 2> %t.stderr
+// RUN: FileCheck --check-prefix=SKIPPED-NO-SYS --allow-empty < %t.stderr %s
+
+// SKIPPED-NO-SYS-NOT: .
+
 // RUN: %clang_cc1 -I%S -include Inputs/test3.h -isystem %S/Inputs/SystemHeaderPrefix \
 // RUN: -E -H -sys-header-deps -o /dev/null %s 2> %t.stderr
 // RUN: FileCheck --check-prefix SYSHEADERS < %t.stderr %s
@@ -58,4 +81,4 @@
 // MS-IGNORELIST-NOT: Note
 
 #include 
-#include "Inputs/test.h"
+#include 
Index: clang/lib/Frontend/HeaderIncludeGen.cpp
===
--- clang/lib/Frontend/HeaderIncludeGen.cpp
+++ clang/lib/Frontend/HeaderIncludeGen.cpp
@@ -49,6 +49,18 @@
 
   void FileSkipped(const FileEntryRef , const Token ,
SrcMgr::CharacteristicKind FileType) override;
+
+private:
+  bool ShouldShowHeader(SrcMgr::CharacteristicKind HeaderType) {
+if (!DepOpts.IncludeSystemHeaders && isSystem(HeaderType))
+  return false;
+
+// Show the current header if we are (a) past the predefines, or (b) showing
+// all headers and in the predefines at a depth past the initial file and
+// command line buffers.
+return (HasProcessedPredefines ||
+(ShowAllHeaders && CurrentIncludeDepth > 2));
+  }
 };
 
 /// A callback for emitting header usage information to a file in JSON. Each
@@ -211,29 +223,22 @@
 }
 
 return;
-  } else
+  } else {
+return;
+  }
+
+  if (!ShouldShowHeader(NewFileType))
 return;
 
-  // Show the header if we are (a) past the predefines, or (b) showing all
-  // headers and in the predefines at a depth past the initial file and command
-  // line buffers.
-  bool ShowHeader = (HasProcessedPredefines ||
- (ShowAllHeaders && CurrentIncludeDepth > 2));
   unsigned IncludeDepth = CurrentIncludeDepth;
   if (!HasProcessedPredefines)
 --IncludeDepth; // Ignore indent from .
   else if (!DepOpts.ShowIncludesPretendHeader.empty())
 ++IncludeDepth; // Pretend inclusion by ShowIncludesPretendHeader.
 
-  if (!DepOpts.IncludeSystemHeaders && isSystem(NewFileType))
-ShowHeader = false;
-
-  // Dump the header include information we are past the predefines buffer or
-  // are showing all headers and this isn't the magic implicit 
-  // header.
   // FIXME: Identify headers in a more robust way than comparing their name to
   // "" and "" in a bunch of places.
-  if (ShowHeader && Reason == PPCallbacks::EnterFile &&
+  if (Reason == PPCallbacks::EnterFile &&
   UserLoc.getFilename() != StringRef("")) {
 PrintHeaderInfo(OutputFile, UserLoc.getFilename(), ShowDepth, IncludeDepth,
 MSStyle);
@@ -246,7 +251,7 @@
   if (!DepOpts.ShowSkippedHeaderIncludes)
 return;
 
-  if (!DepOpts.IncludeSystemHeaders && isSystem(FileType))
+  if (!ShouldShowHeader(FileType))
 return;
 
   PrintHeaderInfo(OutputFile, SkippedFile.getName(), ShowDepth,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Makes sense, though @Amir should also take a look.




Comment at: bolt/CMakeLists.txt:122
+
+add_custom_target(bolt DEPENDS ${BOLT_DEPENDS})
+set_target_properties(bolt PROPERTIES FOLDER "BOLT")

Where is BOLT_DEPENDS being set now?

The [CMake 
docs](https://cmake.org/cmake/help/latest/command/add_custom_target.html) say 
DEPENDS should only be used for add_custom_command outputs in the same folder, 
and add_dependencies for everything else, so we want the latter here. I know 
this is just moving existing code, but we may as well fix that while we're 
here. I don't think it makes any difference for Ninja, but it might for other 
generators?



Comment at: bolt/tools/heatmap/CMakeLists.txt:8
 
-add_bolt_tool(llvm-bolt-heatmap
+add_bolt_executable(llvm-bolt-heatmap
   heatmap.cpp

Why this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151595

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

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I searched for `__clang_call_terminate` in the Clang test directory when I was 
working on this diff. Most tests were testing calls to it, not definitions. 
There were a couple of tests checking the definition, but they seemed pretty 
targeted (e.g. 
https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCXX/noexcept.cpp)
 or auto-generated, so I figured having a separate test case for the specific 
thing I cared about was best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151393

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

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

[Shoaib Meenai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8f7b51e4ec09: [CodeGen] Make __clang_call_terminate have an 
unwind table entry (authored by smeenai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151393

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp


Index: clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,NOUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-funwind-tables=1 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,SYNCUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-funwind-tables=2 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,ASYNCUNWIND %s
+
+void caller(void callback()) noexcept { callback(); }
+
+// CHECK: define {{.*}}void @__clang_call_terminate({{[^)]*}}) #[[#ATTRNUM:]]
+// CHECK: attributes #[[#ATTRNUM]] = {
+// NOUNWIND-NOT: uwtable
+// NOUNWIND-SAME: }
+// SYNCUNWIND-SAME: uwtable(sync)
+// ASYNCUNWIND-SAME: uwtable{{ }}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4689,6 +4689,7 @@
   cast(fnRef.getCallee()->stripPointerCasts());
   if (fn->empty()) {
 CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, fn, /*IsThunk=*/false);
+CGM.SetLLVMFunctionAttributesForDefinition(nullptr, fn);
 fn->setDoesNotThrow();
 fn->setDoesNotReturn();
 


Index: clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,NOUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -funwind-tables=1 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,SYNCUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -funwind-tables=2 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,ASYNCUNWIND %s
+
+void caller(void callback()) noexcept { callback(); }
+
+// CHECK: define {{.*}}void @__clang_call_terminate({{[^)]*}}) #[[#ATTRNUM:]]
+// CHECK: attributes #[[#ATTRNUM]] = {
+// NOUNWIND-NOT: uwtable
+// NOUNWIND-SAME: }
+// SYNCUNWIND-SAME: uwtable(sync)
+// ASYNCUNWIND-SAME: uwtable{{ }}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4689,6 +4689,7 @@
   cast(fnRef.getCallee()->stripPointerCasts());
   if (fn->empty()) {
 CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, fn, /*IsThunk=*/false);
+CGM.SetLLVMFunctionAttributesForDefinition(nullptr, fn);
 fn->setDoesNotThrow();
 fn->setDoesNotReturn();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai updated this revision to Diff 525429.
smeenai added a comment.

Call SetLLVMFunctionAttributesForDefinition instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151393

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp


Index: clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,NOUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-funwind-tables=1 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,SYNCUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-funwind-tables=2 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,ASYNCUNWIND %s
+
+void caller(void callback()) noexcept { callback(); }
+
+// CHECK: define {{.*}}void @__clang_call_terminate({{[^)]*}}) #[[#ATTRNUM:]]
+// CHECK: attributes #[[#ATTRNUM]] = {
+// NOUNWIND-NOT: uwtable
+// NOUNWIND-SAME: }
+// SYNCUNWIND-SAME: uwtable(sync)
+// ASYNCUNWIND-SAME: uwtable{{ }}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4689,6 +4689,7 @@
   cast(fnRef.getCallee()->stripPointerCasts());
   if (fn->empty()) {
 CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, fn, /*IsThunk=*/false);
+CGM.SetLLVMFunctionAttributesForDefinition(nullptr, fn);
 fn->setDoesNotThrow();
 fn->setDoesNotReturn();
 


Index: clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,NOUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -funwind-tables=1 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,SYNCUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -funwind-tables=2 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,ASYNCUNWIND %s
+
+void caller(void callback()) noexcept { callback(); }
+
+// CHECK: define {{.*}}void @__clang_call_terminate({{[^)]*}}) #[[#ATTRNUM:]]
+// CHECK: attributes #[[#ATTRNUM]] = {
+// NOUNWIND-NOT: uwtable
+// NOUNWIND-SAME: }
+// SYNCUNWIND-SAME: uwtable(sync)
+// ASYNCUNWIND-SAME: uwtable{{ }}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4689,6 +4689,7 @@
   cast(fnRef.getCallee()->stripPointerCasts());
   if (fn->empty()) {
 CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, fn, /*IsThunk=*/false);
+CGM.SetLLVMFunctionAttributesForDefinition(nullptr, fn);
 fn->setDoesNotThrow();
 fn->setDoesNotReturn();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: efriedma, jyknight, rnk, MaskRay.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
smeenai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This enables debuggers to step past that frame on architectures that
don't use DWARF unwinding (such as armv7) even without debug info. The
problem should theoretically be architecture-agnostic, but according to
https://discourse.llvm.org/t/51633/2 it gets masked on architectures
that use DWARF unwind info.

Fixes https://github.com/llvm/llvm-project/issues/40696


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151393

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp


Index: clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,NOUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-funwind-tables=1 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,SYNCUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions 
-funwind-tables=2 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,ASYNCUNWIND %s
+
+void caller(void callback()) noexcept { callback(); }
+
+// CHECK: define {{.*}}void @__clang_call_terminate({{[^)]*}}) #[[#ATTRNUM:]]
+// CHECK: attributes #[[#ATTRNUM]] = {
+// NOUNWIND-NOT: uwtable
+// NOUNWIND-SAME: }
+// SYNCUNWIND-SAME: uwtable(sync)
+// ASYNCUNWIND-SAME: uwtable{{ }}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4692,6 +4692,11 @@
 fn->setDoesNotThrow();
 fn->setDoesNotReturn();
 
+// Ensure stack traces can progress past this frame (e.g. for debuggers).
+const CodeGenOptions  = CGM.getCodeGenOpts();
+if (CodeGenOpts.UnwindTables)
+  fn->setUWTableKind(llvm::UWTableKind(CodeGenOpts.UnwindTables));
+
 // What we really want is to massively penalize inlining without
 // forbidding it completely.  The difference between that and
 // 'noinline' is negligible.


Index: clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/clang-call-terminate.uwtable.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,NOUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -funwind-tables=1 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,SYNCUNWIND %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fexceptions -fcxx-exceptions -funwind-tables=2 -emit-llvm -o - %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,ASYNCUNWIND %s
+
+void caller(void callback()) noexcept { callback(); }
+
+// CHECK: define {{.*}}void @__clang_call_terminate({{[^)]*}}) #[[#ATTRNUM:]]
+// CHECK: attributes #[[#ATTRNUM]] = {
+// NOUNWIND-NOT: uwtable
+// NOUNWIND-SAME: }
+// SYNCUNWIND-SAME: uwtable(sync)
+// ASYNCUNWIND-SAME: uwtable{{ }}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4692,6 +4692,11 @@
 fn->setDoesNotThrow();
 fn->setDoesNotReturn();
 
+// Ensure stack traces can progress past this frame (e.g. for debuggers).
+const CodeGenOptions  = CGM.getCodeGenOpts();
+if (CodeGenOpts.UnwindTables)
+  fn->setUWTableKind(llvm::UWTableKind(CodeGenOpts.UnwindTables));
+
 // What we really want is to massively penalize inlining without
 // forbidding it completely.  The difference between that and
 // 'noinline' is negligible.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:189
   auto GetTrapBB = [](BuilderTy ) {
-if (TrapBB && SingleTrapBB)
-  return TrapBB;
-
-Function *Fn = IRB.GetInsertBlock()->getParent();
-// FIXME: This debug location doesn't make a lot of sense in the
-// `SingleTrapBB` case.
-auto DebugLoc = IRB.getCurrentDebugLocation();
-IRBuilder<>::InsertPointGuard Guard(IRB);
-TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
-IRB.SetInsertPoint(TrapBB);
-
-auto *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
-CallInst *TrapCall = IRB.CreateCall(F, {});
-TrapCall->setDoesNotReturn();
-TrapCall->setDoesNotThrow();
-TrapCall->setDebugLoc(DebugLoc);
-IRB.CreateUnreachable();
-
+if (DebugTrapBB) {
+  Function *Fn = IRB.GetInsertBlock()->getParent();

oskarwirga wrote:
> nlopes wrote:
> > this seems like code duplication. This pass already has the single-trap 
> > flag to exactly control if you get a single trap BB or one per check for 
> > better debug info.
> Unfortunately, even with the single trap flag it gets optimized out in later 
> passes because the machine code emitted is the exact same 
I believe we end up tail merging the trap instructions. A previous iteration of 
this patch attempted to use the `nomerge` attribute to directly avoid the tail 
merging, but that only works for function calls, not for the `trap` instruction 
ultimately emitted here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654

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

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a reviewer: melver.
smeenai added a comment.

The patch should be uploaded with full context to make review easier.

Adding another potential reviewer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654

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

[PATCH] D149504: [clang][CodeGenPGO] Don't use an invalid index when region counts disagree

[Shoaib Meenai via Phabricator via cfe-commits]

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

Seems pretty reasonable to add a bounds check here. You should probably add a 
comment explaining the reasoning though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149504

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

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Thinking about this a bit more, should the trap not have an associated stack 
trace that can be symbolicated to tell you which line of code was crashing? If 
the issue is that multiple traps can get folded together, the `nomerge` 
attribute (D78659 ) could be useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654

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

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

You should upload this with full context and add some test cases :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654

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

[PATCH] D147764: Fix the two gmoules-prefered-name-* tests

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added reviewers: Michael137, dblaikie, aprantl.
smeenai added a comment.

Thanks! I'm not super familiar with this, so I'm adding the original author and 
reviewers to confirm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147764

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Shoaib Meenai via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D144603: Add option to disable compiler launcher on external projects

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D144603#4162192 , @haowei wrote:

> In D144603#4162048 , @smeenai wrote:
>
>> Hmm, what cache key does ccache use for compilers? Is it the `--version` 
>> output string, the path, or some combination? If the path is involved then I 
>> don't see any value in using ccache for configuring anything past stage1, 
>> since the compiler will (presumably) be installed somewhere else afterwards 
>> and not used directly from the build tree. If it's just `--version`, what 
>> are the cache pollution concerns? The compiler binaries output by all the 
>> stages should produce the same outputs given the same inputs (and presumably 
>> have the same `--version`), right?
>
> I am not sure about ccache. We use an internal distributed build cache system 
> when we build LLVM. The compiler binary hash and the `--version` output will 
> both be used to calculate the cache key so there is little value to use 
> caching past stage1 as the runtime build and stage2+ will be built from a 
> fresh from source compiler and it will always result in cache miss.
>
> I don't believe ccache only uses `--version` output as cache key since two 
> compiler binary with the same version string can have different output (e.g. 
> 2 WIP source tree but track the same git revision).
>
> So think it would be beneficial to add a flag to avoid using compiler 
> launcher past stage1.

Yeah, that sounds good. I also prefer @phosek's first option of letting users 
opt in to compiler launchers for boostrapping builds if they want, since I 
don't see a lot of value in doing that in general, but being conservative and 
having an option for variables to not pass through is likely the most expedient 
approach here, and that works for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144603

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

[PATCH] D144603: Add option to disable compiler launcher on external projects

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Hmm, what cache key does ccache use for compilers? Is it the `--version` output 
string, the path, or some combination? If the path is involved then I don't see 
any value in using ccache for configuring anything past stage1, since the 
compiler will (presumably) be installed somewhere else afterwards and not used 
directly from the build tree. If it's just `--version`, what are the cache 
pollution concerns? The compiler binaries output by all the stages should 
produce the same outputs given the same inputs (and presumably have the same 
`--version`), right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144603

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

[PATCH] D140925: [CMake] Use Clang to infer the target triple

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Ping @ldionne, would you be able to take a look at this soon (or are you okay 
waiving the libc++ blocking requirement for it)? This is really useful for 
Android armv7, where the triple is traditionally spelled 
armv7-none-linux-androideabi but normalized to arvm7-none-linux-android, and 
this patch would resolve that discrepancy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140925

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

[PATCH] D140925: [CMake] Use Clang to infer the target triple

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai accepted this revision.
smeenai added a comment.

@tamas' suggestion would be a good change to make IMO, but I think it's outside 
the scope of this patch, and the patch as-is improves the status quo, so LGTM.

Is there any way to share the normalization logic between the two locations, or 
does compiler-rt's CMake logic still need to be standalone?




Comment at: compiler-rt/lib/builtins/CMakeLists.txt:39
+  if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+set(CXX_TARGET_TRIPLE ${CMAKE_CXX_COMPILER} 
--target=${LLVM_RUNTIME_TRIPLE} -print-target-triple)
+execute_process(COMMAND ${CXX_TARGET_TRIPLE}

Maybe something slightly more wordy like `NORMALIZE_TARGET_TRIPLE_COMMAND` 
would be a clearer name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140925

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

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D127812#4031849 , @ilinpv wrote:

> In D127812#4031313 , @smeenai wrote:
>
>> We can use `-mno-fmv` to avoid that dependency, right? We're interested in 
>> using that for our own code (where we don't make use of function 
>> multi-versioning), and want to prevent the compiler-rt support from being 
>> pulled in from the archive unnecessarily. It'd still be available for users 
>> who needed it.
>
> Right, you will need to explicitly provide '-mno-fmv` then. Currently 
> __aarch64_cpu_features cpu_model.c stuff located in bultins 
> (`libclang_rt.builtins-aarch64.a`). Did I understand you correctly that your 
> apps linked agains libclang_rt.builtins-aarch64.a and if we move function 
> multiversioning part to new library, lets say 
> `libclang_rt.cpu_features-aarch64.a`, that will resolve your concern ? As a 
> sidenote, builtins/cpu_model.c contains X86 CPU features used in function 
> multiversioning on that target as well.

It doesn't need to be in a separate library. It just needs to be in a different 
source file than the outlined atomics, so that it only gets included in the 
link if it's explicitly referenced and not just because outlined atomics are 
used.

You're right that it conceptually makes sense for this to be in `cpu_model.c` 
though. An alternative would be providing an option for compiler-rt to be built 
without the multiversioning support, e.g. if it's built with `-mno-fmv` itself. 
Does that seem reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127812

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

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D127812#4031101 , @ilinpv wrote:

> In D127812#4030881 , @smeenai wrote:
>
>> We're not actually using multi-versioning anywhere, but we're still paying 
>> the size cost for it as a result. Would we consider moving the newly added 
>> functions into their own file (or perhaps moving the outlined atomics 
>> functions into a different file), so that you can use outlined atomics 
>> without also paying the size cost of function multiversioning if you don't 
>> need it?
>
> Function multiversioning expects compiler-rt has __aarch64_cpu_features, it 
> will be broken if compiler-rt miss that ( 
> clang/lib/Driver/ToolChains/Clang.cpp:7231 ). I believe function 
> multiversioning will be used in Android as outline atomics already did.

We can use `-mno-fmv` to avoid that dependency, right? We're interested in 
using that for our own code (where we don't make use of function 
multi-versioning), and want to prevent the compiler-rt support from being 
pulled in from the archive unnecessarily. It'd still be available for users who 
needed it.

>> I also had a couple of general questions, since I think I'm missing 
>> something obvious:
>>
>> - How come we need both `init_cpu_features` and 
>> `init_cpu_features_resolver`? It seems like we're codegenning calls to the 
>> latter where needed, so I'm not sure what the former is adding on top.
>> - From what I can see, we're codegenning calls to 
>> `init_cpu_features_resolver` without any arguments, but the actual 
>> definition of that function has two arguments. How does that work?
>
> hwcaps are ABI specified arguments of ifunc resolver. The constructor 
> init_cpu_features calls getauxval to initialize hwcaps and then pass them 
> explicitly to init_cpu_features_resolver. If resolver called before 
> constructor we get init_cpu_features_resolver hwcap and hwcap2 as arguments 
> from dynamic loader.

Got it, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127812

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

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added subscribers: yozhu, lanza, smeenai.
smeenai added a comment.

I'm investigating a size increase we observed after this change for Meta's 
Android apps. This increases the size of compiler-rt by 1.6 KB, which is small 
by itself, but then compiler-rt is statically linked into each SO, and some of 
our apps have hundreds of SOs, so the increase adds up to a sizeable total. (We 
would ideally have far fewer SOs, but that's a pretty involved change.)

`-mno-fmv` doesn't help. What I've found is that `cpu_model.c` is getting 
pulled in from the compiler-rt archive into our SOs because of references to 
outlined atomics (`__aarch64_have_lse_atomics`), and then it has the static 
constructor `init_cpu_features`, which pulls in `init_cpu_features_resolver`. 
We're not actually using multi-versioning anywhere, but we're still paying the 
size cost for it as a result. Would we consider moving the newly added 
functions into their own file (or perhaps moving the outlined atomics functions 
into a different file), so that you can use outlined atomics without also 
paying the size cost of function multiversioning if you don't need it?

I also had a couple of general questions, since I think I'm missing something 
obvious:

- How come we need both `init_cpu_features` and `init_cpu_features_resolver`? 
It seems like we're codegenning calls to the latter where needed, so I'm not 
sure what the former is adding on top.
- From what I can see, we're codegenning calls to `init_cpu_features_resolver` 
without any arguments, but the actual definition of that function has two 
arguments. How does that work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127812

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

[PATCH] D138975: [perf-training] Support additional test suffixes

[Shoaib Meenai via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138975

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

[PATCH] D138974: [CMake] Support injecting extra dependencies for perf-training

[Shoaib Meenai via Phabricator via cfe-commits]

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

Might be worth a mention in either 
https://llvm.org/docs/AdvancedBuilds.html#multi-stage-pgo, to make it a bit 
more discoverable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138974

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

[PATCH] D138157: Make -fsanitize=scudo use scudo_standalone. Delete check-scudo.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

We're seeing a CMake error after this:

  CMake Error at 
/data/users/smeenai/Source/tcdev/external/llvm-project/compiler-rt/cmake/Modules/AddCompilerRT.cmake:368
 (add_library):
Error evaluating generator expression:
  
  $
  
Objects of target "RTGwpAsan.x86_64" referenced but no such target exists.

What appears to be happening is that 
`COMPILER_RT_SCUDO_STANDALONE_BUILD_SHARED` is ON by default, as is 
`COMPILER_RT_BUILD_GWP_ASAN`. However, `gwp_asan` isn't actually built unless 
it's also present in `COMPILER_RT_SANITIZERS_TO_BUILD`, so the combination of 
defaults is broken.

One option would be to add `gwp_asan` to `COMPILER_RT_SANITIZERS_TO_BUILD` if 
`COMPILER_RT_BUILD_GWP_ASAN` is ON, as in P8299 
. Another would be to make Scudo consider 
`COMPILER_RT_SANITIZERS_TO_BUILD` in addition to `COMPILER_RT_HAS_GWP_ASAN` 
before linking the GWP-ASan libraries. What are your thoughts here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138157

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

[PATCH] D136664: [CMake] Support building crt with the bootstrapping build

[Shoaib Meenai via Phabricator via cfe-commits]

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

In D136664#3884796 , @phosek wrote:

> In D136664#3882989 , @smeenai wrote:
>
>> I might be missing it, but I don't see `crt` depending on `builtins` (or 
>> vice versa). If there is actually no dep, could we build them together in a 
>> single configure, instead of needing to add another one for crt? If there is 
>> a dep and I missed it, then this LGTM.
>
> There is no dependency because `crt` and `builtins` aren't dependent on each 
> other. We could build them in a single configure, but we would another top 
> level `CMakeLists.txt`. Do you have any suggestions for where that should 
> live?

Sorry, this fell off my radar. This is fine as-is; avoiding the extra configure 
could potentially be nice for build times, but we can address that later if 
it's actually an issue. I don't have a great idea for the placement of the 
combined builtins+crt CMakeLists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136664

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

[PATCH] D136664: [CMake] Support building crt with the bootstrapping build

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I might be missing it, but I don't see `crt` depending on `builtins` (or vice 
versa). If there is actually no dep, could we build them together in a single 
configure, instead of needing to add another one for crt? If there is a dep and 
I missed it, then this LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136664

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

[PATCH] D131963: [libc++] Add custom clang-tidy checks

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D131963#3845092 , @philnik wrote:

> In D131963#3831786 , @smeenai wrote:
>
>> In D131963#3811811 , @ldionne 
>> wrote:
>>
>>> I'd be fine with this as-is if it works in the CI.
>>>
>>> IIUC, the current blocker for this is that the `ClangConfig.cmake` 
>>> installed by LLVM is not robust to the dev packages missing. If you do 
>>> `find_package(Clang 16.0)`, it will error out if the dev packages are not 
>>> present, since it will try to reference static libraries (and other 
>>> artifacts) that don't exist and try to create IMPORTED targets for those. 
>>> @smeenai @beanz do you know more about that, or do you know someone that 
>>> does? Do you know who set up the CMake config files so that 
>>> `find_package(Clang)` would work in the first place? I'd also welcome your 
>>> input on the `ClangConfigVersion.cmake.in` changes.
>>>
>>> Just for the context, what we're trying to do here is simply use 
>>> `clang-tidy`'s development packages from the libc++ build to add our own 
>>> custom clang-tidy checks.
>>>
>>> Accepting because this LGTM, although we do have some stuff to resolve 
>>> before this can be shipped.
>>
>> IIRC, the intended solution was to use `LLVM_DISTRIBUTION_COMPONENTS` 
>> (https://llvm.org/docs/BuildingADistribution.html). When you use that 
>> option, the generated CMake package files (at least in the install tree; I'm 
>> not sure about the ones in the build tree) should only contain imported 
>> targets that were part of your distribution. Multi-distribution support 
>> extends this even further, where the file defining the imported targets for 
>> a distribution is only imported if it's present, so things should work as 
>> expected both with and without the dev packages. Is that workable for your 
>> use case?
>
> That's a thing the vendor has to change, right? If we only get the targets 
> which are actually available it should work just fine.

Correct, that would have to be set up by whoever distributes the LLVM package 
you're using.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963

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

[PATCH] D131963: [libc++] Add custom clang-tidy checks

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D131963#3811811 , @ldionne wrote:

> I'd be fine with this as-is if it works in the CI.
>
> IIUC, the current blocker for this is that the `ClangConfig.cmake` installed 
> by LLVM is not robust to the dev packages missing. If you do 
> `find_package(Clang 16.0)`, it will error out if the dev packages are not 
> present, since it will try to reference static libraries (and other 
> artifacts) that don't exist and try to create IMPORTED targets for those. 
> @smeenai @beanz do you know more about that, or do you know someone that 
> does? Do you know who set up the CMake config files so that 
> `find_package(Clang)` would work in the first place? I'd also welcome your 
> input on the `ClangConfigVersion.cmake.in` changes.
>
> Just for the context, what we're trying to do here is simply use 
> `clang-tidy`'s development packages from the libc++ build to add our own 
> custom clang-tidy checks.
>
> Accepting because this LGTM, although we do have some stuff to resolve before 
> this can be shipped.

IIRC, the intended solution was to use `LLVM_DISTRIBUTION_COMPONENTS` 
(https://llvm.org/docs/BuildingADistribution.html). When you use that option, 
the generated CMake package files (at least in the install tree; I'm not sure 
about the ones in the build tree) should only contain imported targets that 
were part of your distribution. Multi-distribution support extends this even 
further, where the file defining the imported targets for a distribution is 
only imported if it's present, so things should work as expected both with and 
without the dev packages. Is that workable for your use case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963

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

[PATCH] D131153: AArch64: disable asynchronous unwind by default for MachO.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I just discovered this and was about to post on Discourse about it. Glad you're 
already on it :)

I don't think this is quite correct though? It'll turn off unwind tables for 
AArch64 entirely, whereas we want to keep sync unwind tables. If we want to 
minimize changes to the existing logic, maybe we could add 
`IsSyncUnwindTablesDefault` instead and have `SyncUnwindTables` default to that 
and `AsyncUnwindTables` default to `!` that?




Comment at: clang/include/clang/Driver/ToolChain.h:501
+  /// IsAsyncUnwindTablesDefault - Does this tool chain use
+  /// -fasync-unwind-tables by default.
+  virtual bool

I believe the option is spelled `-fasynchronous-unwind-tables`.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2898
+bool MachO::IsAsyncUnwindTablesDefault(const ArgList ) const {
+  // AArch64 has compact unwind format, making the size overhead from
+  // asynchronous unwind particularly bad, so disable by default.

This comment is a bit confusing. I thought Darwin supported compact unwind for 
all architectures. Is it that compact unwind can't handle async unwind on arm64 
but can on other architectures?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131153

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

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Yeah, this makes sense to me if we factor out the commonalities. I agree that 
it's nicer to have a single option vs. having to specify every single native 
tool individually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131052

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

[PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

This is breaking our build setup. It causes e.g. the Clang resource headers to 
be installed to `lib64/clang` instead of `lib/clang`. Given this and the other 
breakages, can we revert for now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[PATCH] D131874: [Clang] Tighten restrictions on enum out of range diagnostic to avoid constant initialization

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I confirmed that this fixes all remaining issues on our end. Thank you!


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

https://reviews.llvm.org/D131874

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

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D131528#3723231 , @ayermolo wrote:

> In D131528#3722395 , @shafik wrote:
>
>> In D131528#3719430 , @ayermolo 
>> wrote:
>>
>>> Was this and previous change intended to capture this case?
>>>  const enum NumberType neg_one = (enum NumberType) ((enum NumberType) 0 - 
>>> (enum NumberType) 1);
>>
>> That was not intended and I plan on fixing that.
>
> Ah I see. Thanks for elaborating. Can you tag me in the fix diff please.

D131874  seems like the fix for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131528

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

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D131307#3726643 , @erichkeane 
wrote:

> In D131307#3726631 , @smeenai wrote:
>
>> Was it intended that the warning generated here isn't silenced by `-w`, only 
>> by an explicit `-Wno-enum-constexpr-conversion` (or `-Wno-everythning`), and 
>> that `-Wno-error` doesn't downgrade the error? See 
>> https://godbolt.org/z/s9qPveTWG for an example.
>
> Yes, we've discussed that on this thread before: Clang's behavior for 
> warnings-as-default-error require explicit suppression of the warning, and 
> isn't effected by global warning/error settings.



In D131307#3726644 , @aaron.ballman 
wrote:

> In D131307#3726631 , @smeenai wrote:
>
>> Was it intended that the warning generated here isn't silenced by `-w`, only 
>> by an explicit `-Wno-enum-constexpr-conversion` (or `-Wno-everythning`), and 
>> that `-Wno-error` doesn't downgrade the error? See 
>> https://godbolt.org/z/s9qPveTWG for an example.
>
> Yes. That is the behavior of warnings which default to an error. The idea is: 
> these aren't really *warnings*, they're errors that we let users downgrade 
> for . So `-w` shouldn't blanket disable them or users will be very 
> surprised when that warning turns into a hard error in a future version of 
> the compiler. So you have to explicitly disable warnings that default to an 
> error. The same is true for `-Wno-error` behavior.

Yup, all of that makes sense; I just missed it earlier. Thank you both :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131307

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

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Was it intended that the warning generated here isn't silenced by `-w`, only by 
an explicit `-Wno-enum-constexpr-conversion` (or `-Wno-everythning`), and that 
`-Wno-error` doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG 
for an example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131307

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

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Should there be a test case to verify that the warning isn't triggered in a 
non-constexpr context anymore?


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

https://reviews.llvm.org/D131528

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

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D130058#3708209 , @ayermolo wrote:

> +2 turning it in to warning. It's breaking our builds, due to boost. :(

This was done by D131307 , which I'm hoping 
will land soon :)


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

https://reviews.llvm.org/D130058

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

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

All right, SGTM


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

https://reviews.llvm.org/D131307

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

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D131307#3704709 , @thakis wrote:

> It's already an error, but it's a warning default-mapped to an error. You can 
> -Wno-error=name to downgrade it into a warning, but that requires an explicit 
> action. So people are unlikely to miss it.
>
> This is how we usually handle these breaking changes.
>
> Maybe there could be a test for the -Wno-error= case? But this looks roughly 
> right to me overall. I haven't looked in detail.

Right, but we also want people to understand that the downgrade possibility 
will be going away in the next release (i.e. it'll always be an error and 
there's nothing you can do about that), so they really do want to deal with 
this as a priority.


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

https://reviews.llvm.org/D131307

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

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Thank you!

I'm worried that users might miss this if it's only in the release notes, and 
then we'd be in a similar situation again when we try converting it to an 
error. Maybe you could also include the bit about the diagnostic turning into 
error-only in the diagnostic message itself, to make it more likely for people 
to notice it?


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

https://reviews.llvm.org/D131307

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

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D130058#3698213 , @shafik wrote:

>> Given that we have a non-obvious (at least to me) issue in a widely used 
>> third-party library, would we consider giving users some way to opt out of 
>> this error, at least as a transition tool?
>
> Thank your feedback, this is very helpful. I won't have time to dig into this 
> in detail till tomorrow but it does not look like there is a nice fix here. I 
> think we may want to consider turning this into a warning for some transition 
> period.

Following up because we have an internal integration blocked on this. Are we 
planning to turn this into a warning soon, or should I just put in local 
workarounds in the meantime?


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

https://reviews.llvm.org/D130058

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

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

https://godbolt.org/z/axhbj3MPE is a simpler repro in Godbolt with Boost 1.79 
(the latest version).

I agree that it'll be tricky to manage expectations if we downgrade to a 
warning, but I also suspect that the first time many people will run into this 
new error is when we start doing the RCs for LLVM 16, and we'll discover lots 
more breakage then. I'd hope that giving the warning a sufficiently scary name 
(e.g. suffixing it with `-will-be-removed-in-clang-17`) and having a release 
notes entry would help us actually be able to convert this into an error in the 
future though and not just keep kicking the can down the road. It'd also help 
if we could compile the fixes for issues with this new error in common 
libraries (e.g. the Boost error here) somewhere, so that people using those 
common libraries can reference that to fix their own codebases.


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

https://reviews.llvm.org/D130058

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

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Sorry for chiming in a bit late here.

I'm working on getting a large internal codebase to compile after this change. 
Most of the issues are easy enough to deal with, but there's one Boost error 
I'd like to highlight: P8289 . (We might be 
using a slightly older Boost version, but upgrading it would be a pretty 
significant undertaking, unfortunately, and current Boost appears to have 
similar code: 
https://github.com/boostorg/mpl/blob/8f10d06b9637a67b23a3a11791de5bd6bcd2e112/include/boost/mpl/aux_/integral_wrapper.hpp#L72-L73.)

There's a couple of things that make this error hard to parse:

- The note about the integer value being out of range for the enumeration 
doesn't appear until towards the end of the error (line 55 in my paste), even 
though it's the key to understanding why the argument isn't considered a 
constant expression
- The note doesn't mention //which// enumeration type the value is out of the 
valid range for, and with the mix of macros and templates going on in this 
error message, that's non-trivial to decipher

After digging through the preprocessor output, it turns out that 
`AUX_WRAPPER_VALUE_TYPE` is defined as `T` here (after being undefined and 
redefined a bunch of times, which makes this even trickier), and coupled with 
the types in the templates in the error you can determine that `T` is 
boost::numeric::udt_builtin_mixture_enum 
,
 but it'd be much easier if the diagnostic gave you the type directly and also 
appeared much earlier.

Diagnostic quality aside, I'm not really sure what to do about this error. It 
appears to ultimately originate from here 
,
 which will attempt to compute a `prior` for the first enum value, which is 
incorrect. I can fix the issue by giving `udt_builtin_mixture_enum` and 
int_float_mixture_enum 

 an underlying type of `int`, but I lack the Boost know-how to understand if 
that's reasonable.

Given that we have a non-obvious (at least to me) issue in a widely used 
third-party library, would we consider giving users some way to opt out of this 
error, at least as a transition tool?


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

https://reviews.llvm.org/D130058

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