[PATCH] D116434: [lld-link] Replace LazyObjFile with lazy ObjFile/BitcodeFile

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: lld/COFF/Driver.cpp:211
+  case file_magic::bitcode: {
+    auto *file = make<BitcodeFile>(ctx, mbref, "", 0);
     if (lazy)
----------------
Add `, bool lazy)` to constructor and then this code should read: `ctx.symtab.addFile(make<BitcodeFile>(ctx, mbref, "", 0, lazy));`.
Same below for `ObjFile`.


================
Comment at: lld/COFF/InputFiles.cpp:138
 
-void LazyObjFile::fetch() {
-  if (mb.getBuffer().empty())
-    return;
-
-  InputFile *file;
-  if (isBitcode(mb))
-    file = make<BitcodeFile>(ctx, mb, "", 0, std::move(symbols));
-  else
-    file = make<ObjFile>(ctx, mb, std::move(symbols));
-  mb = {};
-  ctx.symtab.addFile(file);
-}
-
-void LazyObjFile::parse() {
-  if (isBitcode(this->mb)) {
-    // Bitcode file.
-    std::unique_ptr<lto::InputFile> obj =
-        CHECK(lto::InputFile::create(this->mb), this);
-    for (const lto::InputFile::Symbol &sym : obj->symbols()) {
-      if (!sym.isUndefined())
-        ctx.symtab.addLazyObject(this, sym.getName());
-    }
-    return;
-  }
-
+void ObjFile::parseLazy() {
   // Native object file.
----------------
Same comment as `BitcodeFile`, move code into `ObjFile::parse()`


================
Comment at: lld/COFF/InputFiles.cpp:1081
 
+void BitcodeFile::parseLazy() {
+  std::unique_ptr<lto::InputFile> obj = CHECK(lto::InputFile::create(mb), this);
----------------
Can we move this to `BitcodeFile::parse()` and do something like:
```
void BitcodeFile::parse() {
  if (lazy) {
    std::unique_ptr<lto::InputFile> obj = CHECK(lto::InputFile::create(mb), this);
    for (const lto::InputFile::Symbol &sym : obj->symbols())
      if (!sym.isUndefined())
        ctx.symtab.addLazyObject(this, sym.getName());
    return;
  }
  ...
```


================
Comment at: lld/COFF/InputFiles.h:98
+  // True if this is a lazy ObjFile or BitcodeFile.
+  bool lazy = false;
+
----------------
Can you somehow move this below after `fileKind` to prevent excessive padding? Otherwise the `bool` will take 64-bits :-)
See my other comments, this doesn't need to be public.


================
Comment at: lld/COFF/InputFiles.h:368
+  explicit BitcodeFile(COFFLinkerContext &ctx, MemoryBufferRef mb,
+                       StringRef archiveName, uint64_t offsetInArchive);
   ~BitcodeFile();
----------------
Add `, bool lazy)` to constructor.


================
Comment at: lld/COFF/SymbolTable.cpp:40
   log("Reading " + toString(file));
-  file->parse();
+  if (file->lazy) {
+    if (auto *f = dyn_cast<BitcodeFile>(file))
----------------
I wonder if it's not better to leave types to do their own testing of `lazy` internally, in `Type::parse()`, and act accordingly (instead of doing it outside like here).
Same thing applies to pushing to the instances, shouldn't `parse()` do it internally? (but that could be done later)
We should be ending with just `file->parse();` here.


================
Comment at: lld/COFF/SymbolTable.cpp:585
   s->pendingArchiveLoad = true;
-  f->fetch();
+  if (!f->lazy)
+    return;
----------------
You shouldn't be needing this test & assignment here (or it could be an assert?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116434