IRGen: Start using the WriteThinLTOBitcode pass.

[pcc]

Summary

This is the final change necessary to support CFI with ThinLTO.

[tejohnson]

TODO: avoid breaking Darwin.

How does this break Darwin?

clang/lib/CodeGen/BackendUtil.cpp

694 ↗

(On Diff #84792)

Can we transform other callers of createBitcodeWriterPass with EmitSummaryIndex = true or EmitModuleHash = true to instead call createWriteThinLTOBitcodePass as you did here, and then remove those parameters from createBitcodeWriterPass?

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

Is it the case that now we will always split the module with this change? Should that only be done under CFI options?

[mehdi_amini]

In D28843#649207, @tejohnson wrote:

TODO: avoid breaking Darwin.

How does this break Darwin?

IIUC, we don't use the new LTO API, which is handling this gracefully.

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

Devirtualization may happen whenever you have a hidden virtual table IIUC, independently of CFI.

[pcc]

clang/lib/CodeGen/BackendUtil.cpp

694 ↗

(On Diff #84792)

http://llvm-cs.pcc.me.uk/lib/Bitcode/Writer/BitcodeWriterPass.cpp/rcreateBitcodeWriterPass

The only other caller like that is in opt. I think that if we change the "under the hood" parts of opt we should change it to not use a pass at all (c.f. our earlier discussion on D27324), then remove those parameters.

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

To be more precise: we normally add type metadata in LTO mode when the class has hidden visibility. See: http://clang.llvm.org/docs/LTOVisibility.html

That doesn't necessarily imply devirtualization, which is controlled by the flag -fwhole-program-vtables.

[mehdi_amini]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

So with hidden visibility but without CFI or -fwhole-program-vtables, do we split the module? What's the purpose?

[pcc]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

At the moment we would. The purpose is to simplify the overall interface. If I want to compile a subset of my TUs without CFI or devirtualization, I should be able to do that by enabling LTO but not passing the CFI or devirtualization flags. In that case the vtables themselves should still have type metadata so that TUs compiled without CFI/devirtualization can correctly interoperate with TUs compiled with CFI/devirtualization (to handle the cases where a class defined in a TU compiled without CFI/devirt is used by code compiled with LTO/devirt, or where the linker/LTO selects a linkonce_odr vtable from a TU compiled without CFI/devirt).

I'd be open to changing the command line interface so that an additional flag may be used to control the scope of the "LTO unit" and which would just enable type metadata, but ideally I'd like to keep things relatively simple.

[mehdi_amini]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

At the moment we would. The purpose is to simplify the overall interface.

Right, if it was the only reason, I wouldn't be in favor, but you raise a real use case below.

If I want to compile a subset of my TUs without CFI or devirtualization, I should be able to do that by enabling LTO but not passing the CFI or devirtualization flags.

Right, seems legit.

In that case the vtables themselves should still have type metadata so that TUs compiled without CFI/devirtualization can correctly interoperate with TUs compiled with CFI/devirtualization

That's what I wasn't sure about :)

(to handle the cases where a class defined in a TU compiled without CFI/devirt is used by code compiled with LTO/devirt, or where the linker/LTO selects a linkonce_odr vtable from a TU compiled without CFI/devirt).

Make sense, LGTM as is with this explanation!
Thanks.

[tejohnson]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

I would like some way to disable this, at least for debugging.

[mehdi_amini]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

CC1 option then?

[tejohnson]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

CC1 option then?

Sure

[pcc]

clang/test/CodeGenCXX/type-metadata-thinlto.cpp

2 ↗

(On Diff #84792)

D28877

[pcc]

Refresh

[tejohnson]

LGTM

[pcc]

• Add missing test dependency

cfe/trunk/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
   CodeGenPasses.add(
       createTargetTransformInfoWrapperPass(getTargetIRAnalysis()));
 
   switch (Action) {
   case Backend_EmitNothing:
     break;
 
   case Backend_EmitBC:
-    PerModulePasses.add(createBitcodeWriterPass(
-        *OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-        CodeGenOpts.EmitSummaryIndex));
+    if (CodeGenOpts.EmitSummaryIndex)
+      PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+    else
+      PerModulePasses.add(
+          createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
     break;
 
   case Backend_EmitLL:
     PerModulePasses.add(
         createPrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   default:

cfe/trunk/test/CMakeLists.txt

 
 if( NOT CLANG_BUILT_STANDALONE )
   list(APPEND CLANG_TEST_DEPS
     llvm-config
     FileCheck count not
     llc
     llvm-bcanalyzer
     llvm-dis
+    llvm-modextract
     llvm-nm
     llvm-objdump
     llvm-profdata
     llvm-readobj
     llvm-symbolizer
     opt
     )
 

cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp

+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+  virtual void f() {}
+};
+
+A *f() {
+  return new A;
+}