Page MenuHomePhabricator

ABI versioning macros for libc++
ClosedPublic

Authored by eugenis on Aug 3 2015, 4:31 PM.

Details

Summary

A few very basic macros that specify the default ABI version of the library, and can be overridden to pick up newer ABI-changing features.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 31285.Aug 3 2015, 4:31 PM
eugenis retitled this revision from to ABI versioning macros for libc++.
eugenis updated this object.
eugenis added reviewers: mclow.lists, EricWF, chandlerc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: cfe-commits.
compnerd added inline comments.
include/__config
32

This seems unused? The renaming for the major version would be unnecessary if this is not being added.

252

Whats the rationale for renaming this?

I'm trying to follow this proposal (which people seem to agree with in the general):
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-December/040587.html

Renaming _LIBCPP_ALTERNATE_STRING_LAYOUT is to give a unified naming scheme for all ABI-changing features, as proposed in the initial message in the above mail discussion.

EricWF edited edge metadata.Aug 18 2015, 7:03 PM

I don't think the tricky part of this patch is actually implementing the ABI macros. The tricky part is defining how libc++ should use the macros. Some questions I would like to see answered:

  1. How long is a major and minor ABI version supported?
  2. When is the major and minor ABI version bumped?
  3. How will maintaining multiple ABI versions affect code complexity and maintainability?

I would want to see documentation accompany this patch that anwsers some of these questions. It could we written in reStructured text after D12129 lands.

Some more questions:

  • Should bumping the ABI major version bump the SO version?
  • Should bumping the ABI major version change the include path from include/c++/v1 to include/c++/v2? What kind of clang support do we need to do this?
eugenis updated this revision to Diff 33246.Aug 26 2015, 2:15 PM
eugenis edited edge metadata.
eugenis removed rL LLVM as the repository for this revision.

Introduced cmake options for specifying the desired ABI version.
ABI version affects library soname and include path (include/c++/vN).
Baked ABI version into the headers (autogenerated __config_version) so that, ex. with -I/usr/include/c++/v2 you get major abi version=2 w/o additional -D settings.

Clang support for selecting vN include path is coming as a separate change.

How long is a major and minor ABI version supported?

We don't want to bump major version too often, and we want to support both +1 and -1 of the current major version, along with all possible minor versions.

When is the major and minor ABI version bumped?

See Abi.rst for very brief description.

How will maintaining multiple ABI versions affect code complexity and maintainability?

I'm not sure how to answer this. Depends on the feature in question. The logic of selecting features based on the ABI version number should not cause any maintenance burden by itself.

Should bumping the ABI major version bump the SO version?
Should bumping the ABI major version change the include path from include/c++/v1 to include/c++/v2? What kind of clang support do we need to do this?

I've implemented both. We might not need this support now, but it could be useful at the point of v1->v2 migration.

Thanks for doing all of this work. It's really appreciated.

First, I don't really think we should have the notion of a "minor" ABI version. It will be hard enough to maintain 2 major versions and I don't understand what a minor ABI version buys us.

In my opinion libc++ should support the following ABI configurations:

  1. Previous: The previous major ABI version.
  2. Default: The current major ABI version.
  3. Unstable: The current major ABI version + all pending ABI breaking changes.

I would like to see the ABI minor version replaced with _LIBCPP_ABI_UNSTABLE macro that @mclow.lists originally proposed.

Thanks for doing all of this work. It's really appreciated.

First, I don't really think we should have the notion of a "minor" ABI version. It will be hard enough to maintain 2 major versions and I don't understand what a minor ABI version buys us.

Woops, I realize now that I suggested a minor version originally. My apologies for being misleading.

One big problem with this patch is that it prevents the libc++ headers in libcxx/include from being used directly. This restriction seems artificial to me. If you want the "default" ABI configuration then you should be able to use the headers in the source tree. Only if you want some custom configuration (either the previous ABI version or _LIBCPP_ABI_UNSTABLE) should you have to use the headers in the build directory. In this case we should use the mechanism in D11963 (whatever that ends up being after review) and not add a __config_version.

Yes, not being able to use headers in the libcxx source tree is quite unpleasant. It can be fixed by providing a config_version in libcxx/include with the default version values. Or, in the approach of D11963, do something smart in config to default to the right version numbers.

Why do we need _LIBCPP_ABI_UNSTABLE at all? How is it different from setting LIBCPP_ABI_MAJOR_VERSION to the current default version + 1?

Yes, not being able to use headers in the libcxx source tree is quite unpleasant. It can be fixed by providing a config_version in libcxx/include with the default version values. Or, in the approach of D11963, do something smart in config to default to the right version numbers.

I'm not sure what you mean by "smart" because IMO D11963 is pretty dumb, but I would like to see __config have a default value for _LIBCPP_ABI_VERSION wrapped in a #ifndef _LIBCPP_ABI_VERSION.

Why do we need _LIBCPP_ABI_UNSTABLE at all? How is it different from setting LIBCPP_ABI_MAJOR_VERSION to the current default version + 1?

Interesting question. I'm think trying to draw a distinction between the stable ABI versions and unversioned ABI changes that are currently being staged for the next release. My main concern is that using default version + 1 to stage future changes is that it could look like that is a "stable" ABI configuration.

Yes, not being able to use headers in the libcxx source tree is quite unpleasant. It can be fixed by providing a config_version in libcxx/include with the default version values. Or, in the approach of D11963, do something smart in config to default to the right version numbers.

I'm not sure what you mean by "smart" because IMO D11963 is pretty dumb, but I would like to see __config have a default value for _LIBCPP_ABI_VERSION wrapped in a #ifndef _LIBCPP_ABI_VERSION.

Yes, that.

Why do we need _LIBCPP_ABI_UNSTABLE at all? How is it different from setting LIBCPP_ABI_MAJOR_VERSION to the current default version + 1?

Interesting question. I'm think trying to draw a distinction between the stable ABI versions and unversioned ABI changes that are currently being staged for the next release. My main concern is that using default version + 1 to stage future changes is that it could look like that is a "stable" ABI configuration.

OK. Then _LIBCPP_ABI_UNSTABLE won't bump the ABI version (as seen in library soname and header path)?

OK. Then _LIBCPP_ABI_UNSTABLE won't bump the ABI version (as seen in library soname and header path)?

Yeah. That was how I imagined it.

There is a bit of a problem with testing libc++. If the library is built for the non-default ABI, we can not build tests against headers in libc++ source. And the headers in the build directory are only updated when cmake is re-run. I guess the latter can be fixed by updating the headers with a custom_target instead of a file(COPY).

There is a bit of a problem with testing libc++. If the library is built for the non-default ABI, we can not build tests against headers in libc++ source. And the headers in the build directory are only updated when cmake is re-run. I guess the latter can be fixed by updating the headers with a custom_target instead of a file(COPY).

Yuck. I'll fix that in D11963.

eugenis updated this revision to Diff 33381.Aug 27 2015, 5:34 PM

Remove minor version, added abi_unstable,.
Keeping __config_version until the other change lands.
Tests use headers from the build directory.

FYI I'm focusing on getting D11963 in so that it stops blocking this patch.

FYI I'm focusing on getting D11963 in so that it stops blocking this patch.

eugenis updated this revision to Diff 37015.Oct 9 2015, 6:21 PM
eugenis set the repository for this revision to rL LLVM.

Rebased on D13407

EricWF accepted this revision.Oct 11 2015, 5:35 PM
EricWF edited edge metadata.

LGTM but this is still blocked by D13407. Ill let you know once that has landed.

include/__config
251

Is this the last remaining reference to _LIBCPP_ALTERNATIVE_STRING_LAYOUT?
If so please leave a note about how this is the old name and is used here to be backward compatible.

252

I think he's demonstrating the patches functionality and intended usage by converting an existing option. Future ABI options should probably start with the macro prefix '_LIBCPP_ABI' anyway.

This revision is now accepted and ready to land.Oct 11 2015, 5:35 PM

D13407 has landed and this is good to go.

eugenis updated this revision to Diff 37293.Oct 13 2015, 3:33 PM
eugenis edited edge metadata.
eugenis marked an inline comment as done.
eugenis added inline comments.
include/__config
252

Exactly.

Please address the inline comment. I think with that change we can hold off modifying libc++abi.

test/libcxx/test/config.py
444

Please allow abi_version to be optional before committing. IE

abi_version = self.get_lit_conf('abi_version',  None)
if abi_version is not None:
  self.cxx.compile_flags += ['-D_LIBCPP_ABI_VERSION=' + abi_version']

That should allow us to put off the changes to libc++abi.

jroelofs added inline comments.
include/__config_site.in
13

This doesn't look right to me. What do you want this to expand to when it is defined, and what do you want it to expand to when it is not defined?

jroelofs added inline comments.Oct 13 2015, 4:17 PM
include/__config_site.in
13

Never mind. After re-reading the cmake docs, I see what it's supposed to do.

eugenis added inline comments.Oct 13 2015, 4:36 PM
test/libcxx/test/config.py
444

OK. This would hold while ABI version is 1.

eugenis updated this revision to Diff 37299.Oct 13 2015, 4:39 PM

LGTM. We'll fix the libc++abi issue later.

eugenis closed this revision.Oct 13 2015, 5:29 PM

Landed as r250254, thanks for the review!