This is an archive of the discontinued LLVM Phabricator instance.

[clang][Fuchsia] Turn on relative-vtables by default for Fuchsia
ClosedPublic

Authored by leonardchan on May 12 2021, 3:15 PM.

Details

Summary

All fuchsia targets will now use the relative-vtables ABI by default. Also remove -fexperimental-relative-c++-abi-vtables from test RUNs targeting fuchsia.

Diff Detail

Event Timeline

leonardchan created this revision.May 12 2021, 3:15 PM
leonardchan requested review of this revision.May 12 2021, 3:15 PM
leonardchan planned changes to this revision.
leonardchan edited the summary of this revision. (Show Details)May 12 2021, 3:19 PM
phosek added inline comments.May 12 2021, 11:17 PM
clang/lib/Frontend/CompilerInvocation.cpp
3296

Could we instead enable it inside FuchsiaCXXABI?

leonardchan added inline comments.May 13 2021, 11:09 AM
clang/lib/Frontend/CompilerInvocation.cpp
3296

So we can't necessarily turn this on in just FuchsiaCXXABI/CodeGen because many of the vtable offsets and components are constructed in the AST stage which is later consumed by codegen and the CXXABI classes to make the vtables. More specifically, it's the ItaniumVTableContext created in ASTContext::getVTableContext() in charge of calculating these offsets.

I put it here because controlling the flag seems like the easiest way to set a default value given the target, and it's available either during or before the AST stage.

Change the tablegen definition of -f[no-]experimental-relative-c++-abi-vtables from a BoolFOption to two separate flags so we can control the default value of the RelativeCXXABIVTables LangOption more explicitly. This is needed so we can set the default value of this option depending on the target.

Ready for reviews. Let me know if I should add other reviewers for the flag/option processing. This shouldn't impact how the experimental flag is used. It's just processing it that's different.

phosek added inline comments.May 13 2021, 7:06 PM
clang/include/clang/Basic/TargetCXXABI.h
69

I think that supports here is a bit misleading, I'd expect the return value to signal whether relative vtables C++ ABI is supported or not but not control the default. Maybe usesRelativeVTables?

leonardchan marked an inline comment as done.
phosek accepted this revision.May 14 2021, 12:37 PM

LGTM

This revision is now accepted and ready to land.May 14 2021, 12:37 PM
This revision was landed with ongoing or failed builds.Jun 1 2021, 3:46 PM
This revision was automatically updated to reflect the committed changes.