[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] D140925: [CMake] Use Clang to infer the target triple

[Louis Dionne via Phabricator via cfe-commits]

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



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)

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.


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

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

SGTM this was previously blocked on other changes, but those all landed so I 
can go ahead, rebase and land it today.


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 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] D140925: [CMake] Use Clang to infer the target triple

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D140925#4151524 , @smeenai wrote:

> 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.

This patch looks reasonable. The target triple example can be added into "and 
avoids the issue where the build uses a different triple spelling." @phosek


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 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] D140925: [CMake] Use Clang to infer the target triple

[Tamás Szelei via Phabricator via cfe-commits]

tamas added inline comments.



Comment at: runtimes/CMakeLists.txt:168
+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}

phosek wrote:
> tamas wrote:
> > I think there is one downside to this: the `normalize` function will not, 
> > in fact, normalize alternative spellings for equivalent architectures (and 
> > likely other components, I haven't checked that):
> > ```
> > + clang -print-target-triple -target amd64-unknown-linux-musl
> > amd64-unknown-linux-musl
> > + clang -print-target-triple -target x86_64-unknown-linux-musl
> > x86_64-unknown-linux-musl
> > ```
> > So the issue where the distribution build can install runtimes in paths 
> > that won't be found by the driver still remains 
> > (https://github.com/llvm/llvm-project/issues/57781). One way to fix this 
> > could be adding a new switch that does a more opinionated normalization 
> > (for example, always picks `x86_64` in the above). Sort of a reverse for 
> > `Triple::parseArch` etc. where it always returns one certain spelling when 
> > there are multiple common alternatives. Alternatively, `normalize` could be 
> > changed to do this, but that might subtle breaking consequences for users 
> > which are hard to foresee.
> I think this is an orthogonal issue to the one that this change is trying to 
> address. Clang currently uses normalized triples to for its include 
> directories (libc++ headers) and runtime libraries. This change ensures that 
> the triple spelling used by CMake and Clang matches, but it doesn't change 
> the normalization logic.
> 
> From Clang's perspective, `amd64-unknown-linux-musl` and 
> `x86_64-unknown-linux-musl` are two different triples and so it'd use 
> different search directories. We could consider changing the normalization 
> rules to normalize these to the same triple, but that should be done 
> separately (and would likely require further discussion to understand the 
> potential consequences of such a change).
I don't mean to raise this as a potential blocker and this is certainly 
somewhat orthogonal. However, I think this not true: 

> From Clang's perspective, amd64-unknown-linux-musl and 
> x86_64-unknown-linux-musl are two different triples

`amd64` and `x86_64` are very explictly parsed as the same: 
https://github.com/llvm/llvm-project/blob/6dc85bd3fde7df2999fda07e9e9f2e83d52c6125/llvm/lib/TargetParser/Triple.cpp#L454

I'm not sure if it's a good idea to change the current normalization logic, 
hence my suggestion for a new flag. But this is probably a discussion for 
discourse/discord.


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

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: runtimes/CMakeLists.txt:168
+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}

tamas wrote:
> I think there is one downside to this: the `normalize` function will not, in 
> fact, normalize alternative spellings for equivalent architectures (and 
> likely other components, I haven't checked that):
> ```
> + clang -print-target-triple -target amd64-unknown-linux-musl
> amd64-unknown-linux-musl
> + clang -print-target-triple -target x86_64-unknown-linux-musl
> x86_64-unknown-linux-musl
> ```
> So the issue where the distribution build can install runtimes in paths that 
> won't be found by the driver still remains 
> (https://github.com/llvm/llvm-project/issues/57781). One way to fix this 
> could be adding a new switch that does a more opinionated normalization (for 
> example, always picks `x86_64` in the above). Sort of a reverse for 
> `Triple::parseArch` etc. where it always returns one certain spelling when 
> there are multiple common alternatives. Alternatively, `normalize` could be 
> changed to do this, but that might subtle breaking consequences for users 
> which are hard to foresee.
I think this is an orthogonal issue to the one that this change is trying to 
address. Clang currently uses normalized triples to for its include directories 
(libc++ headers) and runtime libraries. This change ensures that the triple 
spelling used by CMake and Clang matches, but it doesn't change the 
normalization logic.

From Clang's perspective, `amd64-unknown-linux-musl` and 
`x86_64-unknown-linux-musl` are two different triples and so it'd use different 
search directories. We could consider changing the normalization rules to 
normalize these to the same triple, but that should be done separately (and 
would likely require further discussion to understand the potential 
consequences of such a change).


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

[Tamás Szelei via Phabricator via cfe-commits]

tamas added inline comments.



Comment at: runtimes/CMakeLists.txt:168
+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}

I think there is one downside to this: the `normalize` function will not, in 
fact, normalize alternative spellings for equivalent architectures (and likely 
other components, I haven't checked that):
```
+ clang -print-target-triple -target amd64-unknown-linux-musl
amd64-unknown-linux-musl
+ clang -print-target-triple -target x86_64-unknown-linux-musl
x86_64-unknown-linux-musl
```
So the issue where the distribution build can install runtimes in paths that 
won't be found by the driver still remains 
(https://github.com/llvm/llvm-project/issues/57781). One way to fix this could 
be adding a new switch that does a more opinionated normalization (for example, 
always picks `x86_64` in the above). Sort of a reverse for `Triple::parseArch` 
etc. where it always returns one certain spelling when there are multiple 
common alternatives. Alternatively, `normalize` could be changed to do this, 
but that might subtle breaking consequences for users which are hard to foresee.


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

[Petr Hosek via Phabricator via cfe-commits]

phosek created this revision.
phosek added reviewers: smeenai, ldionne, mstorsjo, mgorny.
Herald added subscribers: Enna1, abrachet.
Herald added a project: All.
phosek requested review of this revision.
Herald added projects: clang, Sanitizers, libc++, libc++abi.
Herald added subscribers: libcxx-commits, Sanitizers, cfe-commits.
Herald added a reviewer: libc++.
Herald added a reviewer: libc++abi.

When using Clang as a compiler, use Clang to normalize the triple that's
used to construct path for runtime library build and install paths. This
ensures that paths are consistent and avoids the issue where the build
uses a different triple spelling.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140925

Files:
  clang/cmake/caches/Fuchsia-stage2.cmake
  compiler-rt/lib/builtins/CMakeLists.txt
  runtimes/CMakeLists.txt


Index: runtimes/CMakeLists.txt
===
--- runtimes/CMakeLists.txt
+++ runtimes/CMakeLists.txt
@@ -164,6 +164,20 @@
 set(LLVM_RUNTIME_TRIPLE "${LLVM_HOST_TRIPLE}" CACHE STRING
   "Triple used as runtime instrallation path.")
 
+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}
+RESULT_VARIABLE result
+OUTPUT_VARIABLE output
+OUTPUT_STRIP_TRAILING_WHITESPACE)
+  if(result EQUAL 0)
+set(LLVM_RUNTIME_TRIPLE ${output})
+  else()
+string(REPLACE ";" " " CXX_TARGET_TRIPLE "${CXX_TARGET_TRIPLE}")
+message(WARNING "Failed to execute `${CXX_TARGET_TRIPLE}` to normalize 
target triple.")
+  endif()
+endif()
+
 option(LLVM_INCLUDE_TESTS "Generate build targets for the runtimes unit 
tests." ON)
 option(LLVM_INCLUDE_DOCS "Generate build targets for the runtimes 
documentation." ON)
 option(LLVM_ENABLE_SPHINX "Use Sphinx to generate the runtimes documentation." 
OFF)
Index: compiler-rt/lib/builtins/CMakeLists.txt
===
--- compiler-rt/lib/builtins/CMakeLists.txt
+++ compiler-rt/lib/builtins/CMakeLists.txt
@@ -35,6 +35,19 @@
   if (NOT LLVM_RUNTIMES_BUILD)
 load_llvm_config()
   endif()
+  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}
+  RESULT_VARIABLE result
+  OUTPUT_VARIABLE output
+  OUTPUT_STRIP_TRAILING_WHITESPACE)
+if(result EQUAL 0)
+  set(LLVM_RUNTIME_TRIPLE ${output})
+else()
+  string(REPLACE ";" " " CXX_TARGET_TRIPLE "${CXX_TARGET_TRIPLE}")
+  message(WARNING "Failed to execute `${CXX_TARGET_TRIPLE}` to normalize 
target triple.")
+endif()
+  endif()
   construct_compiler_rt_default_triple()
 
   include(SetPlatformToolchainTools)
Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -86,7 +86,7 @@
   set(RUNTIMES_${target}_LLVM_ENABLE_RUNTIMES "compiler-rt;libcxx" CACHE 
STRING "")
 endif()
 
-foreach(target 
aarch64-unknown-linux-gnu;armv7-unknown-linux-gnueabihf;i386-unknown-linux-gnu;x86_64-unknown-linux-gnu)
+foreach(target 
aarch64-linux-gnu;armv7-linux-gnueabihf;i386-linux-gnu;x86_64-linux-gnu)
   if(LINUX_${target}_SYSROOT)
 # Set the per-target builtins options.
 list(APPEND BUILTIN_TARGETS "${target}")


Index: runtimes/CMakeLists.txt
===
--- runtimes/CMakeLists.txt
+++ runtimes/CMakeLists.txt
@@ -164,6 +164,20 @@
 set(LLVM_RUNTIME_TRIPLE "${LLVM_HOST_TRIPLE}" CACHE STRING
   "Triple used as runtime instrallation path.")
 
+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}
+RESULT_VARIABLE result
+OUTPUT_VARIABLE output
+OUTPUT_STRIP_TRAILING_WHITESPACE)
+  if(result EQUAL 0)
+set(LLVM_RUNTIME_TRIPLE ${output})
+  else()
+string(REPLACE ";" " " CXX_TARGET_TRIPLE "${CXX_TARGET_TRIPLE}")
+message(WARNING "Failed to execute `${CXX_TARGET_TRIPLE}` to normalize target triple.")
+  endif()
+endif()
+
 option(LLVM_INCLUDE_TESTS "Generate build targets for the runtimes unit tests." ON)
 option(LLVM_INCLUDE_DOCS "Generate build targets for the runtimes documentation." ON)
 option(LLVM_ENABLE_SPHINX "Use Sphinx to generate the runtimes documentation." OFF)
Index: compiler-rt/lib/builtins/CMakeLists.txt
===
--- compiler-rt/lib/builtins/CMakeLists.txt
+++ compiler-rt/lib/builtins/CMakeLists.txt
@@ -35,6 +35,19 @@
   if (NOT LLVM_RUNTIMES_BUILD)
 load_llvm_config()
   endif()
+  if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)