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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:42
@@ -40,2 +41,3 @@
   LinkerScript LS;
+  DynamicList DL;
   Config = &C;
----------------
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.

================
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();
----------------
You might want to make this piece of code a separate function because it is repeated at least twice.

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

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

================
Comment at: ELF/Writer.cpp:836-837
@@ -834,1 +835,4 @@
     return true;
+  if (Config->DynamicList)
+    if (DynList->isInDynamicList(B.getName()))
+      return true;
----------------
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.


http://reviews.llvm.org/D18959