[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:2934
+    ABICompatArg->render(Args, CmdArgs);
+
   // Add runtime flag for PS4 when PGO or Coverage are enabled.
----------------
probinson wrote:
> ```
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D36501