This is an archive of the discontinued LLVM Phabricator instance.

COFF: Emit a symbol table if /debug is specified
ClosedPublic

Authored by majnemer on Jul 7 2015, 6:34 PM.

Details

Summary

Providing a symbol table in the executable is quite useful when
debugging a fully-linked executable without having to reconstruct one
from DWARF.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 29230.Jul 7 2015, 6:34 PM
majnemer retitled this revision from to COFF: Emit a symbol table if /debug is specified.
majnemer updated this object.
majnemer added a reviewer: ruiu.
majnemer added a subscriber: llvm-commits.
ruiu edited edge metadata.Jul 7 2015, 6:59 PM

Can you add a test for the symbol table? I don't think long-section-name test covers the new feature.

COFF/InputFiles.cpp
149

Add a comment saying that we want to preserve DWARF debug sections only when /debug is on.

COFF/Writer.cpp
288

Return early by inverting the condition.

301

I prefer

auto *D = dyn_cast<...
if (!D || !D->isLive())
  continue;

to reduce indentation level.

317–320

Make this a separate member function.

328–329

If these actual values are 0, you don't need to set.

492

Return early.

504

Update comment ("then" no longer makes sense.)

COFF/Writer.h
64

Add a comment saying that this is 1-based section index.

65

Add a blank line before this line.

majnemer updated this revision to Diff 29236.Jul 7 2015, 9:26 PM
majnemer marked 7 inline comments as done.
majnemer edited edge metadata.
  • Address Rui's comments.
majnemer added inline comments.Jul 7 2015, 9:30 PM
COFF/Writer.cpp
317–320

I'm not sure what you mean here, the section that you highlighted isn't reusable elsewhere in the linker.

328–329

We must initialize it because otherwise the StorageClass and NumberOfAuxSymbols will be garbage and we would latter copy this garbage into the executable in writeHeader.

ruiu accepted this revision.Jul 8 2015, 9:10 AM
ruiu edited edge metadata.

LGTM

COFF/Writer.cpp
317–320

I meant this piece of code is repeated twice in this function. This is more like a personal perference, so if you don't want to make this change, I'm fine with that. I'll try to do this myself.

... = Strtab.size() + 4; // +4 for the size field
Strtab.insert(Strtab.end(), Name.begin(), Name.end());
Strtab.push_back('\0');
This revision is now accepted and ready to land.Jul 8 2015, 9:10 AM
This revision was automatically updated to reflect the committed changes.