[PATCH] D155659: [WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I wonder why the following example is still incorrect. I haven't carefully studied the LTO part of code. It seems that you do handle `isUsedInRegularObj`.

  cat > a.h <<'eof'
  struct A { virtual int foo(); };
  int bar(A *a);
  eof
  cat > a.cc <<'eof'
  #include "a.h"
  int A::foo() { return 1; }
  int bar(A *a) { return a->foo(); }
  eof
  cat > b.cc <<'eof'
  #include "a.h"
  struct B : A { int foo() { return 2; } };
  int baz() { B b; return bar(&b); }
  eof
  cat > main.cc <<'eof'
  #include "a.h"
  #include <stdio.h>
  extern int baz();
  int main() {
    A a;
    printf("%d %d\n", bar(&a), baz());
  }
  eof
  
  clang++ -c -flto=thin -fwhole-program-vtables -O main.cc a.cc b.cc
  clang++ -c -O b.cc -o b0.o

  % clang++ -flto=thin -Wl,--lto-whole-program-visibility -fuse-ld=lld main.o a.o b.o && ./a.out
  1 2
  % clang++ -Wl,--lto-validate-all-vtables-have-type-infos -flto=thin -Wl,--lto-whole-program-visibility -fuse-ld=lld main.o a.o b0.o && ./a.out
  1 1



================
Comment at: lld/ELF/Config.h:246
+  bool ltoValidateAllVtablesHaveTypeInfos;
+  bool ltoAllVtablesHaveTypeInfos;
   bool ltoWholeProgramVisibility;
----------------
Move `ltoAllVtablesHaveTypeInfos` (not an option) to `Ctx`


================
Comment at: lld/ELF/Driver.cpp:1039
+    using Elf_Sym = typename ELFT::Sym;
+    for (const Elf_Sym &s : f->template getELFSyms<ELFT>()) {
+      if (s.st_shndx != SHN_UNDEF) {
----------------
`getGlobalELFSyms` to skip local symbols


================
Comment at: lld/ELF/Driver.cpp:1071
+
+  config->ltoAllVtablesHaveTypeInfos = true;
+  // Check for unmatched RTTI symbols
----------------
and delete the assignment below


================
Comment at: lld/ELF/Driver.cpp:2848
 
+  // Handle -lto-validate-all-vtables-had-type-infos
+  if (config->ltoValidateAllVtablesHaveTypeInfos)
----------------



================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll:13
+; RUN: llvm-as %S/Inputs/devirt_validate_vtable_typeinfos.ll -o %t2.bc
+; RUN: llc -filetype=obj %t2.bc -o %t2.o
+; RUN: llc -relocation-model=pic -filetype=obj %t2.bc -o %t2_pic.o
----------------
You can remove the relocation-model=static object file as there is no testable difference.

Then, consider renaming `%t2_pic.o` to `%t2.o`


================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll:84
+
+; VALIDATE-DAG: single-impl: devirtualized a call to _ZN1D1mEi
+
----------------
```
; VALIDATE-NOT: single-impl:
; VALIDATE:     single-impl: devirtualized a call to _ZN1D1mEi
; VALIDATE-NOT: single-impl:
```


================
Comment at: lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll:163
+
+%struct.A = type { ptr }
+%struct.B = type { %struct.A }
----------------
Consider pasting the source code as well for readability and upgradability?

I haven't carefully studied the tests yet...


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:797
+      // and won't exist in IsVisibleToRegularObj. The full TypeID will
+      // will be present and participate in invalidation
+      if (typeID.ends_with(".virtual"))
----------------
typo: `will will`

Add a period.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:871
+    // and won't exist in IsVisibleToRegularObj. The full TypeID will
+    // will be present and participate in invalidation
+    if (StringRef(typeID.first).ends_with(".virtual"))
----------------
typo: `will will`

append a period.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:876
+    if (IsVisibleToRegularObj(typeID.first)) {
+      for (const TypeIdOffsetVtableInfo &P : typeID.second)
+        VisibleToRegularObjSymbols.insert(P.VTableVI.getGUID());
----------------
delete braces in this nested case when the only body has just one line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659