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][cmake] Compile _Unwind* routines with -fexceptions #121819

Conversation

ilovepi
Copy link
Contributor

@ilovepi ilovepi commented Jan 6, 2025

When building libunwind with LTO, we found that routines, like
_Unwind_RaiseException were marked nounwind. This causes problems when
libunwind is then used with exception throwing code, since many of the
routines are marked nounwind and the compiler infers that something
like a try/catch block cannot throw resulting in a miscompile
(see #120657). Similarly, in #56825, it was pointed out that marking
_Unwind_Resume as nounwind causes bad exception table generation.

This patch adds the -fexceptions flag to the build of the C files that
define these routines, as proposed in #56825.

Fixes #56825 #120657

Created using spr 1.3.6-beta.1
@ilovepi ilovepi requested a review from a team as a code owner January 6, 2025 19:15
@llvmbot
Copy link
Member

llvmbot commented Jan 6, 2025

@llvm/pr-subscribers-libunwind

Author: Paul Kirth (ilovepi)

Changes

When building libunwind with LTO, we found that routines, like
_Unwind_RaiseException were marked nounwind. This causes problems when
libunwind is then used with exception throwing code, since many of the
routines are marked nounwind. This causes miscompiles, and the
incorrect generation of unwind tables (see #120657).

In #56825, it was pointed out that these C routines should be compiled
with -fexceptions, to prevent this from happening. Since we only add
the -fexceptions flag to C sources, there shouldn't be any conflict
with any C++ sources or interactions with libcxx/libcxxabi.

Fixes #56825 #120657


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

1 Files Affected:

  • (modified) libunwind/src/CMakeLists.txt (+1-1)
diff --git a/libunwind/src/CMakeLists.txt b/libunwind/src/CMakeLists.txt
index e7ea57734cca97..72dd3f5bca9960 100644
--- a/libunwind/src/CMakeLists.txt
+++ b/libunwind/src/CMakeLists.txt
@@ -20,7 +20,7 @@ set(LIBUNWIND_C_SOURCES
     )
 set_source_files_properties(${LIBUNWIND_C_SOURCES}
                             PROPERTIES
-                              COMPILE_FLAGS "-std=c99")
+                              COMPILE_FLAGS "-std=c99 -fexceptions")
 
 set(LIBUNWIND_ASM_SOURCES
     UnwindRegistersRestore.S

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second paragraph can mention _Unwind_Resume explicitly.

@@ -20,7 +20,7 @@ set(LIBUNWIND_C_SOURCES
)
set_source_files_properties(${LIBUNWIND_C_SOURCES}
PROPERTIES
COMPILE_FLAGS "-std=c99")
COMPILE_FLAGS "-std=c99 -fexceptions")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also leave a comment here explaining why we need -fexceptions?

@ilovepi
Copy link
Contributor Author

ilovepi commented Jan 7, 2025

Is there any known issue w/ the NDK 13 bot? The log cuts out midway through tests, so I assume this is some kind of timeout or another issue w/ the emulator. the complete log only shows some warnings when launching the emulator, but IDK if those are relevant. They seem benign to me, but I'm no expert.

https://buildkite.com/llvm-project/libcxx-ci/builds/39796#01943d0f-1d31-4e60-a670-d2bc43adcd4e

Created using spr 1.3.6-beta.1
@alexrp
Copy link
Member

alexrp commented Jan 8, 2025

add_cxx_compile_flags_if_supported(-fno-exceptions)
add_cxx_compile_flags_if_supported(-fno-rtti)

I wonder if it would be less bug-prone in the long term to just build the whole library with -fexceptions instead of trying to apply -fexceptions to just the right files at this point in time. Feels like a brittle optimization for not much gain?

@Andarwinux
Copy link
Contributor

I wonder if it would be less bug-prone in the long term to just build the whole library with -fexceptions instead of trying to apply -fexceptions to just the right files at this point in time. Feels like a brittle optimization for not much gain?

This will break MinGW LTO libunwind.

@alexrp
Copy link
Member

alexrp commented Jan 8, 2025

I wonder if it would be less bug-prone in the long term to just build the whole library with -fexceptions instead of trying to apply -fexceptions to just the right files at this point in time. Feels like a brittle optimization for not much gain?

This will break MinGW LTO libunwind.

Can you clarify how?

@Andarwinux
Copy link
Contributor

I wonder if it would be less bug-prone in the long term to just build the whole library with -fexceptions instead of trying to apply -fexceptions to just the right files at this point in time. Feels like a brittle optimization for not much gain?

This will break MinGW LTO libunwind.

Can you clarify how?

Maybe this is a hidden bug, but I haven't investigated it too deeply, @mstorsjo may know better.
If you build MinGW LTO libunwind without -fno-exceptions or with -fexceptions, libunwind references __gcc_personality_seh0, which results in undefined symbols. cmake normally adds -fno-exceptionson, but not on the first build, because the toolchain is incomplete which causes cmake to think that the compiler doesn't support -fno-exceptions, which is how I found out about the problem.

@mstorsjo
Copy link
Member

mstorsjo commented Jan 8, 2025

Maybe this is a hidden bug, but I haven't investigated it too deeply, @mstorsjo may know better.

Sorry, no clue offhand here.

If you build MinGW LTO libunwind without -fno-exceptions or with -fexceptions, libunwind references __gcc_personality_seh0, which results in undefined symbols.

Perhaps this is a symbol which we need to implicitly mark as needed in the linker? Because before the LTO, I think it only sees a definition of this symbol, but nothing referencing it, so the LTO logic thinks it doesn't need to retain/expose this symbol - while after the LTO, the generated code does add new references to it.

I don't find any precedence for doing this for LTO so far, but we have a couple cases of doing something similar for the regular GC in LLD.

cmake normally adds -fno-exceptionson, but not on the first build, because the toolchain is incomplete which causes cmake to think that the compiler doesn't support -fno-exceptions, which is how I found out about the problem.

It does add -fno-exceptions even on the first bootstrap build for me. See e.g. https://github.com/mstorsjo/llvm-mingw/actions/runs/12662020869/job/35286224252#step:3:23134:

-- The C compiler identification is Clang 20.0.0
-- The CXX compiler identification is Clang 20.0.0
-- The ASM compiler identification is Clang with GNU-like command-line
-- Found assembler: /home/runner/work/llvm-mingw/llvm-mingw/install/llvm-mingw/bin/i686-w64-mingw32-clang
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Detecting CXX compile features
-- Detecting CXX compile features - done
[...]
-- Performing Test CXX_SUPPORTS_FUNWIND_TABLES_FLAG
-- Performing Test CXX_SUPPORTS_FUNWIND_TABLES_FLAG - Success
-- Performing Test CXX_SUPPORTS_FNO_EXCEPTIONS_FLAG
-- Performing Test CXX_SUPPORTS_FNO_EXCEPTIONS_FLAG - Success

@Andarwinux
Copy link
Contributor

Andarwinux commented Jan 9, 2025

It does add -fno-exceptions even on the first bootstrap build for me. See e.g. https://github.com/mstorsjo/llvm-mingw/actions/runs/12662020869/job/35286224252#step:3:23134:

-- The C compiler identification is Clang 20.0.0
-- The CXX compiler identification is Clang 20.0.0
-- The ASM compiler identification is Clang with GNU-like command-line
-- Found assembler: /home/runner/work/llvm-mingw/llvm-mingw/install/llvm-mingw/bin/i686-w64-mingw32-clang
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Detecting CXX compile features
-- Detecting CXX compile features - done
[...]
-- Performing Test CXX_SUPPORTS_FUNWIND_TABLES_FLAG
-- Performing Test CXX_SUPPORTS_FUNWIND_TABLES_FLAG - Success
-- Performing Test CXX_SUPPORTS_FNO_EXCEPTIONS_FLAG
-- Performing Test CXX_SUPPORTS_FNO_EXCEPTIONS_FLAG - Success

I'm sorry, but this was caused by my own mistake: my modified clang++ wrapper has a -lc++abi, which I forgot what it was previously introduced to solve.

If you build MinGW LTO libunwind without -fno-exceptions or with -fexceptions, libunwind references __gcc_personality_seh0, which results in undefined symbols.

Perhaps this is a symbol which we need to implicitly mark as needed in the linker? Because before the LTO, I think it only sees a definition of this symbol, but nothing referencing it, so the LTO logic thinks it doesn't need to retain/expose this symbol - while after the LTO, the generated code does add new references to it.

I don't find any precedence for doing this for LTO so far, but we have a couple cases of doing something similar for the regular GC in LLD.

Should I open a new issue for this? Since it won't actually happen in reality unless libunwind does switch to use -fexceptions to compile the whole library, I'm not sure what to do.

@ilovepi
Copy link
Contributor Author

ilovepi commented Jan 9, 2025

Should I open a new issue for this? Since it won't actually happen in reality unless libunwind does switch to use -fexceptions to compile the whole library, I'm not sure what to do.

I think its fine to file an issue like this to track that we know this is a limitation. I'd just make it explicit that this is a potential issue for the future, and that would need to be addressed before we would make such a change. In my opinion discussions like this in a code review are a bit harder to find than a proper issue, and having an issue handy that points to this discussion is a good way to make sure we don't lose context.

Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@ilovepi ilovepi changed the base branch from main to users/ilovepi/spr/main.libunwindcmake-compile-_unwind-routines-with-fexceptions January 10, 2025 20:57
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@ilovepi ilovepi changed the base branch from users/ilovepi/spr/main.libunwindcmake-compile-_unwind-routines-with-fexceptions to main January 14, 2025 19:55
@ilovepi
Copy link
Contributor Author

ilovepi commented Jan 14, 2025

Are there lingering concerns here or are we OK landing this?

Created using spr 1.3.6-beta.1
@ilovepi
Copy link
Contributor Author

ilovepi commented Jan 15, 2025

Hmm, the failing bot seems to be some kind of configuration error on the bot (or issue w/ the docker daemon?) https://github.com/llvm/llvm-project/actions/runs/12775719644/job/35614829092

Fix typo

Co-authored-by: Petr Hosek <[email protected]>
@ilovepi ilovepi merged commit 92f1f99 into main Jan 16, 2025
10 of 14 checks passed
@ilovepi ilovepi deleted the users/ilovepi/spr/libunwindcmake-compile-_unwind-routines-with-fexceptions branch January 16, 2025 21:44
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jan 16, 2025

LLVM Buildbot has detected a new failure on builder libunwind-sphinx-docs running on gribozavr3 while building libunwind at step 3 "Install pip dependencies".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/149/builds/34

Here is the relevant piece of the build log for the reference
Step 3 (Install pip dependencies) failure: 'pip install -r llvm/docs/requirements-hashed.txt' (failure)
Upon execvpe pip install -r llvm/docs/requirements-hashed.txt ['pip install -r llvm/docs/requirements-hashed.txt'] in environment id 140095679092208
:Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/twisted/internet/process.py", line 445, in _fork
    environment)
  File "/usr/lib/python2.7/dist-packages/twisted/internet/process.py", line 523, in _execChild
    os.execvpe(executable, args, environment)
  File "/usr/lib/python2.7/os.py", line 355, in execvpe
    _execvpe(file, args, env)
  File "/usr/lib/python2.7/os.py", line 370, in _execvpe
    func(file, *argrest)
OSError: [Errno 2] No such file or directory

DKLoehr pushed a commit to DKLoehr/llvm-project that referenced this pull request Jan 17, 2025
…121819)

When building libunwind with LTO, we found that routines, like
_Unwind_RaiseException were marked `nounwind`. This causes problems when
libunwind is then used with exception throwing code, since many of the
routines are marked `nounwind` and the compiler infers that something
like a try/catch block cannot throw resulting in a miscompile
(see llvm#120657). Similarly, in llvm#56825, it was pointed out that marking
_Unwind_Resume as `nounwind` causes bad exception table generation.

This patch adds the `-fexceptions` flag to the build of the C files that
define these routines, as proposed in llvm#56825.

Fixes llvm#56825 llvm#120657

---------

Co-authored-by: Petr Hosek <[email protected]>
alexrp added a commit to alexrp/zig that referenced this pull request Jan 18, 2025
See: llvm/llvm-project#121819

This fixes LTO for libunwind, so also re-enable support for that.
alexrp added a commit to alexrp/zig that referenced this pull request Jan 18, 2025
See: llvm/llvm-project#121819

This fixes LTO for libunwind, so also re-enable support for that.

Closes ziglang#12828.
Fri3dNstuff pushed a commit to Fri3dNstuff/zig that referenced this pull request Jan 27, 2025
See: llvm/llvm-project#121819

This fixes LTO for libunwind, so also re-enable support for that.

Closes ziglang#12828.
ilovepi added a commit that referenced this pull request Jan 27, 2025
…119252) (#121820)

The previous failures were addressed with CMake changes in #121819
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.

nounwind declaration of _Unwind_Resume generates bad gcc_except_table
8 participants