leonardchan added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Fuchsia.cpp:275
+ if (DriverArgs.getLastArgValue(options::OPT_fcxx_abi_EQ) == "Fuchsia")
+ CC1Args.push_back("-fexperimental-relative-c++-abi-vtables");
}
----------------
phosek wrote:
> mcgrathr wrote:
> > 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.
> >
> Couldn't we make this the default in `FuchsiaCXXABI` (akin to
> `HasThisReturn`)?
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85802/new/
https://reviews.llvm.org/D85802
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
