Page MenuHomePhabricator

[llvm-readobj] [COFF] Print the symbol index for relocations
ClosedPublic

Authored by mstorsjo on Fri, Dec 28, 2:16 PM.

Details

Summary

There can be multiple local symbols with the same name (for e.g. comdat sections), and thus the symbol name itself isn't enough to disambiguate symbols.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Fri, Dec 28, 2:16 PM

Neither of the tools that print relocations currently, llvm-readobj, llvm-objdump or yaml2obj, disambiguate between relocations pointing to different symbols with the same name.

ruiu added inline comments.Fri, Dec 28, 2:22 PM
include/llvm/Object/COFF.h
438 ↗(On Diff #179663)

Instead of storing an index as a member, can you compute it when you need it from the offset from the beginning of a symbol table?

Btw, do you think the index should be printed in hex? That's what dumpbin -relocations does.

mstorsjo marked an inline comment as done.Fri, Dec 28, 11:43 PM
mstorsjo added inline comments.
include/llvm/Object/COFF.h
438 ↗(On Diff #179663)

Yes, that'd be possible, but COFFSymbolRef doesn't have a reference to either the start of the symbol table or the surrounding COFFObjectFile, so it'd have to be a getIndexOf(COFFSymbolRef) member in COFFObjectFile in that case.

mstorsjo updated this revision to Diff 179726.Sun, Dec 30, 4:54 AM

Removed the extra member from COFFSymbol, added a method to COFFObjectFile for calculating it instead.

The question about printing hex vs decimal is still open for opinions.

alexshap added inline comments.Mon, Dec 31, 10:25 PM
tools/llvm-readobj/COFFDumper.cpp
1381 ↗(On Diff #179726)

one minor concern: if I understand correctly if the symbol is not found (Symbol == Obj->symbol_end()) this code still will print SymbolIndex = 0. Maybe we can do smth similar to how SymbolName is handled ?

mstorsjo marked an inline comment as done.Tue, Jan 1, 12:03 AM
mstorsjo added inline comments.
tools/llvm-readobj/COFFDumper.cpp
1381 ↗(On Diff #179726)

Sure, I guess -1 is the only sensible thing to print then. The index can technically be any uint32_t, so storing it in a local int64_t to allow for -1 should be safe then.

mstorsjo updated this revision to Diff 179946.Wed, Jan 2, 2:49 PM

Updated to print -1 for cases when the symbol isn't found.

alexshap accepted this revision.Wed, Jan 2, 3:19 PM
This revision is now accepted and ready to land.Wed, Jan 2, 3:19 PM
ruiu accepted this revision.Wed, Jan 2, 3:29 PM

LGTM

This revision was automatically updated to reflect the committed changes.