[IRMover] Don't materialise values from different source module

[evgeny777]

Summary

This fixes PR38372

[steven_wu]

Can you add a bit more explanation other than just saying fixing PR38372?

test/ThinLTO/X86/fwd-decl.ll

24

I don't think debug info are relevant for the test case. You can remove them from both module.

[evgeny777]

test/ThinLTO/X86/fwd-decl.ll

24

No, I can't

The DIFlagFwdDecl forces IRMover to look for definition in a different module, which has also been loaded by LTO. This causes IRMover to see both %struct.TA = type {} and %struct.TA = type opaque during a single move operation and make a mixture of types in a destination module.

Removal of debug info also eliminates the error.

[steven_wu]

test/ThinLTO/X86/fwd-decl.ll

24

Will be good if you put the information into the commit message. That makes sense now. Thanks for explanation.

LGTM. But I will wait for Teresa to take a look. Thanks for fixing this.

[evgeny777]

Any comments? Teresa?

[tejohnson]

Sorry for the late reply - I am just today back from vacation and catching up. This LGTM, but as Steven noted, please add a description to the commit message.

[trentxintong]

Hi @evgeny777 @tejohnson

I dumped the Composite right after the 2nd file is merged. Things look good except the DITemplateValueParameter debuginfo looks a bit odd. Does this look correct to you ?

(gdb) p Composite.dump()
; ModuleID = 'ld-temp.o'
source_filename = "ld-temp.o"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%struct.TA = type {}

@gv = external dso_local global %struct.TA*

define i32 @c() !dbg !13 {

%1 = bitcast %struct.TA** @gv to i8*
unreachable

}

define i1 @b(%struct.TA*) {

unreachable

}

define i1 @a(%struct.TA*) {

%2 = call i1 @b(%struct.TA* %0)
unreachable

}

!llvm.dbg.cu = !{!0, !6}
!llvm.module.flags = !{!11, !12}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug, retainedTypes: !2)
!1 = !DIFile(filename: "f2", directory: "")
!2 = !{!3}
!3 = distinct !DICompositeType(tag: DW_TAG_class_type, scope: !5, file: !4, identifier: "SHARED")
!4 = !DIFile(filename: "f1", directory: "")
!5 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !6)
!6 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !4, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug, retainedTypes: !7)
!7 = !{!8, !3}
!8 = !DICompositeType(tag: DW_TAG_class_type, scope: !5, file: !4, templateParams: !9)
!9 = !{!10}
!10 = !DITemplateValueParameter(value: i1 (%"type 0x4439360"*)* @b)
!11 = !{i32 2, !"Debug Info Version", i32 3}
!12 = !{i32 1, !"ThinLTO", i32 0}
!13 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !0)

[tejohnson]

That new DITemplateValueParameter type doesn't look correct to me.

[evgeny777]

It turned out that the problem is more complex than it seemed initially, because this forward declaration causes metadata loader to do really horrible things. Here is how metadata of test/ThinLTO/X86/fwd-decl.ll looks after both modules are loaded by addRegularLTO:

!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = !{i32 1, !"ThinLTO", i32 0}
!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug, retainedTypes: !4)
!3 = !DIFile(filename: "f2", directory: "")
!4 = !{!5}
!5 = distinct !DICompositeType(tag: DW_TAG_array_type, scope: !6, identifier: "SHARED")
!6 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !7)
!7 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !8, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug, retainedTypes: !9)
!8 = !DIFile(filename: "f1", directory: "")
!9 = !{!10, !5}
!10 = !DICompositeType(tag: DW_TAG_class_type, scope: !6, file: !8, templateParams: !11)
!11 = !{!12}
!12 = !DITemplateValueParameter(value: i1 (%struct.TA.0*)* @b)

Note that module metadata now contains both compile units. The module itself is broken because %struct.TA.0 is not defined (it is defined in Inputs/fwd-decl.ll after renaming)

This combined with metadata caching in IRMover causes bug found by @trentxintong.
One possible way to fix it is to disable metadata caching for specific module when metadata import causes materialising of GV from a different source module (see updated patch).

Just curious: how such debugging info was produced? Is it synthetic example or is it derived from real compiler output?

[evgeny777]

In D50066#1197101, @tejohnson wrote:

That new DITemplateValueParameter type doesn't look correct to me.

I'm getting !DITemplateValueParameter(value: null) when module is passed to optimizer. This isn't correct either, but module is no longer broken.

[tejohnson]

In D50066#1198625, @evgeny777 wrote:

In D50066#1197101, @tejohnson wrote:

That new DITemplateValueParameter type doesn't look correct to me.

I'm getting !DITemplateValueParameter(value: null) when module is passed to optimizer. This isn't correct either, but module is no longer broken.

I've added @dblaikie to the review to comment on what the debug metadata should look like in this case.

In D50066#1198597, @evgeny777 wrote:

[...]

Note that module metadata now contains both compile units. The module itself is broken because %struct.TA.0 is not defined (it is defined in Inputs/fwd-decl.ll after renaming)

This combined with metadata caching in IRMover causes bug found by @trentxintong.
One possible way to fix it is to disable metadata caching for specific module when metadata import causes materialising of GV from a different source module (see updated patch).

Just curious: how such debugging info was produced? Is it synthetic example or is it derived from real compiler output?

I think @trentxintong mentioned this was reduced from an application. Out of curiosity, what makes it look like it might be synthetic?

[evgeny777]

Out of curiosity, what makes it look like it might be synthetic?

I'm not a debug info expert, but this looks unusual:

!8 = distinct !DISubprogram(unit: !2)
!9 = !DICompositeType(tag: DW_TAG_array_type, identifier: "SHARED", scope: !8)

With no code in Inputs/fwd-decl.ll actually referencing !8 or !9

[trentxintong]

@evgeny777 Do you mind I give this bug a try ? Thanks.

[evgeny777]

@trentxintong Sure, please go on. I'm on vacation till Sep, 12

[trentxintong]

@evgeny777
Thanks!. The approach I am considering is to bring in something, like a stub (e.g. the declaration completely unmapped, we cant map it fully anyways) and eventually using it as a handle to RAUW when we have properly mapped in the SGV. Do you think this would work ?

lib/Linker/IRMover.cpp

class IRLinker {
   /// See IRMover::move().
   std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor;
 
   TypeMapTy TypeMap;
   GlobalValueMaterializer GValMaterializer;
   LocalValueMaterializer LValMaterializer;
 
   /// A metadata map that's shared between IRLinker instances.
-  MDMapT &SharedMDs;
+  MDMapT *SharedMDs;
 
   /// Mapping of values from what they used to be in Src, to what they are now
   /// in DstM.  ValueToValueMapTy is a ValueMap, which involves some overhead
   /// due to the use of Value handles which the Linker doesn't actually need,
   /// but this allows us to reuse the ValueMapper code.
   ValueToValueMapTy ValueMap;
   ValueToValueMapTy AliasValueMap;
 

 public:
   IRLinker(Module &DstM, MDMapT &SharedMDs,
            IRMover::IdentifiedStructTypeSet &Set, std::unique_ptr<Module> SrcM,
            ArrayRef<GlobalValue *> ValuesToLink,
            std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor,
            bool IsPerformingImport)
       : DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)),
         TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this),
-        SharedMDs(SharedMDs), IsPerformingImport(IsPerformingImport),
+        SharedMDs(&SharedMDs), IsPerformingImport(IsPerformingImport),
         Mapper(ValueMap, RF_MoveDistinctMDs | RF_IgnoreMissingLocals, &TypeMap,
                &GValMaterializer),
         AliasMCID(Mapper.registerAlternateMappingContext(AliasValueMap,
                                                          &LValMaterializer)) {
     ValueMap.getMDMap() = std::move(SharedMDs);
     for (GlobalValue *GV : ValuesToLink)
       maybeAdd(GV);
     if (IsPerformingImport)
       prepareCompileUnitsForImport();
   }
-  ~IRLinker() { SharedMDs = std::move(*ValueMap.getMDMap()); }
+  ~IRLinker() {
+    if (SharedMDs)
+      *SharedMDs = std::move(*ValueMap.getMDMap());
+  }
 
   Error run();
   Value *materialize(Value *V, bool ForAlias);
 };
 }
 
 /// The LLVM SymbolTable class autorenames globals that conflict in the symbol
 /// table. This is good for all clients except for us. Go through the trouble
Value *LocalValueMaterializer::materialize(Value *SGV) {
   return TheIRLinker.materialize(SGV, true);
 }
 
 Value *IRLinker::materialize(Value *V, bool ForAlias) {
   auto *SGV = dyn_cast<GlobalValue>(V);
   if (!SGV)
     return nullptr;
 
+  if (SGV->getParent() != SrcM.get()) {
+    // Import of some netadata node (like DITemplateValueParameter) made us
+    // materializing from a different source module. We can't do this because
+    // we might not have computed type mapping in this other module yet.
+    // Also we don't cache any MD nodes from current source module as this may
+    // cause broken metadata in the composite module.
+    SharedMDs = nullptr;
+    return nullptr;
+  }
+
   Expected<Constant *> NewProto = linkGlobalValueProto(SGV, ForAlias);
   if (!NewProto) {
     setError(NewProto.takeError());
     return nullptr;
   }
   if (!*NewProto)
     return nullptr;
 

test/ThinLTO/X86/Inputs/fwd-decl.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.TA = type opaque
+@gv = external dso_local global %struct.TA *
+
+define i1 @b(%struct.TA*) {
+  unreachable
+}
+
+define i1 @a(%struct.TA* %a) {
+  call i1 @b(%struct.TA* %a)
+  unreachable
+}
+
+!llvm.module.flags = !{!0, !1}
+!llvm.dbg.cu = !{!2}
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = !{i32 1, !"ThinLTO", i32 0}
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, retainedTypes: !4)
+!3 = !DIFile(filename: "f1", directory: "")
+!4 = !{!5, !9}
+!5 = !DICompositeType(tag: DW_TAG_class_type, file: !3, templateParams: !6, scope: !8)
+!6 = !{!7}
+
+; The reference to @b and struct.TA.0 (renamed)  that will be loaded in %a.o
+!7 = !DITemplateValueParameter(value: i1 (%struct.TA*)* @b)
+!8 = distinct !DISubprogram(unit: !2)
+!9 = !DICompositeType(tag: DW_TAG_array_type, identifier: "SHARED", scope: !8)

test/ThinLTO/X86/fwd-decl.ll

+; Ensures IRMover only materializes values from current source module.
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/fwd-decl.ll -o %t2.bc
+; RUN: llvm-lto2 run -save-temps -o %t3.bc %t1.bc %t2.bc -r %t1.bc,c,px -r %t2.bc,a,px -r %t2.bc,b,px -r %t1.bc,gv -r %t2.bc,gv
+; RUN: llvm-dis %t3.bc.0.0.preopt.bc -o - | FileCheck %s
+
+; CHECK:        define i1 @a(%struct.TA* %a)
+; CHECK-NEXT:     %1 = call i1 @b(%struct.TA* %a)
+
+; Check that metadata was correctly imported.
+; CHECK:        !DITemplateValueParameter(value: i1 (%struct.TA*)* @b)
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.TA = type {}
+
+@gv = dso_local global %struct.TA * zeroinitializer
+
+define i32 @c() !dbg !6 {
+  bitcast %struct.TA ** @gv to i8*
+  unreachable
+}
+
+!llvm.module.flags = !{!0, !1}
+!llvm.dbg.cu = !{!2}
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = !{i32 1, !"ThinLTO", i32 0}
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, retainedTypes: !4)
+!3 = !DIFile(filename: "f2", directory: "")
+!4 = !{!5}
+!5 = !DICompositeType(tag: DW_TAG_class_type, file: !3, flags: DIFlagFwdDecl, identifier: "SHARED")
+!6 = distinct !DISubprogram(unit: !2)
+
+