This is an archive of the discontinued LLVM Phabricator instance.

[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

leonardchan created this revision.Aug 11 2020, 6:23 PM
leonardchan requested review of this revision.Aug 11 2020, 6:23 PM
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

phosek added inline comments.Oct 12 2020, 5:14 PM
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

FWIW, the existence of this flag and the TargetCXXABI abstraction (which predates the flag, but might otherwise be more simply factored/implemented with inheritance rather than switch tables) came up in discussion D135326 and D119051.

(eg: the RecordLayoutBuilders are split by Itanium and Microsoft - but there's loads of triple checks throughout including for C++ layout choices (eg: getBaseOrPreferredBaseAlignFromUnpacked, empty base handling, etc) that aren't captured by the TargetCXXABI abstraction)

& so the question was where to put this next C++ Itanium ABI fix which is currently applied to Itanium in general, except for PS, Darwin, and Clang ABI <= 15.

Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 3:33 PM
Herald added a subscriber: abrachet. · View Herald Transcript

Does Fuchsia still need this? If those experiments have stabilized, maybe we can just remove it. Also, it should probably have been a -cc1 flag in the first place.

Does Fuchsia still need this? If those experiments have stabilized, maybe we can just remove it. Also, it should probably have been a -cc1 flag in the first place.

We still use this for making gnu-compatible multilibs (that is, multilibs which use regular itanium vtables): https://github.com/llvm/llvm-project/blob/2c7b7eca85c2ccc9b0d5c65169213ce499652f92/clang/cmake/caches/Fuchsia-stage2.cmake#L200

AFAICT there doesn't seem to be a way where one can select the C++ ABI independently of the target platform.

Does Fuchsia still need this? If those experiments have stabilized, maybe we can just remove it. Also, it should probably have been a -cc1 flag in the first place.

We still use this for making gnu-compatible multilibs (that is, multilibs which use regular itanium vtables): https://github.com/llvm/llvm-project/blob/2c7b7eca85c2ccc9b0d5c65169213ce499652f92/clang/cmake/caches/Fuchsia-stage2.cmake#L200

Naive question, but what does it mean to target Fuschia but be gnu-compatible? (how would that be different than using whatever gnu OS (linux, etc) the other code was built for)

AFAICT there doesn't seem to be a way where one can select the C++ ABI independently of the target platform.

If it comes down to it, we can make this a Fuchsia-specific flag, so that Fuchsia + alternative C++ ABI is essentially a sort of subtarget. That way we don't have to support the arbitrary Cartesian product of OS + ABI.

With that said, while I don't know the details of ELF multilibs, presumably there has to be some kind of target-like differentiation between the slices so the linker can pick the appropriate one. So maybe it's better to just call GNU-compatible Fuchsia a different target environment, the same way we consider Win32-oriented and MinGW-oriented Windows to be different target environments ultimately running on the same kernel.

Naive question, but what does it mean to target Fuschia but be gnu-compatible? (how would that be different than using whatever gnu OS (linux, etc) the other code was built for)

(Probably poor wording on my part.) When I mention gnu-compatible here, I don't mean ABI compatible to the OS but ABI compatible to whatever another compiler might generate. For example, if you want to use gcc to compile some code targeting fuchsia, gcc won't emit relative vtables but if you want to link against stuff like libcxx, libunwind, etc. then you'd need versions of those libs that also target fuchsia but would use the "regular" vtable layout.

Naive question, but what does it mean to target Fuschia but be gnu-compatible? (how would that be different than using whatever gnu OS (linux, etc) the other code was built for)

(Probably poor wording on my part.) When I mention gnu-compatible here, I don't mean ABI compatible to the OS but ABI compatible to whatever another compiler might generate. For example, if you want to use gcc to compile some code targeting fuchsia, gcc won't emit relative vtables but if you want to link against stuff like libcxx, libunwind, etc. then you'd need versions of those libs that also target fuchsia but would use the "regular" vtable layout.

Oh, I didn't mean to refer to the GNU OS - but I was trying to understand what you meant by gnu-compatible. Does GCC have a fuschia target? I'm trying to understand what compiler you're trying to be compatible with.

(this sounds sort of like either some non-fuschia ABI or that you have two Fuschia ABIs? & starts to remind me of the idea I've mentioned from time to time of a "floating" ABI flag that just says "this is whatever you want to do so long as Clang is consistent with itself at the same revision" and then a non-floating ABI. So are you trying to be compatible with an older version of Clang's Fuschia support when you mean "gnu-compatible"?)

The C++ ABI is not part of the Fuchsia system ABI, nor what we call the "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ ABI they like. Only the backend ABI independent of language-specific issues is necessary to interoperate with other code on Fuchsia.

I suspect the Fuchsia project is not in fact volunteering to maintain a port of every imaginable C++ ABI to Fuchsia. (Many of these "ABIs" are specifically stuff like "the Itanium ABI as modified for iOS ARM64" and are not really portable anyway.) You may be interested in supporting multiple C++ ABI variants, but you've still got a list, and it's not even a very long list. So we can support this driver option, but we can lock it down to Fuchsia (or any other OSes in the future that want to support multiple ABIs), and we can lock down the options it allows.

I would like to know how you build multilib across C++ ABIs, though. My (admittedly weak) understanding was that ELF multilib support was just based on arch strings.

It's certainly correct that we envision each target having an explicit list of viable C++ ABIs to select from.

AIUI Clang has no generic multilib support but it's very patchily supported differently in different per-target drivers. The Fuchsia target support in the driver has explicit logic about the supported multilibs, which we also use for sanitizer configurations, -fno-exceptions, and such.

Okay. So Fuchsia multilib support uses a lot of more fine-grained options rather than being arch-driven?

Can you elaborate about "each target"? Were you anticipating this being a broader feature outside of Fuchsia, or does Fuchsia actually encompass multiple OS targets?

I suspect the Fuchsia project is not in fact volunteering to maintain a port of every imaginable C++ ABI to Fuchsia. (Many of these "ABIs" are specifically stuff like "the Itanium ABI as modified for iOS ARM64" and are not really portable anyway.) You may be interested in supporting multiple C++ ABI variants, but you've still got a list, and it's not even a very long list. So we can support this driver option, but we can lock it down to Fuchsia (or any other OSes in the future that want to support multiple ABIs), and we can lock down the options it allows.

I would like to know how you build multilib across C++ ABIs, though. My (admittedly weak) understanding was that ELF multilib support was just based on arch strings.

We use the multilib support that exists in the bootstrapping build:

https://github.com/llvm/llvm-project/blob/d8af31ecede0c54ec667ab687784149e806c9e4c/clang/cmake/caches/Fuchsia-stage2.cmake#L196
https://github.com/llvm/llvm-project/blob/d8af31ecede0c54ec667ab687784149e806c9e4c/clang/lib/Driver/ToolChains/Fuchsia.cpp#L266

Okay. So Fuchsia multilib support uses a lot of more fine-grained options rather than being arch-driven?

Yes, see https://github.com/llvm/llvm-project/blob/d8af31ecede0c54ec667ab687784149e806c9e4c/clang/lib/Driver/ToolChains/Fuchsia.cpp#L218

The multilib support is used pretty extensively by the GNU driver to match GCC, see https://github.com/llvm/llvm-project/blob/d8af31ecede0c54ec667ab687784149e806c9e4c/clang/lib/Driver/ToolChains/Gnu.cpp#L2624.

Fuchsia as far as I'm aware is the only target that uses multilib support to switch the C++ ABI (through the -fc++abi= flag).

Can you elaborate about "each target"? Were you anticipating this being a broader feature outside of Fuchsia, or does Fuchsia actually encompass multiple OS targets?

I believe that "each target" in that context meant targets other than Fuchsia.

For Fuchsia, I expect that we'll only ever need the Fuchsia C++ ABI (the default) and Itanium C++ ABI (for compatibility with code built with GCC). If GCC ever gains the support for Fuchsia C++ ABI, then we wouldn't need to support Itanium C++ ABI at all.

I'm quite sure that we will always want a means to select the original Itanium ABI. It's also quite likely that there will be future innovations in the Fuchsia C++ ABI and we'll go through migration periods of supporting additional variants and changing the default for Fuchsia targets.

My earlier comment referred to -fc++-abi generally: that each target would be expected to enable a specific set of selections available, the Fuchsia targets being the first to have a set defined.

It's certainly possible that other targets will want to support multiple C++ ABIs in the future, but it's okay for us to design things around the situation we've got today. A lot of these "ABIs" are just target-specific variants; if it simplifies code to just make ABI queries just be target queries instead of abstracting the target C++ ABI, and the resulting burden on targets with multiple supported ABIs is very small, that seems like an acceptable trade-off. If things change and we get a lot of targets asking to support multiple ABIs, there's no reason we can't revisit this decision.

The C++ ABI is not part of the Fuchsia system ABI, nor what we call the "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ ABI they like. Only the backend ABI independent of language-specific issues is necessary to interoperate with other code on Fuchsia.

Sure enough - but I'm still sort of confused by why the Fuschia Clang target/compiler needs more than one C++ ABI. What is it interoperating with? (GCC doesn't have a Fuschia target implemented, does it? So what's it mean to target the GCC C++ ABI? what is compiling the code that Fuschia is trying to interoperate with when Clang targets Fuschia with a non-default C++ ABI?)

The C++ ABI is not part of the Fuchsia system ABI, nor what we call the "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ ABI they like. Only the backend ABI independent of language-specific issues is necessary to interoperate with other code on Fuchsia.

Sure enough - but I'm still sort of confused by why the Fuschia Clang target/compiler needs more than one C++ ABI. What is it interoperating with? (GCC doesn't have a Fuschia target implemented, does it? So what's it mean to target the GCC C++ ABI? what is compiling the code that Fuschia is trying to interoperate with when Clang targets Fuschia with a non-default C++ ABI?)

When we use GCC we're using the generic ELF targets. I think it's sufficient for us to tell you that indeed we do want the option of multiple C++ ABIs to select from without justifying everything about our work to a Clang reviewer before we can proceed with meeting the requirements of our system.

The C++ ABI is not part of the Fuchsia system ABI, nor what we call the "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ ABI they like. Only the backend ABI independent of language-specific issues is necessary to interoperate with other code on Fuchsia.

Sure enough - but I'm still sort of confused by why the Fuschia Clang target/compiler needs more than one C++ ABI. What is it interoperating with? (GCC doesn't have a Fuschia target implemented, does it? So what's it mean to target the GCC C++ ABI? what is compiling the code that Fuschia is trying to interoperate with when Clang targets Fuschia with a non-default C++ ABI?)

When we use GCC we're using the generic ELF targets. I think it's sufficient for us to tell you that indeed we do want the option of multiple C++ ABIs to select from without justifying everything about our work to a Clang reviewer before we can proceed with meeting the requirements of our system.

Would the generic ELF target support in Clang be adequate to meet that requirement, then? (so Fuschia target could be the custom C++ ABI (& custom C ABI if you likee) and a generic ELF target could be used for GCC ELF compatibility) - then we wouldn't need any C++ ABI customizability?

The C++ ABI is not part of the Fuchsia system ABI, nor what we call the "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ ABI they like. Only the backend ABI independent of language-specific issues is necessary to interoperate with other code on Fuchsia.

Sure enough - but I'm still sort of confused by why the Fuschia Clang target/compiler needs more than one C++ ABI. What is it interoperating with? (GCC doesn't have a Fuschia target implemented, does it? So what's it mean to target the GCC C++ ABI? what is compiling the code that Fuschia is trying to interoperate with when Clang targets Fuschia with a non-default C++ ABI?)

When we use GCC we're using the generic ELF targets. I think it's sufficient for us to tell you that indeed we do want the option of multiple C++ ABIs to select from without justifying everything about our work to a Clang reviewer before we can proceed with meeting the requirements of our system.

Would the generic ELF target support in Clang be adequate to meet that requirement, then? (so Fuschia target could be the custom C++ ABI (& custom C ABI if you likee) and a generic ELF target could be used for GCC ELF compatibility) - then we wouldn't need any C++ ABI customizability?

No. All the aspects of the Fuchsia target not specific to C++ we still want done the same way when interoperating with this code. We're quite confident that what we want is a Fuchsia target with multiple options for the C++ ABI. If you don't want that flexibility to be available to other targets, then fine (though I think that's a poor choice, personally). Second-guessing everything about how we're organized our system and build is not a very helpful tactic in compiler reviews. It would be much appreciated if this review were constrained to how the compiler should work rather than what we should want to do.

The C++ ABI is not part of the Fuchsia system ABI, nor what we call the "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ ABI they like. Only the backend ABI independent of language-specific issues is necessary to interoperate with other code on Fuchsia.

Sure enough - but I'm still sort of confused by why the Fuschia Clang target/compiler needs more than one C++ ABI. What is it interoperating with? (GCC doesn't have a Fuschia target implemented, does it? So what's it mean to target the GCC C++ ABI? what is compiling the code that Fuschia is trying to interoperate with when Clang targets Fuschia with a non-default C++ ABI?)

When we use GCC we're using the generic ELF targets. I think it's sufficient for us to tell you that indeed we do want the option of multiple C++ ABIs to select from without justifying everything about our work to a Clang reviewer before we can proceed with meeting the requirements of our system.

Would the generic ELF target support in Clang be adequate to meet that requirement, then? (so Fuschia target could be the custom C++ ABI (& custom C ABI if you likee) and a generic ELF target could be used for GCC ELF compatibility) - then we wouldn't need any C++ ABI customizability?

No. All the aspects of the Fuchsia target not specific to C++ we still want done the same way when interoperating with this code.

What sort of aspects does that include? Those aspects wouldn't be respected by the other side of that ABI boundary, built with GCC, right?

We're quite confident that what we want is a Fuchsia target with multiple options for the C++ ABI. If you don't want that flexibility to be available to other targets, then fine (though I think that's a poor choice, personally). Second-guessing everything about how we're organized our system and build is not a very helpful tactic in compiler reviews. It would be much appreciated if this review were constrained to how the compiler should work rather than what we should want to do.

LLVM reviews do include design discussions about what's appropriate to include and support in the LLVM project. It's not about second guessing your choices, but about understanding them & discussing how they best fit (or don't) in the LLVM project.

jansvoboda11 removed a subscriber: jansvoboda11.

So, some context for how this came up/what I'm trying to address:

So I started here: D117616 (for the googlers playing at home, see: b/198958855 for the proximal motivation) - not packing non-POD members of packed structs.
That lead to D119051 (changing the Itanium definition of POD to allow defaulted special member functions, to match GCC)
Which included a discussion about where to implement this property (areDefaultedSMFStillPOD) - I guess I had it in TargetInfo initially, @rnk asked about whether I could move a related query (getTailPaddingUseRules) from TargetCXXInfo to TargetInfo to keep related things together.
I tried that refactor in D135326, chatted with @rjmccall about the TargetCXXABI abstraction and we were both confused/didn't know why the -fcxx-abi flag exists, though in favor of the TargetCXXABI abstraction existing, so that at least some things don't need to be duplicated between targets, but not that it should be varied within an operating system.
Which got me to this discussion here.

So these configurable dimensions are adding friction to development and risk targets not getting the ABI they probably want (for instance, Fuchsia's probably missed these GCC ABI fixes related to POD/packing). It's unclear which C++ ABI things need to go in the TargetCXXABI versus all the other stuff that's implemented in ItaniumCXXABI/MicrosoftCXXABI, etc. Though perhaps that can be addressed anyway, regardless of whether it's variable within an OS - though the latter does add to the confusion.

The way I'd picture this, with what I know at the moment, is that TargetCXXABI is only the parts of the ABI that are necessary during parsing (which explains why Itanium/MicrosoftCXXABI in clangCodeGen don't come up here - they're exclusive to codegen/don't impact the AST), and could be implemented as mixins to TargetInfo - different TargetInfos could inheret from different CXXABIs, but I guess they could compose to support this CXXABI-varying-within-OS, but complicates the interactions with the codegen CXXABI abstractions, I think.

It's awkward, I think, to have TargetCXXABI sort of have an ad-hoc target/abi enum inside it anyway - and maybe would be nice if it used inheritance/virtual functions the same as TargetInfo does. We could have an ItaniumTargetCXXABI that is whatever the purest form of that is we have at the moment, and parts could be overridden by targets that want particular subsets/old versions of that - though perhaps it'll be simple enough to say ClangVersionTargetCXXABI derives from ItaniumTargetCXXABI and overrides the few things based on the clang cxx-abi-version flag...

My understanding of the fuchsia situation is that there's a clang compiled project and the fuchsia floating ABI, but then a desire to build some code with GCC that doesn't implement that ABI - but GCC can't build everything (libc++ in particular) so you need to build that with clang, but targeting the GCC ABI. I would think/hope the way to address that would be to tell Clang you're targeting whatever OS GCC is targeting (though if I understand correctly that's a bit of a pain having to specify all the library paths, etc, from scratch like you have to do for GCC? But hopefully if you can do it for GCC you can do it for Clang in a similar way?). This might remove the need for Clang to support this use case & simplify Clang's already fairly complicated ABI handling & reduce the chance of divergence/ABI bugs being introduced to this complicated matrix of compatibility.