[PATCH] D95694: [clang][RelativeVTablesABI] Place non-local vtables in comdat groups

[Leonard Chan via Phabricator via cfe-commits]

leonardchan created this revision.
leonardchan added reviewers: mcgrathr, phosek.
leonardchan added a project: clang.
Herald added a subscriber: pengfei.
leonardchan requested review of this revision.

Currently, it's possible for vtables that fit precisely in a mergable constants group to not be collected by `--gc-sections` even when `-fdata-sections` is enabled. This results in some vtables that would normally be garbage collected to persist after linking, causing extra bloat in rodata.

This happens because constants that do not need relocations according to `Constant::needsRelocation` have the option of being placed into these `cstN` sections. That is, a vtable would get placed into `.section .rodata.cst16` rather than something like `.section .rodata.{vtable}` under `-fdata-sections`, and `--gc-sections` would not collect the vtable in `.section .rodata.cst16`.

Note that this doesn't affect vtables under the default Itanium ABI that *could* fit into one of these mergeable sections but *would* get placed only into `.rodata`. Example:

  class A {
   public:
    virtual void func();
    virtual void func2();
  };
  
  void A::func() {}
  void A::func2() {}

compiled with `./bin/clang++ -c /tmp/test.cc -target x86_64-linux-gnu -fno-pic -fno-rtti -fdata-sections -S -o -` generates

  	.type	_ZTV1A, at object                  # @_ZTV1A
  	.section	.rodata._ZTV1A,"a", at progbits
  	.globl	_ZTV1A
  	.p2align	3
  _ZTV1A:
  	.quad	0
  	.quad	0
  	.quad	_ZN1A4funcEv
  	.quad	_ZN1A5func2Ev
  	.size	_ZTV1A, 32

The only reason this isn't placed into `.rodata.cst32` is because they need dynamic relocations according to `Constant::needsRelocation` (so they take a different branch <https://github.com/llvm/llvm-project/blob/d3e8b9fdc0de18ff92764f1be0a9b5a13fbbe9de/llvm/lib/Target/TargetLoweringObjectFile.cpp#L246> that relative vtables would), but because this is made with a static relocation model (non-PIC mode), the vtable can be safely placed in `.rodata`. This is a unique scenario that is only surfacing now from the relative vtables work.

To keep this contained under the realm of RV changes, a simple fix would be to place these kinds of vtables in comdat groups when appropriate, then `--gc-sections` can collect them appropriately.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95694

Files:
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CGVTables.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/RelativeVTablesABI/vtable-in-comdat.cpp


Index: clang/test/CodeGenCXX/RelativeVTablesABI/vtable-in-comdat.cpp
===================================================================
--- /dev/null
+++ clang/test/CodeGenCXX/RelativeVTablesABI/vtable-in-comdat.cpp
@@ -0,0 +1,24 @@
+// Check that all vtables under the relative ABI are placed in a comdat group.
+// This is necessary because if the vtable happens to fit in a mergable
+// constants section, then the LLVM backend will not place the symbol in its own
+// unique section even if -fdata-sections is enabled. (That is, in the object
+// file the vtable is defined, it will be in section .rodata.cst{N} instead of
+// something like .rodata.{vtable}). This can cause vtables to be retained that
+// normally would be collected by --gc-sections. Placing them in a comdat group
+// can guarantee that they will be placed in a unique section group and garbage
+// collected.
+
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -S -o - -emit-llvm -fexperimental-relative-c++-abi-vtables -fno-rtti | FileCheck %s
+
+// This will result in a vtable of size 16, which would normally be placed in
+// .rodata.cst16.
+class A {
+public:
+  virtual void func();
+  virtual void func2();
+};
+
+void A::func() {}
+void A::func2() {}
+
+// CHECK: @_ZTV1A = {{.*}}, comdat, align 4
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===================================================================
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1772,6 +1772,9 @@
 
   if (VTContext.isRelativeLayout() && !VTable->isDSOLocal())
     CGVT.GenerateRelativeVTableAlias(VTable, VTable->getName());
+
+  if (CGVT.CanPlaceRelativeVTableInComdat(VTable))
+    VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 }
 
 bool ItaniumCXXABI::isVirtualOffsetNeededForVTableField(
Index: clang/lib/CodeGen/CGVTables.h
===================================================================
--- clang/lib/CodeGen/CGVTables.h
+++ clang/lib/CodeGen/CGVTables.h
@@ -154,6 +154,10 @@
   /// when a vtable may not be dso_local.
   void GenerateRelativeVTableAlias(llvm::GlobalVariable *VTable,
                                    llvm::StringRef AliasNameRef);
+
+  /// Check if we can place the vtable in a comdat group under the relative
+  /// layout.
+  bool CanPlaceRelativeVTableInComdat(const llvm::GlobalVariable *VTable);
 };
 
 } // end namespace CodeGen
Index: clang/lib/CodeGen/CGVTables.cpp
===================================================================
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -936,9 +936,19 @@
   if (UsingRelativeLayout && !VTable->isDSOLocal())
     GenerateRelativeVTableAlias(VTable, OutName);
 
+  if (CanPlaceRelativeVTableInComdat(VTable))
+    VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
+
   return VTable;
 }
 
+bool CodeGenVTables::CanPlaceRelativeVTableInComdat(
+    const llvm::GlobalVariable *VTable) {
+  return CGM.supportsCOMDAT() && getItaniumVTableContext().isRelativeLayout() &&
+         !VTable->hasLocalLinkage() && !VTable->isDeclarationForLinker() &&
+         !VTable->hasComdat();
+}
+
 // If the VTable is not dso_local, then we will not be able to indicate that
 // the VTable does not need a relocation and move into rodata. A frequent
 // time this can occur is for classes that should be made public from a DSO


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95694.320186.patch
Type: text/x-patch
Size: 3379 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210129/ef9fb46e/attachment.bin>