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

Sun Nov 1 13:52:29 PST 2015

> 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:

> 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.

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).

> @@ -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.

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