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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:104
@@ -102,4 +103,3 @@
     llvm::outs() << Path << "\n";
-  auto MBOrErr = MemoryBuffer::getFile(Path);
-  if (!MBOrErr) {
-    error(MBOrErr, "cannot open " + Path);
+  llvm::Optional<MemoryBufferRef> Buffer = readFile(Path);
+  if (!Buffer.hasValue())
----------------
Remove `llvm::` (since in this .cpp file we have `using namespace llvm`.)

================
Comment at: ELF/Driver.cpp:136
@@ -138,1 +135,3 @@
 
+llvm::Optional<MemoryBufferRef> LinkerDriver::readFile(StringRef Path) {
+  auto MBOrErr = MemoryBuffer::getFile(Path);
----------------
Ditto

================
Comment at: ELF/Driver.cpp:149
@@ +148,3 @@
+void LinkerDriver::readDynamicList(StringRef Path) {
+  if (llvm::Optional<MemoryBufferRef> Buffer = readFile(Path))
+    parseDynamicList(*Buffer);
----------------
Ditto

================
Comment at: ELF/Driver.cpp:320-321
@@ -305,2 +319,4 @@
   Config->Sysroot = getString(Args, OPT_sysroot);
+  if (Args.hasArg(OPT_dynamic_list))
+    readDynamicList(getString(Args, OPT_dynamic_list));
 
----------------
This place does not seems the right position to add this code because this looks different from other lines just above this one. I'd probably add this at end of this function.

================
Comment at: ELF/DynamicList.cpp:16-23
@@ +15,10 @@
+
+#include "DynamicList.h"
+#include "Config.h"
+#include "Driver.h"
+#include "ScriptParser.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
+
----------------
Remove unused includes. I don't think we need Driver.h, for example.

================
Comment at: ELF/DynamicList.cpp:25-28
@@ +24,6 @@
+
+using namespace llvm;
+using namespace llvm::object;
+using namespace lld;
+using namespace lld::elf;
+
----------------
Please remove unused ones.

================
Comment at: ELF/DynamicList.h:13-15
@@ +12,5 @@
+
+#include "lld/Core/LLVM.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/MemoryBuffer.h"
----------------
Remove.

================
Comment at: ELF/DynamicList.h:21
@@ +20,3 @@
+
+class DynamicListParser;
+
----------------
Remove.

================
Comment at: ELF/Options.td:169
@@ -168,1 +168,3 @@
 
+def dynamic_list : Separate<["--", "-"], "dynamic-list">,
+  HelpText<"Dynamic list to use">;
----------------
Yes, please sort.

================
Comment at: ELF/Writer.cpp:1112-1117
@@ -1111,1 +1111,8 @@
 
+  // The --dynamic-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.
+  for (StringRef S : Config->DynamicList)
+    if (SymbolBody *B = Symtab.find(S))
+      B->MustBeInDynSym = true;
+
----------------
I don't think this piece of code needs to be added to this place. This function is large and complex, so no new code should be added unless it is absolutely needed. One way to avoid doing this is to move this code to SymbolTable.cpp just like scanShlibUndefines and call the new function from the driver.


http://reviews.llvm.org/D18959