[PATCH] D34126: [PDB] Add a module descriptor for every object file

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lld/COFF/PDB.cpp:113-115
+    auto ExpectedModDesc = Builder.getDbiBuilder().addModuleInfo(Path);
+    if (Error E = ExpectedModDesc.takeError())
+      fatal(E, "failed to add module to DBI stream");
----------------
A slightly cleaner way to do this would be to make an `ExitOnError` outside of this for loop, then just write `auto &MD = Err(Builder.getDbiBuilder().addModuleInfo(Path);`

```
ExitOnError Err("Error while adding objects to PDB");
for (...) {
  auto &MD = Err(Builder.getDbiBuilder().addModuleInfo(Path));
```


================
Comment at: lld/test/COFF/pdb-lib.s:3-4
+# RUN: llvm-mc -filetype=obj -triple=i686-windows-msvc %s -o foo.obj
+# RUN: grep BAR[:] %s | sed -e 's/.*BAR[:] //' | \
+# RUN:    llvm-mc -filetype=obj -triple=i686-windows-msvc - -o bar.obj
+# RUN: llvm-lib bar.obj -out:bar.lib
----------------
I know this keeps it all in one file, but sed on the input file is kind of a huge hack, what about putting the code in an input file under Inputs/?

Maybe in the future we could remove dependency on sed by making a tool which essentially strips a file of everything but a single label.  Like:

```
llvm-strip --keep-label=BAR --remove-label-headers < %s > bar.asm
```


================
Comment at: lld/test/COFF/pdb-lib.s:12
+# CHECK-NEXT:     Name: {{.*pdb-lib.s.tmp[/\\]foo.obj}}
+# CHECK-NEXT:     Debug Stream Index: 9
+# CHECK-NEXT:     Object File Name: {{.*pdb-lib.s.tmp[/\\]foo.obj}}
----------------
I'd remove anything fragile like debug stream indices.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp:296
+  memset(&SC, 0, sizeof(SC));
+  SC.ISect = (uint16_t)~0U; // This represents nil.
+  SC.Off = SecHdr->PointerToRawData;
----------------
Is this correct?  Shouldn't it be the index of the section?


https://reviews.llvm.org/D34126