This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Should print unknown d_tag in hex format
ClosedPublic

Authored by Higuoxing on Feb 27 2019, 11:51 PM.

Details

Summary

Currently, llvm-objdump prints "unknown" instead of d_tag value in hex format. Because getDynamicTagAsString returns "unknown" rather than empty
string.

Diff Detail

Event Timeline

Higuoxing created this revision.Feb 27 2019, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 11:51 PM
grimar added inline comments.Feb 28 2019, 12:01 AM
tools/llvm-objdump/ELFDump.cpp
179

Should getDynamicTagAsString just return nullptr on unknown flag maybe?
(It seems used only once in LLVM, for the place below, btw)

@jhenderson, what do you think?

Higuoxing marked an inline comment as done.Feb 28 2019, 12:07 AM
Higuoxing added inline comments.
tools/llvm-objdump/ELFDump.cpp
179

There are several functions that return "unknown". I am not so confident to change those codes in /lib. In my opinion, I think those should return "".

jhenderson added inline comments.Feb 28 2019, 1:57 AM
tools/llvm-objdump/ELFDump.cpp
179

I'd say neither is the right thing to do. I actually think just printing the hex number is not good enough. I think it should be something like <unknown:>0x12345678, and I would highly recommend doing that inside getDynamicTagAsString(). It seems rather odd to be comparing against "unknown". The alternative would be to change the interface to return an Optional<> instead of a string, but the name of the function just makes me think I wouldn't need to do anything with the output at all.

Addressed @jhenderson 's comment

  • return unknown tag name as <unknown:>0x12345678
jhenderson accepted this revision.Mar 1 2019, 1:50 AM

LGTM, assuming the case printing is correct, and the two other small comments are addressed.

lib/Object/ELF.cpp
473 ↗(On Diff #188847)

This prints the hex digits as lower-case. Does that match what the rest of the llvm-objdump printing does?

test/tools/llvm-objdump/elf-dynamic-section.test
61

Maybe worth adding a hex digit to the tag value, to show upper/lower case behaviour.

tools/llvm-objdump/ELFDump.cpp
187

As we're messing about with different string types (const char *, std::string, StringRef etc), it's probably best to not use auto here.

This revision is now accepted and ready to land.Mar 1 2019, 1:50 AM
Higuoxing updated this revision to Diff 189027.Mar 1 2019, 8:18 PM

Addressed @jhenderson 's comment

  • Change auto to std::string
  • Update test case to make sure that hex values are printed in lower case. (Match GNU objdump's output)
This revision was automatically updated to reflect the committed changes.