This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump][XCOFF] Print more symbol info in relocation
ClosedPublic

Authored by jasonliu on Apr 20 2020, 8:25 AM.

Details

Summary

Print more symbol info in relocation printing when --symbol-description is specified.

Diff Detail

Event Timeline

jasonliu created this revision.Apr 20 2020, 8:25 AM
DiggerLin added inline comments.Apr 20 2020, 11:51 AM
llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test
39

nit: space align

llvm/tools/llvm-objdump/XCOFFDump.cpp
15–17

MCDisassembler.h already in the llvm-objdump.h

67–68

why not use StringRef ?

jasonliu marked 3 inline comments as done.Apr 20 2020, 12:13 PM
jasonliu added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test
39

Discussed offline: there is no issue here.

llvm/tools/llvm-objdump/XCOFFDump.cpp
67–68

How do we use StringRef if we need to do some string manipulation?

DiggerLin added inline comments.Apr 20 2020, 1:25 PM
llvm/tools/llvm-objdump/XCOFFDump.cpp
37

SymName is a std::string . It should be
SymName = demangle(SymName); ?

jhenderson accepted this revision.Apr 21 2020, 1:04 AM

Looks good to me, with the minor fixes @DiggerLin suggested.

This revision is now accepted and ready to land.Apr 21 2020, 1:04 AM
jasonliu updated this revision to Diff 258983.Apr 21 2020, 6:50 AM
jasonliu marked an inline comment as done.

Address coments.

jasonliu marked 2 inline comments as done.Apr 21 2020, 6:50 AM
jhenderson accepted this revision.Apr 21 2020, 7:06 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
73–74

This part can be done with Twine, realizing the string at the point of the assignment.

74–78

To reduce the amount of std::string operations, the append of this could be moved into the concatenation in the if above and an append in an else. Even if we were to have this line, it should use the StringRef iterators directly.

84

Same comment regarding Twine.

jasonliu updated this revision to Diff 259053.Apr 21 2020, 10:43 AM

Use Twine whenever possible.

jasonliu marked 3 inline comments as done.Apr 21 2020, 10:44 AM

LGTM with minor comment. The output looks great; thanks!

llvm/tools/llvm-objdump/XCOFFDump.cpp
78

I said "append", and I meant it. With Result.append(SymbolName.begin(), SymbolName.end()), if the string is small, only one copy would be done (as opposed to two, plus move operation overhead). If the string is long, then we still save on the move operation.

jasonliu marked 2 inline comments as done.Apr 22 2020, 6:53 AM
jasonliu added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
78

Thanks for the detailed explanation. Addressed in the commit.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 7:01 AM