Page MenuHomePhabricator

[ELF2] Add support for -L and -l command line switches
ClosedPublic

Authored by ikudrin on Sep 24 2015, 7:35 AM.

Details

Reviewers
ruiu
rafael
Summary

Just a basic variant.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 35625.Sep 24 2015, 7:35 AM
ikudrin retitled this revision from to [ELF2] Add support for -L and -l command line switches.
ikudrin updated this object.
ikudrin added a reviewer: rafael.
ikudrin added a project: lld.
ikudrin added a subscriber: llvm-commits.
rafael added inline comments.Sep 24 2015, 1:10 PM
ELF/Driver.cpp
63

Just emit the error from this function instead of returning ErrorOr

65

I don't think it is safe to use Twine link this:

http://llvm.org/docs/ProgrammersManual.html#the-twine-class

197

Not much value in adding this TODO now :-)

ELF/Driver.h
40 ↗(On Diff #35625)

prepare is a bit too generic.

How about something like openInputFiles?

42 ↗(On Diff #35625)

These 2 can be static helpers.

test/elf2/libsearch.s
21

You don't actually need any instructions in the test.

ruiu added a subscriber: ruiu.Sep 24 2015, 3:50 PM
ruiu added inline comments.
ELF/Driver.cpp
63

Please write a brief function comment saying that this function searches a given file path from paths specified with -L.

65

I'd use shorter name -- StaticName and DynamicName would be probably enough.

136

Most command line options are handled directly inside this function for clarity, and I'd think this should be done in the same way rather than delegating a task to a different function. We could do like this here.

for (auto *Arg : Args.filtered(OPT_l, OPT_INPUT)) {
  if (OPT_l) {
    Inputs.push_back(openLibrary(Arg->getValue());
    continue;
  }
  Inputs.push_back(openObject(Arg->getValue());
}
ikudrin updated this revision to Diff 35707.Sep 25 2015, 1:10 AM
  • Rebased to the top
  • Addressed review comments
ikudrin marked 3 inline comments as done.Sep 25 2015, 1:23 AM
ikudrin added inline comments.
ELF/Driver.cpp
63

The function will be used later for search libraries from handler of linker scripts. Error messages should be different in that case.

136

The loop will be extended to handle additional switches like -T, --no-whole-archive, --whole-archive, --as-needed, --no-as-needed and so on. I think that a smaller function for a specific subtask is a bit more comprehensible than one big function which does everything.

197

The comment clarifies the existence of the method with only one line of code within.

ELF/Driver.h
40 ↗(On Diff #35625)

Accepted. Thanks!

42 ↗(On Diff #35625)

They can't as they use the openFile() method.

test/elf2/libsearch.s
20

Thanks. I wonder, if we still need the 'REQUIRES: x86' instruction here?

denis-protivensky added inline comments.
ELF/Driver.cpp
72

Maybe better to have SmallVector<SmallString<64>, 2> Libs instead?
This solves couple of problems:

  1. Removes ambiguity when HasColonPrefix == true, because you'll put just one name into the vector instead of having two equal names like now.
  2. Removes code duplication in the search dir loop, because you'll have both dynamic and static names in the vector and will iterate over them in the unified way.
  3. When -static is added, you won't need to make complex checks or code changes. Just check for static flag and don't add dynamic name to the vector.
95

This check is not correct. Colon prefix affects only name being searched, but it doesn't allow to specify absolute name which should be used.

190

You need a negative test for this error case.

test/elf2/libsearch.s
9

Need to check that lld -flavor gnu2 -o %t3 -L%T -lls %t.o also fails with undefined symbol.

12

Check also this combination as correct: lld -flavor gnu2 -o %t3 %t.o -lls -L%T

ikudrin updated this revision to Diff 35715.Sep 25 2015, 4:46 AM
ikudrin marked an inline comment as done.
  • Reworked the function searchLibrary
  • Added two test cases
ikudrin marked 5 inline comments as done.Sep 25 2015, 4:56 AM
ikudrin added inline comments.
ELF/Driver.cpp
72

Thanks! I think the function is much better now.

89

Thanks for pointing it out. The logic was copied from the old ELF driver, but I agree that it's better to follow the behaviour of ld here.

test/elf2/libsearch.s
10

It won't fail with the current approach to symbol resolution.

ikudrin marked 3 inline comments as done.Sep 25 2015, 4:56 AM
ELF/Driver.cpp
77

Looks like you broke the logic a bit: you must put .so file first to check for it first.

This also means we don't have a test case to cover order of picking DSOs before archive files.
You may do lld -flavor gnu2 -o %t3 %t.o -L%T -lls command before creating archive file in the libsearch.s test and ensure no errors produced.

rafael added inline comments.Sep 25 2015, 7:08 AM
ELF/Driver.cpp
70

Do not return a ErrorOr. If we ever have to handle it differently, we will do evaluate how when needed.

For now just call error from within this function.

rafael added inline comments.Sep 25 2015, 7:13 AM
ELF/Driver.cpp
197

The TODO is not relevant to this patch. Please leave it out.

ruiu added a comment.Sep 25 2015, 8:51 AM

This patch doesn't seem very consistent with existing code in minor points such as naming convention, error handling, use of TODO, or notation used in comments. Could you please check other code and mimic what they do?

ELF/Driver.cpp
136

At least for now, we don't need that separate function. We can create any function when we need it. Please keep it in-line with other code.

ELF/Driver.h
43–45 ↗(On Diff #35715)

These function names are too generic. Please name them after what they actually do.

ikudrin updated this revision to Diff 35730.Sep 25 2015, 10:03 AM
  • Fixed priority in library search. The corresponding test is added.
  • Simplified the code by removing additional methods and a composite return value.
ikudrin marked 12 inline comments as done.Sep 25 2015, 10:06 AM
In D13135#253473, @ruiu wrote:

This patch doesn't seem very consistent with existing code in minor points such as naming convention, error handling, use of TODO, or notation used in comments. Could you please check other code and mimic what they do?

Thank you for your comments! I tried my best to follow the current style, but if I miss something I'll be very pleased to be pointed to.

ruiu added inline comments.Sep 25 2015, 10:30 AM
ELF/Driver.cpp
66

Better to use std::vector<std::string> because Twine::str() returns an std::string. Converting that to SmallString (through StringRef) is a bit odd.

75

Please write the real type instead of auto if the real type is not apparent in a very narrow context (for example, on RHS).

82
error(Twine("Unable to find library -l") + Path)

(because we wrote like this in other places.)

132–133

These two lines can be written in one line.

ikudrin updated this revision to Diff 35746.Sep 25 2015, 11:28 AM

Addressed latest comments.

ikudrin marked 4 inline comments as done.Sep 25 2015, 11:30 AM
ruiu accepted this revision.Sep 25 2015, 12:01 PM
ruiu added a reviewer: ruiu.

LGTM with this nit. Hold on for a while for other reviewers. Thanks!

ELF/Driver.cpp
70

Please do not try to optimize unless proved to be necessary. I'm pretty sure that this is marginal or even slightly negative.

This revision is now accepted and ready to land.Sep 25 2015, 12:01 PM
ikudrin updated this revision to Diff 35836.Sep 28 2015, 12:30 AM
ikudrin edited edge metadata.

Remove premature optimization.

ikudrin marked an inline comment as done.Sep 28 2015, 12:31 AM
rafael accepted this revision.Sep 28 2015, 5:50 AM
rafael edited edge metadata.

LGTM.

ikudrin closed this revision.Sep 28 2015, 6:13 AM

r248708