[PATCH] D78983: [ELF] Add --print-archive-stats=

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Driver.h:33
 
+  std::vector<InputFile *> files;
+
----------------
MaskRay wrote:
> alexshap wrote:
> > if this vector is not modified outside of this class then probably it would be good to keep it private and provide a read-only accessor instead.
> The code style in lld may be a bit special. In many places public member variables are used in place of public member functions (especially getters and setters)...
I think a bit more consistent way would be to add a `archiveFiles` vector and initialize it from `doParseFile`:


```
template <class ELFT> static void doParseFile(InputFile *file) {
  if (!isCompatible(file))
    return;

  // Binary file
  if (auto *f = dyn_cast<BinaryFile>(file)) {
    binaryFiles.push_back(f);
    f->parse();
    return;
  }

  // .a file
  if (auto *f = dyn_cast<ArchiveFile>(file)) {
    f->parse(); 
    /// <- HERE
    return;
  }
....
```

This would be consistent with other vectors we have:

```
std::vector<BinaryFile *> binaryFiles;
std::vector<BitcodeFile *> bitcodeFiles;
std::vector<LazyObjFile *> lazyObjFiles;
std::vector<InputFile *> objectFiles;
std::vector<SharedFile *> sharedFiles;
```


================
Comment at: lld/ELF/InputFiles.cpp:1171
+  Error err = Error::success();
+  for (const Archive::Child &c : file->children(err)) {
+    (void)c;
----------------
MaskRay wrote:
> alexshap wrote:
> >  1.  what about 
> >       std::distance(file->child_begin(err), file->child_end());  ?
> >  2. just curious why is the error consumed ? e.g. alternatively this could return Expected<...>
> 1. `difference_type` for these iterator types is not defined so `std::distance` can't be used:
> ```
> /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_iterator_base_funcs.h:138:5: note: candidate template ignored: substitution failure
>  [with _InputIterator = llvm::fallible_iterator<llvm::object::Archive::ChildFallibleIterator>]: no type named 'difference_type' in 'std::iterator_trai
> ts<llvm::fallible_iterator<llvm::object::Archive::ChildFallibleIterator> >'
> ```
> 
> > 2. just curious why is the error consumed ? e.g. alternatively this could return Expected<...>
> 
> --whole-archive and a special case call `getArchiveMembers`. When the control flow reaches here, `err` will guaranteed to be success. For the rest the archive may be corrupted but we just don't care about the error here.
What about this?

```
size_t ArchiveFile::getMemberCount() const {
  Error err = Error::success();
  auto range = file->children(err);
  // A comment about why `consumeError` is OK.
  consumeError(std::move(err));

  size_t count = 0;
  for (auto i = range.begin(), e = range.end(); i != e; ++i)
    ++count;
  return count;
}
```


================
Comment at: lld/ELF/MapFile.cpp:263
 
+void writeArchiveStats() {
+  if (config->printArchiveStats.empty())
----------------
Should this code belong to `MapFile.cpp`? (I am not sure. The file header says it is for `-Map`, though we also handle `-cref` here)


================
Comment at: lld/ELF/MapFile.cpp:270
+  if (ec) {
+    error("cannot open " + config->printArchiveStats + ": " + ec.message());
+    return;
----------------
I'd suggest to add the option name. We do it sometimes:

```
\lld\ELF\Driver.cpp(1408):    error("-image-base: number expected, but got " + s);
\lld\ELF\Driver.cpp(1479):    error("--undefined-glob: " + toString(pat.takeError()));
```


================
Comment at: lld/ELF/Options.td:301
+def print_archive_stats: J<"print-archive-stats=">,
+  HelpText<"Print the numbers of members and fetched members for each archive into the speicified file">;
+
----------------
speicified -> specified

Perhaps, make it more generic? Something like "Write archives usage statistic to the .."


================
Comment at: lld/ELF/Writer.cpp:604
 
   // Handle --print-map(-M)/--Map and --cref. Dump them before checkSections()
   // because the files may be useful in case checkSections() or openFile()
----------------
The comment needs an update.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78983/new/

https://reviews.llvm.org/D78983