[libcxx-commits] [PATCH] D127444: [libc++] Use ABI tags instead of internal linkage to provide per-TU insulation

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D127444#3578601 <https://reviews.llvm.org/D127444#3578601>, @EricWF wrote:

> Hmm, so I'm not sure this is the best idea. At worst, I think it'll introduce ODR bugs if we use it on anything other than inline functions [2], and when applied only to inline functions it helps only in a very very limited number of cases [1].

Thanks a lot for taking a look. I think it's important to understand that `_LIBCPP_HIDE_FROM_ABI` is only (or at least should only) ever be applied to inline functions. That's required for it today already, since we use the `internal_linkage` attribute. In fact, all of the same failures that you rightly point out in your example already fail today when we use the `internal_linkage` attribute, because each TU will get its own definition of the RTTI/function-local-static/whatever.

Furthermore, if someone mixes two different versions of libc++ in the same final linked image without using `internal_linkage` today (aka without specifying `_LIBCPP_HIDE_FROM_ABI_PER_TU=1`), then they are at risk of ODR violations because we don't know which version of (for example) `__copy_impl` the linker will use. This applies to basically any internal function that we permit ourselves to change the preconditions/postconditions of without changing the name or the mangling, which is effectively anything that's marked as `_LIBCPP_HIDE_FROM_ABI`.

So, in other words -- you are correct that the cases that you highlight will fail with this change, but they already do. And in the configuration where these tests wouldn't fail, users are not supposed to be mixing versions of libc++. With that in mind, do you think this change makes more sense?



================
Comment at: libcxx/include/__config:687
+// on two levels:
+// 1. The symbol is given hidden visibility, which ensures that users won't start exporting
+//    symbols from their dynamic library by means of using the libc++ headers. This ensures
----------------
EricWF wrote:
> Symbols that are linkonce_odr and that get emitted inside a users dylib are never linked to from outside the dylib. So they're already hermetic in that way.
> 
I mean, they can still get exported from the dylib if they have default visibility, no?

They generally won't be used because anyone compiling against the same headers would *also* get a `linkonce_odr` definition in their own dylib/exe and their code would use that, but technically, the symbols are still exported from (both) final linked images. Unless I missed something special about `linkonce_odr` symbols?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127444