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

[ben via Phabricator via cfe-commits]

bd1976llvm marked 5 inline comments as done.
bd1976llvm added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:402
+  if (Config->DepLibs)
+    if (fs::exists(Specifier))
+      Driver->addFile(Specifier, /*WithLOption=*/false);
----------------
jyknight wrote:
> 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.
> 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.

I made the mistake of testing on an older copy of the binutils (from a 14.04 ubuntu). I get the same results as you with a more recent versions of the gnu tools. Apologies. Interestingly, binutils from 14.04 also allow absolute paths via -l:<absolute path> which is another behavior that has been removed.

> 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.

In this case are you envisioning that both of these files are competing copies of the same library, or is the "clang" in the current directory another type of file? I ask because if the "clang" in the current directory is not a library then the link will fail fast and the developer can then move this file somewhere else. If you had two different copies of the same library hanging around, then picking up the wrong one might cause bad behavior at runtime - that is the scenario that I consider to be remote.

For the example that you gave the change in precedence would be a fix, as it would cause the linker to pick up "libclang.so" and ignore "clang" in the current directory. FWIW we have been shipping with this behavior on PlayStation and it has not caused an issue for game links. 

If you have a hard objection to implicitly searching in the current directory *at all* then we will have to go with another option. I would rather not remove absolute path support as it would reduce MSVC compatibility. Potential other options are:

- We could remove support for cwd-relative paths and instead check the path to see if it is absolute and load the library directly if it is. 

- Another variation would be if the string contains at least one path separator, treat it as a path. That's simpler than checking for an absolute path. Users could also do 'pragma comment(lib, "./my.lib")' for relative paths too if they really wanted.



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

https://reviews.llvm.org/D60274