Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG758504b8ab64: [libc++] Simplify the visibility attributes
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__config | ||
|---|---|---|
| 533 | If we want to eventually refactor the Windows side of availability macros, I think we should have _LIBCPP_VISIBILITY_HIDDEN and _LIBCPP_VISIBILITY_DEFAULT instead of a macro parameter. | |
- Use a new approach
| libcxx/include/__config | ||
|---|---|---|
| 533 | There isn't really any common ground between non-COFF and COFF in terms of visibility macros. In MinGW only _LIBCPP_OVERRIDABLE_FUNC_VIS and _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS are even defined non-empty, so I don't think it makes a lot of sense to try to unify non-COFF and COFF visibility macros. | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | This is causing breakages in Chromium because we're unable to set _LIBCPP_OVERRIDABLE_FUNC_VIS at build time - see https://crbug.com/1343370#c6 | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | I'm not sure I understand why you set _LIBCPP_OVERRIDABLE_FUNC_VIS (which isn't customization point AFAIK). If you're building against the dylib the visibility should be default and if you don't you set _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS so the symbols become whatever the set visibility is. Why doesn't that work for your configuration? | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | Chromium is setting _LIBCPP_DISABLE_VISIBILITY_ANOTATIONS because it is linking its own copy of libc++ statically. I think Chromium doesn't need overridability of this macro, it just needs to ensure that operator new is not given hidden visibility, it has to always have default visibility, even when libc++ visibility annotations are disabled. So, maybe an alternative fix here would be to always define _LIBCPP_OVERRIDEABLE_FUNC_VIS to __attribute__((visibility("default"))) instead of using _LIBCPP_VISIBILITY. That seems like the right thing to do when libc++ is being statically linked, which I think is the use case for disabling libc++ visibility annotations. WDYT? | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | Disabling the visibility annotations is interesting for people who build dynamic libraries and don't want to depend on libc++ dynamically. That is only possible by hiding all symbols, including operator new. AFAIK Chromium is linking everything statically into an executable and doesn't build dynamic libraries. There it should be irrelevant whether _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is set or not. Or am I missing something? | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | I read more Chromium comments, and it seems that this actually has to do with tcmalloc. If you link libc++ statically *and* use a malloc replacement library such as tcmalloc, the libc++ declarations must not be marked hidden in order for the override to work across dynamic libraries. tcmalloc needs to override operator new / delete in other dynamic libraries. Linking libc++ statically should not prevent users from exporting their own operator new override, right? Since I don't think this was an intentional behavior change, can we please revert to the previous behavior by bringing back the ifndef? If you want to drop support for this override point, we need some other solution for this use case. See the full comments if you like: defines += [
# This resets the visibility to default only for the various
# flavors of operator new and operator delete. These symbols
# are weak and get overriden by Chromium-provided ones, but if
# these symbols had hidden visibility, this would make the
# Chromium symbols hidden too because elf visibility rules
# require that linkers use the least visible form when merging,
# and if this is hidden, then when we merge it with tcmalloc's
# operator new, hidden visibility would win. However, tcmalloc
# needs a visible operator new to also override operator new
# references from system libraries.
# TODO(lld): Ask lld for a --force-public-visibility flag or
# similar to that overrides the default elf merging rules, and
# make tcmalloc's gn config pass that to all its dependencies,
# then remove this override here.
"_LIBCPP_OVERRIDABLE_FUNC_VIS=__attribute__((__visibility__(\"default\")))",
] | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | What I don't understand is why Chromium disables the visibility macros in the first place and why the default visibility is hidden. AFAIK it shouldn't make any difference, since visibility is somewhat irrelevant for static linking. | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | Chrome does not export a C++ interface, so it compiles with -fvisibility=hidden and disables the visibility macros, which would make libc++ symbols visible outside the DSO. It's not clear to me why tcmalloc needs to override operator new in other system DSOs, but that is, apparently, a requirement, and it interacts with these annotations in libc++. | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | But Chromium doesn't export any interface at all. Or is it itself a shared library? | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | I don't know the details, but compiling with -fvisibility=hidden and limiting default visibility symbols is a standard best practice, even if Chrome ultimately becomes an executable with no dynamic symbols. The old Drepper article comes to mind. Maybe to export tcmalloc operator new, Chrome uses the --export-dynamic linker flag. So, to recap, I'm trying to argue that this use case is valid:
This is the use case which requires overriding the visibility of these symbols, but we/Chrome would be happy with any short term solution for this use case. | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | I'm not saying the use-case isn't valid. I'm saying that you can avoid your problem by
I don't understand why these options (especially 1) don't work for you. | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | I'm not familiar with these macros, but here is some archaeology: Our use of _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS seems to have started at https://chromium-review.googlesource.com/c/chromium/buildtools/+/518311/ The scope was broadened by https://chromium-review.googlesource.com/c/chromium/src/+/601529/
While we link our code statically, we do load dynamic libraries from the system, and we don't want them to use our libc++ symbols because that breaks things (from http://crbug.com/751360 "gl_unittests loads libGLESv2.so which also statically links libc++, but because of the visibility for libc++ symbols is set to default, the dynamic linker resolves libGLESv2.so's symbol for the vtable of 'std::__1::basic_streambuf<char>' with the one from gl_unittests body later causing the CFI unrelated cast exception") .. except we do want default visibility for new/delete, I guess for tcmalloc as rnk mentioned above (the macro was first set in https://codereview.chromium.org/2937433003). | |
| libcxx/include/__config | ||
|---|---|---|
| 543 | Are you saying that libGLES exports libc++ symbols? That sounds a lot like a libGLES bug to me. If they're linking statically they shouldn't export any symbols from libc++. Anyways, I've uploaded 0f050528fd08 for now. | |
| libcxx/include/__config | ||
|---|---|---|
| 543 |
That's possible, but I think it's worth focusing on the executable, gl_unittests, which is exporting the vtable symbol for std::__1::basic_streambuf<char>. It doesn't matter that gl_unittests is an executable, it still exports default visibility symbols, and that's why Chrome uses both -fvisibility=hidden + _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS. Anyway, thank you for landing a workaround. | |
If we want to eventually refactor the Windows side of availability macros, I think we should have _LIBCPP_VISIBILITY_HIDDEN and _LIBCPP_VISIBILITY_DEFAULT instead of a macro parameter.