This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Fall back to search dirs for linker scripts specified with -T
ClosedPublic

Authored by arichardson on Nov 16 2017, 7:43 AM.

Details

Summary

This matches the behaviour of ld.bfd:
https://sourceware.org/binutils/docs/ld/Options.html#Options

If scriptfile does not exist in the current directory, ld looks for it in
the directories specified by any preceding ‘-L’ options. Multiple ‘-T’
options accumulate.

Diff Detail

Repository
rL LLVM

Event Timeline

arichardson created this revision.Nov 16 2017, 7:43 AM
ruiu added inline comments.Nov 16 2017, 7:07 PM
ELF/Driver.cpp
867 ↗(On Diff #123180)

Early-break after this line.

ELF/DriverUtils.cpp
223 ↗(On Diff #123180)

By convention, we don't have a blank line between a function comment and a function definition.

ELF/ScriptParser.cpp
355 ↗(On Diff #123180)

Please return after this line. In general, we prefer early return/break/continue.

grimar edited edge metadata.Nov 17 2017, 12:57 AM

Thanks for this patch, few comments/suggestions below.

ELF/DriverUtils.cpp
220 ↗(On Diff #123180)

I think should be script file as it is not a single word.
btw, can we just copy paste the GNU spec ?

I think we usually just describe LLD behavior and mention it agrees/disagrees with gnu linkers.
I do not think putting URLs here is very useful in that particular case as behavior is simple.
So I would rewrite this so that will something that just describes the behavior.

test/ELF/linkerscript/linker-script-in-search-path.s
1 ↗(On Diff #123180)

Could you put this below REQUIRES ? I think that is more consistent with other tests,
where REQURES is usually a first line. Probably that is not always true, but anyways..

13 ↗(On Diff #123180)

Please put ERROR check line right after this one, that way it allows keep check calls and check lines
be close to each other and look more isolated and convinent to read:

# If the linker script specified with -T is missing we should emit an error
# RUN: not ld.lld -Tfoo.script %t.o 2>&1 | FileCheck %s -check-prefix ERROR
# ERROR: error: cannot find linker script foo.script

The same for CHECK

23 ↗(On Diff #123180)

Does not seem you need any code here.

26 ↗(On Diff #123180)

You need new line here, that is consistent with other testcases.

arichardson marked 8 inline comments as done.

addressed reviews

grimar accepted this revision.Nov 17 2017, 6:53 AM

I have no futher comments, thanks !

This revision is now accepted and ready to land.Nov 17 2017, 6:53 AM

(please wait approvement from Rui)

arichardson added inline comments.Nov 17 2017, 8:52 AM
ELF/DriverUtils.cpp
220 ↗(On Diff #123180)

I just copied those two sentences from the bfd manual so I didn't bother to fix any typos. Anyway, I've deleted it now.

ruiu accepted this revision.Nov 17 2017, 9:59 PM

LGTM with these changes.

ELF/DriverUtils.cpp
217 ↗(On Diff #123329)

Remove else since the last if ends with return.

219 ↗(On Diff #123329)

Instead of returning None, directly return findFromSearchPaths(Name).

test/ELF/linkerscript/linker-script-in-search-path.s
20 ↗(On Diff #123329)

Please remove the redundant trailing newline.

This revision was automatically updated to reflect the committed changes.
arichardson marked 2 inline comments as done.