Page MenuHomePhabricator

[libunwind] Remove static_assert / __has_feature macros
ClosedPublic

Authored by rprichard on Aug 22 2020, 5:24 PM.

Details

Summary

The static_assert macro broke on GCC when a scope had two asserts and a
condition that depended on a template parameter. Remove the macro and
rely on the compiler's C++11 static_assert feature.

The __has_feature macro was only used here to determine whether to
define the static_assert macro.

Diff Detail

Event Timeline

rprichard created this revision.Aug 22 2020, 5:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 22 2020, 5:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard requested review of this revision.Aug 22 2020, 5:24 PM

This change fixes this GCC build failure, https://reviews.llvm.org/D86102#2232290.

I guess this fix makes sense - I'd be surprised if we don't already require C++11 for building in general.

However I do see that when building, the cmake project files don't seem to force building in C++11 mode (at least when building, I don't see any -std=c++11 or similar), so this would break building with older versions of GCC that do support C++11 but still default to C++98, so I guess it'd be safest if we'd have something that enforces the version of C++ that actually is required.

I guess this fix makes sense - I'd be surprised if we don't already require C++11 for building in general.

I see many uses of auto and nullptr in libunwind. In release/10.x, on targets using dl_iterate_phdr, AddressSpace.hpp used a lambda function.

The change that broke GCC builds (D86102) adds additional uses of alignof, which is also C++11 only. (The previous uses were in a test file.)

However I do see that when building, the cmake project files don't seem to force building in C++11 mode (at least when building, I don't see any -std=c++11 or similar), so this would break building with older versions of GCC that do support C++11 but still default to C++98, so I guess it'd be safest if we'd have something that enforces the version of C++ that actually is required.

When I built libunwind using GCC (for use with glibc), using libunwind/CMakeLists.txt, I saw ninja passing -std=c++11 on the C++ compile command lines. I think it was caused by the CXX_STANDARD 11 in libunwind/src/CMakeLists.txt (e.g. for unwind_shared). When I removed the CXX_STANDARD 11 lines, I no longer saw -std=c++11 on the command lines.

mstorsjo accepted this revision.Aug 24 2020, 5:10 AM

I guess this fix makes sense - I'd be surprised if we don't already require C++11 for building in general.

I see many uses of auto and nullptr in libunwind. In release/10.x, on targets using dl_iterate_phdr, AddressSpace.hpp used a lambda function.

The change that broke GCC builds (D86102) adds additional uses of alignof, which is also C++11 only. (The previous uses were in a test file.)

However I do see that when building, the cmake project files don't seem to force building in C++11 mode (at least when building, I don't see any -std=c++11 or similar), so this would break building with older versions of GCC that do support C++11 but still default to C++98, so I guess it'd be safest if we'd have something that enforces the version of C++ that actually is required.

When I built libunwind using GCC (for use with glibc), using libunwind/CMakeLists.txt, I saw ninja passing -std=c++11 on the C++ compile command lines. I think it was caused by the CXX_STANDARD 11 in libunwind/src/CMakeLists.txt (e.g. for unwind_shared). When I removed the CXX_STANDARD 11 lines, I no longer saw -std=c++11 on the command lines.

Oh, ok, I see - maybe CMake deduced that the option wasn't needed in my case, when building with a compiler that defaults to a new enough version.

In any case, as we seem to already rely on C++11 features extensively, this surely should be ok - so LGTM, and sorry for the unexpected breakage.

This revision is now accepted and ready to land.Aug 24 2020, 5:10 AM
This revision was automatically updated to reflect the committed changes.

Oh, ok, I see - maybe CMake deduced that the option wasn't needed in my case, when building with a compiler that defaults to a new enough version.

Seems plausible. Are you using MSVC or clang-cl, by any chance?

In any case, as we seem to already rely on C++11 features extensively, this surely should be ok - so LGTM, and sorry for the unexpected breakage.

No problem. I think you couldn't have anticipated that two static_asserts would break something.