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

[ben via Phabricator via llvm-commits]

bd1976llvm marked an inline comment 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:
> > > 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)?



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

https://reviews.llvm.org/D60274