Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libunwind] Compile the asm as well as the C++ source #86351

Closed
wants to merge 1 commit into from

Conversation

JonChesterfield
Copy link
Collaborator

When a CMakeLists.txt is missing a 'project' statement you get the default supported languages of C and CXX. https://cmake.org/cmake/help/latest/command/project.html. The help says ASM should be listed last.

CMake doesn't raise an error about the .S files it has been told about when project is missing. It silently ignores them. In this case, the symptom is an undefined symbol *jumpto in the library.

Working theory for why this isn't more obviously broken everywhere is the 'runtimes' CMakeLists.txt does contain a 'project' statement which lists ASM and/or by default linking shared libraries with undefined symbols succeeds.

The string immediately after project appears to be arbitrary, chosen 'Unwind' to match the capitalization of 'Runtimes'.

For completeness, this also removes the following warning when building libunwind by itself:

CMake Warning (dev) in CMakeLists.txt:
No project() command is present. The top-level CMakeLists.txt file must
contain a literal, direct call to the project() command. Add a line of
code such as

project(ProjectName)

near the top of the file, but after cmake_minimum_required().

CMake is pretending there is a "project(Project)" command on the first
line.
This warning is for project developers. Use -Wno-dev to suppress it.

This gives no hint that the consequence of ignoring this warning is cmake will ignore your assembly.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 22, 2024

@llvm/pr-subscribers-libunwind

Author: Jon Chesterfield (JonChesterfield)

Changes

When a CMakeLists.txt is missing a 'project' statement you get the default supported languages of C and CXX. https://cmake.org/cmake/help/latest/command/project.html. The help says ASM should be listed last.

CMake doesn't raise an error about the .S files it has been told about when project is missing. It silently ignores them. In this case, the symptom is an undefined symbol *jumpto in the library.

Working theory for why this isn't more obviously broken everywhere is the 'runtimes' CMakeLists.txt does contain a 'project' statement which lists ASM and/or by default linking shared libraries with undefined symbols succeeds.

The string immediately after project appears to be arbitrary, chosen 'Unwind' to match the capitalization of 'Runtimes'.

For completeness, this also removes the following warning when building libunwind by itself:

>CMake Warning (dev) in CMakeLists.txt:
> No project() command is present. The top-level CMakeLists.txt file must
> contain a literal, direct call to the project() command. Add a line of
> code such as
>
> project(ProjectName)
>
> near the top of the file, but after cmake_minimum_required().
>
> CMake is pretending there is a "project(Project)" command on the first
> line.
> This warning is for project developers. Use -Wno-dev to suppress it.

This gives no hint that the consequence of ignoring this warning is cmake will ignore your assembly.


Full diff: https://github.com/llvm/llvm-project/pull/86351.diff

1 Files Affected:

  • (modified) libunwind/CMakeLists.txt (+1)
diff --git a/libunwind/CMakeLists.txt b/libunwind/CMakeLists.txt
index 806d5a783ec39c..01d3b72b73e842 100644
--- a/libunwind/CMakeLists.txt
+++ b/libunwind/CMakeLists.txt
@@ -3,6 +3,7 @@
 #===============================================================================
 
 cmake_minimum_required(VERSION 3.20.0)
+project(Unwind LANGUAGES C CXX ASM)
 
 set(LLVM_COMMON_CMAKE_UTILS "${CMAKE_CURRENT_SOURCE_DIR}/../cmake")
 

Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link

✅ With the latest revision this PR passed the C/C++ code formatter.

@mstorsjo
Copy link
Member

This build configuration was explicitly made disallowed in 6f17768, with an error message explaining the situation. However that error message was later removed in 0a22dfc.

@arichardson
Copy link
Member

It might make sense to restore the message(FATAL_ERROR) since that is any easy mistake to make considering it used to be the default way of building libunwind.

@JonChesterfield
Copy link
Collaborator Author

JonChesterfield commented Mar 23, 2024

I'm sorry to hear that. Reading through https://libcxx.llvm.org/BuildingLibcxx.html now to see if I can make ENABLE_RUNTIMES behave itself under cross compilation. I'm familiar with this in the context of the "bootstrapping" build from the docs, the build clang first one, which drops (most) arguments passed to cmake. Hopefully building runtimes directly doesn't involve that quirk.

Building libunwind, then libc++, then libcxxabi to create three archives which are combined works on main at present as far as I can tell, provided one hacks around the missing asm files in libunwind.

If we don't want to land this trivial patch, we should make the causal link between a missing symbol in a dynamic library which segv's on use and that the cmake invocation which used to work and still looks like it works much more obvious. Restoring the fatal error would be an improvement.

@mstorsjo
Copy link
Member

I'm sorry to hear that. Reading through https://libcxx.llvm.org/BuildingLibcxx.html now to see if I can make ENABLE_RUNTIMES behave itself under cross compilation. I'm familiar with this in the context of the "bootstrapping" build from the docs, the build clang first one, which drops (most) arguments passed to cmake. Hopefully building runtimes directly doesn't involve that quirk.

Indeed, pointing cmake at llvm-project/runtimes and specifying libunwind in ENABLE_RUNTIMES should work pretty much the same as pointing cmake at libunwind directly, just with a bunch of boilerplate centralized.

I agree that it probably would be good to keep the error message explaining this, especially if it otherwise seems to almost work.

@JonChesterfield
Copy link
Collaborator Author

I think I'm getting the behaviour I want out of the following incantation which does not require this patch. This doesn't attempt to build shared library versions which removes a bunch of failure modes.

cmake -D CMAKE_BUILD_TYPE=Release                                              \
      -D CMAKE_SYSTEM_NAME=Linux                                               \
      -D CMAKE_C_COMPILER=$CC                                                  \
      -D CMAKE_CXX_COMPILER=$CXX                                               \
      -D CMAKE_C_FLAGS="$GLOBALFLAGS"                                          \
      -D CMAKE_CXX_FLAGS="$GLOBALFLAGS"                                        \
      -D CMAKE_INSTALL_LIBDIR=lib                                              \
      -D CMAKE_INSTALL_PREFIX="$installdir"                                    \
      -D LLVM_ENABLE_RUNTIMES="compiler-rt;libcxx;libcxxabi;libunwind"         \
      -D LLVM_ENABLE_ASSERTIONS=On                                             \
      -D COMPILER_RT_BUILD_LIBFUZZER=NO                                        \
      -D COMPILER_RT_BUILD_PROFILE=NO                                          \
      -D COMPILER_RT_BUILD_MEMPROF=NO                                          \
      -D COMPILER_RT_BUILD_ORC=NO                                              \
      -D COMPILER_RT_BUILD_GWP_ASAN=NO                                         \
      -D COMPILER_RT_BUILD_SANITIZERS=NO                                       \
      -D COMPILER_RT_BUILD_XRAY=NO                                             \
      -D COMPILER_RT_DEFAULT_TARGET_TRIPLE=$TRIPLE                             \
      -D LIBUNWIND_USE_COMPILER_RT=ON                                          \
      -D LIBCXXABI_USE_COMPILER_RT=ON                                          \
      -D LIBCXX_USE_COMPILER_RT=ON                                             \
      -D LIBUNWIND_ENABLE_SHARED=OFF                                           \
      -D LIBCXXABI_ENABLE_SHARED=OFF                                           \
      -D LIBCXX_ENABLE_SHARED=OFF                                              \
      -D LIBCXXABI_USE_LLVM_UNWINDER=ON                                        \
      -D LIBCXX_HAS_MUSL_LIBC=ON                                               \
      -D LIBCXXABI_ENABLE_STATIC_UNWINDER=ON                                   \
      -D LIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON                                   \
      -D LIBCXX_INCLUDE_BENCHMARKS=OFF                                         \
      -D LIBCXX_CXX_ABI=libcxxabi                                              \
      -D CMAKE_EXE_LINKER_FLAGS="-static -nostdlib -nostartfiles"              \
      -G Ninja                                                                 \
      -S $HOME/llvm-project/runtimes

@JonChesterfield JonChesterfield deleted the libunwind branch April 3, 2024 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants