Page MenuHomePhabricator

Reland "Enable `-funwind-tables` flag when building libunwind"
ClosedPublic

Authored by broadwaylamb on Dec 6 2019, 6:48 AM.

Details

Summary

Relands https://reviews.llvm.org/D70815.

The original commit set CMAKE_TRY_COMPILE_TARGET_TYPE to
STATIC_LIBRARY globally in libunwind/CMakeLists.txt, which effectively
disabled the linking step in CMake checks.

This broke some builds (see 938c70b86c7d2165f8c28d5700e9c1ac1263307e).

Here we set CMAKE_TRY_COMPILE_TARGET_TYPE to
STATIC_LIBRARY only when checking for presence of the -funwind-tables
flag, and then set it back to the original value so it doesn't affect
other checks.

Diff Detail

Event Timeline

broadwaylamb created this revision.Dec 6 2019, 6:48 AM
Herald added a project: Restricted Project. · View Herald Transcript

Thanks - this one seems to work fine for my use case at least.

I'm not sure if it might be good to keep the flag set for at least a bit wider range of tests, at least the other add_cxx_compile_flags_if_supported tests right next to this one. Not sure if they are triggering issues like references to __aeabi_unwind_cpp_pr0, but it still might be good for consistency in any case?

@mstorsjo thanks for verifying!

When preparing this patch I've looked through the flags being added and couldn't find those that would benefit from this.

  1. Obviously, this won't affect any -W flags.
  2. Same for -fstrict-aliasing.
  3. -EHsc is MSVC-specific and won't generate any new calls (will it?).
  4. -fno-exceptions and -fno-rtti won't add any new calls either.
  5. Same for -nostdinc++.
  6. -D and -U are only for the preprocessor.

So, it seems like here we only want -funwind-tables to be guarded with this CMake setting.

but it still might be good for consistency in any case?

Now that you've demonstrated the issues that this flag might uncover, my opinion is we probably only want this flag to be set in places we know for sure it is needed, with a clear explanation why it is needed.

@mstorsjo thanks for verifying!

When preparing this patch I've looked through the flags being added and couldn't find those that would benefit from this.

  1. Obviously, this won't affect any -W flags.
  2. Same for -fstrict-aliasing.
  3. -EHsc is MSVC-specific and won't generate any new calls (will it?).
  4. -fno-exceptions and -fno-rtti won't add any new calls either.
  5. Same for -nostdinc++.
  6. -D and -U are only for the preprocessor.

    So, it seems like here we only want -funwind-tables to be guarded with this CMake setting.

but it still might be good for consistency in any case?

Now that you've demonstrated the issues that this flag might uncover, my opinion is we probably only want this flag to be set in places we know for sure it is needed, with a clear explanation why it is needed.

Well I guess it's whether you aim to be as safe as possible from this kind of issue, or consistency/simplicity with broader groupings. I'd say in general this flag should be rather safe for anything which is a test for a compiler feature (instead of trying to dissect exactly which compiler options might add symbol references), but clearly not for tests for linker features.

But I won't insist, if you prefer keeping it this narrow.

Since the original patch has been approved and the bug because of which this has been reverted is fixed, I'm going to commit this.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 10 2019, 5:32 PM
This revision was automatically updated to reflect the committed changes.