[PATCH] D14195: Move metadata linking after lazy global materialization/linking.

Mon Nov 2 16:13:18 PST 2015

> On 2015-Nov-01, at 18:42, Teresa Johnson <tejohnson at google.com> wrote:
> 
> On Sun, Nov 1, 2015 at 1:52 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> 
>>> On 2015-Oct-29, at 21:47, Teresa Johnson <tejohnson at google.com> wrote:
>>> 
>>> tejohnson created this revision.
>>> tejohnson added reviewers: dexonsmith, tra, dblaikie.
>>> tejohnson added a subscriber: llvm-commits.
>>> 
>>> Currently, named metadata is linked before the LazilyLinkGlobalValues
>>> list is walked and materialized/linked. As a result, references
>>> from DISubprogram and DIGlobalVariable metadata to yet unmaterialized
>>> functions and variables cause them to be added to the lazy linking
>>> list and their definitions are materialized and linked.
>>> 
>>> This makes the llvm-link -only-needed option not have the intended
>>> effect when debug information is present, as the otherwise unneeded
>>> functions/variables are still linked in.
>>> 
>>> Additionally, for ThinLTO I have implemented a mechanism to only link
>>> in debug metadata needed by imported functions. Moving named metadata
>>> linking after lazy GV linking will facilitate applying this mechanism
>>> to the LTO and "llvm-link -only-needed" cases as well.
>>> 
>>> http://reviews.llvm.org/D14195
>>> 
>>> Files:
>>> lib/Linker/LinkModules.cpp
>>> test/Linker/Inputs/linkage_debug.b.ll
>>> test/Linker/Inputs/linkage_debug.c.ll
>>> test/Linker/link-flags-debug.ll
>>> 
>>> <D14195.38784.patch>
>> 
>> Code LGTM, but I have some comments on the testcases:
> 
> Thanks for the review, some responses below.
> 
>> 
>>> Index: test/Linker/link-flags-debug.ll
>>> ===================================================================
>>> --- /dev/null
>>> +++ test/Linker/link-flags-debug.ll
>> 
>> This has a `.ll` extension, but it's not valid textual IR.
>> 
>>> @@ -0,0 +1,15 @@
>>> +; RUN: llvm-as %S/Inputs/linkage_debug.b.ll -o %t.b.bc
>>> +; RUN: llvm-as %S/Inputs/linkage_debug.c.ll -o %t.c.bc
>>> +; RUN: llvm-link -S -only-needed %t.b.bc %t.c.bc | FileCheck %s -check-prefix=B -check-prefix=C -check-prefix=CN
>>> +; RUN: llvm-link -S -internalize -only-needed %t.b.bc %t.c.bc | FileCheck %s -check-prefix=B -check-prefix=CI -check-prefix=CN
>>> +
>>> +C-LABEL: @X = global i32 5
>>> +CI-LABEL: @X = internal global i32 5
>>> +CN-LABEL:@U = external global i32
>>> +
>>> +B-LABEL: define void @bar() {
>>> +
>>> +C-LABEL: define i32 @foo()
>>> +CI-LABEL: define internal i32 @foo()
>>> +
>>> +CN-LABEL:declare i32 @unused()
>>> Index: test/Linker/Inputs/linkage_debug.c.ll
>>> ===================================================================
>>> --- /dev/null
>>> +++ test/Linker/Inputs/linkage_debug.c.ll
>> 
>> I don't really love the filenaming scheme.  One that Rafael started
>> using a while ago is really handy.  Put the check lines in one of the
>> LLVM IR files, give them the same names, and put them in different
>> directories:
>>  - test/Linker/<name>.ll
>>  - test/Linker/Inputs/<name>.ll.
>> 
>> Since both files have the same name, it's clear that they belong
>> together.  It also let's you mix CHECK lines with the actual code from
>> one of the linked files.
> 
> SGTM - I had cloned this test case and structure from another test
> case that is the same but without the metadata (the same names minus
> the "-debug". Will change this new test to the above structure
> instead.
> 
>> 
>> Regarding <name>, you have two different ones here ("link-flags-debug"
>> and "linkage_debug"), and I don't find either tells me what the test is
>> for.  Maybe this could be "only-needed-named-metadata.ll" (or anything
>> that describes the specific thing you're testing).
> 
> I still like having "debug" in there since there is some
> debug-specific behavior I want to test, more on that below. I will use
> the name "only-needed-debug-metadata".
> 
>> 
>>> @@ -0,0 +1,29 @@
>>> + at X = global i32 5
>>> + at U = global i32 6
>>> +define i32 @foo() { ret i32 7, !dbg !20 }
>>> +define i32 @unused() { ret i32 8, !dbg !21 }
>>> +
>>> +!llvm.dbg.cu = !{!0}
>>> +!llvm.module.flags = !{!16, !17}
>>> +!llvm.ident = !{!18}
>>> +
>>> +!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.8.0 (trunk 251407) (llvm/trunk 251401)", isOptimized: true, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3, globals: !13)
>>> +!1 = !DIFile(filename: "linkage.c.c", directory: ".")
>>> +!2 = !{}
>>> +!3 = !{!4, !10}
>>> +!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 4, type: !5, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, function: i32 ()* @foo, variables: !8)
>> 
>> I suspect this isn't at all specific to debug info.  Can you reproduce
>> the problem with a testcase that uses hand-written named metadata (I'd
>> recommend !llvm.named or something)?  That would make the testcase (and
>> failure) much easier to understand and maintain.
> 
> I can probably provoke the behavior being tested right now using other
> non-debug metadata that references the symbols. But the metadata
> importing patch (for ThinLTO) that I am sending for review soon will
> also include handling for this case so that it only imports the needed
> subroutine metadata, and at that point plan to change the test to add
> a check that the DISubprogram metadata and descendants for the unused
> function are not linked in.

That seems better as a separate test to me.

> 
>> 
>>> +!5 = !DISubroutineType(types: !6)
>>> +!6 = !{!7, !7}
>>> +!7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
>>> +!8 = !{}
>>> +!10 = distinct !DISubprogram(name: "unused", scope: !1, file: !1, line: 8, type: !11, isLocal: false, isDefinition: true, scopeLine: 8, isOptimized: true, function: i32 ()* @unused, variables: !2)
>>> +!11 = !DISubroutineType(types: !12)
>>> +!12 = !{!7}
>>> +!13 = !{!14, !15}
>>> +!14 = !DIGlobalVariable(name: "X", scope: !0, file: !1, line: 1, type: !7, isLocal: false, isDefinition: true, variable: i32* @X)
>>> +!15 = !DIGlobalVariable(name: "U", scope: !0, file: !1, line: 2, type: !7, isLocal: false, isDefinition: true, variable: i32* @U)
>>> +!16 = !{i32 2, !"Dwarf Version", i32 4}
>>> +!17 = !{i32 2, !"Debug Info Version", i32 3}
>>> +!18 = !{!"clang version 3.8.0 (trunk 251407) (llvm/trunk 251401)"}
>>> +!20 = !DILocation(line: 4, column: 13, scope: !4)
>>> +!21 = !DILocation(line: 9, column: 3, scope: !10)
>> 
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413