This is an archive of the discontinued LLVM Phabricator instance.

Prepare Compiler-RT for GnuInstallDirs, matching libcxx, document all
ClosedPublic

Authored by Ericson2314 on Jul 10 2021, 1:55 PM.

Details

Reviewers
mstorsjo
phosek
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG1e03c37b97b6: Prepare Compiler-RT for GnuInstallDirs, matching libcxx, document all
Summary

This is a second attempt at D101497, which landed as
9a9bc76c0eb72f0f2732c729a460abbd5239c2e3 but had to be reverted in
8cf7ddbdd4e5af966a369e170c73250f2e3920e7.

This issue was that in the case that COMPILER_RT_INSTALL_PATH is
empty, expressions like "${COMPILER_RT_INSTALL_PATH}/bin" evaluated to
"/bin" not "bin" as intended and as was originally.

One solution is to make COMPILER_RT_INSTALL_PATH always non-empty,
defaulting it to CMAKE_INSTALL_PREFIX. D99636 adopted that approach.
But, I think it is more ergonomic to allow those project-specific paths
to be relative the global ones. Also, making install paths absolute by
default inhibits the proper behavior of functions like
GNUInstallDirs_get_absolute_install_dir which make relative install
paths absolute in a more complicated way.

Given all this, I will define a function like the one asked for in
https://gitlab.kitware.com/cmake/cmake/-/issues/19568 (and needed for a
similar use-case).


Original message:

Instead of using COMPILER_RT_INSTALL_PATH through the CMake for
complier-rt, just use it to define variables for the subdirs which
themselves are used.

This preserves compatibility, but later on we might consider getting rid
of COMPILER_RT_INSTALL_PATH and just changing the defaults for the
subdir variables directly.


There was a seaming bug where the (non-Apple) per-target libdir was
${target} not lib/${target}. I suspect that has to do with the docs
on COMPILER_RT_INSTALL_PATH saying was the library dir when that's no
longer true, so I just went ahead and fixed it, allowing me to define
fewer and more sensible variables.

That last part should be the only behavior changes; everything else
should be a pure refactoring.


I added some documentation of these variables too. In particular, I
wanted to highlight the gotcha where -DSomeCachePath=... without the
:PATH will lead CMake to make the path absolute. See [1] for
discussion of the problem, and [2] for the brief official documentation
they added as a result.

[1]: https://cmake.org/pipermail/cmake/2015-March/060204.html

[2]: https://cmake.org/cmake/help/latest/manual/cmake.1.html#options

In 38b2dec37ee735d5409148e71ecba278caf0f969 the problem was somewhat
misidentified and so :STRING was used, but :PATH is better as it
sets the correct type from the get-go.


D99484 is the main thrust of the GnuInstallDirs work. Once this lands,
it should be feasible to follow both of these up with a simple patch for
compiler-rt analogous to the one for libcxx.

Diff Detail

Event Timeline

Ericson2314 created this revision.Jul 10 2021, 1:55 PM
Ericson2314 requested review of this revision.Jul 10 2021, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2021, 1:55 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

+1, this one works where the previous one failed for me - thanks!

I haven't followed all the intricate cmake details here so I'd prefer to leave it to @phosek to actually re-approve this, but it seems to not break things for me now at least.

Ericson2314 added a comment.EditedJul 10 2021, 10:32 PM

For reference, here is the "diff of diffs"

 diff --git a/compiler-rt/cmake/base-config-ix.cmake b/compiler-rt/cmake/base-config-ix.cmake
-index 1edab43e7c0d..c4d78098d070 100644
+index 1edab43e7c0d..ed2ef0e4829c 100644
 --- a/compiler-rt/cmake/base-config-ix.cmake
 +++ b/compiler-rt/cmake/base-config-ix.cmake
 @@ -68,8 +68,8 @@ else()
@@ -144,7 +166,25 @@ index 1edab43e7c0d..c4d78098d070 100644
    option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests." OFF)
    option(COMPILER_RT_ENABLE_WERROR "Fail and stop if warning is triggered" OFF)
    # Use a host compiler to compile/link tests.
-@@ -89,16 +89,24 @@ if(NOT DEFINED COMPILER_RT_OS_DIR)
+@@ -85,20 +85,45 @@ else()
+   set(COMPILER_RT_TEST_COMPILER_ID GNU)
+ endif()
+
++function(extend_install_path joined_path current_segment)
++  if("${current_segment}" STREQUAL "")
++    set(temp_path "${COMPILER_RT_INSTALL_PATH}")
++  elseif("${COMPILER_RT_INSTALL_PATH}" STREQUAL "")
++    set(temp_path "${current_segment}")
++  elseif(IS_ABSOLUTE "${current_segment}")
++    message(WARNING "Since \"${current_segment}\" is absolute, it overrides COMPILER_RT_INSTALL_PATH: \"${temp_path}\".")
++    set(temp_path "${current_segment}")
++  else()
++    set(temp_path "${temp_path}/${current_segment}")
++  endif()
++  set(${joined_path} "${temp_path}" PARENT_SCOPE)
++endfunction()
++
+ if(NOT DEFINED COMPILER_RT_OS_DIR)
    string(TOLOWER ${CMAKE_SYSTEM_NAME} COMPILER_RT_OS_DIR)
  endif()
  if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
@@ -156,23 +196,26 @@ index 1edab43e7c0d..c4d78098d070 100644
 -  set(COMPILER_RT_LIBRARY_OUTPUT_DIR
 +  set(COMPILER_RT_OUTPUT_LIBRARY_DIR
 +    ${COMPILER_RT_OUTPUT_DIR}/lib)
-+  set(COMPILER_RT_INSTALL_LIBRARY_DIR
-+    ${COMPILER_RT_INSTALL_PATH}/lib CACHE PATH
++  extend_install_path(default_install_path lib)
++  set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH
 +    "Path where built compiler-rt libraries should be installed.")
 +else(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
 +  set(COMPILER_RT_OUTPUT_LIBRARY_DIR
      ${COMPILER_RT_OUTPUT_DIR}/lib/${COMPILER_RT_OS_DIR})
 -  set(COMPILER_RT_LIBRARY_INSTALL_DIR
 -    ${COMPILER_RT_INSTALL_PATH}/lib/${COMPILER_RT_OS_DIR})
-+  set(COMPILER_RT_INSTALL_LIBRARY_DIR
-+    ${COMPILER_RT_INSTALL_PATH}/lib/${COMPILER_RT_OS_DIR} CACHE PATH
++  extend_install_path(default_install_path "lib/${COMPILER_RT_OS_DIR}")
++  set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH
 +    "Path where built compiler-rt libraries should be installed.")
  endif()
-+set(COMPILER_RT_INSTALL_BINARY_DIR "${COMPILER_RT_INSTALL_PATH}/bin" CACHE PATH
++extend_install_path(default_install_path bin)
++set(COMPILER_RT_INSTALL_BINARY_DIR "${default_install_path}" CACHE PATH
 +  "Path where built compiler-rt executables should be installed.")
-+set(COMPILER_RT_INSTALL_INCLUDE_DIR "${COMPILER_RT_INSTALL_PATH}/include" CACHE PATH
++extend_install_path(default_install_path include)
++set(COMPILER_RT_INSTALL_INCLUDE_DIR "${default_install_path}" CACHE PATH
 +  "Path where compiler-rt headers should be installed.")
-+set(COMPILER_RT_INSTALL_DATA_DIR "${COMPILER_RT_INSTALL_PATH}/share" CACHE PATH
++extend_install_path(default_install_path share)
++set(COMPILER_RT_INSTALL_DATA_DIR "${default_install_path}" CACHE PATH
 +  "Path where compiler-rt data files should be installed.")

  if(APPLE)

The test failures seem to all be libcxx and thus presumably unrelated.

phosek added inline comments.Jul 11 2021, 10:50 PM
compiler-rt/cmake/base-config-ix.cmake
97

This isn't set before so it should be always empty in which case you'll end up with an absolute path (that is /${current_segment}) right? Isn't that a bug?

Fix bugs, add lots of docs

Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 12 2021, 9:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Ericson2314 retitled this revision from Prepare Compiler-RT for GnuInstallDirs, matching libcxx to Prepare Compiler-RT for GnuInstallDirs, matching libcxx, document all.Jul 12 2021, 9:46 AM
Ericson2314 edited the summary of this revision. (Show Details)
Ericson2314 marked an inline comment as done.Jul 12 2021, 9:51 AM
Ericson2314 added inline comments.
compiler-rt/cmake/base-config-ix.cmake
97

Eek! that's embarrassing. Turns out that testing with -D is more fraught than I thought, and I wasn't hitting that case. Fixed the bug and documented the -D subtlety too.

Ericson2314 marked an inline comment as done.Jul 12 2021, 9:52 AM
Ericson2314 added inline comments.
compiler-rt/docs/BuildingCompilerRT.rst
12–22 ↗(On Diff #357969)

Modeled this file off libc++'s, including taking these two paragraphs. They seem appropriate enough here.

phosek added inline comments.Jul 12 2021, 1:28 PM
compiler-rt/docs/BuildingCompilerRT.rst
12–22 ↗(On Diff #357969)

I'm not sure how appropriate this is actually since Compiler-RT is usually shipped together with the compiler, not the operating system (unlike libc++) since the LLVM and Compiler-RT ABI is tightly coupled. I'd omit this for now.

16 ↗(On Diff #357969)

compiler-rt?

59 ↗(On Diff #357969)

Is this what you meant here?

87 ↗(On Diff #357969)

This variable is used for data files (like resources), not headers.

Ericson2314 marked 2 inline comments as done.

Fixes from comments, Thanks!

compiler-rt/docs/BuildingCompilerRT.rst
12–22 ↗(On Diff #357969)

Fair enough. Removed.

16 ↗(On Diff #357969)

Ah there is no such section for compiler-rt/docs. I deleted and reworded.

59 ↗(On Diff #357969)

Oops. Yes.

87 ↗(On Diff #357969)

Says "data" like the CMake variale description does now too.

phosek accepted this revision.Jul 12 2021, 6:12 PM

LGTM

This revision was not accepted when it landed; it landed in state Needs Review.Jul 13 2021, 8:21 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
arphaman added inline comments.
compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake
509

It looks like this change broke the macho_embedded layout for Darwin's compiler-rt build, so now the clang driver isn unable to find these libraries.

I will commit a change that uses COMPILER_RT_OUTPUT_DIR again for the macho_embedded libraries.

Ericson2314 added inline comments.Sep 21 2021, 10:12 AM
compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake
509

Can you help me understand this better? COMPILER_RT_OUTPUT_LIBRARY_DIR should be defined to be the same thing unless target-specific directories are used, Is the problem in the latter case?

arphaman added inline comments.Dec 8 2021, 2:21 PM
compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake
509

The problem is that Darwin was emitting macho_embedded libraries under usr/lib/clang/<version>/lib/darwin/macho_embedded, but this change moved them to usr/lib/clang/<version>/lib/macho_embedded, so now the clang driver isn't able to find them.

I think using COMPILER_RT_OUTPUT_LIBRARY_DIR is the right thing, but we still want to be compatible with the existing layout unless we change the driver. I think changing the driver might be a better approach though, so I will try that.

arphaman added inline comments.Dec 8 2021, 2:24 PM
compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake
509

Sorry, I meant to say that Darwin was emitting macho_embedded libraries under usr/lib/clang/<version>/lib/macho_embedded, but this change moved them to usr/lib/clang/<version>/lib/darwin/macho_embedded, so now the clang driver isn't able to find them.

tstellar added inline comments.
compiler-rt/cmake/base-config-ix.cmake
126

Was there supposed to be an extend_install_path call here?

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 4:49 PM
Herald added a subscriber: Enna1. · View Herald Transcript