Page MenuHomePhabricator

[clang] Add -fc++-abi= flag for specifying which C++ ABI to use
ClosedPublic

Authored by leonardchan on Aug 11 2020, 6:23 PM.

Details

Summary

This implements the flag proposed in RFC http://lists.llvm.org/pipermail/cfe-dev/2020-August/066437.html.

The goal is to add a way to override the default target C++ ABI through a compiler flag. This makes it easier to test and transition between different C++ ABIs through compile flags rather than build flags.

In this patch:

  • Store -fc++-abi= in a LangOpt. This isn't stored in a CodeGenOpt because there are instances outside of codegen where Clang needs to know what the ABI is (particularly through ASTContext::createCXXABI), and we should be able to override the target default if the flag is provided at that point.
  • Expose the existing ABIs in TargetCXXABI as values that can be passed through this flag.
    • Create a .def file for these ABIs to make it easier to check flag values.
    • Add an error for diagnosing bad ABI flag values.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
leonardchan edited the summary of this revision. (Show Details)Aug 11 2020, 6:23 PM
hansec removed a reviewer: hansec.Aug 12 2020, 6:25 AM

I think I was added to this in error.

mcgrathr added inline comments.Aug 12 2020, 11:25 AM
clang/include/clang/Basic/TargetCXXABI.h
42

The option UI should use lowercase values by default, or else just do case-insensitive matching.

clang/lib/Driver/ToolChains/Fuchsia.cpp
274 ↗(On Diff #284944)

This should match "fuchsia", either only lowercase or case-insensitive. IMHO it seems wise to handle this in some way such that individual tests like this are not free-form string comparisons that could have typos, but test the TargetCXXABI enum value after decoding from string.

This should be a separate change. We'll need to do staged roll-out, first of a compiler where -fc++abi=compat (or itanium whatever it's called) is available (complete with multilibs et al), and then later of a compiler where -fc++abi=fuchsia (and the default for *-fuchsia targets) has changed meaning.

275 ↗(On Diff #284944)

It's surprising to me that this is the way to do this. Isn't there code in the actual front end that tests the TargetCXXABI value? That seems like the place where it makes sense to have Fuchsia imply specific settings, rather than in the driver.

phosek added inline comments.Aug 12 2020, 12:54 PM
clang/lib/Driver/ToolChains/Fuchsia.cpp
275 ↗(On Diff #284944)

Couldn't we make this the default in FuchsiaCXXABI (akin to HasThisReturn)?

leonardchan marked 2 inline comments as done.
leonardchan edited the summary of this revision. (Show Details)
  • Check for lowercase flag values
  • Remove fuchsia-specific changes
clang/lib/Driver/ToolChains/Fuchsia.cpp
275 ↗(On Diff #284944)

It's surprising to me that this is the way to do this. Isn't there code in the actual front end that tests the TargetCXXABI value? That seems like the place where it makes sense to have Fuchsia imply specific settings, rather than in the driver.

Yeah, this was just some confusion on my part but we addressed this offline. Removing this specific part for now since we'll just want to land the flag in this patch. In a future patch when we want to change the meaning behind -fc++-abi=fuchsia, I'll probably add something like TargetCXXABI::canUseRelVTables() which checks the ABI kind instead of the flag directly.

Couldn't we make this the default in FuchsiaCXXABI (akin to HasThisReturn)?

Right now, relative vtables are enabled through https://github.com/llvm/llvm-project/blob/62ef1cb2079123b86878e4bfed3c14db448f1373/clang/lib/AST/ASTContext.cpp#L10652, so we can't explicitly turn this on though FuchsiaCXXABI in codegen, but down the line we can update the condition to also make it the default in for the Fuchsia enum kind. The reason why it's not exclusively controlled in codegen is because there are parts outside of codegen where we need to tweak Itanium, specifically VCallAndVBaseOffsetBuilder which exists in the AST level and we need to modify to properly calculate vtable offsets.

Store a TargetCXXABI::Kind instead of a string in LangOpts.

MaskRay added inline comments.
clang/include/clang/Basic/LangOptions.h
337

This is usually represented with llvm::Optional

clang/lib/Driver/ToolChains/Clang.cpp
5325

See A->render

leonardchan marked 2 inline comments as done.

*ping* Any objections to this?

phosek accepted this revision.Oct 12 2020, 5:14 PM

LGTM

clang/include/clang/Basic/LangOptions.h
337

I'd call it just CXXABI which matches the other variables.

clang/lib/Frontend/CompilerInvocation.cpp
4019

Other flags like -funwindlib use either empty string or "platform", perhaps we should support the same for -fc++-abi?

This revision is now accepted and ready to land.Oct 12 2020, 5:14 PM
leonardchan marked an inline comment as done.Oct 13 2020, 11:47 AM
leonardchan added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
4019

Would an empty string already imply the platform default ABI? If so, I believe this already handles that case in ASTContext::getCXXABIKind when getting the ABI kind.

This revision was landed with ongoing or failed builds.Oct 14 2020, 12:31 PM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Oct 14 2020, 4:07 PM

I'd like to point out that we used to have a very similar flag, but we removed it back in 2014: http://reviews.llvm.org/D2545

Are you sure you need all the flexibility that this flag allows? For example, this will let users ask for the MSVC C++ ABI on Linux. I really don't want to support the mips, wasm, microsoft, or ios C++ ABI on arbitrary targets, and I don't want to have to teach clang to diagnose all the unsupported ways users can use this flag. The smaller we can make the space of options, the better, and the less conditional soup we'll have in the future.

In D85802#2331290, @rnk wrote:

I'd like to point out that we used to have a very similar flag, but we removed it back in 2014: http://reviews.llvm.org/D2545

Are you sure you need all the flexibility that this flag allows? For example, this will let users ask for the MSVC C++ ABI on Linux. I really don't want to support the mips, wasm, microsoft, or ios C++ ABI on arbitrary targets, and I don't want to have to teach clang to diagnose all the unsupported ways users can use this flag. The smaller we can make the space of options, the better, and the less conditional soup we'll have in the future.

Yeah I see how this can lead to a lot of complexity. Perhaps we can add some mechanism in Clang that determines which ABIs are supported for specific platforms? That is, when targetting Linux, Clang will diagnose an error if using -fc++-abi=microsoft. This could cover the case where a specific platform can support multiple ABIs, but those ABIs won't be supported by other platforms.

rnk added a comment.Oct 15 2020, 10:37 AM

Perhaps we can add some mechanism in Clang that determines which ABIs are supported for specific platforms? That is, when targetting Linux, Clang will diagnose an error if using -fc++-abi=microsoft. This could cover the case where a specific platform can support multiple ABIs, but those ABIs won't be supported by other platforms.

Right, but the simplest code is the code that doesn't exist: if the option doesn't exist, there's no need to diagnose anything. The rest of the "C++ ABIs" aren't really their own C++ ABIs, they are just a reflection of the target triple.

What is the actual use case? I read the RFC, but ever since I became a manager, my reading comprehension level dropped back to grade school, and I apologize for that. As I understood it, you need a flag that enables all the Itanium ABI extensions (relative vtables, and others) used in Fuchsia, at once. A single relative vtable ABI flag isn't sufficient (would it be?).

In D85802#2332766, @rnk wrote:

Perhaps we can add some mechanism in Clang that determines which ABIs are supported for specific platforms? That is, when targetting Linux, Clang will diagnose an error if using -fc++-abi=microsoft. This could cover the case where a specific platform can support multiple ABIs, but those ABIs won't be supported by other platforms.

Right, but the simplest code is the code that doesn't exist: if the option doesn't exist, there's no need to diagnose anything. The rest of the "C++ ABIs" aren't really their own C++ ABIs, they are just a reflection of the target triple.

What is the actual use case? I read the RFC, but ever since I became a manager, my reading comprehension level dropped back to grade school, and I apologize for that. As I understood it, you need a flag that enables all the Itanium ABI extensions (relative vtables, and others) used in Fuchsia, at once. A single relative vtable ABI flag isn't sufficient (would it be?).

We want to use Fuchsia C++ ABI by default when targeting Fuchsia. Fuchsia C++ ABI is a derivate of Itanium C++ ABI with various performance optimizations (similar to other derivative C++ ABIs like WebAssembly), and we plan on further evolving it, for example always using relative vtables. The problem is that we occasionally need to integrate code built with Clang with code produced by other compilers that don't support Fuchsia C++ ABI like GCC, so we would like to have an option to switch back to Itanium C++ ABI as the "legacy compatible" C++ ABI.

rnk added a comment.Oct 15 2020, 10:58 AM

Would "f[no-]fuchsia-c++-abi-extensions" (or shorter, -ffuchsia-c++-abi) do the trick? I know it doesn't map well onto our current internal option representation, but I don't think the internal representation is particularly good. I'd rather limit the user-visible driver interface to give us the flexibility to change the internal representation in the future.

In D85802#2332808, @rnk wrote:

Would "f[no-]fuchsia-c++-abi-extensions" (or shorter, -ffuchsia-c++-abi) do the trick? I know it doesn't map well onto our current internal option representation, but I don't think the internal representation is particularly good. I'd rather limit the user-visible driver interface to give us the flexibility to change the internal representation in the future.

For our use case yes, we could have -f[no-]fuchsia-c++-abi which would be enabled by default when the target is Fuchsia.

In D85802#2332808, @rnk wrote:

Would "f[no-]fuchsia-c++-abi-extensions" (or shorter, -ffuchsia-c++-abi) do the trick? I know it doesn't map well onto our current internal option representation, but I don't think the internal representation is particularly good. I'd rather limit the user-visible driver interface to give us the flexibility to change the internal representation in the future.

For our use case yes, we could have -f[no-]fuchsia-c++-abi which would be enabled by default when the target is Fuchsia.

Will send out another patch that will use this instead. We can probably revert this in the meantime.

In D85802#2332808, @rnk wrote:

Would "f[no-]fuchsia-c++-abi-extensions" (or shorter, -ffuchsia-c++-abi) do the trick? I know it doesn't map well onto our current internal option representation, but I don't think the internal representation is particularly good. I'd rather limit the user-visible driver interface to give us the flexibility to change the internal representation in the future.

For our use case yes, we could have -f[no-]fuchsia-c++-abi which would be enabled by default when the target is Fuchsia.

Will send out another patch that will use this instead. We can probably revert this in the meantime.

Have you considered using a different target triple for the different C++ ABI modes?

In D85802#2332808, @rnk wrote:

Would "f[no-]fuchsia-c++-abi-extensions" (or shorter, -ffuchsia-c++-abi) do the trick? I know it doesn't map well onto our current internal option representation, but I don't think the internal representation is particularly good. I'd rather limit the user-visible driver interface to give us the flexibility to change the internal representation in the future.

For our use case yes, we could have -f[no-]fuchsia-c++-abi which would be enabled by default when the target is Fuchsia.

Will send out another patch that will use this instead. We can probably revert this in the meantime.

Have you considered using a different target triple for the different C++ ABI modes?

We didn't but that might be even better than introducing a custom flag. I see that Itanium is already in the list of environments so we could most likely just use that.

rnk added a comment.Oct 22 2020, 3:29 PM

Thanks, I like the new direction, hopefully others agree. Sorry for providing slow feedback.

leonardchan reopened this revision.Apr 20 2021, 4:43 PM

Re-opening this based on discussions in D93668. The consensus is that this is OK as long as the compiler knows which ABIs work on which targets and rejects the unsupported configurations. (Still need to update this.)

This will also be used as a driver flag for selecting multilibs as part of the Fuchsia toolchain.

This revision is now accepted and ready to land.Apr 20 2021, 4:43 PM
leonardchan planned changes to this revision.Apr 20 2021, 4:43 PM
jansvoboda11 added inline comments.Apr 21 2021, 12:20 AM
clang/lib/Frontend/CompilerInvocation.cpp
4025

The original command-line arguments must be generated from this in GenerateLangArgs. See https://clang.llvm.org/docs/InternalsManual.html#compiler-invocation for more details.

This revision is now accepted and ready to land.Apr 21 2021, 3:05 PM

I got LGTM for this before, but I'll leave it open for a few days to address any new comments folks might have.

clang/lib/Frontend/CompilerInvocation.cpp
4025

Could you clarify more on this? I'm guessing you mean I should use one of the Marshalling Options to set Opts.CXXABI, but none of them seem very fitting here. MarshallingInfoEnum looks to be the most appropriate, but I think using it will require manually copying all the string and enum values from TargetCXXABI.def. If possible, I'd like to avoid hardcoding the enums and strings in TargetCXXABI.def elsewhere so when another ABI gets added, we'd only need to change that .td file and not multiple other places.

jansvoboda11 requested changes to this revision.EditedApr 22 2021, 1:53 AM

The fact that tests pass in assert builds without the argument generation suggests the feature doesn't have sufficient test coverage. Currently, the flag will be dropped during CompilerInvocation command-line round-trip and won't have any effect.

The test in clang/test/Frontend/invalid-cxx-abi.cpp tests only the driver side and I don't think that's good enough. Can you please add a test that actually hits -cc1 and checks the flag has the desired behavior?

More context: https://lists.llvm.org/pipermail/cfe-dev/2021-February/067714.html

clang/lib/Frontend/CompilerInvocation.cpp
4025

Yeah, I don't think using the marshalling infrastructure here would be a net positive. I think the best course of action is to write the code manually in GenerateLangArgs. Something like:

if (Opts.CXXABI)
  GenerateArg(Args, OPT_fcxx_abi_EQ, TargetCXXABI::getSpelling(*Opts.CXXABI), SA);
This revision now requires changes to proceed.Apr 22 2021, 1:53 AM
leonardchan marked an inline comment as done.

The fact that tests pass in assert builds without the argument generation suggests the feature doesn't have sufficient test coverage. Currently, the flag will be dropped during CompilerInvocation command-line round-trip and won't have any effect.

The test in clang/test/Frontend/invalid-cxx-abi.cpp tests only the driver side and I don't think that's good enough. Can you please add a test that actually hits -cc1 and checks the flag has the desired behavior?

More context: https://lists.llvm.org/pipermail/cfe-dev/2021-February/067714.html

I added cc1 invocations to the test and an extra one to assert that the flag is actually making it to codegen. I'm still unsure though how GenerateLangArgs should work though. Prior to this, I was still able to assert through both a driver and cc1 invocation that I get the desired behavior in a debug build with assertions enabled.

jansvoboda11 accepted this revision.Apr 23 2021, 4:25 AM

I added cc1 invocations to the test and an extra one to assert that the flag is actually making it to codegen.

My point wasn't that you were using %clang in your tests instead of %clang_cc1. My point was that you were only checking that the driver accepts correct arguments and rejects invalid arguments, but were not checking that the argument actually influences codegen. This LGTM now though.

I'm still unsure though how GenerateLangArgs should work though.

There are use-cases in the compiler (e.g. dependency scanning), where we need to generate command-line from an instance of CompilerInvocation. This means that each function parsing command-line arguments (ParseLangArgs) needs to have a counterpart that does the opposite (GenerateLangArgs).

We ensure all -cc1 options can be generated this way by doing a round-trip in assert builds: command-line => ParseLangArgs => CompilerInvocation => GenerateLangArgs => command-line => ParseLangArgs => CompilerInvocation.

If some option doesn't get generated in GenerateLangArgs, it won't make it to the second CompilerInvocation which is used by the rest of the compiler. If we have good tests that verify the option actually does what it's supposed to, they will fail and we can fix the omission.

Prior to this, I was still able to assert through both a driver and cc1 invocation that I get the desired behavior in a debug build with assertions enabled.

That's odd. Does your CMakeCache.txt file have CLANG_ROUND_TRIP_CC1_ARGS:BOOL=OFF by any chance? It's possible it didn't get switched to ON when D97462 landed.

With the way the tests are written now, I'd expect them to fail in assert build without the code generating -fcxx-abi=.

This revision is now accepted and ready to land.Apr 23 2021, 4:25 AM

I don't see any checks to ensure that the selected ABI is compatible with the specified target, is that something you plan on implementing in a follow up change?

That's odd. Does your CMakeCache.txt file have CLANG_ROUND_TRIP_CC1_ARGS:BOOL=OFF by any chance? It's possible it didn't get switched to ON when D97462 landed.

With the way the tests are written now, I'd expect them to fail in assert build without the code generating -fcxx-abi=.

Ah, I did not have that flag ON by default (incrementally working off an old invocation), and turning this on without the GenerateArg bit causes cxx-abi-switch.cpp to fail. Thanks for catching this!

I don't see any checks to ensure that the selected ABI is compatible with the specified target, is that something you plan on implementing in a follow up change?

Added a check for this.

That's odd. Does your CMakeCache.txt file have CLANG_ROUND_TRIP_CC1_ARGS:BOOL=OFF by any chance? It's possible it didn't get switched to ON when D97462 landed.

With the way the tests are written now, I'd expect them to fail in assert build without the code generating -fcxx-abi=.

Ah, I did not have that flag ON by default (incrementally working off an old invocation), and turning this on without the GenerateArg bit causes cxx-abi-switch.cpp to fail. Thanks for catching this!

Great, thanks for confirming.

phosek added inline comments.Apr 28 2021, 12:01 AM
clang/lib/Frontend/CompilerInvocation.cpp
4008

I'd consider extracting this into a method.

4011–4016

These should probably be only supported on Apple platforms?

4019

I think you should be checking OS here, not the architecture since we want to use the same ABI across all architectures.

leonardchan updated this revision to Diff 341280.
leonardchan marked 3 inline comments as done.

Will submit this later today unless there's any more comments.

phosek accepted this revision.May 3 2021, 6:20 PM

LGTM

echristo added inline comments.
clang/test/Frontend/invalid-cxx-abi.cpp
1

Following up on an offline conversation :)

This should be a driver test rather than (what it ended up being) as a full end to end test including code generation. You'll just want to check that the clang command line is handled and translated, etc. It'll also let you keep some of the more elaborate target triples that ended up being removed. Note that the cc1 tests are also doing this so need to be changed as well.

I'd probably also suggest removing the not clang ones and putting those into a different file, but that's largely stylistic.

MTC added a subscriber: MTC.May 6 2021, 7:00 PM