[PATCH] D60274: [ELF] Implement Dependent Libraries Feature
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 4 16:07:49 PDT 2019
ruiu added a comment.
First round of review for the lld part -- this patch generally looking good. Nice and compact and should be useful. I think I like this.
================
Comment at: lld/ELF/Config.h:124
CallGraphProfile;
+ bool DepLibs;
bool AllowMultipleDefinition;
----------------
We use the same name for a variable and a command line, thus `IgnoreDeplibs`.
================
Comment at: lld/ELF/Driver.cpp:763
+ Config->DepLibs = !Args.hasArg(OPT_ignore_deplibs);
Config->AllowMultipleDefinition =
----------------
Maybe something like `-autolink/-no-autolink` is more conventional.
================
Comment at: lld/ELF/Driver.cpp:1500
+ // appended to the Files vector.
+ for (size_t I = 0u; I < Files.size(); ++I)
+ Symtab->addFile<ELFT>(Files[I]);
----------------
nit: 0u → 0
================
Comment at: lld/ELF/InputFiles.cpp:400
+static void processDepLib(StringRef Specifier, const InputFile *F) {
+ if (Config->DepLibs)
----------------
Please add a function comment, e.g.
An ELF object file may contain an `.autolink` section. If exists, the section contains a library name such as `m` for libm. This function resolves a given name as if it were given via -l. This ELF extension is called autolinking and currently unique to LLVM and lld.
================
Comment at: lld/ELF/InputFiles.cpp:401
+static void processDepLib(StringRef Specifier, const InputFile *F) {
+ if (Config->DepLibs)
+ if (fs::exists(Specifier))
----------------
Let's flip the condition and return early.
================
Comment at: lld/ELF/InputFiles.cpp:654
+ case SHT_LLVM_DEPLIBS: {
+ if (!Config->Relocatable) {
+ ArrayRef<uint8_t> Contents =
----------------
Flip the condition and return early.
================
Comment at: lld/ELF/InputFiles.cpp:657
+ CHECK(this->getObj().getSectionContents(&Sec), this);
+ for (const uint8_t *P = Contents.begin(), *E = Contents.end(); P < E;) {
+ StringRef A = reinterpret_cast<const char *>(P);
----------------
Instead of allowing multiple names in a single .autolink section, why don't you create multiple .autolink sections?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60274/new/
https://reviews.llvm.org/D60274
More information about the llvm-commits
mailing list