This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use the stdlib=<LIB> Lit feature instead of use_system_cxx_lib
ClosedPublic

Authored by ldionne on Mar 18 2023, 1:31 PM.

Details

Reviewers
Mordante
MaskRay
EricWF
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGed61d6a46611: [libc++] Use the stdlib=<LIB> Lit feature instead of use_system_cxx_lib
Summary

The use_system_cxx_lib Lit feature was only used for back-deployment
testing. However, one immense hole in that setup was that we didn't
have a proper way to test Apple's own libc++ outside of back-deployment,
which was embodied by the fact that we needed to define _LIBCPP_DISABLE_AVAILABILITY
when testing (see change in libcxx/utils/libcxx/test/params.py).

This led to the apple-system testing configuration not checking for
availability markup, which is obviously quite bad since the library
we ship actually has availability markup.

Using stdlib=<VENDOR>-libc++ instead to encode back-deployment restrictions
on tests is simpler and it makes it possible to naturally support tests
such as availability markup checking even in the tip-of-trunk Apple-libc++
configuration.

Diff Detail

Event Timeline

ldionne created this revision.Mar 18 2023, 1:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 18 2023, 1:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Mar 18 2023, 1:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 18 2023, 1:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 506317.Mar 18 2023, 1:36 PM

Trigger CI with parent diff.

Mordante accepted this revision.Mar 19 2023, 11:26 AM
Mordante added a subscriber: Mordante.

Thanks, I really like this patch! LGTM when the CI passes,

libcxx/utils/ci/apple-install-libcxx.sh
114

Is there a LLVM or rdar bug report we can link to?

libcxx/utils/libcxx/test/features.py
339

I see this modifies the same line again, but this time I agree with a separate patch.

MaskRay accepted this revision.Mar 20 2023, 9:58 AM
EricWF accepted this revision.Mar 20 2023, 11:47 AM
This revision is now accepted and ready to land.Mar 20 2023, 11:47 AM
ldionne updated this revision to Diff 508983.Mar 28 2023, 6:19 AM
ldionne marked 2 inline comments as done.

Add reference to bug

libcxx/utils/ci/apple-install-libcxx.sh
114

https://llvm.org/PR61762 I'll add it in the comment.

ldionne updated this revision to Diff 509153.Mar 28 2023, 3:07 PM

Rebase and fix a few missed tests.

This revision was landed with ongoing or failed builds.Mar 30 2023, 3:58 AM
This revision was automatically updated to reflect the committed changes.
bcain added a subscriber: bcain.Apr 25 2023, 5:47 AM

IIRC, before this change we could use use_system_cxx_lib with LLVM_LIT_ARGS to indicate "do not test the just-built libc++, instead test the libc++ build toolchain's libc++". Perhaps this is what is meant by "back deployment"?

Or possibly this change to use_system_cxx_lib happened in an earlier commit?

But in any case, it seems like that feature is no longer present because stdlib=llvm-libc++ can't distinguish between the one from the build toolchain and the just-built library, right? What's an appropriate/recommended way to make this sort of change?

...

But in any case, it seems like that feature is no longer present because stdlib=llvm-libc++ can't distinguish between the one from the build toolchain and the just-built library, right? What's an appropriate/recommended way to make this sort of change?

sorry - that was a confusing way to word my question - s/make this sort of change/test this way/