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

Thu Apr 22 01:54:01 PDT 2021

jansvoboda11 requested changes to this revision.
jansvoboda11 added a comment.
This revision now requires changes to proceed.

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?

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3523
+    else
+      Opts.CXXABI = TargetCXXABI::getKind(CXXABI);
+  }
----------------
leonardchan wrote:
> jansvoboda11 wrote:
> > 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.
> 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.
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);
```

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85802/new/

https://reviews.llvm.org/D85802