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

[Higuoxing]

Summary

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

[grimar]

tools/llvm-objdump/ELFDump.cpp

179 ↗

(On Diff #188680)

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]

tools/llvm-objdump/ELFDump.cpp

179 ↗

(On Diff #188680)

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]

tools/llvm-objdump/ELFDump.cpp

179 ↗

(On Diff #188680)

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.

[Higuoxing]

Addressed @jhenderson 's comment

• return unknown tag name as <unknown:>0x12345678

[jhenderson]

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 ↗

(On Diff #188847)

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

tools/llvm-objdump/ELFDump.cpp

179 ↗

(On Diff #188847)

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

[Higuoxing]

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)

llvm/trunk/include/llvm/Object/ELF.h

public:
   Expected<ArrayRef<Elf_Word>> getSHNDXTable(const Elf_Shdr &Section,
                                              Elf_Shdr_Range Sections) const;
 
   StringRef getRelocationTypeName(uint32_t Type) const;
   void getRelocationTypeName(uint32_t Type,
                              SmallVectorImpl<char> &Result) const;
   uint32_t getRelativeRelocationType() const;
 
-  const char *getDynamicTagAsString(unsigned Arch, uint64_t Type) const;
-  const char *getDynamicTagAsString(uint64_t Type) const;
+  std::string getDynamicTagAsString(unsigned Arch, uint64_t Type) const;
+  std::string getDynamicTagAsString(uint64_t Type) const;
 
   /// Get the symbol for a given relocation.
   Expected<const Elf_Sym *> getRelocationSymbol(const Elf_Rel *Rel,
                                                 const Elf_Shdr *SymTab) const;
 
   static Expected<ELFFile> create(StringRef Object);
 
   bool isMipsELF64() const {

llvm/trunk/lib/Object/ELF.cpp

while (NumRelocs) {
     if (ErrStr)
       return createError(ErrStr);
   }
 
   return Relocs;
 }
 
 template <class ELFT>
-const char *ELFFile<ELFT>::getDynamicTagAsString(unsigned Arch,
+std::string ELFFile<ELFT>::getDynamicTagAsString(unsigned Arch,
                                                  uint64_t Type) const {
 #define DYNAMIC_STRINGIFY_ENUM(tag, value)                                     \
   case value:                                                                  \
     return #tag;
 
 #define DYNAMIC_TAG(n, v)
   switch (Arch) {
   case ELF::EM_HEXAGON:

 #include "llvm/BinaryFormat/DynamicTags.def"
 #undef DYNAMIC_TAG
 #undef MIPS_DYNAMIC_TAG
 #undef HEXAGON_DYNAMIC_TAG
 #undef PPC64_DYNAMIC_TAG
 #undef DYNAMIC_TAG_MARKER
 #undef DYNAMIC_STRINGIFY_ENUM
   default:
-    return "unknown";
+    return "<unknown:>0x" + utohexstr(Type, true);
   }
 }
 
 template <class ELFT>
-const char *ELFFile<ELFT>::getDynamicTagAsString(uint64_t Type) const {
+std::string ELFFile<ELFT>::getDynamicTagAsString(uint64_t Type) const {
   return getDynamicTagAsString(getHeader()->e_machine, Type);
 }
 
 template <class ELFT>
 Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
   ArrayRef<Elf_Dyn> Dyn;
   size_t DynSecSize = 0;
 

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

 # CHECK-NEXT:   FLAGS_1              0x0000000087654321
 # CHECK-NEXT:   VERSYM               0x0000000000001000
 # CHECK-NEXT:   VERDEF               0x0000000000001000
 # CHECK-NEXT:   VERDEFNUM            0x0000000000000000
 # CHECK-NEXT:   VERNEED              0x0000000000001000
 # CHECK-NEXT:   VERNEEDNUM           0x0000000000000000
 # CHECK-NEXT:   AUXILIARY            D
 # CHECK-NEXT:   FILTER               U
+# CHECK-NEXT:   <unknown:>0x1234abcd 0x0000000000000001
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
- Name:    .dynamic
      - Tag:   DT_VERNEED
        Value: 0x1000
      - Tag:   DT_VERNEEDNUM
        Value: 0x0
      - Tag:   DT_AUXILIARY
        Value: 0x1
      - Tag:   DT_FILTER
        Value: 0x3
+     - Tag:   0x1234abcd
+       Value: 0x1
      - Tag:   DT_NULL
        Value: 0x0
 ProgramHeaders:
   - Type: PT_LOAD
     VAddr: 0x1000
     Sections:
       - Section: .dynstr
       - Section: .dynamic
   - Type: PT_DYNAMIC
     VAddr: 0x1010
     Sections:
       - Section: .dynamic

llvm/trunk/tools/llvm-objdump/ELFDump.cpp

void printDynamicSection(const ELFFile<ELFT> *Elf, StringRef Filename) {
   if (!DynamicEntriesOrError)
     report_error(Filename, DynamicEntriesOrError.takeError());
 
   outs() << "Dynamic Section:\n";
   for (const auto &Dyn : *DynamicEntriesOrError) {
     if (Dyn.d_tag == ELF::DT_NULL)
       continue;
 
-    StringRef Str = StringRef(Elf->getDynamicTagAsString(Dyn.d_tag));
-
-    if (Str.empty()) {
-      std::string HexStr = utohexstr(static_cast<uint64_t>(Dyn.d_tag), true);
-      outs() << format("  0x%-19s", HexStr.c_str());
-    } else {
-      // We use "-21" in order to match GNU objdump's output.
-      outs() << format("  %-21s", Str.data());
-    }
+    std::string Str = Elf->getDynamicTagAsString(Dyn.d_tag);
+    outs() << format("  %-21s", Str.c_str());
 
     const char *Fmt =
         ELFT::Is64Bits ? "0x%016" PRIx64 "\n" : "0x%08" PRIx64 "\n";
     if (Dyn.d_tag == ELF::DT_NEEDED || Dyn.d_tag == ELF::DT_RPATH ||
         Dyn.d_tag == ELF::DT_RUNPATH || Dyn.d_tag == ELF::DT_SONAME ||
         Dyn.d_tag == ELF::DT_AUXILIARY || Dyn.d_tag == ELF::DT_FILTER) {
       Expected<StringRef> StrTabOrErr = getDynamicStrTab(Elf);
       if (StrTabOrErr) {