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

[Rui Ueyama via Phabricator via llvm-commits]

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