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

[ben via Phabricator via llvm-commits]

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<std::string> 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(&Sec), this);
+      for (const uint8_t *P = Contents.begin(), *E = Contents.end(); P < E;) {
+        StringRef A = reinterpret_cast<const char *>(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<DepLib> DepLibs;
 };
----------------
pcc wrote:
> Maybe just `Range<Str>`?
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