This is an archive of the discontinued LLVM Phabricator instance.

Add flag to request Clang is ABI-compatible with older versions of itself
ClosedPublic

Authored by rsmith on Aug 8 2017, 7:25 PM.

Details

Summary

This patch adds a flag -fclang-abi-compat that can be used to request that Clang attempts to be ABI-compatible with some older version of itself.

This is provided on a best-effort basis; right now, this can be used to undo the ABI change in r310401, reverting Clang to its prior C++ ABI for pass/return by value of class types affected by that change, and to undo the ABI change in r262688, reverting Clang to using integer registers rather than SSE registers for passing <1 x long long> vectors. The intent is that we will maintain this backwards compatibility path as we make ABI-breaking fixes in future.

Diff Detail

Event Timeline

rsmith created this revision.Aug 8 2017, 7:25 PM
rjmccall edited edge metadata.Aug 8 2017, 9:22 PM

Yeah, I think having an internal C++ ABI version makes a lot more sense than having a million different flags. Is there a reason to expose this as a knob to users at all?

Yeah, I think having an internal C++ ABI version makes a lot more sense than having a million different flags. Is there a reason to expose this as a knob to users at all?

I don't see any reason anyone would want to use this other than to restore the ABI to that of Clang <= 4, so an overall "clang ABI version" flag would seem reasonable to me.

probinson edited edge metadata.Aug 24 2017, 12:22 PM

Locally we have a couple different tactics for dealing with changes that we can't support. A more coherent approach would be great.
For example we defined a new TargetCXXABI::Kind value that is mostly GenericItaniumABI except where it isn't.
I personally did not do most of the various ABI-related tweaks so I don't claim to have a good handle on them, and we have been slow to get these things upstream; but I'd love to make that happen.

rsmith updated this revision to Diff 112634.Aug 24 2017, 4:17 PM
rsmith retitled this revision from add flag to undo ABI change in r310401 to Add flag to request Clang is ABI-compatible with older versions of itself.
rsmith edited the summary of this revision. (Show Details)
hans accepted this revision.Aug 24 2017, 5:52 PM
hans added a subscriber: hans.

I'm not familiar with the details of the ABI changes, but the mechanics of the code look good to me.

This revision is now accepted and ready to land.Aug 24 2017, 5:52 PM

I would prefer a diagnostic if PS4 && >3.2. Whether you map "latest" to 3.2 or diagnose that also, up to you, I'm okay either way.

I would prefer a diagnostic if PS4 && >3.2. Whether you map "latest" to 3.2 or diagnose that also, up to you, I'm okay either way.

On reflection, I don't think doing anything special for PS4 here is really correct. (Suppose Clang 5 has some (accidental) ABI change in it for PS4, and we fix that in Clang 6 to once again follow the psABI and do what Clang 3.2 did. In that case, building with -fclang-abi-compat=5 should theoretically reinstate that accidental ABI change.) The old-Clang compatibility mode and the platform ABI are separate, and we should not conflate them. New patch on the way.

The only thing I would say here is that, as a platform vendor, I would like the ability to opt in to ABI fixes that aren't in my base-line version. In particular, we're generally more aggressive about taking C++ ABI fixes on Darwin when there's a strong argument that the impact is likely to be low, like for some of these indirect-ABI cases that only make a difference for classes with e.g. only deleted copy/move constructors but a trivial destructor.

rsmith updated this revision to Diff 112733.Aug 25 2017, 1:15 PM
rsmith edited the summary of this revision. (Show Details)

The only thing I would say here is that, as a platform vendor, I would like the ability to opt in to ABI fixes that aren't in my base-line version. In particular, we're generally more aggressive about taking C++ ABI fixes on Darwin when there's a strong argument that the impact is likely to be low, like for some of these indirect-ABI cases that only make a difference for classes with e.g. only deleted copy/move constructors but a trivial destructor.

I don't think the idea of a base-line version matches with the direction of the patch, and I've removed the part of the patch that has a base-line Clang version for PS4. I think that if your platform ABI dictates that you do thing X differently from, say, the x86_64 psABI or the Itanium C++ ABI, then that's simply part of your platform ABI, and not a clang compatibility feature, regardless of whether the historical reason your platform ABI made that choice was due to clang compatibility.

probinson added inline comments.Aug 25 2017, 2:17 PM
lib/Driver/ToolChains/Clang.cpp
2939
else if (getToolChain().getTriple().isPS4())
  CmdArgs.push_back("-fclang-abi-compat=3.2");

Which lets us avoid piles of PS4 special cases all over everywhere.
Sony is historically very conservative about compatibility, and we'll be happier defaulting it this way. Setting platform-specific defaults in the driver seems to be pretty common already, this is just one more.

rsmith added inline comments.Aug 25 2017, 3:38 PM
lib/Driver/ToolChains/Clang.cpp
2939

I initially thought that made sense too, but I'm now fairly convinced that it doesn't.

Let's take Darwin as an example. There are some facets of Darwin's platform ABI that are determined by what old versions of Clang did, and other facets of its platform ABI that have changed to match changes to the x86_64 psABI and Itanium C++ ABI. But it's irrelevant where the platform ABI rules come from; the point is that there is some set of platform ABI rules that you could write down, and Clang attempts to implement those rules correctly by default.

The flag being added in this patch provides the ability to request that Clang does something else: that it produces code that is ABI-compatible with what a prior version of itself did when targeting that platform ABI. In particular, we fixed the C++ calling convention for certain rare class types in Clang 5 to conform to (an update to) the Itanium C++ ABI rules, and this patch allows that to be undone.

It seems to me that the situation for PS4 is exactly the same. There is some platform ABI that you could write down, and Clang attempts to be compatible with that by default. And it's irrelevant whether that's the ABI that Clang 3.2 used or the ABI of GCC 2.95; it's just the platform ABI. This is not a "be compatible with clang 3.2" mode, it is (as far as I can tell) the actual platform ABI.

Let me repeat an example I gave before: suppose Clang 5 has some (accidental) ABI change in it for PS4, and we fix that in Clang 6 to once again follow the platform ABI by doing what Clang 3.2 did. In that case, building with -fclang-abi-compat=5 should theoretically reinstate that accidental ABI change.

Hopefully that should clarify that this does *not* actually let us avoid PS4 special cases anywhere. ABI choices depend on both the platform ABI *and* on the version of Clang that we're providing compatibility with (if any).

That said, it's clearly not up to me what the PS4 platform ABI is. If you want to say that the PS4 platform ABI is actually something other than what Clang 3.2 does, but all object code on your system is compiled in a compatibility mode that diverges from the platform ABI and matches Clang 3.2, then I would concede that we can remove the PS4 platform special cases elsewhere and set a default here. But that sounds like a very strange decision to make, and it creates a horrible problem for the meaning of the -fclang-abi-compat flag: if someone in the future specifies -fclang-abi-compat=5 when targeting PS4, and Clang 5 by default set -fclang-abi-compat=3.2, then are we targeting what Clang 5 would have done by default or what Clang 5 would have done when told to be compatible with itself? As you can see, this default would create a lot of confusion.

probinson added inline comments.Aug 25 2017, 4:19 PM
lib/Driver/ToolChains/Clang.cpp
2939

On the other hand, if all the places that check ClangABICompat also check for PS4 and Darwin, then specifying -fclang-abi-compat while targeting PS4 or Darwin has no effect, and also no diagnostic. Which seems to make -fclang-abi-compat totally pointless. Is there a non-PS4/Darwin use-case for this flag?

rsmith added inline comments.Aug 25 2017, 4:38 PM
lib/Driver/ToolChains/Clang.cpp
2939

-fclang-abi-compat requests that we generate code that is ABI-compatible with a prior Clang version. The use case (for *any* platform) is that you have some code built with an earlier version of Clang that had a bug in its implementation of the platform ABI, and you want to generate more code that is ABI-compatible with it.

-fclang-abi-compat=4, for instance, has an effect on Darwin: it turns off the recent bug fix to the calling convention for passing trivially-copyable-but-not-trivially-moveable C++ class types. (But it does nothing on PS4, because -- with this patch -- Clang 5 does not change that aspect of the ABI for PS4.)

Sorry it took so long to wrap my head around this, but it has been about 20 years since I've had to accommodate an intentional ABI breakage, and that's actually what you want to do. Remembering that case, I have a model for this one, and I understand what you are doing now.

Be that as it may, -fclang-abi-compat is meaningless on PS4, because every time there's a case for checking ClangABICompat, PS4 will want to do it the 3.2 way regardless. And we'll just make those checks in all those places. At least it will be relatively easy to audit, and I'm sure our licensees will be happy to ignore the new flag.

So, I am convinced, and the patch LGTM.

rsmith closed this revision.Aug 25 2017, 6:05 PM

Committed as 311823.