This is an archive of the discontinued LLVM Phabricator instance.

Fix ELF/linkerscript/input-archive.s w/ @ in path
ClosedPublic

Authored by thopre on Apr 29 2020, 10:04 AM.

Details

Summary

Lld test ELF/linkerscript/input-archive.s fails when path contain a @
because is not accepted in unquoted token in linker scripts which leads
to the path being broken in 2 around the @. This commit quotes the path
used in the linker script created by this and similar testcases allowing
the test to pass even in the presence of an @ sign in the path.

Diff Detail

Event Timeline

thopre created this revision.Apr 29 2020, 10:04 AM

This is ok, but can you fix all tests in one patch? For example, linkerscript/thunk-gen-mips.s has the same problem.

because @ is a keyword in linker script which leads to the path being broken in 2 around the @

@ is not a keyword.

@ is not considered part of an unquoted token.

thopre edited the summary of this revision. (Show Details)Apr 29 2020, 11:19 AM
thopre updated this revision to Diff 260969.Apr 29 2020, 11:19 AM

Also fix thunk-gen-mips test

You did not fix lld/test/ELF/linkerscript/thunk-gen-mips.s initially. This made me wonder how you found the issue of lld/test/ELF/linkerscript/input-archive.s.

Have you invoked ninja check-lld to ensure every test is good now?

You did not fix lld/test/ELF/linkerscript/thunk-gen-mips.s initially. This made me wonder how you found the issue of lld/test/ELF/linkerscript/input-archive.s.

Have you invoked ninja check-lld to ensure every test is good now?

Wild guess: @thopre doesn't have all targets enabled?

thopre marked an inline comment as done.Apr 30 2020, 8:48 AM

You did not fix lld/test/ELF/linkerscript/thunk-gen-mips.s initially. This made me wonder how you found the issue of lld/test/ELF/linkerscript/input-archive.s.

Have you invoked ninja check-lld to ensure every test is good now?

Wild guess: @thopre doesn't have all targets enabled?

Indeed, but that wouldn't have caught it anyway. See comment I've added on thunk-gen-mips.s

lld/test/ELF/linkerscript/thunk-gen-mips.s
18–20

CHECK-ANY is not a valid FileCheck directive and the FileCheck invocation uses the default prefix. This test is only testing the presence of SYMBOL TABLE in the output of llvm-objdump -t :-)

thopre marked an inline comment as done.Apr 30 2020, 8:50 AM
thopre added inline comments.
lld/test/ELF/linkerscript/thunk-gen-mips.s
18–20

The test seems to have bitrotted as a result. This is the output of objdump -t I get when I run it manually:

SYMBOL TABLE:
00000001 l *ABS* 00000000 MAIN
00000001 l *ABS* 00000000 TARGET
00108050 l .got 00000000 .hidden _gp
0010003c l F .text 00000010 __LA25Thunk_too_far
00000030 g .text 00000000 _start
00100050 g F .text 0000000c too_far

So wrong address and flag for _start, wrong address and size for too_far.

I presume CHECK-ANY was meant to be CHECK-DAG and thus the order does not matter.

thopre updated this revision to Diff 261248.Apr 30 2020, 8:51 AM

Fix quoting in ELF/linkerscript/thunk-gen-mips.s

MaskRay added inline comments.Apr 30 2020, 9:24 AM
lld/test/ELF/linkerscript/thunk-gen-mips.s
7

A better fix is to change the outermost " to ' and use unescaped "%t"

thopre updated this revision to Diff 261272.Apr 30 2020, 10:02 AM

Use single quoting for linker script

thopre marked an inline comment as done.Apr 30 2020, 10:02 AM
MaskRay accepted this revision.Apr 30 2020, 10:44 AM

Please add [test] to the commit subject. You can do this before committing arcfilter; git pull --rebase origin master; last-minute-testing(ninja check-lld-elf) && git push origin HEAD:master where arcfilter is a shell function which drops unneeded Phabricator tags (Subscribers:, Summary:, Tags:, Reviewers:) but keeps Differential Revision: and Reviewed By:

arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}
This revision is now accepted and ready to land.Apr 30 2020, 10:44 AM
This revision was automatically updated to reflect the committed changes.