This is an archive of the discontinued LLVM Phabricator instance.

Differential D79270

[test] Fix lld's ELF/linkerscript/thunk-gen-mips.s
ClosedPublic

Authored by thopre on May 1 2020, 3:29 PM.

Details

Reviewers
MaskRay
arichardson
ruiu
psmith
atanasyan
• espindola
Commits
rGd735c7048c6c: [test] Fix lld's ELF/linkerscript/thunk-gen-mips.s
Summary

Lld test ELF/linkerscript/thunk-gen-mips.s was accidentally disabled due
to the use of wrong FileCheck directives. As a result the test seems to
have bitrotted as it fails to pass if fixing the directive. To ease
updates to the test in case of change of the start address the checks
have been changed to use numeric variables to express all the addresses
based on the start address.

Diff Detail

Event Timeline

thopre created this revision.May 1 2020, 3:29 PM
Herald added a reviewer: • espindola. · View Herald TranscriptMay 1 2020, 3:29 PM
Herald added subscribers: jrtc27, sdardis, emaste. · View Herald Transcript
thopre edited the summary of this revision. (Show Details)May 1 2020, 3:31 PM
MaskRay added inline comments.May 1 2020, 4:02 PM
lld/test/ELF/linkerscript/thunk-gen-mips.s
18–20

What does [[#%x, START_ADDR:<0x100000]] mean?

s/was/were/ I think

Harbormaster completed remote builds in B55503: Diff 261560.May 1 2020, 8:01 PM
atanasyan accepted this revision.May 1 2020, 11:20 PM

LGTM

Initially the test case just checks that LLD does not crash under some conditions. It's not mandatory to test exact symbols' values and/or offsets between symbols. But after this fix the test additionally becomes more tolerant to exact symbol addresses. It's fine.
As to me, I'd remove the "Ideally we'd use..." comment.

This revision is now accepted and ready to land.May 1 2020, 11:20 PM
thopre marked 2 inline comments as done.May 2 2020, 2:37 PM
thopre added inline comments.
lld/test/ELF/linkerscript/thunk-gen-mips.s
18–20

It is not supported by FileCheck yet but the syntax for numeric expression was designed to allow for it. The meaning would be:

Match an lower case hex number (%x) that is less in value than 0x100000 and assign its value to START_ADDR. But reading the testcase again it is clear the address of start does not matter but only the offset of the 2 other symbols relative to it. Therefore I'm going to remove the 0x30 and just set START_ADDR to whatever is the address of start unconditionally.

thopre updated this revision to Diff 261667.May 2 2020, 2:38 PM
thopre marked an inline comment as done.

Set START_ADDR from llvm-objdump output instead of expecting 0x30.

thopre edited the summary of this revision. (Show Details)May 2 2020, 2:38 PM
Closed by commit rGd735c7048c6c: [test] Fix lld's ELF/linkerscript/thunk-gen-mips.s (authored by thopre). · Explain WhyMay 2 2020, 3:22 PM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B55560: Diff 261667.May 2 2020, 3:54 PM

Revision Contents

PathSize
lld/
test/
ELF/
linkerscript/
6 lines

Diff 261670

lld/test/ELF/linkerscript/thunk-gen-mips.s

Show All 9 Lines
# RUN: "%t"(.text) \ # RUN: "%t"(.text) \
# RUN: . = . + 0x100000 ; \ # RUN: . = . + 0x100000 ; \
# RUN: "%t1"(.text) \ # RUN: "%t1"(.text) \
# RUN: } \ # RUN: } \
# RUN: }' > %t.script # RUN: }' > %t.script
# RUN: ld.lld -o %t.exe --script %t.script %t %t1 # RUN: ld.lld -o %t.exe --script %t.script %t %t1
# RUN: llvm-objdump -t %t.exe | FileCheck %s # RUN: llvm-objdump -t %t.exe | FileCheck %s
# CHECK: SYMBOL TABLE: # CHECK: SYMBOL TABLE:
# CHECK-ANY: 00000000 .text 00000000 _start # CHECK-DAG: [[#%x, START_ADDR:]] g .text 00000000 _start
# CHECK-ANY: 0010000c l F .text 00000010 __LA25Thunk_too_far # CHECK-DAG: {{0*}}[[#THUNK_ADDR:START_ADDR+0x100000+12]] l F .text 00000010 __LA25Thunk_too_far
# CHECK-ANY: 00100020 g F .text 00000024 too_far # CHECK-DAG: {{0*}}[[#THUNK_ADDR+20]] g F .text 0000000c too_far
MaskRayUnsubmitted
Done
ReplyInline Actions

What does [[#%x, START_ADDR:<0x100000]] mean?

s/was/were/ I think

MaskRay: What does `[[#%x, START_ADDR:<0x100000]]` mean? s/was/were/ I think
thopreAuthorUnsubmitted
Done
ReplyInline Actions

It is not supported by FileCheck yet but the syntax for numeric expression was designed to allow for it. The meaning would be:

Match an lower case hex number (%x) that is less in value than 0x100000 and assign its value to START_ADDR. But reading the testcase again it is clear the address of start does not matter but only the offset of the 2 other symbols relative to it. Therefore I'm going to remove the 0x30 and just set START_ADDR to whatever is the address of start unconditionally.

thopre: It is not supported by FileCheck yet but the syntax for numeric expression was designed to…
.ifdef MAIN .ifdef MAIN
.global _start .global _start
_start: _start:
j too_far j too_far
nop nop
.endif .endif
Show All 12 Lines