[NativeSession] Add column numbers to NativeLineNumber.

[akhuang]

Summary

This adds column numbers if they are present, and otherwise
sets the column number to be zero.

Bug: https://bugs.llvm.org/show_bug.cgi?id=41795

[amccarth]

This looks right, but I'd like to see a test.

llvm-pdbutil is used in a DIA test for line numbers (llvm\test\DebugInfo\PDB\DIA\pdbdump-linenumbers.test), so perhaps that could be the model for a native one that also checks the column numbers. That would require a tweak to llvm-pdbutil to make essentially the same change as you have here, to include column numbers in the output when they're present.

llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp

476

As I understand it, the line numbers and column numbers are in parallel arrays (streams). So, for each line number you take the next column number (if there are column numbers at all).

Assuming that's right:

I don't see a need to change the for loop to use the line number iterators. Yeah, it creates some source code symmetry with the column number iterators, but I'm not sure that makes that more understandable here. I won't block on it, but I'd prefer the original for (const LineNumberEntry &LN : Group.LineNumbers).

481

It seems that if there is no column info, then ColIt would always equal ColsEnd, so perhaps that first part of the condition is not necessary. Perhaps, just before the loop, you could assert that either Lines.hasColumnInfo() or that ColIt is ColsEnd and then simplify the condition here.

The assertion would be a nice place to add a comment like

// If there are column numbers, then they are in a parallel stream to the line numbers.

[akhuang]

thanks for the comments! I added another llvm-symbolizer test with a pdb that contains column numbers.

It looks like llvm-pdbutil does already output column info, but I guess PDBs don't normally contain column info.

[akhuang]

Add test case, and address other comments.

[akhuang]

rebase

[amccarth]

Very close now. It looks like the change I requested led to a bug. Once that's corrected, this'll look great.

Thanks for adding the test. Perhaps you could generate PDBs both with and without column info and then the test could check that it works either way (which should have caught the new bug). If there's already an existing test that catches the bug (skipping the line numbers whether there are no column numbers), that's sufficient.

Feel free to ping me if I've not been clear.

llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp

470

Sorry, I wasn't clear enough before because it looks like I confused you. This if statement is unnecessary and, in fact, wrong. You still want to go through the line numbers in this group even if there's no column information.

When I said assert, I literally meant an assert like this:

llvm_assert(Lines.hasColumnInfo() || ColIt == ColsEnd);

Note that it's ||, not &&. If we have column info, great. If we don't, then ensure that the iterator alone makes that clear in order to use the simpler test in the loop.

[akhuang]

llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp

470

Makes sense. I was avoiding using asserts in case some section happened to be messed up, in which case we would just skip it instead of asserting. (Although I don't know if this is actually something that could happen.)

I think my if statement is just the inverse of your suggested assert? It continues if it claims there is column info and the column info is empty.

The original test case in llvm/test/tools/llvm-symbolizer/pdb/pdb-native.test doesn't have column info and passes.

[akhuang]

Update loop per discussion.

[amccarth]

LGTM

[nikic]

The test-columns.pdb file added here is 6MB large. Is it possible to reduce its size while preserving the spirit of the test?

[akhuang]

In D81950#3692893, @nikic wrote:

The test-columns.pdb file added here is 6MB large. Is it possible to reduce its size while preserving the spirit of the test?

I think it can be built without the system headers, which would reduce the PDB size by a lot. I can make a patch for that.

llvm/include/llvm/DebugInfo/PDB/Native/NativeLineNumber.h

 #include "llvm/DebugInfo/PDB/IPDBLineNumber.h"
 #include "llvm/DebugInfo/PDB/Native/NativeSession.h"
 
 namespace llvm {
 namespace pdb {
 class NativeLineNumber : public IPDBLineNumber {
 public:
   explicit NativeLineNumber(const NativeSession &Session,
-                            const codeview::LineInfo Line, uint32_t Length,
+                            const codeview::LineInfo Line,
+                            uint32_t ColumnNumber, uint32_t Length,
                             uint32_t Section, uint32_t Offset,
                             uint32_t SrcFileId);
 
   uint32_t getLineNumber() const override;
   uint32_t getLineNumberEnd() const override;
   uint32_t getColumnNumber() const override;
   uint32_t getColumnNumberEnd() const override;
   uint32_t getAddressSection() const override;
   uint32_t getAddressOffset() const override;
   uint32_t getRelativeVirtualAddress() const override;
   uint64_t getVirtualAddress() const override;
   uint32_t getLength() const override;
   uint32_t getSourceFileId() const override;
   uint32_t getCompilandId() const override;
   bool isStatement() const override;
 
 private:
   const NativeSession &Session;
   const codeview::LineInfo Line;
+  uint32_t ColumnNumber;
   uint32_t Section;
   uint32_t Offset;
   uint32_t Length;
   uint32_t SrcFileId;
 };
 } // namespace pdb
 } // namespace llvm
 #endif

llvm/include/llvm/DebugInfo/PDB/Native/SymbolCache.h

class SymbolCache {
   IMap::Allocator IMapAllocator;
   IMap AddrToModuleIndex;
 
   Expected<ModuleDebugStreamRef> getModuleDebugStream(uint32_t Index) const;
 
   struct LineTableEntry {
     uint64_t Addr;
     codeview::LineInfo Line;
+    uint32_t ColumnNumber;
     uint32_t FileNameIndex;
     bool IsTerminalEntry;
   };
 
   std::vector<LineTableEntry> findLineTable(uint16_t Modi) const;
   mutable DenseMap<uint16_t, std::vector<LineTableEntry>> LineTable;
 
   SymIndexId createSymbolPlaceholder() {

llvm/lib/DebugInfo/PDB/Native/NativeLineNumber.cpp

 //===- NativeLineNumber.cpp - Native line number implementation -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/PDB/Native/NativeLineNumber.h"
 
 using namespace llvm;
 using namespace llvm::pdb;
 
 NativeLineNumber::NativeLineNumber(const NativeSession &Session,
                                    const codeview::LineInfo Line,
-                                   uint32_t Section, uint32_t Offset,
-                                   uint32_t Length, uint32_t SrcFileId)
-    : Session(Session), Line(Line), Section(Section), Offset(Offset),
-      Length(Length), SrcFileId(SrcFileId) {}
+                                   uint32_t ColumnNumber, uint32_t Section,
+                                   uint32_t Offset, uint32_t Length,
+                                   uint32_t SrcFileId)
+    : Session(Session), Line(Line), ColumnNumber(ColumnNumber),
+      Section(Section), Offset(Offset), Length(Length), SrcFileId(SrcFileId) {}
 
 uint32_t NativeLineNumber::getLineNumber() const { return Line.getStartLine(); }
 
 uint32_t NativeLineNumber::getLineNumberEnd() const {
   return Line.getEndLine();
 }
 
-uint32_t NativeLineNumber::getColumnNumber() const { return 0; }
+uint32_t NativeLineNumber::getColumnNumber() const { return ColumnNumber; }
 
 uint32_t NativeLineNumber::getColumnNumberEnd() const { return 0; }
 
 uint32_t NativeLineNumber::getAddressSection() const { return Section; }
 
 uint32_t NativeLineNumber::getAddressOffset() const { return Offset; }
 
 uint32_t NativeLineNumber::getRelativeVirtualAddress() const {

llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp

for (const auto &SS : ExpectedModS->getSubsectionsArray()) {
 
     uint32_t RelocSegment = Lines.header()->RelocSegment;
     uint32_t RelocOffset = Lines.header()->RelocOffset;
     for (const LineColumnEntry &Group : Lines) {
       if (Group.LineNumbers.empty())
         continue;
 
       std::vector<LineTableEntry> Entries;
+
+      // If there are column numbers, then they should be in a parallel stream
+      // to the line numbers.
+      auto ColIt = Group.Columns.begin();
+      auto ColsEnd = Group.Columns.end();
+
       for (const LineNumberEntry &LN : Group.LineNumbers) {
-        LineInfo Line(LN.Flags);
         uint64_t VA =
             Session.getVAFromSectOffset(RelocSegment, RelocOffset + LN.Offset);
-        Entries.push_back({VA, Line, Group.NameIndex, false});
+        LineInfo Line(LN.Flags);
+        uint32_t ColNum = 0;
+
+        if (Lines.hasColumnInfo() && ColIt != ColsEnd) {
+          ColNum = ColIt->StartColumn;
+          ++ColIt;
+        }
+        Entries.push_back({VA, Line, ColNum, Group.NameIndex, false});
       }
 
       // Add a terminal entry line to mark the end of this subsection.
-      LineInfo LastLine(Group.LineNumbers.back().Flags);
       uint64_t VA = Session.getVAFromSectOffset(
           RelocSegment, RelocOffset + Lines.header()->CodeSize);
-      Entries.push_back({VA, LastLine, Group.NameIndex, true});
+      LineInfo LastLine(Group.LineNumbers.back().Flags);
+      uint32_t ColNum =
+          (Lines.hasColumnInfo()) ? Group.Columns.back().StartColumn : 0;
+      Entries.push_back({VA, LastLine, ColNum, Group.NameIndex, true});
+
       EntryList.push_back(Entries);
     }
   }
 
   // Sort EntryList, and add flattened contents to the line table.
   std::sort(EntryList.begin(), EntryList.end(),
             [](const std::vector<LineTableEntry> &LHS,
                const std::vector<LineTableEntry> &RHS) {
if (LineIter->Addr > VA + Length)
       break;
 
     uint32_t LineSect, LineOff;
     Session.addressForVA(LineIter->Addr, LineSect, LineOff);
     uint32_t LineLength = std::next(LineIter)->Addr - LineIter->Addr;
     auto ChecksumIter =
         ExpectedChecksums->getArray().at(LineIter->FileNameIndex);
     uint32_t SrcFileId = getOrCreateSourceFile(*ChecksumIter);
-    NativeLineNumber LineNum(Session, LineIter->Line, LineSect, LineOff,
-                             LineLength, SrcFileId);
+    NativeLineNumber LineNum(Session, LineIter->Line, LineIter->ColumnNumber,
+                             LineSect, LineOff, LineLength, SrcFileId);
     LineNumbers.push_back(LineNum);
     ++LineIter;
   }
   return std::make_unique<NativeEnumLineNumbers>(std::move(LineNumbers));
 }
 
 std::unique_ptr<PDBSymbolCompiland>
 SymbolCache::getOrCreateCompiland(uint32_t Index) {

llvm/test/tools/llvm-symbolizer/pdb/Inputs/test.cpp

 // To generate the corresponding EXE/PDB, run:
 // cl /Zi test.cpp
+// To generate the PDB with column numbers, run:
+// clang-cl /Zi -gcolumn-info test.cpp
 
 namespace NS {
 struct Foo {
   void bar() {}
 };
 }
 
 void foo() {

llvm/test/tools/llvm-symbolizer/pdb/pdb-native-columns.test

+RUN: echo 0x140006BA0 > %t.input
+RUN: echo 0x140006C00 >> %t.input
+RUN: echo 0x140006BB0 >> %t.input
+RUN: echo 0x140006C10 >> %t.input
+RUN: echo 0x140006C20 >> %t.input
+RUN: echo 0x140006C30 >> %t.input
+RUN: echo 0x140006C40 >> %t.input
+RUN: echo 0x140006C70 >> %t.input
+RUN:    llvm-symbolizer -obj="%p/Inputs/test-columns.exe" -use-native-pdb-reader < %t.input \
+RUN:    | FileCheck %s
+
+This tests that the symbolizer outputs column info when it is present in the pdb.
+
+CHECK: foo(void)
+CHECK-NEXT: test.cpp:11:1
+CHECK: {{^private_symbol$}}
+CHECK-NEXT: test.cpp:14:1
+CHECK: {{^main}}
+CHECK-NEXT: test.cpp:16:0
+CHECK: {{^foo_cdecl$}}
+CHECK-NEXT: test.cpp:25:27
+CHECK: {{^foo_stdcall$}}
+CHECK-NEXT: test.cpp:26:31
+CHECK: {{^foo_fastcall$}}
+CHECK-NEXT: test.cpp:27:33
+CHECK: {{^foo_vectorcall}}
+CHECK-NEXT: test.cpp:28:37
+CHECK: NS::Foo::bar(void)
+CHECK-NEXT: test.cpp:6:0