Skip to content

Commit d9881e6

Browse files
author
Yuanfang Chen
committed
[IRMover] Avoid materializing global value that belongs to not-yet-linked module
We saw the same assertion failure mentioned here https://bugs.llvm.org/show_bug.cgi?id=42063 in our internal tests. The failure happens in the same circumstance as D47898 and D66814 where uniqueing of DICompositeTypes causes `Mapper::mapValue` to be called on GlobalValues(`G`) from a not-yet-linked module(`M`). The following type-mapping for `G` may not complete correctly (fail to unique types etc. depending on the the complexity of the types) because IRLinker::computeTypeMapping is not done for `M` in this path. D47898 and D66814 fixed some type-mapping issue after Mapper::mapValue is called on `G`. However, it seems it did not handle some complex cases. I think we should delay linking globals like `G` until its owing module is linked. In this way, we could save unnecessary type mapping and prune these corner cases. It is also supposed to reduce the total number of structs ending up in the combined module. D47898 is reverted (its test is kept) because it regresses the test case here. D66814 could also be reverted (the `check-all` looks good). But it looks reasonable anyway, so I thought I should keep it. Also tested the patch with clang self-host regularLTO/ThinLTO build, things look good as well. Reviewed By: tejohnson Differential Revision: https://reviews.llvm.org/D87001
1 parent 80ef412 commit d9881e6

File tree

4 files changed

+151
-10
lines changed

4 files changed

+151
-10
lines changed

‎llvm/lib/Linker/IRMover.cpp

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -242,23 +242,14 @@ Type *TypeMapTy::get(Type *Ty, SmallPtrSet<StructType *, 8> &Visited) {
242242
bool IsUniqued = !isa<StructType>(Ty) || cast<StructType>(Ty)->isLiteral();
243243

244244
if (!IsUniqued) {
245-
StructType *STy = cast<StructType>(Ty);
246-
// This is actually a type from the destination module, this can be reached
247-
// when this type is loaded in another module, added to DstStructTypesSet,
248-
// and then we reach the same type in another module where it has not been
249-
// added to MappedTypes. (PR37684)
250-
if (STy->getContext().isODRUniquingDebugTypes() && !STy->isOpaque() &&
251-
DstStructTypesSet.hasType(STy))
252-
return *Entry = STy;
253-
254245
#ifndef NDEBUG
255246
for (auto &Pair : MappedTypes) {
256247
assert(!(Pair.first != Ty && Pair.second == Ty) &&
257248
"mapping to a source type");
258249
}
259250
#endif
260251

261-
if (!Visited.insert(STy).second) {
252+
if (!Visited.insert(cast<StructType>(Ty)).second) {
262253
StructType *DTy = StructType::create(Ty->getContext());
263254
return *Entry = DTy;
264255
}
@@ -579,6 +570,13 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
579570
if (!SGV)
580571
return nullptr;
581572

573+
// When linking a global from other modules than source & dest, skip
574+
// materializing it because it would be mapped later when its containing
575+
// module is linked. Linking it now would potentially pull in many types that
576+
// may not be mapped properly.
577+
if (SGV->getParent() != &DstM && SGV->getParent() != SrcM.get())
578+
return nullptr;
579+
582580
Expected<Constant *> NewProto = linkGlobalValueProto(SGV, ForIndirectSymbol);
583581
if (!NewProto) {
584582
setError(NewProto.takeError());

‎llvm/test/LTO/X86/Inputs/type-mapping-bug4_0.ll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
2+
target triple = "x86_64-unknown-linux-gnu"
3+
4+
%class.CWBD = type { float }
5+
%"class.std::_Unique_ptr_base" = type { %class.CWBD* }
6+
7+
%class.CB = type opaque
8+
9+
!llvm.module.flags = !{!0, !1}
10+
!0 = !{i32 1, !"ThinLTO", i32 0}
11+
!1 = !{i32 2, !"Debug Info Version", i32 3}

‎llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
2+
target triple = "x86_64-unknown-linux-gnu"
3+
4+
%class.CCSM = type opaque
5+
%class.CWBD = type { float }
6+
7+
%"class.std::_Unique_ptr_base" = type { %class.CWBD* }
8+
9+
%class.CB = type { %"class.std::unique_ptr_base.1" }
10+
; (stage1.1)
11+
; %class.std::unique_ptr_base.1(t1.o) is mapped to %class.std::unique_ptr_base(t0.o)
12+
; %class.CCSM(t1.o) is mapped to %class.CWBD(t0.o)
13+
%"class.std::unique_ptr_base.1" = type { %class.CCSM* }
14+
15+
; (stage1.2)
16+
; %class.CCSM(t1.o) -> %class.CWBD(t0.o) mapping of stage1.1 maps this to
17+
; "declare void @h(%class.CWBD*)"
18+
declare void @h(%class.CCSM*)
19+
define void @j() {
20+
call void @h(%class.CCSM* undef)
21+
ret void
22+
}
23+
24+
define void @a() {
25+
; Without the fix in D87001 to delay materialization of @d until its module is linked
26+
; (stage1.3)
27+
; mapping `%class.CB* undef` creates the first instance of %class.CB (%class.CB).
28+
; (stage2)
29+
; mapping `!6` starts the stage2, during which second instance of %class.CB (%class.CB.1)
30+
; is created for the mapped @d declaration.
31+
; define void @d(%class.CB.1*)
32+
; After this, %class.CB (t2.o) (aka %class.CB.1) and
33+
; %"class.std::unique_ptr_base.2" (t2.o) are added to DstStructTypesSet.
34+
call void @llvm.dbg.value(metadata %class.CB* undef, metadata !6, metadata !DIExpression()), !dbg !4
35+
ret void
36+
}
37+
38+
declare void @llvm.dbg.value(metadata, metadata, metadata)
39+
40+
!llvm.module.flags = !{!0, !1}
41+
!llvm.dbg.cu = !{!2}
42+
!0 = !{i32 1, !"ThinLTO", i32 0}
43+
!1 = !{i32 2, !"Debug Info Version", i32 3}
44+
!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3)
45+
!3 = !DIFile(filename: "f2", directory: "")
46+
47+
!4 = !DILocation(line: 117, column: 34, scope: !7)
48+
49+
; This DICompositeType refers to !5 in type-mapping-bug4.ll
50+
!5 = !DICompositeType(tag: DW_TAG_structure_type, flags: DIFlagFwdDecl, identifier: "SHARED")
51+
52+
!6 = !DILocalVariable(name: "this", arg: 1, scope: !7, flags: DIFlagArtificial | DIFlagObjectPointer)
53+
!7 = distinct !DISubprogram(name: "a", type: !8, unit: !2)
54+
!8 = !DISubroutineType(types: !9)
55+
!9 = !{null, !5}

‎llvm/test/LTO/X86/type-mapping-bug4.ll

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
; RUN: opt -module-summary -o %t0.o %S/Inputs/type-mapping-bug4_0.ll
2+
; RUN: opt -module-summary -o %t1.o %S/Inputs/type-mapping-bug4_1.ll
3+
; RUN: opt -module-summary -o %t2.o %s
4+
; RUN: llvm-lto2 run -save-temps -o %t3 %t0.o %t1.o %t2.o -r %t1.o,a,px -r %t2.o,d,px -r %t1.o,h,x -r %t2.o,h,x -r %t1.o,j,px
5+
; RUN: llvm-dis < %t3.0.0.preopt.bc | FileCheck %s
6+
7+
; stage0: linking t0.o
8+
; stage1: linking t1.o
9+
; stage2: during linking t1.o, mapping @d
10+
; stage3: linking t2.o
11+
12+
; Stage0 is not described because it is not interesting for the purpose of this test.
13+
; Stage1 and stage2 are described in type-mapping-bug4_1.ll.
14+
; Stage3 is described in this file.
15+
16+
; CHECK: %class.CCSM = type opaque
17+
; CHECK: %class.CB = type { %"class.std::unique_ptr_base.1" }
18+
; CHECK: %"class.std::unique_ptr_base.1" = type { %class.CCSM* }
19+
20+
; CHECK: define void @j() {
21+
; CHECK: call void @h(%class.CCSM* undef)
22+
; CHECK: ret void
23+
; CHECK: }
24+
25+
; CHECK: declare void @h(%class.CCSM*)
26+
27+
; CHECK: define void @a() {
28+
; CHECK: call void @llvm.dbg.value(metadata %class.CB* undef, metadata !10, metadata !DIExpression())
29+
; CHECK: ret void
30+
; CHECK: }
31+
32+
; CHECK: declare void @llvm.dbg.value(metadata, metadata, metadata) #0
33+
34+
; CHECK: define void @d(%class.CB* %0) {
35+
; CHECK: %2 = getelementptr inbounds %class.CB, %class.CB* undef, i64 0, i32 0, i32 0
36+
; CHECK: ret void
37+
; CHECK: }
38+
39+
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
40+
target triple = "x86_64-unknown-linux-gnu"
41+
42+
; (stage3) Remapping this type returns itself due to D47898 and stage1.3
43+
%class.CB = type { %"class.std::unique_ptr_base.2" }
44+
45+
; (stage3) Remapping this type returns itself due to D47898 and stage2
46+
%"class.std::unique_ptr_base.2" = type { %class.CCSM* }
47+
48+
%class.CCSM = type opaque
49+
50+
; (stage3) computeTypeMapping add the mapping %class.CCSM -> %class.CWBD due to stage1.2
51+
declare void @h(%class.CCSM*)
52+
53+
define void @d(%class.CB*) {
54+
; Without the fix in D87001 to delay materialization of @d until its module is linked
55+
; (stage3)
56+
; * SourceElementType of getelementptr is remapped to itself.
57+
; * ResultElementType of getelementptr is incorrectly remapped to %class.CWBD*.
58+
; Its type should be %class.CCSM*.
59+
%2 = getelementptr inbounds %class.CB, %class.CB* undef, i64 0, i32 0, i32 0
60+
ret void
61+
}
62+
63+
!llvm.dbg.cu = !{!2}
64+
!llvm.module.flags = !{!0, !1}
65+
!0 = !{i32 1, !"ThinLTO", i32 0}
66+
!1 = !{i32 2, !"Debug Info Version", i32 3}
67+
!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, retainedTypes: !4)
68+
!3 = !DIFile(filename: "f1", directory: "")
69+
!4 = !{!5}
70+
71+
; This DICompositeType is referenced by !5 in Inputs/type-mapping-bug4_1.ll
72+
; causing the function type in !7 to be added to its module.
73+
!5 = !DICompositeType(tag: DW_TAG_structure_type, templateParams: !6, identifier: "SHARED")
74+
!6 = !{!7}
75+
76+
; The reference to d and %class.CB that gets loaded into %t1.o
77+
!7 = !DITemplateValueParameter(value: void (%class.CB*)* @d)

0 commit comments

Comments
 (0)