This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Don't use dllimport for a static member in a template
ClosedPublic

Authored by mstorsjo on Feb 21 2021, 3:35 PM.

Details

Summary

This fixes clang warnings (that are treated as errors when running the test suite):

libcxx/include/string:4409:59: error: definition of dllimport static field [-Werror,-Wdllimport-static-field-def]
               basic_string<_CharT, _Traits, _Allocator>::npos;

The warning is normally not visible as long as the libc++ headers are treated as system headers.

The same construct is always an error in MSVC.

(One _LIBCPP_FUNC_VIS was added in
rG2d8f23f571635c1fb983b40c4c2548716a5b65b6, which broke DLL builds.
rG59919c4d6b6370da7133bbca0d31844e21646bb1 fixed this by adding another
_LIBCPP_FUNC_VIS on the declaration for consistency, but the underlying
issue remained, that one can't use dllimport here.)

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 21 2021, 3:35 PM
mstorsjo requested review of this revision.Feb 21 2021, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2021, 3:35 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Feb 22 2021, 8:56 AM
ldionne added a subscriber: ldionne.

Is this really the right fix? I think Clang is warning about an actual issue, i.e. that we're defining npos in the test file when we shouldn't be? If we just silence this warning in the test suite, it'll fire when people build against libc++ on Windows.

What's the Microsoft recommendation for using dllimport on static data members?

This revision now requires changes to proceed.Feb 22 2021, 8:56 AM

Is this really the right fix? I think Clang is warning about an actual issue, i.e. that we're defining npos in the test file when we shouldn't be? If we just silence this warning in the test suite, it'll fire when people build against libc++ on Windows.

Yeah, if we're running into this warning here, technically we would warn on the same when included in user code. In practice we don't, as warnings from system headers (as the libc++ headers are at that point) normally are muted though.

(We're not defining npos in the test files, it just warns about the declarations in the headers.)

What's the Microsoft recommendation for using dllimport on static data members?

I'd like to summon @rnk and @hans here. In my experience, at least the testsuite runs fine in DLL configurations if we revert rG2d8f23f571635c1fb983b40c4c2548716a5b65b6 and rG59919c4d6b6370da7133bbca0d31844e21646bb1.

As far as I can see it, the first one adds _LIBCPP_FUNC_VIS, with this explanation:

[libc++] Explicitly mark basic_string<...>::npos with default visibility.

This ensures that the version compiled into the library isn't accidentally hidden.

I presume this is a fix for non-windows systems. (It doesn't come with references to review or other longer elaborations.) And the other one adds a second matching _LIBCPP_FUNC_VIS (in order not to break windows DLL builds altogether).

Is the correct fix to just omit the _LIBCPP_FUNC_VIS from the npos declaration (in both instances) in windows configurations? (I guess we don't yet have any macro like _LIBCPP_FUNC_VIS_EXCEPT_ON_WINDOWS?)

rnk added a comment.Feb 22 2021, 12:42 PM

Is this really the right fix? I think Clang is warning about an actual issue, i.e. that we're defining npos in the test file when we shouldn't be? If we just silence this warning in the test suite, it'll fire when people build against libc++ on Windows.

What's the Microsoft recommendation for using dllimport on static data members?

I guess MSVC is more strict, they just reject dllimport when applied to a data definition:
https://gcc.godbolt.org/z/Mqs113
I can't recall why we made this a warning in clang instead of an error.

It seems wrong to use the _LIBCPP_FUNC_VIS attribute on things that are not functions, so maybe we need a _LIBCPP_DATA_VIS attribute.

In D97168#2579763, @rnk wrote:

Is this really the right fix? I think Clang is warning about an actual issue, i.e. that we're defining npos in the test file when we shouldn't be? If we just silence this warning in the test suite, it'll fire when people build against libc++ on Windows.

What's the Microsoft recommendation for using dllimport on static data members?

I guess MSVC is more strict, they just reject dllimport when applied to a data definition:
https://gcc.godbolt.org/z/Mqs113
I can't recall why we made this a warning in clang instead of an error.

It seems wrong to use the _LIBCPP_FUNC_VIS attribute on things that are not functions, so maybe we need a _LIBCPP_DATA_VIS attribute.

Thanks! A _LIBCPP_DATA_VIS sounds like a good way forward to me.

mstorsjo updated this revision to Diff 325777.Feb 23 2021, 6:57 AM
mstorsjo retitled this revision from [libcxx] [test] Add -Wno-dllimport-static-field-def when building tests to [libcxx] Don't use dllimport for a static member in a template.
mstorsjo edited the summary of this revision. (Show Details)
mstorsjo added a reviewer: rnk.

Added _LIBCPP_DATA_VIS as suggested by @rnk. A bit unsure about the naming though, as it's not data in general (which can be dllimported just fine), but static members in templates.

mstorsjo updated this revision to Diff 325781.Feb 23 2021, 7:01 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Retrigger CI.

@rnk - Does this sound good to you? And is there a more exact name, as dllimport of data is fin in general, this is just about a static member in an (instantiated) template?

ldionne requested changes to this revision.Feb 25 2021, 11:25 AM
ldionne added inline comments.
libcxx/include/__config
673 ↗(On Diff #325781)

Maybe this can just be called _LIBCPP_DEFAULT_VISIBILITY?

And we could maybe also reuse this macro in the various _LIBCPP_FUNC_VIS & other macros (as a separate patch, I can try to clean this up).

This revision now requires changes to proceed.Feb 25 2021, 11:25 AM

I do like this approach better than silencing the warning, though.

mstorsjo added inline comments.Feb 25 2021, 11:30 AM
libcxx/include/__config
673 ↗(On Diff #325781)

Hmm, not entirely sure, that changes the semantics a bit: Currently the attributes say what they're supposed to annotate (function visibility, type visibility, etc), but that'd make it say what it does (on one platform) - which actually isn't the same across all platforms (the point here being we expressly want a different visibility for windows targets, for static members in a template, contrary to the other cases).

rnk added inline comments.Feb 25 2021, 11:37 AM
libcxx/include/__config
673 ↗(On Diff #325781)

Yeah, this is tough. Seeing the new name, I'm not sure I like it. @ldionne is right, we can export data. The restriction is that things with weak linkage can't be imported/exported, since they don't belong only to the libc++ DLL.

Do you think it's reasonable to use _LIBCPP_TEMPLATE_VIS? std::basic_string<T...>::npos is a "templated entitiy" (temploid?). It should work as is.

mstorsjo added inline comments.Mar 2 2021, 8:30 AM
libcxx/include/__config
673 ↗(On Diff #325781)

_LIBCPP_TEMPLATE_VIS seems ok to me; a new _LIBCPP_TEMPLATE_DATA_VIS would work too if we'd want a more precise name.

mstorsjo updated this revision to Diff 327468.Mar 2 2021, 8:31 AM

Use the existing _LIBCPP_TEMPLATE_VIS.

rnk accepted this revision.Mar 2 2021, 10:56 AM

Sorry for so much discussion about such a small fix. :)

ldionne accepted this revision.Mar 2 2021, 12:30 PM

Thanks! I think it was useful to have that discussion though, since otherwise we might not have converged on the best fix (which IMO we have now).

This revision is now accepted and ready to land.Mar 2 2021, 12:30 PM
mstorsjo updated this revision to Diff 327546.Mar 2 2021, 12:46 PM

Switched back to adding a new macro, _LIBCPP_TEMPLATE_DATA_VIS, as _LIBCPP_TEMPLATE_VIS can expand to __attribute__ ((__type_visibility__("default"))) which doesn't work for data. (Sorry I hadn't noticed that CI had flagged the previous version as failed almost immediately.)

mstorsjo requested review of this revision.Mar 2 2021, 1:53 PM

(Marking this as needing new review so it shows up correctly in the review queue, as I had to change it a bit)

mstorsjo updated this revision to Diff 327702.Mar 3 2021, 12:55 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Reupload to rerun CI (which failed in some tests due to timeouts/spurious breakage last time).

mstorsjo updated this revision to Diff 327918.Mar 3 2021, 2:18 PM

Restart CI again.

ldionne accepted this revision.Mar 3 2021, 2:37 PM
This revision is now accepted and ready to land.Mar 3 2021, 2:37 PM