Page MenuHomePhabricator

[llvm-libtool-darwin] Add support for -l and -L
ClosedPublic

Authored by sameerarora101 on Aug 7 2020, 11:07 AM.

Details

Summary

Add support for passing in libraries via -l and -L options. Depends
on D85334.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sameerarora101 added inline comments.Aug 7 2020, 11:13 AM
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
2

It doesn't make sense to test -L option individually as the directory specified under that option is used (prepended) only if some library is also specified with option -l.

To test -l individually, I would need to create a library in one of the standard directories (/lib/, /usr/lib, /usr/local/lib). Is there a way I can do that in a test file? Thanks.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
84โ€“88

I defined StandarDirs based on cctools' libtool:

/* the standard directories to search for -lx names */
char *standard_dirs[] = {
    "/lib/",
    "/usr/lib/",
    "/usr/local/lib/",
    NULL
};
smeenai added inline comments.Aug 10 2020, 8:31 PM
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
2

I don't think so. Other tools have options (like --sysroot) to re-root the search, but it doesn't appear that libtool has anything like that, so I don't think we'll be able to test that case.

8

I'd prefer for a file ending in .o to actually be an object file and not an archive :)

86

Other test ideas:

  • If multiple directories specified with -L have the same named file in them, the file from the first directory should be selected.
  • You should be able to combine -l with input files specified normally (i.e. as paths).
  • What happens if we specify the same -l option twice? (It's okay if it differs from cctools' behavior, but we should make a note.)
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
77

How about LibrarySearchDirs?

84

Nit: I'd prefer a std::array. I think you can make it constexpr too?

I'd also prefer StandardSearchDirs.

85โ€“87

I don't think you need the trailing slashes.

101

Any reason for creating this variable? SmallString can be implicitly converted to StringRef instead, and it can also be converted to std::string explicitly (e.g. via std::string(Path)).

103

Nit: you don't need the braces

110

The duplication between the two loops is unfortunate, but I don't think the body of the loops is large enough to justify moving it out into a function (especially if you get rid of the PathString variable). What we really want is a C++ equivalent to Python's itertools.chain, and I think we might get that with ranges in C++20.

124

const &

125

One of the following should work, I think.

StringRef BaseName = s;
StringRef BaseName(s);
129

I would construct a Twine and have searchForFile take const Twine & instead.

sameerarora101 marked 11 inline comments as done.Aug 11 2020, 12:31 PM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
8

I just wanted to check that a library ending in .o is not prepended/appended with some additional string lol. How about the name archive-object.o?

86

Thanks, added now.

For the second test case, I assumed you meant passing in a library with -l along with a normal input file? Or did you mean prepending -l to an input file name?

For the third case, libtool throws a warning (similar to what it does when we specify the same input file twice). I have added a comment about that in my test case above now.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
101

Ah forgot about the explicit conversion, thanks!

125

Both work ๐Ÿ˜„

129

Seems like I can't construct a Twine before the if condition and modify it appropriately in the corresponding branches (which is needed so as to have the Twine variable in scope after the if condition). Furthermore, I also cannot directly call searchForFile(<Twine>); within each branch and assign the result to Expected<std::string> FullPath, as I cannot declare Expected<std::string> FullPath; before the if branch.

Sorry if I didn't get what you were trying to explain here ?

sameerarora101 marked 4 inline comments as done.Aug 11 2020, 12:33 PM

Add more tests.

smeenai added inline comments.Aug 11 2020, 12:37 PM
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
8

I think the intent is, even though the option is -l (with the l presumably standing for library), you can still use it to specify input object files (so that you don't have to specify their full path). What I meant was to just have an object file and use -l to add that, instead of creating an archive and giving it a .o extension.

86

Yup, that's what I meant for the second case.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
129

Hmm, good points. Lemme think if there's anything else we can do here.

llvm/test/tools/llvm-libtool-darwin/L-and-l.test
8

Ah I see. Ok, I'll update it.

jhenderson added inline comments.Aug 12 2020, 1:26 AM
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
8

You should place lines like this that are tied to an individual test-case, below the comment that goes with the case, as that explains the purpose of the line.

27
117

I might suggest changing the -l file's name here to something even more arbitrary such as "-lfile-will-not-exist". Since libtool searches certain directories by default, there's a (small) chance the file being searched for is in one of those directories. The weirder we make the name, the smaller this chance, and therefore the less likely this test case will fail on somebody's system.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
72โ€“74

I just want to make sure the behaviour here is definitely what's documented and that it doesn't search for libfoo.o.a for -lfoo.o? We probably need a test case for that.

97

Could this be moved into the loop body, to avoid the Path.clear() line?

110

Perhaps a lambda could work okay here:

auto FindLib = [](ArrayRef<StringRef> SearchDirs) {
  for (StringRef Dir : SearchDirs) {
    SmallString<128> Path;
    sys::path::append(Path, Dir, FileName);

    if (sys::fs::exists(Path))
      return std::string(Path);
  }
  return "";
};

std::string Found = FindLib(LibrarySearchDirs);
if (Found.empty())
  Found = FindLib(StandardSearchDirs);
if (!Found.empty())
  return Found;
// report error
116
121

Can't you just do for(StringRef BaseName : Libraries)?

If not, at least do s -> S

129

How about something like:

Expected<std::string> FullPath = searchForFile(BaseName.endswith(".o") ?
                                               BaseName.str() :
                                               "lib" + BaseName + ".a");
sameerarora101 marked 8 inline comments as done.Aug 12 2020, 1:02 PM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
72โ€“74

Yup, the behavior here is definitely what's documented. My first test case is testing this only where I pass in -larchive-object.o. Also, I have now changed that test by passing in -l%basename_t.tmp-input1.o instead of archive-object as per @smeenai's recommendation above.

110

I like the lambda too, updated now. Thanks!

116

it is not trying to locate '%s' but might add lib and .a to the string and try locating lib%s.a, right? That's what I meant up there. I can change it to what you're suggesting if it doesn't make sense?

129

works, thanks.

sameerarora101 marked 3 inline comments as done.

Use lambda function.

sameerarora101 added inline comments.Aug 12 2020, 1:31 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
84

Seems like I cannot have constexpr with std::array<std::string, 3>. It says, "constexpr variable cannot have non-literal type 'const std::array<std::string, 3>' (aka 'const array<basic_string<char>, 3>')".

smeenai added inline comments.Aug 12 2020, 2:21 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
72โ€“74

I think @jhenderson meant that we should also add a case where you have e.g. a file called libfoo.o.a, you pass -lfoo.o, and you make sure that you get an error about a missing file (i.e. confirming that when you have a -l argument ending in .o, you only search for the name as-is).

sameerarora101 marked an inline comment as done.Aug 12 2020, 2:28 PM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
72โ€“74

Oh, i see now. I'll add a test. Thanks for clarifying!

sameerarora101 updated this revision to Diff 285184.EditedAug 12 2020, 2:53 PM
sameerarora101 marked an inline comment as done.

Add another test to check that files ending in '.o' are matched as-is.

smeenai added inline comments.Aug 12 2020, 3:59 PM
llvm/docs/CommandGuide/llvm-libtool-darwin.rst
65

Nit: remove the trailing slash on /lib/

llvm/test/tools/llvm-libtool-darwin/L-and-l.test
101

Can you test this for a name ending with .a as well?

103

This one too?

112

You need the -L%t/dirname as well, right?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
84

Ah, oh well.

106

I'd prefer an Optional<std::string> instead of having the empty string represent the absence.

116

FileName will have already had the lib and .a additions at this point though, right?

129

Ah, I keep forgetting about ternary expressions.

jhenderson added inline comments.Aug 13 2020, 12:33 AM
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
24

Maybe worth naming archives with a single member in them after the member's name, e.g. here libinput2.a

I think that helps readability, also possibly reusability (see below).

58โ€“59

Rather than create two new archives here, you could just create one, in otherDirname, containing input1.o. You already have a library in dirname containing input2.o, so if you name the new one the same, you can use that one too.

110

Let's also add a test case with a file extension other than these two, e.g. .ext and show that the behaviour is specific to '.o'.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
121

It's not particularly idiomatic to put const with StirngRef.

129

I avoid them in most cases, but this seems like a sensible usage.

sameerarora101 marked 14 inline comments as done.Aug 13 2020, 8:53 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
101

Did you mean for a file to which '.a' is appended, right? (The name itself doesn't end in '.a' but since the name doesn't end in '.o', 'lib' and '.a' are added to it)?

If that is what you meant, added a test for it now. Thanks.

112

yup, added. thanks

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
116

ah yes, you are right. Replaced as suggested.

sameerarora101 marked 3 inline comments as done.Aug 13 2020, 8:53 AM

Add more tests.

jhenderson added inline comments.Aug 14 2020, 12:17 AM
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
96โ€“99

One more test for this area: a file called something like file, and show that -lfile doesn't find it (since lib and .a are added). Relatedly, maybe also show that -llibfile.a looks for liblibfile.a not libfile.a?

sameerarora101 marked an inline comment as done.Aug 14 2020, 6:54 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/L-and-l.test
96โ€“99

Added now, thanks.

sameerarora101 marked an inline comment as done.

Add 2 more tests for file not found.

jhenderson accepted this revision.Aug 14 2020, 7:02 AM

No more comments from me, thanks!

This revision is now accepted and ready to land.Aug 14 2020, 7:02 AM
smeenai accepted this revision.Aug 14 2020, 9:59 AM

LGTM

This revision was automatically updated to reflect the committed changes.