This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix testing on windows with c++experimental enabled
ClosedPublic

Authored by mstorsjo on Mar 23 2021, 5:05 AM.

Details

Summary

The straightforward AddLinkFlag('-lc++experimental') approach doesn't
work on e.g. MSVC; instead use logic similar to how libc++ itself
is linked, but with the exception that libc++experimental always is
linked statically.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 23 2021, 5:05 AM
mstorsjo requested review of this revision.Mar 23 2021, 5:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 5:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo planned changes to this revision.Mar 23 2021, 5:44 AM

Sorry, this one doesn't work as is; the link flags substitutions are already set up when libcxx.test.newconfig.configure returns, so it's too late to add anything to it after that...

mstorsjo updated this revision to Diff 332648.Mar 23 2021, 6:09 AM

As it's too late to update link_flags after running newconfig.configure, add a simplified form of conditional logic in a lambda instead. This lambda requires knowledge about what kind of library naming/linking form is going to be used though, which is (not nicely) passed via a flag in available_features.

ldionne requested changes to this revision.Mar 23 2021, 7:46 AM
ldionne added a subscriber: ldionne.

Can we try detecting MSVC this way instead:

diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index e54cecef0ab8..2fb7e003ce03 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -14,6 +14,8 @@ import sys
 _isClang      = lambda cfg: '__clang__' in compilerMacros(cfg) and '__apple_build_version__' not in compilerMacros(cfg)
 _isAppleClang = lambda cfg: '__apple_build_version__' in compilerMacros(cfg)
 _isGCC        = lambda cfg: '__GNUC__' in compilerMacros(cfg) and '__clang__' not in compilerMacros(cfg)
+_isMSVC       = lambda cfg: '_MSC_VER' in compilerMacros(cfg)
+_msvcVersion  = lambda cfg: (int(compilerMacros(cfg)['_MSC_VER']) // 100, int(compilerMacros(cfg)['_MSC_VER']) % 100)
 
 DEFAULT_FEATURES = [
   Feature(name='fcoroutines-ts',
@@ -81,6 +83,10 @@ DEFAULT_FEATURES = [
   Feature(name=lambda cfg: 'gcc-{__GNUC__}'.format(**compilerMacros(cfg)),                                                         when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}.{__GNUC_MINOR__}'.format(**compilerMacros(cfg)),                                        when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}.{__GNUC_MINOR__}.{__GNUC_PATCHLEVEL__}'.format(**compilerMacros(cfg)),                  when=_isGCC),
+
+  Feature(name='msvc',                                                                                                             when=_isMSVC),
+  Feature(name=lambda cfg: 'msvc-{}'.format(*_msvcVersion(cfg)),                                                                   when=_isMSVC),
+  Feature(name=lambda cfg: 'msvc-{}.{}'.format(*_msvcVersion(cfg)),                                                                when=_isMSVC),
 ]
 
 # Deduce and add the test features that that are implied by the #defines in

I'm sure there's stuff to fix with that, but basically I'd rather add a new feature to detect msvc correctly all the time than hardcode it based on windows.

This revision now requires changes to proceed.Mar 23 2021, 7:46 AM

Can we try detecting MSVC this way instead:

diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index e54cecef0ab8..2fb7e003ce03 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -14,6 +14,8 @@ import sys
 _isClang      = lambda cfg: '__clang__' in compilerMacros(cfg) and '__apple_build_version__' not in compilerMacros(cfg)
 _isAppleClang = lambda cfg: '__apple_build_version__' in compilerMacros(cfg)
 _isGCC        = lambda cfg: '__GNUC__' in compilerMacros(cfg) and '__clang__' not in compilerMacros(cfg)
+_isMSVC       = lambda cfg: '_MSC_VER' in compilerMacros(cfg)
+_msvcVersion  = lambda cfg: (int(compilerMacros(cfg)['_MSC_VER']) // 100, int(compilerMacros(cfg)['_MSC_VER']) % 100)
 
 DEFAULT_FEATURES = [
   Feature(name='fcoroutines-ts',
@@ -81,6 +83,10 @@ DEFAULT_FEATURES = [
   Feature(name=lambda cfg: 'gcc-{__GNUC__}'.format(**compilerMacros(cfg)),                                                         when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}.{__GNUC_MINOR__}'.format(**compilerMacros(cfg)),                                        when=_isGCC),
   Feature(name=lambda cfg: 'gcc-{__GNUC__}.{__GNUC_MINOR__}.{__GNUC_PATCHLEVEL__}'.format(**compilerMacros(cfg)),                  when=_isGCC),
+
+  Feature(name='msvc',                                                                                                             when=_isMSVC),
+  Feature(name=lambda cfg: 'msvc-{}'.format(*_msvcVersion(cfg)),                                                                   when=_isMSVC),
+  Feature(name=lambda cfg: 'msvc-{}.{}'.format(*_msvcVersion(cfg)),                                                                when=_isMSVC),
 ]
 
 # Deduce and add the test features that that are implied by the #defines in

I'm sure there's stuff to fix with that, but basically I'd rather add a new feature to detect msvc correctly all the time than hardcode it based on windows.

Thanks, I'll have a try with that to see if it works. As all parameters are evaluated before all features (in newconfig.configure), would these be set already at the point when we need them when handling a parameter?

mstorsjo updated this revision to Diff 332777.Mar 23 2021, 1:44 PM

Add a _isMSVC helper function in features.py and add a 'msvc' feature via that.
As the features themselves are processed after processing parameters, we can't check for the feature itself, but use the _isMSVC function.

ldionne accepted this revision.Apr 21 2021, 2:38 PM

LGTM, but please consider populating the version.

libcxx/utils/libcxx/test/features.py
86

Do we want to also populate the MSVC version like we do for GCC and Clang?

This revision is now accepted and ready to land.Apr 21 2021, 2:38 PM
mstorsjo added inline comments.Apr 21 2021, 10:52 PM
libcxx/utils/libcxx/test/features.py
86

Sure, I can do that for consistency.

In cases when testing with libc++, it's actually Clang impersonating MSVC - but the version of MSVC that Clang tries to impersonate can still be useful to know. (Clang has to emulate certain MSVC bugs depending on the emulated MSVC version at times. And in some cases, whether Clang correctly emulates newer MSVC features, or stopping emulating old MSVC bugs, depends on the Clang version as well.) So in practice, using the version for anything can be tricky/messy, but I can add it in any case.