Page MenuHomePhabricator

[libc++abi] Use from-scratch testing configs for libc++abi by default
ClosedPublic

Authored by ldionne on May 9 2022, 8:57 AM.

Details

Summary

Like we have been doing for libc++ for a while now, start using
from-scratch testing configurations for libc++abi.

As a fly-by fix, remove the LIBCXXABI_NO_TIMER macro, which was defined
but never used.

Diff Detail

Event Timeline

ldionne created this revision.May 9 2022, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 8:57 AM
ldionne requested review of this revision.May 9 2022, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 8:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 430704.May 19 2022, 8:47 AM

Rebase and try to fix CI.

ldionne updated this revision to Diff 431678.May 24 2022, 7:53 AM
ldionne edited the summary of this revision. (Show Details)

Try to fix CI.

ldionne updated this revision to Diff 431737.May 24 2022, 11:20 AM

Rebase onto main -- I'm not seeing the CI issue locally, maybe rebasing will fix it?

EricWF accepted this revision.May 24 2022, 11:28 AM
EricWF added a subscriber: EricWF.

I'm not sure I understand what "from scratch" means given this patch, but I don't need to understand to see that to LGTM it. LGTM once the inline comments are addressed.

libcxxabi/test/libcxxabi/test/config.py
52

If we're using the same feature name for libc++ and libc++abi, I assume that means that they cannot vary independently.

If so, should we just use _LIBCPP_HAS_NO_THREADS instead, and kill the LIBCXXABI_HAS_NO_THREADS macro?

libcxxabi/test/test_exception_storage.pass.cpp
17

Is this the same test_macros.h header as libc++, or a different one?

libcxxabi/test/test_fallback_malloc.pass.cpp
17

This makes the test non-modular. Could you add an // XFAIL or UNSUPPORTED to that affect?

18

Doesn't the LIT config handle defining this macro?

This revision is now accepted and ready to land.May 24 2022, 11:28 AM
ldionne marked 4 inline comments as done.May 25 2022, 6:13 AM

I'm not sure I understand what "from scratch" means given this patch, but I don't need to understand to see that to LGTM it. LGTM once the inline comments are addressed.

Yeah, it's not a great name. It's how I've been calling the new style of configuration file where you define only the base Lit substitutions required for your environment instead of using the logic in config.py that tried to handle all configurations (and you'd add various knobs to tweak its behavior instead).

libcxxabi/test/libcxxabi/test/config.py
52

This is actually an intermediate state -- after this patch lands, I plan to rename libcpp-has-no-threads to just no-threads, like I've done for no-exceptions.

If so, should we just use _LIBCPP_HAS_NO_THREADS instead, and kill the LIBCXXABI_HAS_NO_THREADS macro?

I think this probably makes sense. I'm adding it to my TODO list for an upcoming patch.

libcxxabi/test/test_exception_storage.pass.cpp
17

It's the same. It does mean that TEST_HAS_NO_THREADS is defined based on _LIBCPP_HAS_NO_THREADS, if that's where you were driving. But in accordance with your comment above, libc++abi will be using the libc++ macro anyway in the future.

The underlying problem is that libc++abi doesn't have a way to persist its configuration state like libc++ (via __config_site). So we have to either pass it explicitly with -DLIBCXXABI_FOO, or piggy-back on the same state saved in libc++'s __config_site.

libcxxabi/test/test_fallback_malloc.pass.cpp
18

It doesn't, and I don't think it should, since this is effectively a macro that *should* only matter when compiling the libc++abi sources.

ldionne updated this revision to Diff 431960.May 25 2022, 6:14 AM
ldionne marked 3 inline comments as done.

Fix bootstrapping build CI failure.

EricWF added inline comments.May 26 2022, 6:57 AM
libcxxabi/test/libcxxabi/test/config.py
52

Ah, so _LIBCXXABI_HAS_NO_THREADS is handled by the old-style config but not the new one.

libcxxabi/test/test_fallback_malloc.pass.cpp
18

Are you sure. See the above added inline comment.