[llvm-readelf/llvm-objdump] - Improve/refactor the implementation of SHT_LLVM_ADDRSIG section dumping.

[grimar]

Summary

This patch:

• Adds a llvm-readobj/llvm-readelf test file for SHT_LLVM_ADDRSIG sections. (we do not have any)
• Enables dumping of SHT_LLVM_ADDRSIG with --all.
• Changes the logic to report a warning instead of an error when something goes wrong during dumping (allows to continue dumping SHT_LLVM_ADDRSIG and other sections on error).
• Refactors a piece of logic to a new toULEB128Array helper which might be used for GNU-style dumping implementation.

[jhenderson]

test/tools/llvm-readobj/elf-addrsig.test

1 ↗

(On Diff #222987)

'##'

FWIW, it looks like there's some coverage in the MC tests, but I think it makes sense to have a dedicated test here.

30 ↗

(On Diff #222987)

mailformed -> malformed

33 ↗

(On Diff #222987)

MAILFORMED -> MALFORMED

80 ↗

(On Diff #222987)

llvm-readelf does not dump it, and should not dump it either with --all (see also the discussion about .stack_sizes dumping), so this comment should be updated to say so.

82 ↗

(On Diff #222987)

Why rebuild?

[grimar]

test/tools/llvm-readobj/elf-addrsig.test

80 ↗

(On Diff #222987)

Yes. What I meant is that when it will start dumping it with --addrsig, it will be possible to replace --implicit-check-not=warning --implicit-check-not=error with something better like --implicit-check-not=addrsig_header_blah_blah

I'll update the comment.

[grimar]

• Addressed review comments.
• Updated --all test case.

test/tools/llvm-readobj/elf-addrsig.test

82 ↗

(On Diff #222987)

No need to rebuild + I've moved this test right below the 1st test.

[jhenderson]

test/tools/llvm-readobj/elf-addrsig.test

30 ↗

(On Diff #223175)

dumps any SHT_LLVM_ADDRSIG section

31 ↗

(On Diff #223175)

This is a little confusing to read to me (I originally thought "--all dumps it but llvm-readelf doesn't - but llvm-readelf is not an option of llvm-readobj"). Perhaps replacing "when --all..." with "when --all is specified for LLVM style, but not for GNU style" would be better?

32 ↗

(On Diff #223175)

be implemented -> is implemented

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

test/tools/llvm-readobj/elf-addrsig.test

31 ↗

(On Diff #223204)

Nit: missing trailing full stop.

llvm/trunk/test/tools/llvm-readobj/all.test

 # RUN: yaml2obj %s -o %t.o
 # RUN: llvm-readobj -a %t.o | FileCheck %s --check-prefix ALL
 # RUN: llvm-readobj --all %t.o | FileCheck %s --check-prefix ALL
 
 # ALL: Format: ELF32-i386
 # ALL: Arch: i386
 # ALL: AddressSize: 32bit
 # ALL: LoadName:
 # ALL: ElfHeader {
 # ALL: Sections [
 # ALL: Relocations [
 # ALL: Symbols [
 # ALL: ProgramHeaders [
 # ALL: Version symbols {
 # ALL: SHT_GNU_verdef {
 # ALL: SHT_GNU_verneed {
+# ALL: Addrsig [
 # ALL: Notes [
 # ALL: StackSizes [
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_386

llvm/trunk/test/tools/llvm-readobj/elf-addrsig.test

+## Show that llvm-readobj can dump SHT_LLVM_ADDRSIG sections.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
+# RUN: llvm-readobj --addrsig %t1.o | FileCheck -DFILE=%t1.o %s --check-prefix LLVM
+# RUN: not llvm-readelf --addrsig %t1.o 2>&1 | FileCheck -DFILE=%t1.o %s --check-prefix GNU
+
+# LLVM:      Addrsig [
+# LLVM-NEXT:   Sym: foo (1)
+# LLVM-NEXT:   Sym: bar (2)
+# LLVM-NEXT: ]
+
+# GNU: error: '[[FILE]]': --addrsig: not implemented
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .llvm_addrsig
+    Type: SHT_LLVM_ADDRSIG
+    Symbols:
+      - Name: foo
+      - Name: bar
+Symbols:
+  - Name: foo
+  - Name: bar
+
+## Check that llvm-readobj dumps any SHT_LLVM_ADDRSIG section when --all
+## is specified for LLVM style, but not for GNU style.
+## TODO: Refine the llvm-readelf check when GNU-style dumping is implemented.
+
+# RUN: llvm-readobj --all %t1.o | FileCheck %s --check-prefix LLVM
+# RUN: llvm-readelf --all %t1.o 2>&1 | FileCheck %s --implicit-check-not=warning --implicit-check-not=error
+
+## Check we report a warning when SHT_LLVM_ADDRSIG is broken (e.g. contains a malformed uleb128).
+
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: llvm-readobj --addrsig %t2.o 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=MALFORMED
+
+# MALFORMED: warning: '[[FILE]]': malformed uleb128, extends past end
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .llvm_addrsig
+    Type: SHT_LLVM_ADDRSIG
+    Content: "FF"
+
+## Check we report a warning when SHT_LLVM_ADDRSIG references a symbol that can't be
+## dumped (e.g. the index value is larger than the number of symbols in .symtab).
+
+# RUN: yaml2obj --docnum=3 %s -o %t3.o
+# RUN: llvm-readobj --addrsig %t3.o 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=INVALID-INDEX
+
+# INVALID-INDEX:       Addrsig [
+# INVALID-INDEX-NEXT:    Sym: foo (1)
+# INVALID-INDEX-EMPTY:
+# INVALID-INDEX-NEXT:    warning: '[[FILE]]': unable to get symbol from section [index 2]: invalid symbol index (255)
+# INVALID-INDEX-NEXT:    Sym: <?> (255)
+# INVALID-INDEX-NEXT:    Sym: bar (2)
+# INVALID-INDEX-NEXT:  ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .llvm_addrsig
+    Type: SHT_LLVM_ADDRSIG
+    Symbols:
+      - Index: 1
+      - Index: 255
+      - Index: 2
+Symbols:
+  - Name: foo
+  - Name: bar

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

public:
   Elf_Rel_Range dyn_rels() const;
   Elf_Rela_Range dyn_relas() const;
   Elf_Relr_Range dyn_relrs() const;
   std::string getFullSymbolName(const Elf_Sym *Symbol, StringRef StrTable,
                                 bool IsDynamic) const;
   void getSectionNameIndex(const Elf_Sym *Symbol, const Elf_Sym *FirstSym,
                            StringRef &SectionName,
                            unsigned &SectionIndex) const;
-  std::string getStaticSymbolName(uint32_t Index) const;
+  Expected<std::string> getStaticSymbolName(uint32_t Index) const;
   std::string getDynamicString(uint64_t Value) const;
   StringRef getSymbolVersionByIndex(StringRef StrTab,
                                     uint32_t VersionSymbolIndex,
                                     bool &IsDefault) const;
 
   void printSymbolsHelper(bool IsDynamic) const;
   void printDynamicEntry(raw_ostream &OS, uint64_t Type, uint64_t Value) const;
 
StringRef ELFDumper<ELFT>::getSymbolVersion(StringRef StrTab,
   return this->getSymbolVersionByIndex(StrTab, Versym->vs_index, IsDefault);
 }
 
 static std::string maybeDemangle(StringRef Name) {
   return opts::Demangle ? demangle(Name) : Name.str();
 }
 
 template <typename ELFT>
-std::string ELFDumper<ELFT>::getStaticSymbolName(uint32_t Index) const {
+Expected<std::string>
+ELFDumper<ELFT>::getStaticSymbolName(uint32_t Index) const {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  StringRef StrTable = unwrapOrError(
-      ObjF->getFileName(), Obj->getStringTableForSymtab(*DotSymtabSec));
-  Elf_Sym_Range Syms =
-      unwrapOrError(ObjF->getFileName(), Obj->symbols(DotSymtabSec));
-  if (Index >= Syms.size())
-    reportError(createError("Invalid symbol index"), ObjF->getFileName());
-  const Elf_Sym *Sym = &Syms[Index];
-  return maybeDemangle(
-      unwrapOrError(ObjF->getFileName(), Sym->getName(StrTable)));
+  Expected<const typename ELFT::Sym *> SymOrErr =
+      Obj->getSymbol(DotSymtabSec, Index);
+  if (!SymOrErr)
+    return SymOrErr.takeError();
+
+  Expected<StringRef> StrTabOrErr = Obj->getStringTableForSymtab(*DotSymtabSec);
+  if (!StrTabOrErr)
+    return StrTabOrErr.takeError();
+
+  Expected<StringRef> NameOrErr = (*SymOrErr)->getName(*StrTabOrErr);
+  if (!NameOrErr)
+    return NameOrErr.takeError();
+  return maybeDemangle(*NameOrErr);
 }
 
 template <typename ELFT>
 StringRef ELFDumper<ELFT>::getSymbolVersionByIndex(StringRef StrTab,
                                                    uint32_t SymbolVersionIndex,
                                                    bool &IsDefault) const {
   size_t VersionIndex = SymbolVersionIndex & VERSYM_VERSION;
 

 
 template <class ELFT>
 void GNUStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {
   OS << "GNUStyle::printCGProfile not implemented\n";
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printAddrsig(const ELFFile<ELFT> *Obj) {
-    OS << "GNUStyle::printAddrsig not implemented\n";
+  reportError(createError("--addrsig: not implemented"), this->FileName);
 }
 
 static StringRef getGenericNoteTypeName(const uint32_t NT) {
   static const struct {
     uint32_t ID;
     const char *Name;
   } Notes[] = {
       {ELF::NT_VERSION, "NT_VERSION (version)"},
void LLVMStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {
   ListScope L(W, "CGProfile");
   if (!this->dumper()->getDotCGProfileSec())
     return;
   auto CGProfile = unwrapOrError(
       this->FileName, Obj->template getSectionContentsAsArray<Elf_CGProfile>(
                           this->dumper()->getDotCGProfileSec()));
   for (const Elf_CGProfile &CGPE : CGProfile) {
     DictScope D(W, "CGProfileEntry");
-    W.printNumber("From", this->dumper()->getStaticSymbolName(CGPE.cgp_from),
-                  CGPE.cgp_from);
-    W.printNumber("To", this->dumper()->getStaticSymbolName(CGPE.cgp_to),
-                  CGPE.cgp_to);
+    W.printNumber(
+        "From",
+        unwrapOrError(this->FileName,
+                      this->dumper()->getStaticSymbolName(CGPE.cgp_from)),
+    W.printNumber(
+        "To",
+        unwrapOrError(this->FileName,
+                      this->dumper()->getStaticSymbolName(CGPE.cgp_to)),
     W.printNumber("Weight", CGPE.cgp_weight);
   }
 }
 
+static Expected<std::vector<uint64_t>> toULEB128Array(ArrayRef<uint8_t> Data) {
+  std::vector<uint64_t> Ret;
+  const uint8_t *Cur = Data.begin();
+  const uint8_t *End = Data.end();
+  while (Cur != End) {
+    unsigned Size;
+    const char *Err;
+    Ret.push_back(decodeULEB128(Cur, &Size, End, &Err));
+    if (Err)
+      return createError(Err);
+    Cur += Size;
+  }
+  return Ret;
+}
+
 template <class ELFT>
 void LLVMStyle<ELFT>::printAddrsig(const ELFFile<ELFT> *Obj) {
   ListScope L(W, "Addrsig");
   if (!this->dumper()->getDotAddrsigSec())
     return;
   ArrayRef<uint8_t> Contents = unwrapOrError(
       this->FileName,
       Obj->getSectionContents(this->dumper()->getDotAddrsigSec()));
-  const uint8_t *Cur = Contents.begin();
-  const uint8_t *End = Contents.end();
-  while (Cur != End) {
-    unsigned Size;
-    const char *Err;
-    uint64_t SymIndex = decodeULEB128(Cur, &Size, End, &Err);
-    if (Err)
-      reportError(createError(Err), this->FileName);
+  Expected<std::vector<uint64_t>> V = toULEB128Array(Contents);
+  if (!V) {
+    reportWarning(V.takeError(), this->FileName);
+    return;
+  }
 
-    W.printNumber("Sym", this->dumper()->getStaticSymbolName(SymIndex),
-                  SymIndex);
-    Cur += Size;
+  for (uint64_t Sym : *V) {
+    Expected<std::string> NameOrErr = this->dumper()->getStaticSymbolName(Sym);
+    if (NameOrErr) {
+      W.printNumber("Sym", *NameOrErr, Sym);
+      continue;
+    }
+    reportWarning(NameOrErr.takeError(), this->FileName);
+    W.printNumber("Sym", "<?>", Sym);
   }
 }
 
 template <typename ELFT>
 static void printGNUNoteLLVMStyle(uint32_t NoteType, ArrayRef<uint8_t> Desc,
                                   ScopedPrinter &W) {
   switch (NoteType) {
   default:

llvm/trunk/tools/llvm-readobj/llvm-readobj.cpp

if (opts::All) {
     opts::Symbols = true;
     opts::Relocations = true;
     opts::DynamicTable = true;
     opts::Notes = true;
     opts::VersionInfo = true;
     opts::UnwindInfo = true;
     opts::SectionGroups = true;
     opts::HashHistogram = true;
-    if (opts::Output == opts::LLVM)
+    if (opts::Output == opts::LLVM) {
+      opts::Addrsig = true;
       opts::PrintStackSizes = true;
     }
+  }
 
   if (opts::Headers) {
     opts::FileHeaders = true;
     opts::ProgramHeaders = true;
     opts::SectionHeaders = true;
   }
 
   // Default to stdin if no filename is specified.