[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

Tue Apr 9 08:00:33 PDT 2019

jyknight added inline comments.

================
Comment at: lld/ELF/InputFiles.cpp:402
+  if (Config->DepLibs)
+    if (fs::exists(Specifier))
+      Driver->addFile(Specifier, /*WithLOption=*/false);
----------------
bd1976llvm wrote:
> jyknight wrote:
> > bd1976llvm wrote:
> > > jyknight wrote:
> > > > This bit seems unfortunate. "-lfoo" won't search for a file named "foo", and having this code do so does not seem like a good idea. I'd want this feature to work just like -l.
> > > > 
> > > > Actually, I think it'd be better to preserve the "-l" prefix in the deplibs section. (I'd note that even if libs in the search path are spelled "-lfoo", that doesn't mean we need accept any other options)
> > > > 
> > > > If we want to support loading a filename which not on the search path (but...do we, even? That seems like a kinda scary feature to me...), then it'd be obvious how to spell that: "libfoo.a" and "/baz/bar/foo.a" would open files directly, vs "-lfoo" and "-l:foo.a" would search for them on the search path.
> > > What you  have described is the behavior of the INPUT linker script command:  https://sourceware.org/binutils/docs/ld/File-Commands.html#File-Commands.
> > > 
> > > We have carefully designed this "dependent libraries" feature to avoid mapping "comment lib" pragmas to --library command line options. My reasons:
> > > 
> > > - I don't want the compiler doing string processing to try to satisfy the command line parsing behaviour of the target linker.
> > > 
> > > - I don't want to couple the source code to a GNU-style linker. After all there are other ELF linkers. Your proposal isn't merely an ELF linking proposal, it's a proposal for ELF toolchains with GNU-like linkers (e.g. the arm linker doesn't support the colon prefix http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0474c/Cjahbdei.html).
> > > 
> > > - The syntax is #pragma comment(lib, ...) not #pragma linker-option(library, ...) i.e. the only thing this (frankly rather bizarre) syntax definitely implies is that the argument is related to libraries (and comments ¯\_(ツ)_/¯); it is a bit of a stretch to interpret "comment lib" pragmas as mapping directly to "specifying an additional --library command line option".
> > > 
> > > - I want to avoid GNUism's and use a "general" mechanism. MSVC source code compatibility is a usecase.
> > > 
> > > - It is important to support loading a filename which not on the search path. For example I have seen developers use the following: #pragma comment(lib, __FILE__"\\..\\foo.lib")
> > > 
> > > I would like the following to work on for ELF:
> > > 
> > > #pragma comment(lib, "foo") => add libfoo.a to the link.
> > > #pragma comment(lib, "foo.lib") => add foo.lib to the link
> > > #pragma comment(lib, "c:\\foo.lib") => add c:\foo.lib to the link
> > > 
> > > The way the code is now these will work on both LLD and MSVC (and I think it will work for Apple's linker as well although I haven't checked). In addition, we have been careful to come up with a design that can be implemented by the GNU linkers... with the cost that some complicated links will not be possible to do via autolinking; in these (IMO rare) cases developers will need to use the command line directly instead.
> > > 
> > > What I am trying to accomplish is to try to keep #pragma comment(lib, "foo") compatible across linkers. Otherwise we will be in a situation where you will have to have the equivalent of...
> > > 
> > > #ifdef COFF_LIBRARIES:
> > > #pragma comment(lib, "/DEFAULTLIB:foo.lib")
> > > #'elif ELF_LIBRARIES_GNU_STYLE_LINKER:
> > > #pragma comment(lib, "-lfoo")
> > > #'elif DARWIN_FRAMEWORKS:
> > > #pragma comment(lib, "-framework foo")
> > > #esle
> > > #error Please specify linking model
> > > #endif
> > > 
> > > ... in your source code (or the moral equivalent somewhere else). We can avoid this.
> > > 
> > > Also note that I am not proposing to remove the .linker-options extension. We can add support for .linker-options to LLD in the future if we find a usecase where developers want pragmas that map directly to the linkers command line options for ELF. If this is a usecase then, in the future, we can implement support for #pragma comment(linker, ....) pragmas that lower to .linker-options; #pragma comment(lib, ...) pragmas can continue to lower to .deplibs.
> > It just seems to me a really bad idea to have a situation where specifying `#pragma comment(lib, "foo")` can either open a file named "foo" in the current directory, or search for libfoo.{a,so} in the library search path.
> > 
> > That is pretty much guaranteed to cause problems due to unintentional filename collision in the current-directory.
> > 
> Linkers already have the behaviour of finding the first matching file in the library search paths, and gnu-ld at least seems to add "." implicitly as the first library search path. So, developers already need to make sure that they don't have multiple copies of a library in the wrong place. This behaviour doesn't seem “too much” of a deviation from that. In other words I suspect that the chance of having a library named foo in the current directory and a library named libfoo.a in a library search path is remote, no?
> 
> Would your concerns be alleviated by changing the precedence, like so:
> 
> ```
> static void processDepLib(StringRef Specifier, const InputFile *F) {
>   if (!Config->DepLibs)
>     return;
>   else if (Optional<std::string> S = searchLibrary(Specifier))
>     Driver->addFile(*S, /*WithLOption=*/true);
>   else if (Optional<std::string> S = findFromSearchPaths(Specifier))
>       Driver->addFile(*S, /*WithLOption=*/true);
>   if (fs::exists(Specifier))
>       Driver->addFile(Specifier, /*WithLOption=*/false);
>   else
>     error(toString(F) +
>           ": unable to find library from dependent library specifier: " +
>           Specifier);
> }
> ```
> 
> We could also change this function to error if a given specifier would find a library in more than one location (assuming that there was no early out)?
> 
> gnu-ld at least seems to add "." implicitly as the first library search path

I don't see that behavior. Neither GNU ld.bfd, GNU ld.gold, nor llvm's lld use "." as a default library search path as far as my testing can determine. Neither when invoked directly, nor when invoked through the gcc and clang drivers.

> In other words I suspect that the chance of having a library named foo in the current directory and a library named libfoo.a in a library search path is remote, no?

No, I don't think the chance is remote at all. I can easily imagine linking against a library named "clang", ("libclang.so"), while having a file or directory named "clang" in the current directory, for example.

> Would your concerns be alleviated by changing the precedence

No, not really.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60274/new/

https://reviews.llvm.org/D60274