Page MenuHomePhabricator

[builtins] Add COMPILER_RT_BUILTINS_HIDE_SYMBOLS
ClosedPublic

Authored by rprichard on Dec 16 2020, 5:06 PM.

Details

Summary

On Android, when the builtins are linked into a binary, they are
typically linked using -Wl,--exclude-libs so that the symbols aren't
reexported. For the NDK, compiler-rt's default behavior (build the
builtins archive with -fvisibility=hidden) is better so that builtins
are hidden even without -Wl,--exclude-libs.

Android needs the builtins with non-hidden symbols only for a special
case: for backwards compatibility with old binaries, the libc.so and
libm.so DSOs in the platform need to export some builtins for arm32 and
32-bit x86. See D56977.

Control the behavior with a new flag,
COMPILER_RT_BUILTINS_HIDE_SYMBOLS, that behaves similarly to the
*_HERMETIC_STATIC_LIBRARY in libunwind/libcxx/libcxxabi, so that
Android can build a special builtins variant for libc.so/libm.so.

Unlike the hermetic flags for other projects, this new flag is enabled
by default.

Diff Detail

Event Timeline

rprichard created this revision.Dec 16 2020, 5:06 PM
rprichard requested review of this revision.Dec 16 2020, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 5:06 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

I'm using this change in https://android-review.googlesource.com/c/toolchain/llvm_android/+/1532841.

The named seemed a bit long to me, but it was consistent with the *_HERMETIC_STATIC_LIBRARY option name in the other runtime libraries.

LG

compiler-rt/lib/builtins/CMakeLists.txt
51

Can this be clarified?

rprichard added inline comments.Dec 16 2020, 8:38 PM
compiler-rt/lib/builtins/CMakeLists.txt
51

I suppose -- do you have a suggestion?

Currently the option text is the same as the other *_HERMETIC_STATIC_LIBRARY options, and none of them have comments above them. The flag marks all defined global symbols as hidden. (I suppose on Mach-O, it marks them with .private_extern instead.) Even when the hermetic flag is turned off, some symbols are still hidden.

I think for the other projects, the hermetic switch also inhibits the use of dllexport on Windows, whereas the builtins library never uses dllexport.

I think there's no shared library configuration for the builtins library, and it's unusual to want the builtin symbols exported when they're linked into something.

compnerd accepted this revision.Dec 17 2020, 12:30 PM

I really would prefer the alternate spelling, but the change itself is okay.

compiler-rt/lib/builtins/CMakeLists.txt
51

Im not sure that the name of the option is really that great - hermeticity in this case is not really the generally understood meaning as libraries on the system will not influence the build of the builtins, but rather the library will influence other libraries. I think that this really should be COMPILER_RT_BUILTINS_HIDE_SYMBOLS which really makes it much more obvious what this is doing.

This revision is now accepted and ready to land.Dec 17 2020, 12:30 PM
rprichard updated this revision to Diff 312638.Dec 17 2020, 4:11 PM

Rename flag to COMPILER_RT_BUILTINS_HIDE_SYMBOLS.

rprichard retitled this revision from [builtins] Add COMPILER_RT_BUILTINS_HERMETIC_STATIC_LIBRARY to [builtins] Add COMPILER_RT_BUILTINS_HIDE_SYMBOLS.Dec 17 2020, 4:12 PM
rprichard edited the summary of this revision. (Show Details)
MaskRay accepted this revision.Dec 17 2020, 4:19 PM
compnerd accepted this revision.Thu, Jan 7, 1:24 PM

Thanks!

This revision was automatically updated to reflect the committed changes.