[llvm] r233164 - Linker: Drop function pointers for overridden subprograms
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Mar 24 19:26:33 PDT 2015
Author: dexonsmith
Date: Tue Mar 24 21:26:32 2015
New Revision: 233164
URL: http://llvm.org/viewvc/llvm-project?rev=233164&view=rev
Log:
Linker: Drop function pointers for overridden subprograms
Instead of dropping subprograms that have been overridden, just set
their function pointers to `nullptr`. This is a minor adjustment to the
stop-gap fix for PR21910 committed in r224487, and fixes the crasher
from PR22792.
The problem that r224487 put a band-aid on: how do we find the canonical
subprogram for a `Function`? Since the backend currently relies on
`DebugInfoFinder` (which does a naive in-order traversal of compile
units and picks the first subprogram) for this, r224487 tried dropping
non-canonical subprograms.
Dropping subprograms fails because the backend *also* builds up a map
from subprogram to compile unit (`DwarfDebug::SPMap`) based on the
subprogram lists. A missing subprogram causes segfaults later when an
inlined reference (such as in this testcase) is created.
Instead, just drop the `Function` pointer to `nullptr`, which nicely
mirrors what happens when an already-inlined `Function` is optimized
out. We can't really be sure that it's the same definition anyway, as
the testcase demonstrates.
This still isn't completely satisfactory. Two flaws at least that I can
think of:
- I still haven't found a straightforward way to make this symmetric
in the IR. (Interestingly, the DWARF output is already symmetric,
and I've tested for that to be sure we don't regress.)
- Using `DebugInfoFinder` to find the canonical subprogram for a
function is kind of crazy. We should just attach metadata to the
function, like this:
define weak i32 @foo(i32, i32) !dbg !MDSubprogram(...) {
Added:
llvm/trunk/test/Linker/Inputs/subprogram-linkonce-weak.ll
llvm/trunk/test/Linker/subprogram-linkonce-weak.ll
Modified:
llvm/trunk/lib/Linker/LinkModules.cpp
llvm/trunk/test/Linker/replaced-function-matches-first-subprogram.ll
Modified: llvm/trunk/lib/Linker/LinkModules.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=233164&r1=233163&r2=233164&view=diff
==============================================================================
--- llvm/trunk/lib/Linker/LinkModules.cpp (original)
+++ llvm/trunk/lib/Linker/LinkModules.cpp Tue Mar 24 21:26:32 2015
@@ -1250,9 +1250,10 @@ void ModuleLinker::linkNamedMDNodes() {
/// Drop DISubprograms that have been superseded.
///
-/// FIXME: this creates an asymmetric result: we strip losing subprograms from
-/// DstM, but leave losing subprograms in SrcM. Instead we should also strip
-/// losers from SrcM, but this requires extra plumbing in MapMetadata.
+/// FIXME: this creates an asymmetric result: we strip functions from losing
+/// subprograms in DstM, but leave losing subprograms in SrcM.
+/// TODO: Remove this logic once the backend can correctly determine canonical
+/// subprograms.
void ModuleLinker::stripReplacedSubprograms() {
// Avoid quadratic runtime by returning early when there's nothing to do.
if (OverridingFunctions.empty())
@@ -1262,8 +1263,8 @@ void ModuleLinker::stripReplacedSubprogr
auto Functions = std::move(OverridingFunctions);
OverridingFunctions.clear();
- // Drop subprograms whose functions have been overridden by the new compile
- // unit.
+ // Drop functions from subprograms if they've been overridden by the new
+ // compile unit.
NamedMDNode *CompileUnits = DstM->getNamedMetadata("llvm.dbg.cu");
if (!CompileUnits)
return;
@@ -1274,19 +1275,15 @@ void ModuleLinker::stripReplacedSubprogr
DITypedArray<DISubprogram> SPs(CU.getSubprograms());
assert(SPs && "Expected valid subprogram array");
- SmallVector<Metadata *, 16> NewSPs;
- NewSPs.reserve(SPs.getNumElements());
for (unsigned S = 0, SE = SPs.getNumElements(); S != SE; ++S) {
DISubprogram SP = SPs.getElement(S);
- if (SP && SP.getFunction() && Functions.count(SP.getFunction()))
+ if (!SP || !SP.getFunction() || !Functions.count(SP.getFunction()))
continue;
- NewSPs.push_back(SP);
+ // Prevent DebugInfoFinder from tagging this as the canonical subprogram,
+ // since the canonical one is in the incoming module.
+ SP->replaceFunction(nullptr);
}
-
- // Redirect operand to the overriding subprogram.
- if (NewSPs.size() != SPs.getNumElements())
- CU.replaceSubprograms(DIArray(MDNode::get(DstM->getContext(), NewSPs)));
}
}
Added: llvm/trunk/test/Linker/Inputs/subprogram-linkonce-weak.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/subprogram-linkonce-weak.ll?rev=233164&view=auto
==============================================================================
--- llvm/trunk/test/Linker/Inputs/subprogram-linkonce-weak.ll (added)
+++ llvm/trunk/test/Linker/Inputs/subprogram-linkonce-weak.ll Tue Mar 24 21:26:32 2015
@@ -0,0 +1,16 @@
+define weak i32 @foo(i32 %a, i32 %b) {
+entry:
+ %sum = call i32 @fastadd(i32 %a, i32 %b), !dbg !MDLocation(line: 2, scope: !3)
+ ret i32 %sum, !dbg !MDLocation(line: 3, scope: !3)
+}
+
+declare i32 @fastadd(i32, i32)
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+
+!llvm.dbg.cu = !{!1}
+!1 = !MDCompileUnit(language: DW_LANG_C99, file: !2, subprograms: !{!3}, emissionKind: 2)
+!2 = !MDFile(filename: "foo.c", directory: "/path/to/dir")
+!3 = !MDSubprogram(file: !2, scope: !2, line: 1, name: "foo", function: i32 (i32, i32)* @foo, type: !4)
+!4 = !MDSubroutineType(types: !{})
Modified: llvm/trunk/test/Linker/replaced-function-matches-first-subprogram.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/replaced-function-matches-first-subprogram.ll?rev=233164&r1=233163&r2=233164&view=diff
==============================================================================
--- llvm/trunk/test/Linker/replaced-function-matches-first-subprogram.ll (original)
+++ llvm/trunk/test/Linker/replaced-function-matches-first-subprogram.ll Tue Mar 24 21:26:32 2015
@@ -45,9 +45,9 @@ entry:
!1 = !MDFile(filename: "t1.cpp", directory: "/Users/dexonsmith/data/llvm/staging/test/Linker/repro/d1")
!2 = !{}
-; Extract out each compile unit's single subprogram. The replaced subprogram
-; should be dropped by the first compile unit.
-; CHECK-DAG: ![[SPs1]] = !{![[SP1:[0-9]+]]}
+; Extract out each compile unit's single subprogram. The replaced subprogram's
+; function should drop to null in the first compile unit.
+; CHECK-DAG: ![[SPs1]] = !{![[SP1:[0-9]+]], ![[SP2r:[0-9]+]]}
; CHECK-DAG: ![[SPs2]] = !{![[SP2:[0-9]+]]}
!3 = !{!4, !7}
!4 = !MDSubprogram(name: "foo", line: 2, isLocal: false, isDefinition: true, flags: DIFlagPrototyped, isOptimized: false, scopeLine: 2, file: !1, scope: !5, type: !6, function: i32 ()* @_Z3foov, variables: !2)
@@ -58,6 +58,10 @@ entry:
; subprogram is pointing at the correct function.
; CHECK-DAG: ![[SP1]] = !MDSubprogram({{.*}} function: i32 ()* @_Z3foov
; CHECK-DAG: ![[SP2]] = !MDSubprogram({{.*}} file: ![[FILE:[0-9]+]],{{.*}} function: i32 (%struct.Class*)* @_ZN5ClassIiE3fooEv
+
+; We can't use CHECK-NOT/CHECK-SAME with a CHECK-DAG, so rely on field order to
+; prove that there's no function: here.
+; CHECK-DAG: ![[SP2r]] = {{.*}}!MDSubprogram({{.*}} isOptimized: false, variables:
!7 = !MDSubprogram(name: "foo", line: 2, isLocal: false, isDefinition: true, flags: DIFlagPrototyped, isOptimized: false, scopeLine: 2, file: !8, scope: !9, type: !6, function: i32 (%struct.Class*)* @_ZN5ClassIiE3fooEv, variables: !2)
; The new subprogram should be pointing at the new directory.
Added: llvm/trunk/test/Linker/subprogram-linkonce-weak.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/subprogram-linkonce-weak.ll?rev=233164&view=auto
==============================================================================
--- llvm/trunk/test/Linker/subprogram-linkonce-weak.ll (added)
+++ llvm/trunk/test/Linker/subprogram-linkonce-weak.ll Tue Mar 24 21:26:32 2015
@@ -0,0 +1,140 @@
+; RUN: llvm-link %s %S/Inputs/subprogram-linkonce-weak.ll -S -o %t1
+; RUN: FileCheck %s -check-prefix=LW -check-prefix=CHECK <%t1
+; RUN: llvm-link %S/Inputs/subprogram-linkonce-weak.ll %s -S -o %t2
+; RUN: FileCheck %s -check-prefix=WL -check-prefix=CHECK <%t2
+
+; This testcase tests the following flow:
+; - File A defines a linkonce version of @foo which has inlined into @bar.
+; - File B defines a weak version of @foo (different definition).
+; - Linkage rules state File B version of @foo wins.
+; - @bar still has inlined debug info related to the linkonce @foo.
+;
+; This should fix PR22792, although the testcase was hand-written. There's a
+; RUN line with a crasher for llc at the end with checks for the DWARF output.
+
+; The LW prefix means linkonce (this file) first, then weak (the other file).
+; The WL prefix means weak (the other file) first, then linkonce (this file).
+
+; We'll see @bar before @foo if this file is first.
+; LW-LABEL: define i32 @bar(
+; LW: %sum = add i32 %a, %b, !dbg ![[FOOINBAR:[0-9]+]]
+; LW: ret i32 %sum, !dbg ![[BARRET:[0-9]+]]
+; LW-LABEL: define weak i32 @foo(
+; LW: %sum = call i32 @fastadd(i32 %a, i32 %b), !dbg ![[FOOCALL:[0-9]+]]
+; LW: ret i32 %sum, !dbg ![[FOORET:[0-9]+]]
+
+; We'll see @foo before @bar if this file is second.
+; WL-LABEL: define weak i32 @foo(
+; WL: %sum = call i32 @fastadd(i32 %a, i32 %b), !dbg ![[FOOCALL:[0-9]+]]
+; WL: ret i32 %sum, !dbg ![[FOORET:[0-9]+]]
+; WL-LABEL: define i32 @bar(
+; WL: %sum = add i32 %a, %b, !dbg ![[FOOINBAR:[0-9]+]]
+; WL: ret i32 %sum, !dbg ![[BARRET:[0-9]+]]
+
+define i32 @bar(i32 %a, i32 %b) {
+entry:
+ %sum = add i32 %a, %b, !dbg !MDLocation(line: 2, scope: !4,
+ inlinedAt: !MDLocation(line: 12, scope: !3))
+ ret i32 %sum, !dbg !MDLocation(line: 13, scope: !3)
+}
+
+define linkonce i32 @foo(i32 %a, i32 %b) {
+entry:
+ %sum = add i32 %a, %b, !dbg !MDLocation(line: 2, scope: !4)
+ ret i32 %sum, !dbg !MDLocation(line: 3, scope: !4)
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+
+; CHECK-LABEL: !llvm.dbg.cu =
+; LW-SAME: !{![[LCU:[0-9]+]], ![[WCU:[0-9]+]]}
+; WL-SAME: !{![[WCU:[0-9]+]], ![[LCU:[0-9]+]]}
+!llvm.dbg.cu = !{!1}
+
+; LW: ![[LCU]] = !MDCompileUnit({{.*}} subprograms: ![[LSPs:[0-9]+]]
+; LW: ![[LSPs]] = !{![[BARSP:[0-9]+]], ![[FOOSP:[0-9]+]]}
+; LW: ![[BARSP]] = !MDSubprogram(name: "bar",
+; LW-SAME: function: i32 (i32, i32)* @bar
+; LW: ![[FOOSP]] = {{.*}}!MDSubprogram(name: "foo",
+; LW-NOT: function:
+; LW-SAME: ){{$}}
+; LW: ![[WCU]] = !MDCompileUnit({{.*}} subprograms: ![[WSPs:[0-9]+]]
+; LW: ![[WSPs]] = !{![[WEAKFOOSP:[0-9]+]]}
+; LW: ![[WEAKFOOSP]] = !MDSubprogram(name: "foo",
+; LW-SAME: function: i32 (i32, i32)* @foo
+; LW: ![[FOOINBAR]] = !MDLocation(line: 2, scope: ![[FOOSP]], inlinedAt: ![[BARIA:[0-9]+]])
+; LW: ![[BARIA]] = !MDLocation(line: 12, scope: ![[BARSP]])
+; LW: ![[BARRET]] = !MDLocation(line: 13, scope: ![[BARSP]])
+; LW: ![[FOOCALL]] = !MDLocation(line: 2, scope: ![[WEAKFOOSP]])
+; LW: ![[FOORET]] = !MDLocation(line: 3, scope: ![[WEAKFOOSP]])
+
+; Same as above, but reordered.
+; WL: ![[WCU]] = !MDCompileUnit({{.*}} subprograms: ![[WSPs:[0-9]+]]
+; WL: ![[WSPs]] = !{![[WEAKFOOSP:[0-9]+]]}
+; WL: ![[WEAKFOOSP]] = !MDSubprogram(name: "foo",
+; WL-SAME: function: i32 (i32, i32)* @foo
+; WL: ![[LCU]] = !MDCompileUnit({{.*}} subprograms: ![[LSPs:[0-9]+]]
+; WL: ![[LSPs]] = !{![[BARSP:[0-9]+]], ![[FOOSP:[0-9]+]]}
+; WL: ![[BARSP]] = !MDSubprogram(name: "bar",
+; WL-SAME: function: i32 (i32, i32)* @bar
+; WL: ![[FOOSP]] = {{.*}}!MDSubprogram(name: "foo",
+; Note, for symmetry, this should be "NOT: function:" and "SAME: ){{$}}".
+; WL-SAME: function: i32 (i32, i32)* @foo
+; WL: ![[FOOCALL]] = !MDLocation(line: 2, scope: ![[WEAKFOOSP]])
+; WL: ![[FOORET]] = !MDLocation(line: 3, scope: ![[WEAKFOOSP]])
+; WL: ![[FOOINBAR]] = !MDLocation(line: 2, scope: ![[FOOSP]], inlinedAt: ![[BARIA:[0-9]+]])
+; WL: ![[BARIA]] = !MDLocation(line: 12, scope: ![[BARSP]])
+; WL: ![[BARRET]] = !MDLocation(line: 13, scope: ![[BARSP]])
+
+!1 = !MDCompileUnit(language: DW_LANG_C99, file: !2, subprograms: !{!3, !4}, emissionKind: 2)
+!2 = !MDFile(filename: "bar.c", directory: "/path/to/dir")
+!3 = !MDSubprogram(file: !2, scope: !2, line: 11, name: "bar", function: i32 (i32, i32)* @bar, type: !5)
+!4 = !MDSubprogram(file: !2, scope: !2, line: 1, name: "foo", function: i32 (i32, i32)* @foo, type: !5)
+!5 = !MDSubroutineType(types: !{})
+
+; Crasher for llc.
+; REQUIRES: object-emission
+; RUN: %llc_dwarf -filetype=obj -O0 %t1 -o %t1.o
+; RUN: llvm-dwarfdump %t1.o -debug-dump=info | FileCheck %s -check-prefix=DWLW
+; RUN: %llc_dwarf -filetype=obj -O0 %t2 -o %t2.o
+; RUN: llvm-dwarfdump %t2.o -debug-dump=info | FileCheck %s -check-prefix=DWWL
+
+; DWLW: DW_TAG_compile_unit
+; DWLW: DW_AT_name {{.*}}"bar.c"
+; DWLW: DW_TAG_subprogram
+; DWLW-NOT: DW_AT_{{[lowhigh]*}}_pc
+; DWLW: DW_AT_name {{.*}}foo
+; DWLW-NOT: DW_AT_{{[lowhigh]*}}_pc
+; DWLW: DW_TAG_subprogram
+; DWLW: DW_AT_low_pc
+; DWLW: DW_AT_high_pc
+; DWLW: DW_AT_name {{.*}}bar
+; DWLW: DW_TAG_inlined_subroutine
+; DWLW: DW_AT_abstract_origin
+; DWLW: DW_TAG_compile_unit
+; DWLW: DW_AT_name {{.*}}"foo.c"
+; DWLW: DW_TAG_subprogram
+; DWLW: DW_AT_low_pc
+; DWLW: DW_AT_high_pc
+; DWLW: DW_AT_name {{.*}}foo
+
+; The DWARF output is already symmetric (just reordered).
+; DWWL: DW_TAG_compile_unit
+; DWWL: DW_AT_name {{.*}}"foo.c"
+; DWWL: DW_TAG_subprogram
+; DWWL: DW_AT_low_pc
+; DWWL: DW_AT_high_pc
+; DWWL: DW_AT_name {{.*}}foo
+; DWWL: DW_TAG_compile_unit
+; DWWL: DW_AT_name {{.*}}"bar.c"
+; DWWL: DW_TAG_subprogram
+; DWWL-NOT: DW_AT_{{[lowhigh]*}}_pc
+; DWWL: DW_AT_name {{.*}}foo
+; DWWL-NOT: DW_AT_{{[lowhigh]*}}_pc
+; DWWL: DW_TAG_subprogram
+; DWWL: DW_AT_low_pc
+; DWWL: DW_AT_high_pc
+; DWWL: DW_AT_name {{.*}}bar
+; DWWL: DW_TAG_inlined_subroutine
+; DWWL: DW_AT_abstract_origin
More information about the llvm-commits
mailing list