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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.h:33
 
+  std::vector<InputFile *> files;
+
----------------
grimar wrote:
> 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;
> ```
This will add yet another global variable, but since it is grouped with other similar variables, I think it is fine. In return, we can avoid `private->public` visibility change and expose Driver.h to MapFile.cpp,  it is probably slightly better.

This exposes another issue that D46034 does not clear lazyObjFiles when relinking. I will clear it as well.


================
Comment at: lld/ELF/InputFiles.cpp:1171
+  Error err = Error::success();
+  for (const Archive::Child &c : file->children(err)) {
+    (void)c;
----------------
grimar wrote:
> 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;
> }
> ```
`llvm::object::Archive::child_iterator` keeps a reference to the `Error` object. `consumeError(std::move(err))` has to be called after the iteration is done.

Using `for (auto i = range.begin(), e = range.end(); i != e; ++i)` can avoid an unused loop variable but it seems to become more complex. In the end I still prefer a range-based for loop, even if there is an unused loop variable.


================
Comment at: lld/ELF/MapFile.cpp:263
 
+void writeArchiveStats() {
+  if (config->printArchiveStats.empty())
----------------
grimar wrote:
> 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)
It looks like we are using `MapFile.cpp` for ad-hoc statistical features.. This file hasn't become very complex. I think it is probably fine to keep it here (`writeArchiveStats` takes less than 20 lines). When it becomes more complex, we can consider a file rename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78983