Page MenuHomePhabricator

[libcxx] Don't treat Windows specially with visibility annotations
AbandonedPublic

Authored by phosek on Oct 5 2020, 11:49 AM.

Details

Reviewers
smeenai
EricWF
ldionne
Group Reviewers
Restricted Project
Summary

There's no reason to treat Windows specially, users of libc++ can define
_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS just like they would on other
platforms.

See http://lists.llvm.org/pipermail/libcxx-dev/2020-October/000978.html
for more details.

Diff Detail

Event Timeline

phosek created this revision.Oct 5 2020, 11:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2020, 11:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek requested review of this revision.Oct 5 2020, 11:49 AM

According to git blame this change was authored by @smeenai in rG546442160881b, @smeenai do you know what was the motivation for having a special handling on Windows?

I think the reason for this restriction is that on Windows, __declspec(dllimport) changes the compiler's code generation to assume that the symbol will be imported (i.e. if the compiler sees a call to a function X which is marked __declspec(dllimport), it'll generate an indirect call to __imp_X instead). If you're linking against libc++ statically, the imported version of the symbol will of course not exist, so you want to disable the __declspec(dllimport) annotations by default if you're building just the static library for Windows.

(IIRC, Microsoft's linker actually supports this case; it'll generate a dummy __imp_X for you pointing to the local X symbol, but it'll also give you a warning about doing so. I think LLD COFF supports that as well. It's just less clean to do it that way.)

Since this is Windows-specific behavior (or technically dllimport-specific behavior), perhaps it makes more sense to do some Windows-specific visibility macro adjustments than overloading the cross-platform _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS for this purpose?

ldionne accepted this revision.Oct 5 2020, 1:49 PM

I'm happy with removing this special case, especially since the linker handles it. I believe it makes more sense for a vendor shipping the static archive to disable these annotations explicitly instead.

This revision is now accepted and ready to land.Oct 5 2020, 1:49 PM

I'm a "vendor" building libc++ for windows, both statically and dynamically, and without this, I'll need to manually inject a #define _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS into the installed headers somehow.

At least in my experience, code generation with headers lacking dllimport attributes, but linking against an import library for a DLL, works much better than building user code with dllimport but linking against a static library. (Yes, the linker can sometimes generate the __imp_ stubs for dllimport linking against oneself, but iirc it doesn't work flawlessly for things like libc++.)

Also cc @thieta

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

So to be very clear, I'd prefer this not to be committed, as it will break my build setup, and the way for me to work around it isn't very pretty.

A different potential way forward would be to default to not using visibility annotations on windows, making them opt-in instead of opt-out. Because at least in my experience in mingw configurations, mismatches between actual libc++ vs option set in the user's code works much better if the visibility annotations are disabled. IIRC @thieta just ran into the same issue a few weeks ago as well and concluded the same.

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

Let's add a proper LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS option and config_define_if(LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) like we do for other __config_site macros. Then, you can configure your build with -DLIBCXX_DISABLE_VISIBILITY_ANNOTATIONS and remove the platform specific logic from libc++'s CMake.

WDYT?

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

Let's add a proper LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS option and config_define_if(LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) like we do for other __config_site macros. Then, you can configure your build with -DLIBCXX_DISABLE_VISIBILITY_ANNOTATIONS and remove the platform specific logic from libc++'s CMake.

WDYT?

That sounds good to me and would be easy to configure going forward.

Not sure how well it works in practice with @phosek 's original usecase from http://lists.llvm.org/pipermail/libcxx-dev/2020-October/000978.html, wanting to use one single __config_site for libc++ for multiple targets at once though.

phosek added a comment.Oct 5 2020, 2:49 PM

And to be clear, this isn't about manually defining _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS while building libc++ itself; it needs to be defined whenever user code includes libc++ headers.

Let's add a proper LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS option and config_define_if(LIBCXX_DISABLE_VISIBILITY_ANNOTATIONS _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) like we do for other __config_site macros. Then, you can configure your build with -DLIBCXX_DISABLE_VISIBILITY_ANNOTATIONS and remove the platform specific logic from libc++'s CMake.

WDYT?

That sounds good to me and would be easy to configure going forward.

Not sure how well it works in practice with @phosek 's original usecase from http://lists.llvm.org/pipermail/libcxx-dev/2020-October/000978.html, wanting to use one single __config_site for libc++ for multiple targets at once though.

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

mati865 added a subscriber: mati865.Oct 5 2020, 3:32 PM
phosek added a comment.Oct 5 2020, 6:13 PM

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

__config_site is the only file that differs across targets, but we always concat it with __config so I assumed we're going to have a per-target copy of __config, but we could make this even granular by keeping them separate and using #include <__config_site> guarded by #if __has_include(<__config_site>) ... #endif inside __config. Would that be preferable?

A compile time option sounds good, per target configuration sounds even better!

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

__config_site is the only file that differs across targets, but we always concat it with __config so I assumed we're going to have a per-target copy of __config, but we could make this even granular by keeping them separate and using #include <__config_site> guarded by #if __has_include(<__config_site>) ... #endif inside __config. Would that be preferable?

Actually, I have a patch that changes libc++ to use #include <__config_site>. I've been wanting to do that for a while. Let me put it up for review.

phosek added a comment.Oct 7 2020, 3:21 PM

Possible solution would be to allow per-target __config which is something I was going to propose separately since we have other use cases for it. That is, we would share headers across targets, but each target would have its own __config. This approach is already used by libstdc++. I have a local prototype and the implementation is pretty straightforward.

Do you mean __config, or __config_site?

__config_site is the only file that differs across targets, but we always concat it with __config so I assumed we're going to have a per-target copy of __config, but we could make this even granular by keeping them separate and using #include <__config_site> guarded by #if __has_include(<__config_site>) ... #endif inside __config. Would that be preferable?

Actually, I have a patch that changes libc++ to use #include <__config_site>. I've been wanting to do that for a while. Let me put it up for review.

I have uploaded my change as D89013, which in turn depends on D83911 and D88452. Once your change is up for review, I'll rebase my change on top of it.

Ping, what are we doing with this?

phosek abandoned this revision.Apr 21 2021, 3:09 PM

I don't think we need this anymore.