[PATCH] D18959: [lld] Implement --dynamic-list

[Adhemerval Zanella via llvm-commits]

zatrazz added inline comments.

================
Comment at: ELF/Config.h:54
@@ -52,2 +53,3 @@
   std::vector<llvm::StringRef> Undefined;
+  DynamicList DynList;
   bool AllowMultipleDefinition;
----------------
ruiu wrote:
> Instead of adding DynamicList, please add DenseSet<StringRef>, because the simple set just works.
Right, but since the idea is to visit each dynamic list entry to check against the Symtab symbols, why not just a std::vector<llvm::StringRef> as well?

================
Comment at: ELF/Driver.cpp:135
@@ -138,1 +134,3 @@
 
+ErrorOr<MemoryBufferRef> LinkerDriver::readFile(StringRef Path) {
+  using namespace llvm::sys::fs;
----------------
ruiu wrote:
> Since we handle errors inside this function, it is rather confusing to propagate a detailed error back to callers. I'd use llvm::Optional instead of ErrorOr.
Ack.

================
Comment at: ELF/Driver.cpp:136
@@ +135,3 @@
+ErrorOr<MemoryBufferRef> LinkerDriver::readFile(StringRef Path) {
+  using namespace llvm::sys::fs;
+  auto MBOrErr = MemoryBuffer::getFile(Path);
----------------
ruiu wrote:
> Do you need this?
I will remove it.

================
Comment at: ELF/Driver.cpp:42
@@ -40,2 +41,3 @@
   LinkerScript LS;
+  DynamicList DL;
   Config = &C;
----------------
ruiu wrote:
> I think it is better to not use DynList as a container of the list of dynamic list symbols. You can just add a new set to Config instead and use that as
> 
>   if (Config->DynamicList.count(Sym.getName()))
> 
> or something like that.
Ack.

================
Comment at: ELF/Driver.cpp:143-149
@@ +142,9 @@
+void LinkerDriver::readDynamicList(StringRef Path) {
+  using namespace llvm::sys::fs;
+  auto MBOrErr = MemoryBuffer::getFile(Path);
+  if (!MBOrErr) {
+    error(MBOrErr, "cannot open " + Path);
+    return;
+  }
+  std::unique_ptr<MemoryBuffer> &MB = *MBOrErr;
+  MemoryBufferRef MBRef = MB->getMemBufferRef();
----------------
ruiu wrote:
> You might want to make this piece of code a separate function because it is repeated at least twice.
Ack.

================
Comment at: ELF/DynamicList.cpp:34
@@ +33,3 @@
+//
+//  { symbol1; symbol2; [...]; symbolN }; }
+//
----------------
ruiu wrote:
> You have an extra }.
Ack.

================
Comment at: ELF/DynamicList.cpp:64-66
@@ +63,5 @@
+
+void DynamicListParser::readEntry() {
+  DynList->ExactMatch.insert(next());
+}
+
----------------
ruiu wrote:
> I wouldn't make this a function because it's too short.
Ack.

================
Comment at: ELF/Writer.cpp:836-837
@@ -834,1 +835,4 @@
     return true;
+  if (Config->DynamicList)
+    if (DynList->isInDynamicList(B.getName()))
+      return true;
----------------
ruiu wrote:
> Adding code here is not a good idea because this piece of code would be executed for all symbols. That's too expensive.
> 
> Instead of doing that, run a loop for each symbols in the dynamic list and set MustBeInDynSym flag. It's much cheaper.
Ack.

================
Comment at: ELF/Writer.cpp:1115
@@ +1114,3 @@
+      Symtab.getSymbols();
+  // Process the --dynamic-list options.
+  for (StringRef S : Config->DynList.getSymbols()) {
----------------
ruiu wrote:
> This comment needs to be more explanatory.
> 
>   // The dyanmic list is a feature to give a list of symbols that need
>   // to be exported via the dynamic symbol table. Here is where the
>   // dynamic list is processed.
Ack.

================
Comment at: ELF/Writer.cpp:1124
@@ -1114,3 +1123,3 @@
   std::vector<DefinedCommon *> CommonSymbols;
-  for (auto &P : Symtab.getSymbols()) {
+  for (auto &P : SymtabSymbols) {
     SymbolBody *Body = P.second->Body;
----------------
ruiu wrote:
> Why did you change this?
Because the list is already being copies above to be used in the dynamic list processing. Do we really need to get the mapvector again?


http://reviews.llvm.org/D18959