This is an archive of the discontinued LLVM Phabricator instance.

Differential D30277

[COFF] added test for thinlto
ClosedPublic

Authored by inglorion on Feb 22 2017, 2:54 PM.

Details

Reviewers
ruiu
pcc
Commits
rG851b1f866b0b: [COFF] added test for thinlto
rLLD296042: [COFF] added test for thinlto
rL296042: [COFF] added test for thinlto
Summary

Creates bitcode files suitable for use with ThinLTO, then checks that the linker can build an executable from them.

Diff Detail

Event Timeline

inglorion created this revision.Feb 22 2017, 2:54 PM
Herald added subscribers: Prazek, mehdi_amini. · View Herald TranscriptFeb 22 2017, 2:54 PM
mehdi_amini added a comment.Feb 22 2017, 3:14 PM

Maybe the testing is obvious to someone use to COFF, in which case I'm just out-of-place :)
Otherwise I'd rather have comment explaining what every check is doing.

test/COFF/thinlto.ll
9

is .obj the standard extension for object on Windows? (and .lib for archive?)

11

I don't know what is the /include directive doing? Is it preventing dead-stripping somehow? Or affecting the symbol resolution otherwise?

34

I don't know enough about COFF to know if what you're testing is relevant here. Especially I have no idea why you're checking the HEADERS (or what you're checking).

106

Why do we check this exact sequence of assembly? Overall I don't know what this test is enforcing / verifying right now.

pcc edited edge metadata.Feb 22 2017, 3:16 PM

For this test I wouldn't expect to see anything more than a basic test that uses temporaries to verify that importing is working. See http://llvm-cs.pcc.me.uk/tools/lld/test/ELF/lto/thinlto.ll for example.

pcc added a comment.Feb 22 2017, 3:21 PM

See also r295901 which changes the ELF test to be closer to what I'd expect this test to be doing.

inglorion updated this revision to Diff 89440.Feb 22 2017, 4:15 PM
inglorion edited the summary of this revision. (Show Details)

@pcc: How is this instead?

inglorion added inline comments.Feb 22 2017, 4:19 PM
test/COFF/thinlto.ll
9

Yes.

11

Right. It instructs the linker to retain that symbol, regardless of what it would otherwise do.

106

This was essentially performing the same checks as lto.ll. However, per @pcc's suggestion, I've changed the test to be much simpler and not actually look at the disassembly.

pcc accepted this revision.Feb 22 2017, 4:44 PM

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO. Other than that, LGTM.

This revision is now accepted and ready to land.Feb 22 2017, 4:44 PM
mehdi_amini added inline comments.Feb 22 2017, 4:59 PM
test/COFF/thinlto.ll
106

Thanks, looks good!
(and I agree with the idea of using llvm-nm where possible/appropriate)

inglorion added a comment.Feb 23 2017, 11:30 AM

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

pcc added a comment.Feb 23 2017, 11:36 AM
In D30277#684874, @inglorion wrote:

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

Do you mean that main.exe.lto.obj only contains a definition of main? That is what I'd expect to see if foo was imported into main and inlined.

mehdi_amini added a comment.Feb 23 2017, 11:53 AM
In D30277#684874, @inglorion wrote:

Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.

That was my original plan, but it looks like llvm-nm only tells us about main. Is that enough? Is there a way to get it to give us more information? I wrote the test the way I wrote it because I felt just looking at the output from llvm-nm doesn't really tell us if ThinLTO is working as intended.

If there is no undefined reference to foo from main, then it means it worked :)

inglorion updated this revision to Diff 89554.Feb 23 2017, 1:12 PM

use llvm-nm instead of looking at the bitcode

inglorion edited the summary of this revision. (Show Details)Feb 23 2017, 1:12 PM
mehdi_amini added inline comments.Feb 23 2017, 1:15 PM
test/COFF/thinlto.ll
9

assuming llvm-nm on COFF looks like ELF, what you need to check is not that main is present, but that foo isn't.

pcc added inline comments.Feb 23 2017, 1:15 PM
test/COFF/thinlto.ll
9

; CHECK-NOT: U foo?

inglorion updated this revision to Diff 89561.Feb 23 2017, 2:00 PM

check for absence of undefined foo, instead of presence of main

pcc added a comment.Feb 23 2017, 3:43 PM

LGTM

Closed by commit rL296042: [COFF] added test for thinlto (authored by inglorion). · Explain WhyFeb 23 2017, 3:54 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
test/
COFF/
130 lines

Diff 89430

test/COFF/thinlto.ll

  • This file was added.
; RUN: rm -fr %T/thinlto
; RUN: mkdir %T/thinlto
; RUN: opt -module-summary -o %T/thinlto/main.thinlto.obj %s
; RUN: opt -module-summary -o %T/thinlto/foo.thinlto.obj %S/Inputs/lto-dep.ll
; RUN: llvm-ar cru %T/thinlto/foo.thinlto.lib %T/thinlto/foo.thinlto.obj
; RUN: llc -filetype=obj -o %T/thinlto/main.obj %s
; RUN: llc -filetype=obj -o %T/thinlto/foo.obj %S/Inputs/lto-dep.ll
; RUN: llvm-ar cru %T/thinlto/foo.lib %T/thinlto/foo.obj
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

is .obj the standard extension for object on Windows? (and .lib for archive?)

mehdi_amini: is `.obj` the standard extension for object on Windows? (and `.lib` for archive?)
inglorionAuthorUnsubmitted
Not Done
ReplyInline Actions

Yes.

inglorion: Yes.
pccUnsubmitted
Not Done
ReplyInline Actions

; CHECK-NOT: U foo?

pcc: `; CHECK-NOT: U foo`?
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

assuming llvm-nm on COFF looks like ELF, what you need to check is not that main is present, but that foo isn't.

mehdi_amini: assuming llvm-nm on COFF looks like ELF, what you need to check is not that main is present…
; RUN: lld-link /out:%T/thinlto/main.exe /entry:main /include:f2 /subsystem:console %T/thinlto/main.thinlto.obj %T/thinlto/foo.thinlto.obj
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

I don't know what is the /include directive doing? Is it preventing dead-stripping somehow? Or affecting the symbol resolution otherwise?

mehdi_amini: I don't know what is the `/include` directive doing? Is it preventing dead-stripping somehow?
inglorionAuthorUnsubmitted
Not Done
ReplyInline Actions

Right. It instructs the linker to retain that symbol, regardless of what it would otherwise do.

inglorion: Right. It instructs the linker to retain that symbol, regardless of what it would otherwise do.
; RUN: llvm-readobj -file-headers %T/thinlto/main.exe | FileCheck -check-prefix=HEADERS-11 %s
; RUN: llvm-objdump -d %T/thinlto/main.exe | FileCheck -check-prefix=TEXT-11 %s
; RUN: lld-link /out:%T/thinlto/main.exe /entry:main /include:f2 /subsystem:console %T/thinlto/main.thinlto.obj %T/thinlto/foo.thinlto.lib /verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
; RUN: llvm-readobj -file-headers %T/thinlto/main.exe | FileCheck -check-prefix=HEADERS-11 %s
; RUN: llvm-objdump -d %T/thinlto/main.exe | FileCheck -check-prefix=TEXT-11 %s
; RUN: lld-link /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.thinlto.obj
; RUN: llvm-readobj -file-headers %T/thinlto/main.exe | FileCheck -check-prefix=HEADERS-01 %s
; RUN: llvm-objdump -d %T/thinlto/main.exe | FileCheck -check-prefix=TEXT-01 %s
; RUN: lld-link /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.obj %T/thinlto/foo.thinlto.lib
; RUN: llvm-readobj -file-headers %T/thinlto/main.exe | FileCheck -check-prefix=HEADERS-01 %s
; RUN: llvm-objdump -d %T/thinlto/main.exe | FileCheck -check-prefix=TEXT-01 %s
; RUN: lld-link /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.thinlto.obj %T/thinlto/foo.obj
; RUN: llvm-readobj -file-headers %T/thinlto/main.exe | FileCheck -check-prefix=HEADERS-10 %s
; RUN: llvm-objdump -d %T/thinlto/main.exe | FileCheck -check-prefix=TEXT-10 %s
; RUN: lld-link /out:%T/thinlto/main.exe /entry:main /subsystem:console %T/thinlto/main.thinlto.obj %T/thinlto/foo.lib
; RUN: llvm-readobj -file-headers %T/thinlto/main.exe | FileCheck -check-prefix=HEADERS-10 %s
; RUN: llvm-objdump -d %T/thinlto/main.exe | FileCheck -check-prefix=TEXT-10 %s
; VERBOSE: foo.thinlto.lib({{.*}}foo.thinlto.obj)
; HEADERS-11: AddressOfEntryPoint: 0x1000
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

I don't know enough about COFF to know if what you're testing is relevant here. Especially I have no idea why you're checking the HEADERS (or what you're checking).

mehdi_amini: I don't know enough about COFF to know if what you're testing is relevant here. Especially I…
; TEXT-11: Disassembly of section .text:
; TEXT-11-NEXT: .text:
; TEXT-11-NEXT: xorl %eax, %eax
; TEXT-11-NEXT: retq
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: int3
; TEXT-11-NEXT: movl $2, %eax
; TEXT-11-NEXT: retq
; HEADERS-01: AddressOfEntryPoint: 0x2000
; TEXT-01: Disassembly of section .text:
; TEXT-01-NEXT: .text:
; TEXT-01-NEXT: subq $40, %rsp
; TEXT-01-NEXT: callq 23
; TEXT-01-NEXT: xorl %eax, %eax
; TEXT-01-NEXT: addq $40, %rsp
; TEXT-01-NEXT: retq
; TEXT-01-NEXT: retq
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: int3
; TEXT-01-NEXT: retq
; HEADERS-10: AddressOfEntryPoint: 0x2020
; TEXT-10: Disassembly of section .text:
; TEXT-10-NEXT: .text:
; TEXT-10-NEXT: retq
; TEXT-10-NEXT: nopw %cs:(%rax,%rax)
; TEXT-10-NEXT: retq
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: int3
; TEXT-10-NEXT: subq $40, %rsp
; TEXT-10-NEXT: callq -41
; TEXT-10-NEXT: xorl %eax, %eax
; TEXT-10-NEXT: addq $40, %rsp
; TEXT-10-NEXT: retq
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

Why do we check this exact sequence of assembly? Overall I don't know what this test is enforcing / verifying right now.

mehdi_amini: Why do we check this exact sequence of assembly? Overall I don't know what this test is…
inglorionAuthorUnsubmitted
Not Done
ReplyInline Actions

This was essentially performing the same checks as lto.ll. However, per @pcc's suggestion, I've changed the test to be much simpler and not actually look at the disassembly.

inglorion: This was essentially performing the same checks as lto.ll. However, per @pcc's suggestion, I've…
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

Thanks, looks good!
(and I agree with the idea of using llvm-nm where possible/appropriate)

mehdi_amini: Thanks, looks good! (and I agree with the idea of using llvm-nm where possible/appropriate)
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"
define i32 @main() {
call void @foo()
ret i32 0
}
declare void @foo()
$f1 = comdat any
define i32 @f1() comdat($f1) {
ret i32 1
}
$f2 = comdat any
define i32 @f2() comdat($f2) {
ret i32 2
}
define internal void @internal() {
ret void
}