[PATCH] D34257: [PDB] Start emitting source file and line information

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 2 inline comments as done.
rnk added a comment.

In https://reviews.llvm.org/D34257#782058, @zturner wrote:

> I was able to get the first yaml file down to this:
>  ... 
>  Can we do something like this?  Seems to produce a valid object file and round-trips back.  Will it link?  I don't mind having a separate test that has a more real-world example, but in that case we shouldn't check in the yaml file with instructions on how to generate it, we should just actually compile something as part of the test.  Just seems like we should prefer keeping tests minimal whenever possible so that someone coming along and reading it can understand what all the moving parts are without having to get distracted by irrelevant stuff.


You've removed the relocations list and the .text section, so this seems overly reduced. This object file would probably cause these CHECKs to fail in the test:
CHECK-NEXT:           RelocOffset:     32
CHECK-NEXT:           RelocSegment:    2

I don't like hacking down object file yaml because you can end up with artificial, unreproducible test cases like this that can't easily be edited. For example, suppose we want to test an `#include` mid-function to test filename blocks in line tables. We'd have to do all this reduction again, and it's not automatable. See also `icf-associative.test`, which I just had to modify because it was similarly reduced to contain invalid `.debug$S` contents, which LLD now rejects.

LLD currently does not depend on clang, so its test suite cannot depend on clang. That said, compiler-rt does not depend on clang and it uses clang as part of its test suite, so there is definitely prior art, but it will add more dependencies to check-lld.



================
Comment at: lld/COFF/Chunks.h:102-104
+public:
   // The offset from beginning of the output section. The writer sets a value.
   uint64_t OutputSectionOff = 0;
----------------
ruiu wrote:
> Move above `protected`.
I didn't do that because it would introduce padding bytes. Looking now, there's more we can do to get a compact layout.


================
Comment at: lld/COFF/PDB.cpp:123
+// the other sections, which have line tables.
+static SectionChunk *findMainDebugSChunk(ObjectFile *File) {
+  SectionChunk *MainDebugSChunk = nullptr;
----------------
ruiu wrote:
> Rename SChunk Chunk?
I want to be clear that this is the .debug$S chunk, not the .debug$T chunk.


================
Comment at: lld/COFF/PDB.cpp:134
+    }
+    MainDebugSChunk = DebugChunk;
+  }
----------------
ruiu wrote:
> Do you have to return the last one? If not, you can return here (and return a nullptr at end.)
I want to warn if there are two, so I can return after the warning and use the first one, but I don't want to return early on finding the main chunk. I'm also not sure this early searching is necessary. It's possible we can defer all this work to the end now that we're passing line tables into the PDB directly.


================
Comment at: lld/test/COFF/Inputs/pdb_lines_1.yaml:6
+sections:        
+  - Name:            .drectve
+    Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
----------------
zturner wrote:
> If we're just testing lines, can we delete everything but the `.debug$S` section with the appropriate chunk?  Seems like you could nuke this entire file down to a single `.debug$S` section with nothing but a lines section and a checksums section.
There are actually two .debug$S sections, and it's interesting to test that because it exercises the subsections loop.


================
Comment at: lld/test/COFF/Inputs/pdb_lines_1.yaml:205
+    Alignment:       1
+    SectionData:     04000000F10000004D000000290047110000000000000000000000000E00000004000000090000000310000000000000000000666F6F001C001210280000000000000000000000000000000000000000002042110002004F11000000F20000003000000000000000000000000E000000180000000300000024000000000000000200008004000000030000800900000004000080
+    Subsections:     
----------------
zturner wrote:
> See earlier comment about deleting unnecessary subsections, but that said, don't specify both `SectionData` and `Subsections`.  One or the other.  (I should probably find a way to make the yaml error out if you have both)
Oops, missed this one, there were two sections.


================
Comment at: lld/test/COFF/Inputs/pdb_lines_1.yaml:302
+        Type:            IMAGE_REL_AMD64_ADDR32NB
+symbols:         
+  - Name:            '@comp.id'
----------------
zturner wrote:
> Delete all this stuff too?
I got rid of the @ symbols, but if I remove the unwind info the test starts to break.


================
Comment at: lld/test/COFF/Inputs/pdb_lines_2.yaml:6
+sections:        
+  - Name:            .drectve
+    Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
----------------
zturner wrote:
> Same deleting comments apply to this file.
Removing .drective causes `error: broken object file` from the linker.


https://reviews.llvm.org/D34257