[PATCH] D27716: [ELF] - Implemented --retain-symbols-file option

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Config.h:76
   uint8_t OSABI = 0;
+  llvm::DenseSet<llvm::CachedHashStringRef> RetainSymbolsFile;
   llvm::DenseMap<llvm::CachedHashStringRef, unsigned> SymbolOrderingFile;
----------------
grimar wrote:
> ruiu wrote:
> > Using CachedHashStringRef doesn't make sense here. You still compute string hash values before looking up strings, so it doesn't really cache anything (unless you have a gigantic --retain-symbols-file which is unrealistic). Just use a string set.
> Ok. Currently we know 2 users of this functionality. One of them uses /dev/null as file. That was done I think as workaround for gold which in case of empty file thinks it was not used, what is wrong and inconsistent with bfd.
> So I would probably agree now with statement about "unrealistic", though I implemented that initially assuming it is possible to have such file.
Sort.


================
Comment at: ELF/Driver.cpp:477
 
+static SmallVector<StringRef, 0> getLines(MemoryBufferRef MB) {
+  SmallVector<StringRef, 0> Arr;
----------------
Can you return std::vector<StringRef> instead of SmallVector? We don't use SmallVector that much in our code, so I'd want to avoid using it as part of a function interface.


================
Comment at: ELF/Driver.cpp:480
+  MB.getBuffer().split(Arr, '\n', INT32_MAX, false);
+  return Arr;
+}
----------------
To return a std::vector, I believe you can just do `return {Arr.begin(), Arr.end()}`.


================
Comment at: ELF/Driver.cpp:498-499
+static void parseRetainSymbolsList(MemoryBufferRef MB) {
+  for (StringRef S : getLines(MB))
+    Config->RetainSymbolsFile.insert(S.trim());
+}
----------------
I think that you are inserting an empty string to the set because usually a text file ends with "\n", so the last line is empty. I'd do trim() and remove blank lines in getLines().


================
Comment at: ELF/Driver.cpp:652
+    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue())) {
+      Config->Discard = DiscardPolicy::RetainFile;
+      parseRetainSymbolsList(*Buffer);
----------------
This flag should be set to true whether we readFile succeded or not.


https://reviews.llvm.org/D27716