Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::any is still broken with visbility=hidden and libc++ when used cross DSO boundaries #133725

Closed
milianw opened this issue Mar 31, 2025 · 19 comments
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!

Comments

@milianw
Copy link
Contributor

milianw commented Mar 31, 2025

This is related to #48743 which seems to have fixed this issue for libstdc++, but apparently there are remaining problems when using libc++:

libc++ with clang-20.1.0: https://godbolt.org/z/4TW4rMYoe note that the typeid name is the same, but the hash differs already:

NSt3__110shared_ptrI10WrappedIntEE 134520565932116
NSt3__110shared_ptrI10WrappedIntEE 105617327562758

this then leads to:

libc++abi: terminating due to uncaught exception of type std::bad_any_cast: bad any cast
Program terminated with signal: SIGSEGV

If we don't enforce libc++ and thus default to libstdc++ then everything works:

https://godbolt.org/z/bsGvfzWsh

St10shared_ptrI10WrappedIntE 6190920035151885185
St10shared_ptrI10WrappedIntE 6190920035151885185
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 31, 2025
@ldionne
Copy link
Member

ldionne commented Mar 31, 2025

If you switch to struct __attribute__((visibility("default"))) WrappedInt {, it works: https://godbolt.org/z/KEf6accK9

Without it, you end up generating two copies of the RTTI for WrappedInt, one in main.cpp and one in library.cpp, and both are marked as hidden.

@ldionne ldionne closed this as not planned Won't fix, can't repro, duplicate, stale Mar 31, 2025
@ldionne
Copy link
Member

ldionne commented Mar 31, 2025

(feel free to comment or reopen if you don't think my explanation makes sense)

@EugeneZelenko EugeneZelenko added the question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! label Mar 31, 2025
@milianw
Copy link
Contributor Author

milianw commented Mar 31, 2025

But why is this needed for libc++ when it isn't required for libstdc++? Note that wrapped int is completely inline, and as such should not need to be exported at all - this goes against everything I learned about symbol visibility so far.

@milianw
Copy link
Contributor Author

milianw commented Mar 31, 2025

Maybe to explain this a bit more on why I still consider this to be a defect: Some types are not under our control, think e.g. templates types from some thirdparty libraries, those are usually not exported and thus std::any would break with them like here: https://godbolt.org/z/oWqqjEbM1 With libstdc++ it works fine: https://godbolt.org/z/r8sMbavEc

@StefanoD
Copy link

@milianw We should also mention, that we don't have this problem with Windows and Visual Studio Compiler and GCC with glibc

@philnik777
Copy link
Contributor

@milianw @StefanoD With libstdc++ this most likely happens to work out because the function taking the typeinfo doesn't get inlined and libstdc++ gives all functions default visibility. Because of that it's always the same function that gets the typeinfo. As soon as there is more than one function taking the actual typeinfo you're out of luck. We could work around the issue by having a single function in the library with default visibility and [[gnu::noinline]], but that's super hacky and doesn't solve the problem fundamentally. As Louis already pointed out, the correct way to fix this is to annotate the type if you want to use -fvisibility=hidden. FWIW, if you want to work around it you can add [[clang::type_visibilty("default")]] via pragmas.

@milianw
Copy link
Contributor Author

milianw commented Mar 31, 2025

The "super hacky" thing you describe sounds ideal to me, as this works on all other platforms and only libc++ suffers from this.

Is there at least an arcane way for std::any or similar to detect that one tries to use it with a non-exported type, to then warn/error out? Because right now everything looks fine at compile time only to then fail fatally at runtime (and only on macOS for us, as that's the only platform we target with clang and libc++).

I also would like to have confirmation: Are you really saying one should export fully inline types? That goes against what I learned, one does not want to do that normally.

@StefanoD
Copy link

@philnik777 You break essentially std::any. As @milianw pointed out: For thirdparty libraries, this is out of our control. And this is the only lib which behaves like that. From an user perspective: It's totally unexpected and it breaks std::any

@philnik777
Copy link
Contributor

The "super hacky" thing you describe sounds ideal to me, as this works on all other platforms and only libc++ suffers from this.

It's not. This only happens to work out right now with libstdc++. Not annotating the types properly is a latent bug in your code. Also, other people who annotate their types properly would also complain because we'd essentially be adding an entirely unnecessary optimization barrier.

Is there at least an arcane way for std::any or similar to detect that one tries to use it with a non-exported type, to then warn/error out? Because right now everything looks fine at compile time only to then fail fatally at runtime (and only on macOS for us, as that's the only platform we target with clang and libc++).

Not that I'm aware of. Also note that this is only a problem across dylib boundaries, so most uses don't suffer from this problem.

I also would like to have confirmation: Are you really saying one should export fully inline types? That goes against what I learned, one does not want to do that normally.

Yes, you have to export the typeinfo of all types. I don't know what you've learned, but that's exactly what both libc++ and libstdc++ do to make using typeinfo for stdlib types across dylib boundaries work.

@philnik777 You break essentially std::any. As @milianw pointed out: For thirdparty libraries, this is out of our control.

You can always wrap it into your own type that has the correct annotations. I also pointed out above that you can hack your way around third library code not annotating their code via pragmas. Another option is to not use -fvisibility=hidden and instead annotate your code, as both libc++ and libstdc++ do.

And this is the only lib which behaves like that. From an user perspective: It's totally unexpected and it breaks std::any

Why? You're saying you don't need the typeinfo across dylib boundaries via -fvisibility=hidden and we respect that.

@milianw
Copy link
Contributor Author

milianw commented Apr 1, 2025

I found the source where I learned not to apply export to fully inline types, see: https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews see Value Classes #6 "Never export a non-polymorphic class wholesale"

you can hack your way around third library code not annotating their code via pragmas

How so? Can you please expand on this? Are you saying I should do something like (pseudo code)

#push pragma force export
#include <thirdparty headers>
#pop pragma

@philnik777
Copy link
Contributor

I found the source where I learned not to apply export to fully inline types, see: https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews see Value Classes #6 "Never export a non-polymorphic class wholesale"

Given that the rationale talks specifically about MSVC I don't think it applies here. Also note that we're not asking you to export everything. We're only asking you to export the RTTI.

you can hack your way around third library code not annotating their code via pragmas

How so? Can you please expand on this? Are you saying I should do something like (pseudo code)

#push pragma force export
#include <thirdparty headers>
#pop pragma

Yes, exactly. It would be something like

#pragma clang attribute VisibilityAnnotations.push([[clang::type_visibility("default")]], apply_to=record)
#include <whatever>
#pragma clang attribute VisibilityAnnotations.pop

You can change VisibilityAnnotations to anything you want though.

@milianw
Copy link
Contributor Author

milianw commented Apr 1, 2025

Given that the rationale talks specifically about MSVC I don't think it applies here.

Well, when writing cross platform code, we must find a solution that works everywhere. As such, I cannot ignore MSVC. It sounds as-if we would need separate export macros then, to pleasen the different platforms which makes me quite sad. If only the platforms could work a bit more similar to simplify things dream

Also note that we're not asking you to export everything. We're only asking you to export the RTTI.

This is the first time I hear about the distinction here, thanks! For others who are equally unaware: https://releases.llvm.org/20.1.0/tools/clang/docs/AttributeReference.html#type-visibility

Sadly, this seems to be a clang specific feature, I cannot find something similar for GCC. And there is also apparently no flag to configure it globally while compiling. If that would exist, one would ideally be able to do

clang -ftype-visibility=default -fvisibility=hidden -fvisibility-inlines-hidden

And get a result that works in a similar way to other platforms, right? Can we maybe repurpose this bug report accordingly then, to kindly ask for the addition for this global -ftype-visibility flag?

The "custom pragma" approach you are giving doesn't sound scalable - we would have to do that everywhere we include the thirdparty code after all, and that is extremely errorprone, no?

@philnik777
Copy link
Contributor

Given that the rationale talks specifically about MSVC I don't think it applies here.

Well, when writing cross platform code, we must find a solution that works everywhere. As such, I cannot ignore MSVC. It sounds as-if we would need separate export macros then, to pleasen the different platforms which makes me quite sad. If only the platforms could work a bit more similar to simplify things dream

Well, sadly we don't live in that dream world. In libc++ we currently have nine different visibility macros for all the platform specifics.

Also note that we're not asking you to export everything. We're only asking you to export the RTTI.

This is the first time I hear about the distinction here, thanks! For others who are equally unaware: https://releases.llvm.org/20.1.0/tools/clang/docs/AttributeReference.html#type-visibility

You're welcome. We should probably have been more explicit about this. Louis and I have had long conversations about the visibility attributes in Clang and are therefore quite deep in the topic, so it didn't really occur to me at first that it may not be obvious that RTTI has its own visibility, separate from other symbols. FYI this is also the case for vtables, which should (almost?) always have the same visibility as RTTI.

Sadly, this seems to be a clang specific feature, I cannot find something similar for GCC.

I agree that it's sad. Feel free to comment on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113958.

And there is also apparently no flag to configure it globally while compiling. If that would exist, one would ideally be able to do

clang -ftype-visibility=default -fvisibility=hidden -fvisibility-inlines-hidden

And get a result that works in a similar way to other platforms, right?

Yes, that sounds like a reasonable solution. That's probably what a lot of people actually want in the end.

Can we maybe repurpose this bug report accordingly then, to kindly ask for the addition for this global -ftype-visibility flag?

I think it would be better to open a separate issue with a link to this one.

The "custom pragma" approach you are giving doesn't sound scalable - we would have to do that everywhere we include the thirdparty code after all, and that is extremely errorprone, no?

Actually, never mind. It looks like Clang doesn't support [[clang::type_visibility]] inside #pragma clang attribute.

@ldionne
Copy link
Member

ldionne commented Apr 1, 2025

Depending on the platform you're seeing this on, this could also be due to how typeinfos are compared.

I think libstdc++ performs a string comparison of the typeinfo name on some platforms where we instead use pointer equality (of the RTTI pointer). When using string comparison, types will compare equal even when the RTTIs have messed up visibility. On the other hand, this will also result in genuinely different types in different dylibs that happen to share the same name (which can easily happen at global scope) to compare RTTI-equal. That's equally, if not more, broken.

So I fear there is no perfect answer. If you decide to play with visibility, you're out of the purview of ISO C++ and you can't expect perfect portability. You'll need some amount of platform specific stuff no matter what you do.

@milianw
Copy link
Contributor Author

milianw commented Apr 2, 2025

Depending on the platform you're seeing this on

We saw it working on windows with msvc and on linux with gcc+libstdc++. Only macOS/clang failed us.

On the other hand, this will also result in genuinely different types in different dylibs that happen to share the same name (which can easily happen at global scope) to compare RTTI-equal.

Isn't that an ODR violation, and one reason why we use namespaces and file local linkage etc?

You'll need some amount of platform specific stuff no matter what you do.

Yes, that is expected, and has been the case for the past 20 years or so already that I've been using export macros, in the form that eventually got quasi-standardized via https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html - but note how that is completely unaware of your clang specific type visibility macros. So it's not just "me" who was blissfully unaware of these traps, but I bet a ton of C++/CMake developers and basically the whole KDE/Qt ecosystem at large.

@ldionne
Copy link
Member

ldionne commented Apr 2, 2025

Isn't that an ODR violation, and one reason why we use namespaces and file local linkage etc?

The ISO Standard doesn't have a notion of .dylibs (or .sos), nor does it have a notion of visibility, so saying that it's an ODR violation is only pedantically relevant. Note that if you have a global type named Foo in dylib 1 and it has file local linkage, you still get a conflict of RTTI identity if you use type name comparison with another file-local Foo in dylib 2, even though that's technically not an ODR violation since they both have file local linkage. So that's still broken.

IMO the key takeaway here is that if you're going to try controlling your visibility, you have to be ready to understand how visibility works and part of that means understanding how RTTI comparison works on your platform and how it's affected by visibility. I'm not saying that's easy, it's a total mess, but it's the world we have right now.

@milianw
Copy link
Contributor Author

milianw commented Apr 2, 2025

I was under the misconception of having understood how visibility works, but this issue here clearly shows that's not the case :) So the question now becomes, where can we learn more about your platform, i.e. clang/libc++ - I only found e.g. https://releases.llvm.org/20.1.0/tools/clang/docs/AttributeReference.html#type-visibility but that is very scarce on details and definitely doesn't talk about RTTI comparison. Do you know any resources that would us to better understand all the required nitty gritty details to properly handle this? Again, now how this seems to be broken for everyone that relies on standard CMake functionality to reduce the visibility of symbols, which is in many places advertised as a good thing, mostly to ensure linux builds behave more similar to Windows ones e.g.

@ldionne
Copy link
Member

ldionne commented Apr 3, 2025

I think this comment is useful to understand a bit more about RTTI and visibility: https://github.com/llvm/llvm-project/blob/main/libcxx/include/typeinfo#L120

However, I am not aware of any official documentation that does a holistic coverage of the whole subject, sorry. For most of what I know about this topic, I don't even know why I know it anymore, I just kind of learned it as I went and with trial and error. Whenever we do something funky with visibility in libc++ we try to leave decent documentation behind, but that's far from "teaching material" level.

@milianw
Copy link
Contributor Author

milianw commented Apr 4, 2025

Thank you Louis, this is interesting and a good start. For me as someone who wants to "understand how RTTI comparison works on [my] platform", more exhaustive material would be crucial - esp. targeted at library developers. After all, we seem to know what to do for libstdc++ and msvc to a degree that it works, but I fear we'll now have to sift through the code to understand what to do for libc++ specifically, and what best practices there are. I am well aware of the load of the people involved in these projects, and the extra burden of documentation, so please just take this as a friendly request for considering to convince someone to spent some time on documenting what to do for libc++ as a platform wrt. visibility. As a motivation: Everyone using standard CMake facilities and hidden visibility is doing it wrong apparently, so quite the chance to apply https://xkcd.com/386/ ;-)

Cheers and keep up the good work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!
Projects
None yet
Development

No branches or pull requests

6 participants