[libcxx-commits] [PATCH] D128007: [libc++] Simplify the visibility attributes

Thu Jul 14 06:11:11 PDT 2022

philnik marked 5 inline comments as done.
philnik added inline comments.

================
Comment at: libcxx/include/__config:543
+#    define _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_VISIBILITY("default")
+#    define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_VISIBILITY("default")
+#    define _LIBCPP_EXCEPTION_ABI _LIBCPP_VISIBILITY("default")
----------------
hans wrote:
> philnik wrote:
> > rnk wrote:
> > > philnik wrote:
> > > > rnk wrote:
> > > > > philnik wrote:
> > > > > > rnk wrote:
> > > > > > > philnik wrote:
> > > > > > > > rnk wrote:
> > > > > > > > > philnik wrote:
> > > > > > > > > > ayzhao wrote:
> > > > > > > > > > > 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
> > > > > > > > > > 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?
> > > > > > > > > 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?
> > > > > > > > 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?
> > > > > > > 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\")))",
> > > > > > >       ]
> > > > > > > ```
> > > > > > 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.
> > > > > 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++.
> > > > But Chromium doesn't export any interface at all. Or is it itself a shared library?
> > > 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](https://akkadia.org/drepper/dsohowto.pdf) 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:
> > > - Building a fully static binary with libc++ with ~no dynamic symbols
> > > - Linking in tcmalloc to override all malloc and operator new symbols across the program
> > > 
> > > 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.
> > I'm not saying the use-case isn't valid. I'm saying that you can avoid your problem by
> > 1) Not setting `_LIBCPP_DISBALE_VISIBILITY_ANNOTATIONS` which would give you the visibility attributes you want, or
> > 2) Not using `-fvisibility=hidden`
> > 
> > I don't understand why these options (especially 1) don't work for you.
> 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/
> I think the motivation of that one relates to this question:
> 
> > 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?
> 
> 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).
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128007/new/

https://reviews.llvm.org/D128007