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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Config.h:39
+// For --discard-{all,locals,none} and --retain-symbols-file.
+enum class DiscardPolicy { Default, All, Locals, File, None };
 
----------------
emaste wrote:
> DiscardPolicy File seems confusing to me - in that it seems File would list the symbols to discard. Not sure of a good word to encompass Discard/Retain though.
I agree it was confusing. What about RetainFile ?


================
Comment at: ELF/Config.h:76
   uint8_t OSABI = 0;
+  llvm::DenseSet<llvm::CachedHashStringRef> RetainSymbolsFile;
   llvm::DenseMap<llvm::CachedHashStringRef, unsigned> SymbolOrderingFile;
----------------
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.


================
Comment at: ELF/Driver.cpp:648
 
+  // If --retain-symbols-file used it retains only the symbols listed in the
+  // file filename, discarding all others.
----------------
ruiu wrote:
> If --retain-symbol-file is used, we'll retail only the symbols listed in the file and discard all others.
Done. Thanks.


https://reviews.llvm.org/D27716