This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Add a separate 'windows-dll' feature to check for
ClosedPublic

Authored by mstorsjo on Apr 9 2021, 11:39 AM.

Details

Summary

This allows distinguishing failures in tests that only fail when libcxx
is linked as a DLL, allowing narrowing down XFAILs (avoiding XPASS errors
if not built as a DLL).

If both enable_shared and enable_static are set, the tests link and use
the shared version of the lib.

Diff Detail

Event Timeline

mstorsjo created this revision.Apr 9 2021, 11:39 AM
mstorsjo requested review of this revision.Apr 9 2021, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 11:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

What's the intended use case? Do you have upcoming patches that will make use of it?

What's the intended use case? Do you have upcoming patches that will make use of it?

Yes, I have a couple patches that could use it. This would allow running a CI configuration for a statically linked lib on windows, which allows testing the c++experimental library (and generally increases CI coverage by testing under more diverse conditions) - but right now there's a handful of XPASSes in that setup.

curdeius accepted this revision as: curdeius.Apr 20 2021, 2:41 AM

LGTM.

ldionne requested changes to this revision.Apr 20 2021, 5:47 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/utils/libcxx/test/features.py
156

We don't want to use .enable_shared here. .enable_shared is part of the old config which I'm trying to get rid of one step at a time.

Is there a way to implicitly detect that we're linking against the DLL without being passed that information through cfg.enable_shared?

Alternatively, something we could explore is add a feature that tells us whether the library is linked statically or dynamically (regardless of the platform). Your feature check would then become windows && libcxx-linked-dynamically.

This revision now requires changes to proceed.Apr 20 2021, 5:47 AM
mstorsjo added inline comments.Apr 20 2021, 6:04 AM
libcxx/utils/libcxx/test/features.py
156

Is there a way to implicitly detect that we're linking against the DLL without being passed that information through cfg.enable_shared?

We could source the headers and see if _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is defined or not; it's a bit less direct but should be reliable in practice. (The fact that that define indicates static/shared mode is a windows specific bit though.)

Alternatively, something we could explore is add a feature that tells us whether the library is linked statically or dynamically (regardless of the platform). Your feature check would then become windows && libcxx-linked-dynamically.

I guess that would work - but that boils down to the original issue - I'm not allowed to use the "old config", but I'm not quite familiar enough with the separation between old and new here, and what the actually allowed way of passing info to the new test configure system is.

I presume we're able to introspect the build env by looking for defines and such, but e.g. for non-windows configurations, the fact whether we're building isn't visible in headers. How would you suggest we find the information in that case, and pass it from the parts of the system where we know for sure (where we have the enable_shared variable today)?

ldionne added inline comments.Apr 20 2021, 6:08 AM
libcxx/utils/libcxx/test/features.py
156

Anything in libcxx/test/configs/legacy.cfg.in is the old config, and so is libcxx/utils/libcxx/test/config.py. I'm trying to move stuff from libcxx/utils/libcxx/test/config.py into features.py and params.py, with the goal of replacing legacy.cfg.in by configs like libcxx/test/configs/libcxx-trunk-shared.cfg.in.

One thing we could do is build a dummy program referencing something from std::string (for example) and see whether it depends on any externally defined symbol from libc++. That would satisfy the request for this feature to be automatically detected.

mstorsjo added inline comments.Apr 20 2021, 6:57 AM
libcxx/utils/libcxx/test/features.py
156

So overall, I agree that the introspection approach is good in the sense that it puts testing the newly built libc++ on equal grounds with testing a third party C++ library...

When testing a third party library, one would still need to manually set up the equivalent of libcxx-trunk-shared.cfg.in for ones setup right? And there are still going to be a small set of variables that are configured by the build system (for the one set up automatically when building libc++) and manually for third party library testing, for things that are impossible/impractical to introspect. (Not insisting that this variable is such one, just trying to get a grasp of the big picture.) And one important tradeoff is that the introspection setup doesn't do too many expensive tests, as they are run at the start each time when running one or more tests. (I.e. the tests for defines are quick as they rely on the output of $CC -E -dM that is just run once, but doing a large number of compile+link tests might end up impractical.)

Testing linking and looking at the result might work, but it sounds a bit cumbersome, when working across multiple platforms.

For darwin, one can use something like otool -L, for ELF maybe readelf -d and for PE-COFF maybe [llvm-]objdump -p (which might not necessarily be available in all setups) to get a list of the shared libs that are loaded by an executable. We can't look for literally [lib]c++.[dll|so|dylib] because a third party tested lib might be named something entirely different - so we'd need to look for specific symbol names. otool and readelf don't print them, but one could use e.g. nm -D, and objdump -p prints imported symbols. Then for windows, there's two different C++ symbol mangling schemes, but maybe one would look for e.g. the substring string in the symbols, as I think that could be found in both mangling schemes.

mstorsjo updated this revision to Diff 339528.Apr 22 2021, 2:12 AM

Look for _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS in compilerMacros(cfg) instead of looking at the cfg.enable_shared flag.

This is a fairly straightforward approach which works fine (when looking at libcxx itself, but doesn't generalize to other C++ libraries), but only works on Windows (but we have less need to distinguish between shared and static on other platforms). This avoids needing to do binary inspections to see how it ends up linked.

mstorsjo updated this revision to Diff 340242.Apr 23 2021, 11:11 PM

Rebase to hopefully get a green CI run.

ldionne accepted this revision.Apr 27 2021, 6:22 AM
This revision is now accepted and ready to land.Apr 27 2021, 6:22 AM