Page MenuHomePhabricator

Add markup for libc++ dylib availability
ClosedPublic

Authored by mehdi_amini on Apr 5 2017, 8:04 PM.

Details

Summary

Libc++ is used as a system library on macOS and iOS (amongst others). In order
for users to be able to compile a binary that is intended to be deployed to an
older version of the platform, clang provides the
availability attribute <https://clang.llvm.org/docs/AttributeReference.html#availability>_
that can be placed on declarations to describe the lifecycle of a symbol in the
library.

See docs/DesignDocs/AvailabilityMarkup.rst for more information.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini created this revision.Apr 5 2017, 8:04 PM
jroelofs added inline comments.
utils/libcxx/test/config.py
289 ↗(On Diff #94316)

Can you expand on what the FIXME here wants? Is there something that AvailabilityMarkup.rst doesn't cover?

316 ↗(On Diff #94316)

Why does the availability stuff clash with -verify?

363 ↗(On Diff #94316)

This line, and the one above it are the same. Is that intentional?

387 ↗(On Diff #94316)

Is it worth filtering out none and unknown, as they're often repeated, and you can't tell which part of the triple they came from?

Consider: arm-none-linux-gnueabi vs arm-none-none-eabi.

Or would it be better to include some marker in the features to say which part of the triple it came from, eg:

- with_system_cxx_lib=arch:arm
- with_system_cxx_lib=vendor:none
- with_system_cxx_lib=os:linux
- with_system_cxx_lib=sys:gnueabi
arphaman added inline comments.
include/__config
1160 ↗(On Diff #94316)

Redundant whitespace added?

utils/libcxx/test/config.py
358 ↗(On Diff #94316)

Typo: add_deployment_feature

mehdi_amini marked 5 inline comments as done.Apr 20 2017, 10:48 PM
mehdi_amini added inline comments.
utils/libcxx/test/config.py
289 ↗(On Diff #94316)

I wrote the FIXME before writing the doc. I added a link to the doc instead.

316 ↗(On Diff #94316)

The syntax verification phase isn't expecting too see compile errors from the availability.

358 ↗(On Diff #94316)

French style :)

363 ↗(On Diff #94316)

No, just bad copy/paste :)

387 ↗(On Diff #94316)

I like this idea!
It seems like orthogonal to the availability introduced here, so it'd belong to a separate patch I think.

mehdi_amini marked 4 inline comments as done.

Address comments from @jroelofs and @arphaman

jroelofs added inline comments.Apr 21 2017, 8:15 AM
utils/libcxx/test/config.py
387 ↗(On Diff #94316)

sounds fine to me!

@mclow.lists you mentioned on IRC you may have some suggestions to make this less noisy? Do you still plan to review?

@mclow.lists you mentioned on IRC you may have some suggestions to make this less noisy? Do you still plan to review?

dexonsmith accepted this revision.Apr 29 2017, 9:48 PM

This LGTM, and it's liable to bitrot if it hangs out here any longer. We can always iterate in tree if we find a better way to organize the markup and/or tests.

Eric and Marshall: do you have any objection to this being committed now, so we can get some bots up?

This revision is now accepted and ready to land.Apr 29 2017, 9:48 PM
This revision was automatically updated to reflect the committed changes.
bcain added a subscriber: bcain.Nov 27 2017, 3:06 PM

I think some of the XFAIL: availability may be wrong here. I'd submit a patch, but it's not clear to me what the appropriate fix is.

libcxx/trunk/utils/libcxx/test/config.py
400

This means that 'availability' is present if use_system_cxx_lib even on platforms other than OSX. It's not clear to me whether or not this was intended.

If this was intended then it's probably wrong to "XFAIL: availability" in some of these tests. This means that these tests went from PASS to XFAIL when "use_system_cxx_lib". If perhaps these tests cannot be used without just-built libcxx, then maybe it makes more sense to mark them "UNSUPPORTED: availability"?

thakis added inline comments.
libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/lit.local.cfg
1

Did you mean not in here? I don't understand why we want to skip this tests is availability annotations are present.

mehdi_amini marked an inline comment as done.Mar 11 2019, 9:13 AM
mehdi_amini added subscribers: ldionne, Bigcheese.
mehdi_amini added inline comments.
libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/lit.local.cfg
1

Thanks for looking into this, unfortunately I don't remember the context from 2 years ago. It may have been a typo indeed.

@ldionne and @Bigcheese may be able to help here!

ldionne added inline comments.Mar 11 2019, 9:39 AM
libcxx/trunk/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/lit.local.cfg
1

Hmm, I wasn't aware of this. I guess this is because shared_*** used to be marked as unconditionally unavailable, but that changed. I'll figure out when we can enable these tests.