Page MenuHomePhabricator

Support for relative vtables
Needs ReviewPublic

Authored by leonardchan on Feb 15 2019, 11:49 PM.

Details

Summary

This is a revisit to D20749 which introduces a flag that enables the relative ABI described in https://bugs.llvm.org/show_bug.cgi?id=26723.

This is still a work in progress and am still in the process of debugging some issues I ran into when building Fuchsia with this, but wanted to raise some awareness and see if anyone has any thoughts or comments.

Diff Detail

Event Timeline

leonardchan created this revision.Feb 15 2019, 11:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 11:49 PM
leonardchan edited the summary of this revision. (Show Details)Feb 22 2019, 11:34 AM
leonardchan added reviewers: pcc, phosek, jakehehrlich.
pcc added a comment.Feb 22 2019, 11:44 AM

Can we start with a patch that just exposes a flag that enables the relative ABI unconditionally, and remove all the platform ABI compatibility stuff?

In D58321#1407362, @pcc wrote:

Can we start with a patch that just exposes a flag that enables the relative ABI unconditionally, and remove all the platform ABI compatibility stuff?

Done. Removed the checks requiring LTO, checks in checkClassABI, and relevant tests for them.

rjmccall added inline comments.Feb 25 2019, 9:51 PM
clang/include/clang/Driver/Options.td
1265

Please make this a -cc1 option or otherwise mark it clearly experimental.

Update to current working version without needing LTO

  • Remove hasHiddenLTOVisibility from CXXRecordDecl and rename the flag to -frelative-c++-abi-vtables

I think that you need to update the serialization code in Serialization/ASTReaderDecl.cpp and Serialization/ASTWriterDecl.cpp to account for the new flag.

leonardchan updated this revision to Diff 196180.EditedApr 22 2019, 10:13 PM
leonardchan added a subscriber: mcgrathr.

Uploading my latest version of this patch and reporting some results. Will proceed with formatting, cleanup, and addressing the existing comments and ask for official reviews later.


After toying around with this for a bit, just enabling this flag on all c++ code in zircon shows about a 1% size increase in the final image (x64.zbi), but does its job of moving many vtables from .data.rel.ro to rodata. For each shared library that's included in the image, some of them decrease while most increase in size. These changes though are pretty small and rounded up align to page sizes (4kB), resulting in the overall 1% increase. This is mostly from extra instructions and extra PLT entries, through we have an idea for how we could reduce the PLT size.

We still get the benefits of moving vtables into rodata and having them shared between processes. Below is the section dump for one of our shared libraries (omitting the debug sections):

Without relative ABI

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .note.gnu.build-id NOTE            0000000000000270 000270 000018 00   A  0   0  4
  [ 2] .dynsym           DYNSYM          0000000000000288 000288 000540 18   A  5   1  8
  [ 3] .gnu.hash         GNU_HASH        00000000000007c8 0007c8 000050 00   A  2   0  8
  [ 4] .dynamic          DYNAMIC         0000000000000818 000818 000170 10   A  5   0  8
  [ 5] .dynstr           STRTAB          0000000000000988 000988 000424 00   A  0   0  1
  [ 6] .rela.dyn         RELA            0000000000000db0 000db0 0000a8 18   A  2   0  8  # missing
  [ 7] .relr.dyn         00000013: <unknown> 0000000000000e58 000e58 000080 08   A  0   0  8
  [ 8] .rela.plt         RELA            0000000000000ed8 000ed8 000468 18   A  2  15  8
  [ 9] .rodata           PROGBITS        0000000000001340 001340 0025c8 00 AMS  0   0 16
  [10] .eh_frame_hdr     PROGBITS        0000000000003908 003908 000a1c 00   A  0   0  4
  [11] .eh_frame         PROGBITS        0000000000004328 004328 0032cc 00   A  0   0  8
  [12] .text             PROGBITS        0000000000008000 008000 014c1b 00  AX  0   0 16
  [13] .plt              PROGBITS        000000000001cc20 01cc20 000300 00  AX  0   0 16
  [14] .data.rel.ro      PROGBITS        000000000001d000 01d000 001948 00  WA  0   0 16
  [15] .got.plt          PROGBITS        000000000001e948 01e948 000190 00  WA  0   0  8

With relative ABI

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .note.gnu.build-id NOTE            0000000000000270 000270 000018 00   A  0   0  4
  [ 2] .dynsym           DYNSYM          0000000000000288 000288 000540 18   A  5   1  8
  [ 3] .gnu.hash         GNU_HASH        00000000000007c8 0007c8 000050 00   A  2   0  8
  [ 4] .dynamic          DYNAMIC         0000000000000818 000818 000140 10   A  5   0  8
  [ 5] .dynstr           STRTAB          0000000000000958 000958 000424 00   A  0   0  1
  [ 6] .relr.dyn         00000013: <unknown> 0000000000000d80 000d80 000068 08   A  0   0  8
  [ 7] .rela.plt         RELA            0000000000000de8 000de8 000480 18   A  2  14  8
  [ 8] .rodata           PROGBITS        0000000000001270 001270 002990 00 AMS  0   0 16
  [ 9] .eh_frame_hdr     PROGBITS        0000000000003c00 003c00 000a1c 00   A  0   0  4
  [10] .eh_frame         PROGBITS        0000000000004620 004620 0032cc 00   A  0   0  8
  [11] .text             PROGBITS        0000000000008000 008000 014ffb 00  AX  0   0 16
  [12] .plt              PROGBITS        000000000001d000 01d000 000310 00  AX  0   0 16
  [13] .data.rel.ro      PROGBITS        000000000001e000 01e000 0012d0 00  WA  0   0 16
  [14] .got.plt          PROGBITS        000000000001f2d0 01f2d0 000198 00  WA  0   0  8
  [15] .data             PROGBITS        0000000000020000 020000 000038 00  WA  0   0 16
  [16] .bss              NOBITS          0000000000020038 020038 000100 00  WA  0   0  8

Sections that grow are .rela.plt, .rodata, .text, .plt, and .got.plt whereas sections that shrink are .rela.dyn and .data.rel.ro.

We were also able to build libc++ with relative ABI and got ~2% decrease in size for the final shared library.

Other stuff in this patch:

  • Remove any LTO related code
  • Ensure needsRelocation() catches the relative offset function pointers
leonardchan marked an inline comment as done.
leonardchan retitled this revision from [WIP] Support for relative vtables to Support for relative vtables.

Ok. Formally requesting for code reviews now.

  • Made the flag an experimental flag.
  • Added changes to relevant AST Serialization code.
rjmccall added inline comments.May 1 2019, 10:54 AM
clang/include/clang/AST/DeclCXX.h
531 ↗(On Diff #197196)

Should we proactively generalize this as a "CXXABIVariant" enum, which for now can just be "Standard" and "RelativeVTables"?

Also, I don't want to pre-empt your secret plans, but if Fuchsia is just going to use this as its system C++ ABI, maybe we should plan for that, too.

clang/include/clang/AST/VTableBuilder.h
267

const VTableComponent &, I think.

clang/include/clang/Basic/LangOptions.def
330

Yeah, see, this plays into the question above. I would not want to provide this as a language option for general use. The attribute seems good enough for testing, and if you want a -cc1 option to apply the attribute by default for experimentation during Fuchsia bring-up that's fair, but I don't want something that suggests to users that it's okay to pass this attribute and change the system default.

clang/include/clang/Sema/Sema.h
10976 ↗(On Diff #197196)

Comment / method-name mismatch?

pcc added inline comments.May 1 2019, 11:15 AM
clang/include/clang/AST/DeclCXX.h
531 ↗(On Diff #197196)

At this point I probably would remove this bitfield entirely. This implementation does not support enabling the ABI on a per-class basis, so everywhere that we are currently checking this field we should just be able to check RelativeCXXABIVTables in LangOptions.

ormris added a subscriber: ormris.May 1 2019, 11:40 AM
dancol added a subscriber: dancol.May 14 2019, 3:03 PM
leonardchan marked 5 inline comments as done.
leonardchan added a reviewer: rjmccall.
leonardchan removed a subscriber: rjmccall.
leonardchan added inline comments.
clang/include/clang/AST/DeclCXX.h
531 ↗(On Diff #197196)

The goal for us is to just enable this for all vtables so we don't need this on a per-class basis. Removed the bitfield.

clang/include/clang/Basic/LangOptions.def
330

Ok. So is this the reason for the white list approach mentioned in D17893? As an alternative, would you also be ok with creating a FuchsiaCXXABI that subclasses ItaniumCXXABI, similar to the ARM and WebAssembly ones? This way it doesn't change the system default.

clang/include/clang/Sema/Sema.h
10976 ↗(On Diff #197196)

Removed this method since the bitfield we set here is removed.

rjmccall added inline comments.Wed, May 29, 9:36 PM
clang/include/clang/Basic/LangOptions.def
330

That design for relative v-tables was intended (at least as it was described to me) as an optimization for certain classes in a project that still needed to interoperate with system and other non-project C++ code. It was initially attempting to apply the optimization eagerly to all classes that didn't match a blacklist of classes (and namespaces of classes) that needed to follow the system ABI, which I objected to, which is why I asked for it to be redesigned around applying the optimization only to whitelisted classes/namespaces. None of that applies if you're changing the system ABI rule unless you decide you want to allow classes to opt into standard Itanium v-tables.

If introducing a FuchsiaCXXABI class makes the code easier, go for it.