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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Config.h:76
   uint8_t OSABI = 0;
+  llvm::DenseSet<llvm::CachedHashStringRef> RetainSymbolsFile;
   llvm::DenseMap<llvm::CachedHashStringRef, unsigned> SymbolOrderingFile;
----------------
ruiu wrote:
> 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.
Not sure what is expected place then. I think we just put variable here in sorted groups, since there is no other StrringSets, I assumed it is sorted.
I moved that line after last llvm::StringRef then.


================
Comment at: ELF/Driver.cpp:477
 
+static SmallVector<StringRef, 0> getLines(MemoryBufferRef MB) {
+  SmallVector<StringRef, 0> Arr;
----------------
ruiu wrote:
> 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.
Done.


================
Comment at: ELF/Driver.cpp:498-499
+static void parseRetainSymbolsList(MemoryBufferRef MB) {
+  for (StringRef S : getLines(MB))
+    Config->RetainSymbolsFile.insert(S.trim());
+}
----------------
ruiu wrote:
> 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().
No I am not, last argument of split() does all job:

```
  MB.getBuffer().split(Arr, '\n', INT32_MAX, /*KeepEmpty*/ false);
```

Also set would assert that:
```
    std::pair<typename base::iterator, bool> insert(StringRef Key) {
      assert(!Key.empty());
      return base::insert(std::make_pair(Key, '\0'));
    }
```


================
Comment at: ELF/Driver.cpp:652
+    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue())) {
+      Config->Discard = DiscardPolicy::RetainFile;
+      parseRetainSymbolsList(*Buffer);
----------------
ruiu wrote:
> This flag should be set to true whether we readFile succeded or not.
I think we error out if something is wrong inside readFile() anyways. Though I did that change to
make code shorter.


https://reviews.llvm.org/D27716