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

[mstorsjo]

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

[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?

[mstorsjo]

In D97168#2579077, @ldionne 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.

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]

In D97168#2579077, @ldionne 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.

[mstorsjo]

In D97168#2579763, @rnk wrote:

In D97168#2579077, @ldionne 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]

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]

Retrigger CI.

[mstorsjo]

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

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

[ldionne]

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

[mstorsjo]

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]

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]

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]

Use the existing _LIBCPP_TEMPLATE_VIS.

[rnk]

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

[ldionne]

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

[mstorsjo]

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]

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

[mstorsjo]

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

[mstorsjo]

Restart CI again.

libcxx/utils/libcxx/test/params.py

_warningFlags = [
   '-Wno-unused-command-line-argument',
   '-Wno-attributes',
   '-Wno-pessimizing-move',
   '-Wno-c++11-extensions',
   '-Wno-user-defined-literals',
   '-Wno-noexcept-type',
   '-Wno-aligned-allocation-unavailable',
   '-Wno-atomic-alignment',
+  '-Wno-dllimport-static-field-def',
 
   # GCC warns about places where we might want to add sized allocation/deallocation
   # functions, but we know better what we're doing/testing in the test suite.
   '-Wno-sized-deallocation',
 
   # These warnings should be enabled in order to support the MSVC
   # team using the test suite; They enable the warnings below and
   # expect the test suite to be clean.