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

Tue Nov 3 06:04:21 PST 2015

On Mon, Nov 2, 2015 at 4:13 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> 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.

Ok, reproduced with a simple named metadata and renamed the test accordingly.

Thanks,
Teresa

>
>>
>>>
>>>> +!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
>

-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413