[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLD360984: [ELF] Implement Dependent Libraries Feature
(authored by bd1976llvm, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D60274?vs=195495=199972#toc
Repository:
rLLD LLVM Linker
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60274/new/
https://reviews.llvm.org/D60274
Files:
ELF/Config.h
ELF/Driver.cpp
ELF/Driver.h
ELF/DriverUtils.cpp
ELF/InputFiles.cpp
ELF/Options.td
test/ELF/Inputs/deplibs-lib_bar.s
test/ELF/Inputs/deplibs-lib_foo.s
test/ELF/deplibs-colon-prefix.s
test/ELF/deplibs-corrupt.s
test/ELF/deplibs.s
test/ELF/lto/deplibs.s
Index: ELF/Config.h
===
--- ELF/Config.h
+++ ELF/Config.h
@@ -137,6 +137,7 @@
bool Cref;
bool DefineCommon;
bool Demangle = true;
+ bool DependentLibraries;
bool DisableVerify;
bool EhFrameHdr;
bool EmitLLVM;
Index: ELF/Driver.cpp
===
--- ELF/Driver.cpp
+++ ELF/Driver.cpp
@@ -790,6 +790,7 @@
Config->DefineCommon = Args.hasFlag(OPT_define_common, OPT_no_define_common,
!Args.hasArg(OPT_relocatable));
Config->Demangle = Args.hasFlag(OPT_demangle, OPT_no_demangle, true);
+ Config->DependentLibraries = Args.hasFlag(OPT_dependent_libraries, OPT_no_dependent_libraries, true);
Config->DisableVerify = Args.hasArg(OPT_disable_verify);
Config->Discard = getDiscard(Args);
Config->DwoDir = Args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);
@@ -1548,9 +1549,11 @@
Symtab->trace(Arg->getValue());
// Add all files to the symbol table. This will add almost all
- // symbols that we need to the symbol table.
- for (InputFile *F : Files)
-parseFile(F);
+ // symbols that we need to the symbol table. This process might
+ // add files to the link, via autolinking, these files are always
+ // appended to the Files vector.
+ for (size_t I = 0; I < Files.size(); ++I)
+parseFile(Files[I]);
// Now that we have every file, we can decide if we will need a
// dynamic symbol table.
Index: ELF/InputFiles.cpp
===
--- ELF/InputFiles.cpp
+++ ELF/InputFiles.cpp
@@ -7,6 +7,7 @@
//===--===//
#include "InputFiles.h"
+#include "Driver.h"
#include "InputSection.h"
#include "LinkerScript.h"
#include "SymbolTable.h"
@@ -499,6 +500,27 @@
}
}
+// An ELF object file may contain a `.deplibs` section. If it exists, the
+// section contains a list of library specifiers such as `m` for libm. This
+// function resolves a given name by finding the first matching library checking
+// the various ways that a library can be specified to LLD. This ELF extension
+// is a form of autolinking and is called `dependent libraries`. It is currently
+// unique to LLVM and lld.
+static void addDependentLibrary(StringRef Specifier, const InputFile *F) {
+ if (!Config->DependentLibraries)
+return;
+ if (fs::exists(Specifier))
+Driver->addFile(Specifier, /*WithLOption=*/false);
+ else if (Optional S = findFromSearchPaths(Specifier))
+Driver->addFile(*S, /*WithLOption=*/true);
+ else if (Optional S = searchLibraryBaseName(Specifier))
+Driver->addFile(*S, /*WithLOption=*/true);
+ else
+error(toString(F) +
+ ": unable to find library from dependent library specifier: " +
+ Specifier);
+}
+
template
void ObjFile::initializeSections(
DenseSet ) {
@@ -740,6 +762,24 @@
}
return ::Discarded;
}
+ case SHT_LLVM_DEPENDENT_LIBRARIES: {
+if (Config->Relocatable)
+ break;
+ArrayRef Data =
+CHECK(this->getObj().template getSectionContentsAsArray(), this);
+if (!Data.empty() && Data.back() != '\0') {
+ error(toString(this) +
+": corrupted dependent libraries section (unterminated string): " +
+Name);
+ return ::Discarded;
+}
+for (const char *D = Data.begin(), *E = Data.end(); D < E;) {
+ StringRef S(D);
+ addDependentLibrary(S, this);
+ D += S.size() + 1;
+}
+return ::Discarded;
+ }
case SHT_RELA:
case SHT_REL: {
// Find a relocation target section and associate this section with that.
@@ -1302,6 +1342,9 @@
for (const lto::InputFile::Symbol : Obj->symbols())
Symbols.push_back(createBitcodeSymbol(KeptComdats, ObjSym, *this));
+
+ for (auto L : Obj->getDependentLibraries())
+addDependentLibrary(L, this);
}
static ELFKind getELFKind(MemoryBufferRef MB, StringRef ArchiveName) {
Index: ELF/Options.td
===
--- ELF/Options.td
+++ ELF/Options.td
@@ -71,6 +71,10 @@
"Apply link-time values for dynamic relocations",
"Do not apply link-time values for dynamic relocations (default)">;
+defm
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
phosek added a comment. @bd1976llvm do you plan on landing this? We'd really like to start using this feature. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60274/new/ https://reviews.llvm.org/D60274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
ruiu accepted this revision. ruiu added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60274/new/ https://reviews.llvm.org/D60274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
ruiu added a comment. I don't have any particular comment, and I'll give an LGTM soon as people seem to generally happy about this. If you are not happy about this patch or the feature itself, please shout. This feature is about to land. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60274/new/ https://reviews.llvm.org/D60274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
bd1976llvm added a comment. I am keen to keep this moving. I think there are a few things outstanding: 1. Need to resolve concerns w.r.t the design. 2. I need to find out whether I should be doing validation when reading the metadata in LLVM. 3. The LLD side needs a more detailed review. @jyknight @compnerd can we progress the design conversation? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60274/new/ https://reviews.llvm.org/D60274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
probinson added a comment. I'm okay with the PS4-specific bits. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60274/new/ https://reviews.llvm.org/D60274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
bd1976llvm updated this revision to Diff 195495.
bd1976llvm added a comment.
No longer shortening "dependent libraries" to "deplibs" except for the .deplibs
section (as this takes up bytes on disk).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60274/new/
https://reviews.llvm.org/D60274
Files:
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/CodeGen/TargetInfo.cpp
clang/test/CodeGen/dependent-lib.c
clang/test/CodeGen/elf-linker-options.c
clang/test/CodeGen/pragma-comment.c
clang/test/Modules/autolink.m
lld/ELF/Config.h
lld/ELF/Driver.cpp
lld/ELF/Driver.h
lld/ELF/DriverUtils.cpp
lld/ELF/InputFiles.cpp
lld/ELF/Options.td
lld/test/ELF/Inputs/deplibs-lib_bar.s
lld/test/ELF/Inputs/deplibs-lib_foo.s
lld/test/ELF/deplibs-colon-prefix.s
lld/test/ELF/deplibs-corrupt.s
lld/test/ELF/deplibs.s
lld/test/ELF/lto/deplibs.s
llvm/docs/Extensions.rst
llvm/docs/LangRef.rst
llvm/include/llvm-c/lto.h
llvm/include/llvm/BinaryFormat/ELF.h
llvm/include/llvm/LTO/LTO.h
llvm/include/llvm/LTO/legacy/LTOModule.h
llvm/include/llvm/Object/IRSymtab.h
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
llvm/lib/LTO/LTO.cpp
llvm/lib/LTO/LTOModule.cpp
llvm/lib/MC/MCParser/ELFAsmParser.cpp
llvm/lib/MC/MCSectionELF.cpp
llvm/lib/Object/ELF.cpp
llvm/lib/Object/IRSymtab.cpp
llvm/lib/ObjectYAML/ELFYAML.cpp
llvm/test/Feature/elf-deplibs.ll
llvm/test/LTO/Resolution/X86/symtab-elf.ll
llvm/test/MC/ELF/section.s
llvm/test/Object/X86/irsymtab.ll
llvm/tools/llvm-lto2/llvm-lto2.cpp
llvm/tools/llvm-readobj/ELFDumper.cpp
llvm/tools/lto/lto.cpp
Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -325,6 +325,10 @@
return unwrap(mod)->getLinkerOpts().data();
}
+const char* lto_module_get_dependent_libraries(lto_module_t mod) {
+return unwrap(mod)->getDependentLibraries().data();
+}
+
void lto_codegen_set_diagnostic_handler(lto_code_gen_t cg,
lto_diagnostic_handler_t diag_handler,
void *ctxt) {
Index: llvm/tools/llvm-readobj/ELFDumper.cpp
===
--- llvm/tools/llvm-readobj/ELFDumper.cpp
+++ llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -2830,6 +2830,8 @@
return "LLVM_CALL_GRAPH_PROFILE";
case SHT_LLVM_ADDRSIG:
return "LLVM_ADDRSIG";
+ case SHT_LLVM_DEPENDENT_LIBRARIES:
+return "LLVM_DEPENDENT_LIBRARIES";
// FIXME: Parse processor specific GNU attributes
case SHT_GNU_ATTRIBUTES:
return "ATTRIBUTES";
Index: llvm/tools/llvm-lto2/llvm-lto2.cpp
===
--- llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -360,6 +360,13 @@
if (TT.isOSBinFormatCOFF())
outs() << "linker opts: " << Input->getCOFFLinkerOpts() << '\n';
+if (TT.isOSBinFormatELF()) {
+ outs() << "dependent libraries:";
+ for (auto L : Input->getDependentLibraries())
+outs() << " \"" << L << "\"";
+ outs() << '\n';
+}
+
std::vector ComdatTable = Input->getComdatTable();
for (const InputFile::Symbol : Input->symbols()) {
switch (Sym.getVisibility()) {
Index: llvm/test/Object/X86/irsymtab.ll
===
--- llvm/test/Object/X86/irsymtab.ll
+++ llvm/test/Object/X86/irsymtab.ll
@@ -9,16 +9,17 @@
; BCA: blob data = '\x01\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00D\x00\x00\x00\x01\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x02\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00'
+; BCA-NEXT:blob data = '\x02\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00L\x00\x00\x00\x01\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x02\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00'
; BCA-NEXT:
; BCA-NEXT: blob data = 'foobarproducerx86_64-unknown-linux-gnuirsymtab.ll'
; BCA-NEXT:
-; SYMTAB: version: 1
+; SYMTAB: version: 2
; SYMTAB-NEXT: producer: producer
; SYMTAB-NEXT: target triple: x86_64-unknown-linux-gnu
; SYMTAB-NEXT: source filename: irsymtab.ll
+;
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
compnerd added inline comments.
Comment at: lld/ELF/InputFiles.cpp:662
}
+ case SHT_LLVM_DEPLIBS: {
+if (Config->Relocatable)
Can you make the flag here reflect the name as well?
(`SHT_LLVM_DEPENDENT_LIBRARIES`)
Comment at: lld/ELF/Options.td:74
+defm deplibs: B<"deplibs",
+"Process dependent library specifiers from input files (default)",
I think that I prefer the `--dependent-libraries` flag which is similar to the
recent additions to the linker options.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60274/new/
https://reviews.llvm.org/D60274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
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
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
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
[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
bd1976llvm marked 25 inline comments as done.
bd1976llvm added a comment.
I have updated the diff to address review comments. I think we can continue to
refine this patch in parallel with discussing the design further (I am not
dismissing the concerns raised by @compnerd and @jyknight).
I am unsure if I should be doing validation on the structure of the
llvm.dependent-libraries named metadata e.g. what if there are zero operands or
more than one operand for a metadata node. Does anyone know what LLVM's policy
is?
Comment at: lld/ELF/InputFiles.cpp:406
+ Driver->addFile(*S, /*WithLOption=*/true);
+else if (Optional S = searchLibrary(Specifier))
+ Driver->addFile(*S, /*WithLOption=*/true);
pcc wrote:
> `searchLibrary` contains the support for handling flags like `-l:foo`; I
> guess if you don't want that behaviour here then it will need to be factored
> out of `searchLibrary`.
I was undecided here. Refactoring the code to remove the colon prefix behavior
and adding other checks like only allowing libraries (not objects) to be added
to the link adds complexity that might not be justified. Maybe we should simply
document that some behaviors might work but are unsupported and liable to
change without warning?
Comment at: lld/ELF/InputFiles.cpp:657
+ CHECK(this->getObj().getSectionContents(), this);
+ for (const uint8_t *P = Contents.begin(), *E = Contents.end(); P < E;) {
+StringRef A = reinterpret_cast(P);
ruiu wrote:
> Instead of allowing multiple names in a single .autolink section, why don't
> you create multiple .autolink sections?
This would simplify the implementation but I don't think that the size
increases in the input files is worth it. Also, it is easier to dump the
dependent libraries if they are all in one section.
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:841
// for safe ICF.
+ SHT_LLVM_DEPLIBS = 0x6fff4c04,// LLVM Dependent Library Specifiers.
// Android's experimental support for SHT_RELR sections.
peter.smith wrote:
> pcc wrote:
> > peter.smith wrote:
> > > This is the same value as has been proposed in another ELF extension in
> > > https://reviews.llvm.org/D60242 , May be worth coordinating here. It is
> > > probably worth us having a central place to document the LLVM specific
> > > ELF extensions as we are accumulating enough of them.
> > I don't have any existing dependencies on the value that I've chosen. If
> > Ben has no dependencies of his own, I reckon that whoever ends up
> > committing first can just get this value, and the next person will get the
> > next one. I think this can be our general policy.
> >
> > Do you think that https://llvm.org/docs/Extensions.html#elf-dependent is
> > not sufficient as a place to document our extensions?
> I think it could be, now that I see it fully expanding with some of the
> extensions mentioning the elf terminology. I think we should mention the
> SHT_... and hex value so that someone doesn't have to dig for that in the
> source code.
I think that it is better if the hex value is in this one place and we use the
SHT_ token everywhere else. This reduces the potential for mistakes.
Comment at: llvm/include/llvm/Object/IRSymtab.h:157
+/// Dependent Library Specifiers
+ Range DepLibs;
};
pcc wrote:
> Maybe just `Range`?
Thanks! I went for the struct with a single element representation because that
is used for comdats. Should we make this same change for comdats?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60274/new/
https://reviews.llvm.org/D60274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
