COFF: Friendlier undefined symbol errors.

[pcc]

Summary

This change does three things:

• Try to find the file and line number information of an undefined symbol reference by reading codeview debug info.
• Try to find the name of the function or global variable with the undefined symbol reference by searching the object file's symbol table.
• Prints the information in the same style as the ELF linker.

[pcc]

The error messages look a little odd on non-Windows platforms:

ra/bin/lld-link: error: undefined symbol: ?foo@@YAHXZ
>>> referenced by \path\to\foo.cpp:4
>>>               foo.obj:(main)

(i.e. backslashes rather than slashes in the file path)

@zturner Would there be any downside to using slashes in our codeview output on non-Windows?

[zturner]

In D45467#1062509, @pcc wrote:

The error messages look a little odd on non-Windows platforms:

ra/bin/lld-link: error: undefined symbol: ?foo@@YAHXZ
>>> referenced by \path\to\foo.cpp:4
>>>               foo.obj:(main)

(i.e. backslashes rather than slashes in the file path)

@zturner Would there be any downside to using slashes in our codeview output on non-Windows?

We should probably use the native host slash for all paths. Reason being that MS tools need to be able to link our object files, and we don't have any control over how they handle slashes. Windows' tools handling of forward slashes in tools is pretty inconsistent, but in most cases doesn't work very well, so I would expect things to break if we have slashes (e.g. even if the linker doesn't break and dutifully copies a path with forward slashes into a PDB, the debugger could break).

[zturner]

I just noticed you said on non-Windows. Yea, I think the path should be whatever is native to the host you're building on.

[pcc]

• Remove getDebugChunks() check
• Update and add tests

[smeenai]

In D45467#1062522, @zturner wrote:

I just noticed you said on non-Windows. Yea, I think the path should be whatever is native to the host you're building on.

We cross-compile for Windows on Linux. If there's a change here, it would be awesome if I was CC'd, so I could test it with our setup and make sure VS and WinDbg are still happy.

[pcc]

In D45467#1062612, @smeenai wrote:

In D45467#1062522, @zturner wrote:

I just noticed you said on non-Windows. Yea, I think the path should be whatever is native to the host you're building on.

We cross-compile for Windows on Linux. If there's a change here, it would be awesome if I was CC'd, so I could test it with our setup and make sure VS and WinDbg are still happy.

The change is D45473 and I've cc'd you.

[zturner]

In D45467#1062612, @smeenai wrote:

In D45467#1062522, @zturner wrote:

I just noticed you said on non-Windows. Yea, I think the path should be whatever is native to the host you're building on.

We cross-compile for Windows on Linux. If there's a change here, it would be awesome if I was CC'd, so I could test it with our setup and make sure VS and WinDbg are still happy.

Cross compiling is a weird situation here, because the paths aren't going to make sense anyway (obviously you can't run your debugger on the same machine that the program was built on in that setup, therefore any paths in the PDB are necessarily going to be useless). So I think this should be a property of the machine you're building on (since that's the machine the source resides on, which is what the paths refer to), and have nothing to do with the target.

[pcc]

Ping.

[zturner]

I was out the second half of last week, so can you catch me up on what happened with the other patch and what you're waiting on here? Is the path issue all settled and now you just want feedback on the error message printing?

[ruiu]

lld/COFF/Chunks.h

195–197

If we can always do this, should we store Relocs as an ArrayRef instead of iterator_range in the first place?

lld/COFF/PDB.cpp

1161

Please add a function comment.

1170–1186

Can you factor this code out as a separate function?

1188

Maybe you can factor out this loop as a new function that returns a line table for a given address?

lld/COFF/SymbolTable.cpp

68

Add a function comment.

77–79

Instead of storing two separate values, you might be able to just save the symbol.

108

Use error().

[pcc]

In D45467#1069257, @zturner wrote:

I was out the second half of last week, so can you catch me up on what happened with the other patch and what you're waiting on here? Is the path issue all settled and now you just want feedback on the error message printing?

Yes, that patch landed and we now emit Unix-style paths on Unix hosts. I'm just waiting on review of the error message printing here.

[zturner]

lld/COFF/Chunks.h

195–197

Alternatively, why not just return an iterator_range?

lld/COFF/PDB.cpp

1193

Can we use a DenseMap here?

1210

Don't we already iterate the subsection list for other reasons in this file? Would it be worth piggybacking off of that to save the relevant info from the various subsections into each SectionChunk so that we can just do something like DbgC->Lines

lld/COFF/PDB.h

33–34

Can you take this argument by const& instead of by non-const pointer?

[pcc]

• Address review comments

lld/COFF/Chunks.h

195–197

Switching to ArrayRef made the code a little simpler and was done in D45714.

lld/COFF/PDB.cpp

1210

I don't think that would work. We read CV subsections elsewhere to create a PDB, but that needs to happen after we issue undefined symbol errors because it requires section contents to be relocated, which requires all sections to be laid out and all symbols to be defined. But if we're issuing an undefined symbol error then obviously not all symbols will be defined.

lld/COFF/PDB.h

33–34

Const seems fine but we usually pass pointers around in this part of the code for things like chunks.

[zturner]

Is there a particular reason to use a pointer? Inside the function it isn’t
null checked so we’re already assuming non null.

[pcc]

In D45467#1069517, @zturner wrote:

Is there a particular reason to use a pointer? Inside the function it isn’t
null checked so we’re already assuming non null.

It's just the local convention, but I can change this one if you feel strongly.

[ruiu]

lld/COFF/Chunks.h

195

Now we probably should just export Relocs instead of defining an accessor for it.

lld/COFF/PDB.cpp

1195

auto -> coff_relocation

1277–1278

Can you use real types instead of auto?

lld/COFF/SymbolTable.cpp

198–200

Something like this is I think more straightforward

errorOrWarn("undefined symbol: " + Sym->getName() + getSymbolLocations(File, SymIndex));

[zturner]

In D45467#1069519, @pcc wrote:

In D45467#1069517, @zturner wrote:

Is there a particular reason to use a pointer? Inside the function it isn’t
null checked so we’re already assuming non null.

It's just the local convention, but I can change this one if you feel strongly.

Eh, stick with the convention. Maybe in a follow up if I feel strongly enough I can throw up a patch to change the convention :)

[pcc]

• Address review comments

[ruiu]

LGTM

lld/COFF/SymbolTable.cpp

115

auto -> Location

lld/COFF/Chunks.h

public:
   }
 
   // Allow iteration over the bodies of this chunk's relocated symbols.
   llvm::iterator_range<symbol_iterator> symbols() const {
     return llvm::make_range(symbol_iterator(File, Relocs.begin()),
                             symbol_iterator(File, Relocs.end()));
   }
 
+  ArrayRef<coff_relocation> relocs() const {
+    return {Relocs.begin(), Relocs.end()};
+  }
+
   // Allow iteration over the associated child chunks for this section.
   ArrayRef<SectionChunk *> children() const { return AssocChildren; }
 
   // A pointer pointing to a replacement for this chunk.
   // Initially it points to "this" object. If this chunk is merged
   // with other chunk by ICF, it points to another chunk,
   // and this chunk is considrered as dead.
   SectionChunk *Repl;

lld/COFF/InputFiles.cpp

SectionChunk *ObjFile::readSection(uint32_t SectionNumber,
   //
   // DWARF sections don't need any special handling from the perspective
   // of the linker; they are just a data section containing relocations.
   // We can just link them to complete debug info.
   //
   // CodeView needs a linker support. We need to interpret and debug
   // info, and then write it to a separate .pdb file.
 
-  // Ignore debug info unless /debug is given.
-  if (!Config->Debug && Name.startswith(".debug"))
+  // Ignore DWARF debug info unless /debug is given.
+  if (!Config->Debug && Name.startswith(".debug_"))
     return nullptr;
 
   if (Sec->Characteristics & llvm::COFF::IMAGE_SCN_LNK_REMOVE)
     return nullptr;
   auto *C = make<SectionChunk>(this, Sec);
   if (Def)
     C->Checksum = Def->CheckSum;
 

lld/COFF/PDB.h

 namespace codeview {
 union DebugInfo;
 }
 }
 
 namespace lld {
 namespace coff {
 class OutputSection;
+class SectionChunk;
 class SymbolTable;
 
 void createPDB(SymbolTable *Symtab,
                llvm::ArrayRef<OutputSection *> OutputSections,
                llvm::ArrayRef<uint8_t> SectionTable,
                const llvm::codeview::DebugInfo &BuildId);
+
+std::pair<llvm::StringRef, uint32_t> getFileLine(SectionChunk *C,
+                                                 uint32_t Addr);
 }
 }
 
 #endif

lld/COFF/PDB.cpp

void PDBLinker::addSections(ArrayRef<OutputSection *> OutputSections,
   ExitOnErr(
       DbiBuilder.addDbgStream(pdb::DbgHeaderType::SectionHdr, SectionTable));
 }
 
 void PDBLinker::commit() {
   // Write to a file.
   ExitOnErr(Builder.commit(Config->PDBPath));
 }
+
+static Expected<StringRef>
+getFileName(const DebugStringTableSubsectionRef &Strings,
+            const DebugChecksumsSubsectionRef &Checksums, uint32_t FileID) {
+  auto Iter = Checksums.getArray().at(FileID);
+  if (Iter == Checksums.getArray().end())
+    return make_error<CodeViewError>(cv_error_code::no_records);
+  uint32_t Offset = Iter->FileNameOffset;
+  return Strings.getString(Offset);
+}
+
+std::pair<StringRef, uint32_t> coff::getFileLine(SectionChunk *C,
+                                                 uint32_t Addr) {
+  ExitOnError ExitOnErr;
+
+  DebugStringTableSubsectionRef CVStrTab;
+  DebugChecksumsSubsectionRef Checksums;
+  DebugLinesSubsectionRef Lines;
+  uint32_t OffsetInLinetable;
+
+  uint32_t SecrelReloc;
+  switch (Config->Machine) {
+  case AMD64:
+    SecrelReloc = COFF::IMAGE_REL_AMD64_SECREL;
+    break;
+  case I386:
+    SecrelReloc = COFF::IMAGE_REL_I386_SECREL;
+    break;
+  case ARMNT:
+    SecrelReloc = COFF::IMAGE_REL_ARM_SECREL;
+    break;
+  case ARM64:
+    SecrelReloc = COFF::IMAGE_REL_ARM64_SECREL;
+    break;
+  default:
+    llvm_unreachable("unknown machine type");
+  }
+
+  for (SectionChunk *DbgC : C->File->getDebugChunks()) {
+    if (DbgC->getSectionName() != ".debug$S")
+      continue;
+
+    // Build a mapping of SECREL relocations in DbgC that refer to C.
+    std::map<uint32_t, uint32_t> Secrels;
+    for (auto &R : DbgC->relocs()) {
+      if (R.Type != SecrelReloc)
+        continue;
+
+      if (auto *S = dyn_cast_or_null<DefinedRegular>(
+              C->File->getSymbols()[R.SymbolTableIndex]))
+        if (S->getChunk() == C)
+          Secrels[R.VirtualAddress] = S->getValue();
+    }
+
+    ArrayRef<uint8_t> Contents =
+        consumeDebugMagic(DbgC->getContents(), ".debug$S");
+    DebugSubsectionArray Subsections;
+    BinaryStreamReader Reader(Contents, support::little);
+    ExitOnErr(Reader.readArray(Subsections, Contents.size()));
+
+    for (const DebugSubsectionRecord &SS : Subsections) {
+      switch (SS.kind()) {
+      case DebugSubsectionKind::StringTable: {
+        assert(!CVStrTab.valid() &&
+               "Encountered multiple string table subsections!");
+        ExitOnErr(CVStrTab.initialize(SS.getRecordData()));
+        break;
+      }
+      case DebugSubsectionKind::FileChecksums:
+        assert(!Checksums.valid() &&
+               "Encountered multiple checksum subsections!");
+        ExitOnErr(Checksums.initialize(SS.getRecordData()));
+        break;
+      case DebugSubsectionKind::Lines: {
+        ArrayRef<uint8_t> Bytes;
+        auto Ref = SS.getRecordData();
+        ExitOnErr(Ref.readLongestContiguousChunk(0, Bytes));
+        size_t OffsetInDbgC = Bytes.data() - DbgC->getContents().data();
+
+        // Check whether this line table refers to C.
+        auto I = Secrels.find(OffsetInDbgC);
+        if (I == Secrels.end())
+          break;
+
+        // Check whether this line table covers Addr in C.
+        DebugLinesSubsectionRef LinesTmp;
+        ExitOnErr(LinesTmp.initialize(BinaryStreamReader(Ref)));
+        uint32_t OffsetInC = I->second + LinesTmp.header()->RelocOffset;
+        if (Addr < OffsetInC || Addr >= OffsetInC + LinesTmp.header()->CodeSize)
+          break;
+
+        assert(!Lines.header() &&
+               "Encountered multiple line tables for function!");
+        ExitOnErr(Lines.initialize(BinaryStreamReader(Ref)));
+        OffsetInLinetable = Addr - OffsetInC;
+        break;
+      }
+      default:
+        break;
+      }
+    }
+  }
+
+  if (!Lines.header())
+    return {"", 0};
+
+  uint32_t NameIndex;
+  uint32_t LineNumber;
+  for (const auto &Entry : Lines) {
+    for (auto &LN : Entry.LineNumbers) {
+      if (LN.Offset > OffsetInLinetable) {
+        StringRef Filename =
+            ExitOnErr(getFileName(CVStrTab, Checksums, NameIndex));
+        return {Filename, LineNumber};
+      }
+      LineInfo LI(LN.Flags);
+      NameIndex = Entry.NameIndex;
+      LineNumber = LI.getStartLine();
+    }
+  }
+  StringRef Filename = ExitOnErr(getFileName(CVStrTab, Checksums, NameIndex));
+  return {Filename, LineNumber};
+}

lld/COFF/SymbolTable.cpp

 //===- SymbolTable.cpp ----------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolTable.h"
 #include "Config.h"
 #include "Driver.h"
 #include "LTO.h"
+#include "PDB.h"
 #include "Symbols.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Timer.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include <utility>

 
 static void errorOrWarn(const Twine &S) {
   if (Config->Force)
     warn(S);
   else
     error(S);
 }
 
+static StringRef getSymbolName(SectionChunk *SC, uint32_t Addr) {
+  StringRef CandidateName;
+  uint32_t CandidateValue = 0;
+
+  for (Symbol *S : SC->File->getSymbols()) {
+    auto *D = dyn_cast_or_null<DefinedRegular>(S);
+    if (!D || D->getChunk() != SC || D->getValue() > Addr ||
+        D->getValue() < CandidateValue)
+      continue;
+
+    CandidateValue = D->getValue();
+    CandidateName = D->getName();
+  }
+
+  return CandidateName;
+}
+
+static void printSymbolLocations(ObjFile *File, uint32_t SymIndex) {
+  struct Location {
+    StringRef SymName;
+    std::pair<StringRef, uint32_t> FileLine;
+  };
+  std::vector<Location> Locations;
+
+  if (!File->getDebugChunks().empty()) {
+    for (Chunk *C : File->getChunks()) {
+      auto *SC = dyn_cast<SectionChunk>(C);
+      if (!SC)
+        continue;
+      for (auto &R : SC->relocs()) {
+        if (R.SymbolTableIndex != SymIndex)
+          continue;
+        std::pair<StringRef, uint32_t> FileLine =
+            getFileLine(SC, R.VirtualAddress);
+        StringRef SymName = getSymbolName(SC, R.VirtualAddress);
+        if (!FileLine.first.empty() || !SymName.empty())
+          Locations.push_back({SymName, FileLine});
+      }
+    }
+  }
+
+  if (Locations.empty()) {
+    llvm::errs() << ">>> referenced by " << toString(File) << "\n\n";
+    return;
+  }
+
+  for (auto Loc : Locations) {
+    llvm::errs() << ">>> referenced by ";
+    if (!Loc.FileLine.first.empty())
+      llvm::errs() << Loc.FileLine.first << ":" << Loc.FileLine.second << '\n'
+                   << ">>>               ";
+    llvm::errs() << toString(File);
+    if (!Loc.SymName.empty())
+      llvm::errs() << ":(" << Loc.SymName << ')';
+    llvm::errs() << '\n';
+  }
+  llvm::errs() << '\n';
+}
+
 void SymbolTable::reportRemainingUndefines() {
   SmallPtrSet<Symbol *, 8> Undefs;
   DenseMap<Symbol *, Symbol *> LocalImports;
 
   for (auto &I : SymMap) {
     Symbol *Sym = I.second;
     auto *Undef = dyn_cast<Undefined>(Sym);
     if (!Undef)
if (Undefs.count(B))
       errorOrWarn("<root>: undefined symbol: " + B->getName());
     if (Config->WarnLocallyDefinedImported)
       if (Symbol *Imp = LocalImports.lookup(B))
         warn("<root>: locally defined symbol imported: " + Imp->getName() +
              " (defined in " + toString(Imp->getFile()) + ") [LNK4217]");
   }
 
   for (ObjFile *File : ObjFile::Instances) {
+    size_t SymIndex = -1ull;
     for (Symbol *Sym : File->getSymbols()) {
+      ++SymIndex;
       if (!Sym)
         continue;
-      if (Undefs.count(Sym))
-        errorOrWarn(toString(File) + ": undefined symbol: " + Sym->getName());
+      if (Undefs.count(Sym)) {
+        errorOrWarn("undefined symbol: " + Sym->getName());
+        printSymbolLocations(File, SymIndex);
+      }
       if (Config->WarnLocallyDefinedImported)
         if (Symbol *Imp = LocalImports.lookup(Sym))
           warn(toString(File) + ": locally defined symbol imported: " +
                Imp->getName() + " (defined in " + toString(Imp->getFile()) +
                ") [LNK4217]");
     }
   }
 }
 
 std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name) {