This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] [test] Fix the mingw test config
ClosedPublic

Authored by mstorsjo on Apr 5 2023, 10:02 AM.

Details

Reviewers
phosek
Group Reviewers
Restricted Project
Commits
rG637d572f7b50: [libcxxabi] [test] Fix the mingw test config
Summary

Don't link libc++abi separately in addition to the main -lc++; in
mingw build configs, libc++abi is always bundled into libc++
(via LIBCXX_ENABLE_STATIC_ABI_LIBRARY).

In the case of a shared linked libc++, linking a separate static
libc++abi leads to linker errors.

Define _LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS while building tests.

Diff Detail

Event Timeline

mstorsjo created this revision.Apr 5 2023, 10:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 10:02 AM
mstorsjo requested review of this revision.Apr 5 2023, 10:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 10:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.
libcxxabi/test/configs/llvm-libc++abi-mingw.cfg.in
6

Could we instead define it just in the tests that include libcxxabi source files? I don't think there is a lot of those.

mstorsjo added inline comments.Apr 14 2023, 2:18 PM
libcxxabi/test/configs/llvm-libc++abi-mingw.cfg.in
6

There's a couple of them, and we could do that.

But secondly, we also need to define this for all tests that include cxxabi.h and call functions from there directly. cxxabi.h defaults to dllimport unless this define is set (libcxx has this machinery for customizing the defaults based on the build style etc, via the __config file etc, but libcxxabi doesn't have that). The default of dllimport doesn't work when we're statically linking libcxx+libcxxabi in the end. So even then, we need this defined in one way or another.

(I guess we could rework how the visibility attributes in cxxabi.h work too, but there's little benefit to that outside of the tests, as I don't believe regular user code ever would be including that header.)

mstorsjo updated this revision to Diff 514641.Apr 18 2023, 7:48 AM

Removed the part which isn't needed after D148441.

phosek accepted this revision.Apr 18 2023, 10:12 PM
phosek added a subscriber: phosek.

LGTM

This revision is now accepted and ready to land.Apr 18 2023, 10:12 PM
This revision was landed with ongoing or failed builds.Apr 19 2023, 3:37 AM
This revision was automatically updated to reflect the committed changes.