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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

OK, so i'm convinced this change should be OK assuming we continue to ensure `_LIBCPP_HIDE_FROM_ABI` is only ever applied to inline functions. 
It would also need to be applied to every single inline function the library produces, otherwise a tagged function could call an untagged function,
since the untagged definition can be selected at random from the available definitions.

The other consideration is the cost of this change. While small, it does increase the mangling of every single inline function in libc++ by ~5 bytes.
After a little testing, this appears to translate into an object file size increase of ~1%  when debug information is enabled.

My current opinion is that the binary size increase outweighs the limited benefits the change provides.
But I'm open to being convinced.

@ldionne maybe you could produce a compiling example of a situation this change intends to fix?



================
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
----------------
ldionne wrote:
> 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?
You're right. The linker will choose the symbol in an SO if it absolutely must. But that's only if we don't mark it with `__attribute__((visibility("hidden"))`, which we should be using, since it's both necessary and  sufficient to solve the export problem.

The ABI tag alone does nothing to prevent symbol exporting. For example assuming binary A gets symbol X at V1 from `libB.so`, then `libB.so` is recompiled against a new version, so it exports `X at V2` instead now. Binary A breaks all the same.   

It's also worth noting that we're assuming the ABI has not changed apart from inline functions potentially, otherwise the ABI namespace should have changed, solving the ODR problem entirely. 

Which  limits the utility of this to cases where 

  # a user *statically links* object files produced against two different versions of libc++, AND:
    #  a non-template inline function `F` changes its return type, without changing anything else, so that the mangling stays the same, but the ABI changes; Or,
    # A inline function `G` changes it's pre-conditions/post-conditions without changing the signature, so that input that's valid to `G_0` is not valid to `G_1` or vice-versa.

Arguably, the user shouldn't be doing (1), and we shouldn't make changes of the kind required for (A) or (B) (that don't also change mangling).
If a user does to (1), but we're careful to not do (A) or  (B), then we have a no ODR violation at all -- because either:
  
  # all the available definitions of a function `F` are the same, or
  #  we have a "benign" ODR violation where there are multiple different definitions of `F`, but selecting either definition is OK because the pre/post conditions haven't changed.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127444