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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to