This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use ABI tags instead of internal linkage to provide per-TU insulation
ClosedPublic

Authored by ldionne on Jun 9 2022, 2:41 PM.

Details

Summary

Instead of marking private symbols with internal_linkage (which leads to
one copy per translation unit -- rather wasteful), use an ABI tag that
gets rev'd with each libc++ version. That way, we know that we can't have
name collisions between implementation-detail functions across libc++
versions, so we'll never violate the ODR. However, within a single program,
each symbol still has a proper name with external linkage, which means
that the linker is free to deduplicate symbols even across TUs.

This actually means that we can guarantee that versions of libc++ can
be mixed within the same program without ever having to take a code size
hit, and without having to manually opt-in -- it should just work out of
the box.

Diff Detail

Event Timeline

ldionne created this revision.Jun 9 2022, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 2:41 PM
Herald added a subscriber: mgorny. · View Herald Transcript
ldionne requested review of this revision.Jun 9 2022, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 2:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

It would be awesome if someone with deeper linker expertise could double-check this change. I think it should work in principle, but it would be nice to know if there are pitfalls I'm missing.

I dicussed this internally with linker folks and they confirmed that it should work. I'll wait a bit before landing this, but I think we've got something. @EricWF it would be nice to get your eyes on this as well!

EricWF requested changes to this revision.Jun 13 2022, 9:47 AM

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].

[1]. Since most changes to a function signature affect the mangling as well, there's no additional benefit from further changing the mangling using __abi_tag__. The one case it would catch is if we change only the return type of a non-template function (since the return type is mangled into function templates).
And we shouldn't be changing the return type of user-facing functions, like ever.

[2] All of the following examples break when the attribute is applied to a class type.

// RUN: clang++ -c -o test1.o -DTAG=1 test.cpp && clang++ -o a.out -DTAG=2 test.cpp test1.o && ./a.out
#include <typeinfo>
#include <cassert>
#include <cstdio>

#ifndef TAG
#error TAG must be defined
#endif
#define TO_STR2(X) #X
#define TO_STR(X) TO_STR2(X)

#define HIDDEN __attribute__((__abi_tag__(TO_STR(TAG))))

struct HIDDEN  MyType {
  virtual ~MyType() = default;
  static  int data;
};

inline  int MyType::data = 42;

int* get_data();
const std::type_info* get_info();
void throw_type();

__attribute__((weak)) void passes_param(MyType); // weak only used to demonstrate linker failure at runtime rather than linktime.

#if TAG == 1
int* get_data() {
  return &MyType::data;
}
void throw_type() {
  MyType t{};
  throw t;
}

const std::type_info* get_info() {
  return &typeid(MyType);
}
void passes_param(MyType) {
  printf("INFO: Passed to MyType@TAG=1");
}
#else
int main() {
  int num_errors = 0;
  if (&passes_param) {
    printf("SUCCESS: passes_param(MyType) has definition\n");
  } else {
    ++num_errors;
    printf("ERROR: passes_param(MyType) is undefined\n");
  }
  if (get_data() == &MyType::data) {
    printf("SUCCESS: data@TAG=1 == data@TAG=2\n");
  } else {

    printf("ERROR: data@TAG=1 != data@TAG=2\n");
    ++num_errors;
  }
  if (*get_info() == typeid(MyType)) {

    printf("typeid(MyType)@TAG=1 == typeid(MyType)@TAG=2\n");
  } else {
    printf("ERROR: typeid(MyType)@TAG=1 == typeid(MyType)@TAG=2\n");
    ++num_errors ;
  }
  try {
    throw_type();
    assert(false);
  } catch (MyType const& t) {
    printf("INFO: exception MyType@TAG=1 caught as MyType@TAG=2\n");
  } catch (...) {
    printf("ERROR: exception MyType@TAG=1 not caught\n");
    ++num_errors;
  }
  printf("%d errors...\n", num_errors);
  return num_errors;
}
#endif
libcxx/include/__config
677

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.

681

This actually seems like a rather big footgun to me. At least if it's ever applied to anything other than an inline function.

In the case where it's applied to either (A) a type, or (B) a data member, then, Instead of ensuring that no ODR violations arise, this all but ensures ODR violations do arise. As a result any behavior that depends on uniqueness (typeid, exceptions, static data members, vtables, type mangling, anything with a unique address) will now have one definition per libc++ version, and valid behavior break.

This revision now requires changes to proceed.Jun 13 2022, 9:47 AM

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?

libcxx/include/__config
677

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?

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?

libcxx/include/__config
677

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@V1 from libB.so, then libB.so is recompiled against a new version, so it exports X@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

  1. a user *statically links* object files produced against two different versions of libc++, AND:
    1. 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,
    2. 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:

  1. all the available definitions of a function F are the same, or
  2. 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.

Just out of interest: Does this remove the problem that we always_inline a lot of functions with GCC?

I should add...

We should be explicitly aware of changes that affect the pre/post conditions of a libc++ function, and only do so consciously;
Allowing us to explicitly change the mangling using __abi_tag__ or any other means (typically such changes already rename the function or change its signature)

Just out of interest: Does this remove the problem that we always_inline a lot of functions with GCC?

At this point I hope we're only always_inlining functions for performance/code size reasons, rather than
as ABI management. Though it may still be needed to battle with extern template semantics.

Specifically which always inline usages are you talking about?

Just out of interest: Does this remove the problem that we always_inline a lot of functions with GCC?

At this point I hope we're only always_inlining functions for performance/code size reasons, rather than
as ABI management. Though it may still be needed to battle with extern template semantics.

Specifically which always inline usages are you talking about?

I mean that GCC doesn't provide internal_linkage (https://godbolt.org/z/8jTzq8zvz), which means that we fall back to always_inline with GCC with _LIBCPP_HIDE_FROM_ABI_PER_TU enabled.

Oh and apparently also without it. We define _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION as always_inline if it's not provided (which it isn't with GCC: https://godbolt.org/z/5vvr7ne94).

ldionne added a comment.EditedJun 14 2022, 3:26 PM

(consolidating here in a single thread)

Edit: I'm using the term "implementation detail function" below, but I really mean "functions that we don't want to appear as part of our ABI", aka all functions that are marked with _LIBCPP_HIDE_FROM_ABI or _LIBCPP_INLINE_VISIBILITY today.

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.

We agree here. We do mark implementation-detail symbols with visibility("hidden") for that purpose (via _LIBCPP_HIDDEN).

Which limits the utility of this to cases where

  1. a user statically links object files produced against two different versions of libc++, AND:

    A. 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,

    B. 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.

We're in agreement here. Solving this problem has been the goal of this whole endeavour with internal_linkage and always_inline from the beginning. Initially, libc++ marked everything with always_inline so as to "avoid" this problem entirely (if you don't emit symbols, you can't have ODR issues!). This was true until I made changes to start using internal_linkage circa 2018. The goal of that change was to stop inlining everything, which was bad for code size and debugging.

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:

  1. all the available definitions of a function F are the same, or
  2. 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.

We're in partial agreement. The ability for users to mix statically linked versions of libc++ is something that we used to support, until I changed it to be conditionally supported by vendors (based on whether LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT is used when configuring the library). We still support it on Apple platforms, and AFAICT some users might rely on it.

Where we disagree is when it comes to what is reasonable from libc++ to avoid these ODR violations or make them benign. We currently don't pay attention to how we change implementation-detail functions, and I don't think it is realistic to do it robustly. Think about it -- it means that we would need to know the full set of function signatures (name + parameter list) to have ever been used in a shipping libc++, and we should either never reuse any of these signatures, or if we do, then make sure we have compatible pre/post conditions for that function. For example, can we guarantee that we haven't previously shipped a function named __copy_impl(char*, char*, char*) with pre/post conditions incompatible with the ones we have in our code today? Even back in e.g. LLVM 7?

As you mention, the only way to do it robustly is to never change any function but in minor ways that would make an ODR mismatch effectively a benign ODR violation. However, that places a significant restriction on how we can evolve libc++'s code, and it also increases the barrier to contributing for reasons intangible to anyone not having read this discussion. Since we have a clear technical solution instead, I think we should go for it.

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.

Agreed, that's a requirement, and it was also a requirement prior to this change -- we never use _LIBCPP_HIDE_FROM_ABI on non-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.

Agreed, and we already do that except:

  1. For a few functions that have been forgotten (we sometimes catch missing _LIBCPP_HIDE_FROM_ABI and we add them), and
  2. for a few functions that were large enough that we didn't want to mark with internal_linkage or always_inline, and for which we do go through the trouble of making sure we don't introduce ODR violations. That being said, with this proposed patch, we could mark those functions as well without problem.

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.

I actually think that's the crux of this change.

  1. For configurations where we use LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT=ON, the benefit will be that we stop having duplicate instantiations of things like pop_heap in each translation unit where it is used. This is just a random one, but all functions with _LIBCPP_HIDE_FROM_ABI will get that benefit. For this configuration, this is a net win, there is no cost I can think of.
  2. For configurations of the library where LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT=OFF (e.g. the LLVM release), this will provide the ability for users to statically link against mixed versions of libc++ safely (which some users probably do without even knowing it, but is currently unsafe). The cost will be the ABI tag, which is roughly 5 bytes per symbol, as you mention.

For (2), the cost should only affect the size of object files, since these symbols should be stripped from shipping executables. Given that, my inclination would be to raise correctness above object size, and to try to push efforts such as ABI compression mangling instead, which could deliver absolutely insane improvements to symbol sizes, much beyond saving 5 bytes.

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

Like I said, this is not really to fix any single specific situation -- it's to (1) reduce per-TU duplication when per-TU insulation is provided, (2) provide an ABI safety guarantee to users in all cases, and (3) simplify the library.

Another thing I can do is keep the LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT switch and not provide the ABI safety guarantee for vendors who don't care about it (that way we don't have the ABI tag when we previously wouldn't have had internal_linkage). However, this keeps existing complexity around, and to quote someone I respect a lot: "most users don't know what their ABI needs are, and we need to pick what's right for them". IMO this is such a case.

Just out of interest: Does this remove the problem that we always_inline a lot of functions with GCC?

At this point I hope we're only always_inlining functions for performance/code size reasons, rather than
as ABI management. Though it may still be needed to battle with extern template semantics.

Specifically which always inline usages are you talking about?

It does not entirely remove the need for always_inline, because GCC doesn't support exclude_from_explicit_instantiation. However, it does take us one step closer to not needing always_inline for ABI management. In particular, I plan to do a follow-up patch that would create an inline namespace that gets rev'd at each release for implementation-detail functions, and that would remove the need to use _LIBCPP_HIDE_FROM_ABI on declarations in that namespace (and that would allow ditching always_inline for many functions on GCC).

ldionne updated this revision to Diff 437556.Jun 16 2022, 8:45 AM

Rebase onto main. Gentle ping.

EricWF accepted this revision.Jun 24 2022, 10:46 AM

It's true that this is a strict improvement over the internal_I guess I'm fine with this if we give Google a way to turn it off, so we don't have to pay the cost of the larger manglings.

libcxx/include/__config
637

Can we give users an ability to turn this off?

libcxx/test/libcxx/strings/basic.string/PR42676.sh.cpp
7

This test may still have value if we change it to exercise the __abi_tag__ path instead?

This revision is now accepted and ready to land.Jun 24 2022, 10:46 AM
EricWF requested changes to this revision.Jun 24 2022, 10:55 AM

Actually, I think we need to investigate the mangling scheme when the provided ABI tag is a number, because the mangling is B<size-of-tag><tag>, so for example the ABI tag "abc" on foo produces "_Z3fooB3abcv", where 3 is the length of the tag. Now consider the tag 1, which produces B11v, which looks as though it's a tag of length 11.
This seems to break some demanglers, maybe it also will break more?

This revision now requires changes to proceed.Jun 24 2022, 10:55 AM
EricWF accepted this revision.Jun 24 2022, 5:49 PM

Ah, I see we limit ourselves to valid identifiers. LGTM again, just with a knob to turn it off.

This revision is now accepted and ready to land.Jun 24 2022, 5:49 PM

Ah, I see we limit ourselves to valid identifiers. LGTM again, just with a knob to turn it off.

I'm against this on principle, but I'll do it to unblock this patch. IMO this is the wrong way to reduce the size of object files -- we should be using compression mangling instead, and it's not a super idea to change our overall ABI stability promise just for that reason. But as I said, I'll add an escape hatch with a comment about using compression mangling instead.

ldionne updated this revision to Diff 442564.Jul 6 2022, 7:28 AM

Add escape hatch

Hey Louis,

This change breaks two LLDB tests: TestQueueFromStdModule.py and TestStackFromStdModule.py. I'm not at all familiar with these tests, but it seems the error mode is that the debugger can't find certain symbols:

error: Couldn't lookup symbols:
  __ZNSt3__1miERKNS_13move_iteratorIPP1CEES6_

Given that the bot has been red for 20 hours I went ahead and reverted your change in 61d417ceff90c4bd99bfed082d81bcc33ecf5a45.

thakis added a subscriber: thakis.Jul 7 2022, 9:12 AM

When this relands, maybe you could reland https://github.com/llvm/llvm-project/commit/6148c79a64ff03e5876882dd90d895ddb1783778 with it. (If not, no worries.)

ldionne reopened this revision.Jul 7 2022, 2:40 PM
This revision is now accepted and ready to land.Jul 7 2022, 2:40 PM
ldionne updated this revision to Diff 443066.Jul 7 2022, 2:41 PM

Band-aid for the LLDB issue, and also include 6148c79a in the commit.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 7 2022, 2:41 PM
This revision was landed with ongoing or failed builds.Jul 8 2022, 5:38 AM
This revision was automatically updated to reflect the committed changes.