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

Sun Nov 1 18:42:05 PST 2015

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.

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