[llvm-readobj] - Allow dumping partially corrupted SHT_LLVM_CALL_GRAPH_PROFILE sections.

[grimar]

Summary

The code we have currently reports an error if something is not right with the
profile section. Instead we can report a warning and continue dumping when it is possible.
This patch does it.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

6579–6586

This seems like a pattern we're likely to have in several different parts of the ELFDumper. Is there any code we could share to avoid duplication? Maybe it just makes sense to change getStaticSymbolName to report the warning/return the <?> itself?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

6579–6586

From what I see, the getStaticSymbolName is used in one more place:

template <class ELFT>
void LLVMStyle<ELFT>::printAddrsig(const ELFFile<ELFT> *Obj) {
...
  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);
  }
}

And it looks like it should be reasonable and possible to do what you suggest. Follow-up?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

6579–6586

Follow-up: D83208

[jhenderson]

LGTM, but see my inline comment (happy for you to commit the two patches separately or as one).

llvm/tools/llvm-readobj/ELFDumper.cpp

6579–6586

Normally, I'd say yes, follow-up, but in this case, D83208 looks small enough that it could be done as part of this, especially as most of that change is just undoing what some of this change does (in particular, the GetSymName lamdba replaces getStaticSymbolName in this change and then is just replaced back the other way again). I don't feel strongly about this though.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

6579–6586

The problem is that D83208 is not a NFC. It changes the message reported for --addrsig (adds a prefix with the context):

"warning: '[[FILE]]': unable to get symbol from section [index 2]: invalid symbol index (255)" -> "warning: '[[FILE]]': unable to read the name of symbol with index 255: unable to get symbol from section [index 2]: invalid symbol index (255)"

That is why I slightly prefer to keep them separate.

llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test

- Name: .llvm.call-graph-profile
     Type: SHT_LLVM_CALL_GRAPH_PROFILE
     Entries:
       - From:   foo
         To:     bar
         Weight: 89
       - From:   bar
         To:     foo
         Weight: 98
+## 0x10 is the normal entry size for the SHT_LLVM_CALL_GRAPH_PROFILE section.
+    EntSize: [[ENTSIZE=0x10]]
 Symbols:
   - Name: foo
   - Name: bar
+
+## Check we report a warning when unable to get the content of the SHT_LLVM_CALL_GRAPH_PROFILE section.
+# RUN: yaml2obj %s -DENTSIZE=0xF -o %t2.o
+# RUN: llvm-readobj %t2.o --cg-profile 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=LLVM-ERR
+# RUN: llvm-readelf %t2.o --cg-profile | FileCheck %s --check-prefix=GNU
+
+# LLVM-ERR:      CGProfile [
+# LLVM-ERR-NEXT: warning: '[[FILE]]': unable to dump the SHT_LLVM_CALL_GRAPH_PROFILE section: section [index 1] has an invalid sh_entsize: 15
+# LLVM-ERR-NEXT: ]
+
+## Check we report a warning when unable to dump a name of a symbol.
+# RUN: yaml2obj %s --docnum=2 -o %t3.o
+# RUN: llvm-readobj %t3.o --cg-profile 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=LLVM-BROKEN-SYM
+# RUN: llvm-readelf %t3.o --cg-profile | FileCheck %s --check-prefix=GNU
+
+# LLVM-BROKEN-SYM:      CGProfile [
+# LLVM-BROKEN-SYM-NEXT:   CGProfileEntry {
+# LLVM-BROKEN-SYM-NEXT:     From: A (1)
+# LLVM-BROKEN-SYM-NEXT: warning: '[[FILE]]': unable to read the name of symbol with index 2: st_name (0xff) is past the end of the string table of size 0x5
+# LLVM-BROKEN-SYM-NEXT:     To: <?> (2)
+# LLVM-BROKEN-SYM-NEXT:     Weight: 10
+# LLVM-BROKEN-SYM-NEXT:   }
+# LLVM-BROKEN-SYM-NEXT:   CGProfileEntry {
+# LLVM-BROKEN-SYM-NEXT:     From: <?> (2)
+# LLVM-BROKEN-SYM-NEXT:     To: B (3)
+# LLVM-BROKEN-SYM-NEXT:     Weight: 20
+# LLVM-BROKEN-SYM-NEXT:   }
+# LLVM-BROKEN-SYM-NEXT:   CGProfileEntry {
+# LLVM-BROKEN-SYM-NEXT:     From: (0)
+# LLVM-BROKEN-SYM-NEXT: warning: '[[FILE]]': unable to read the name of symbol with index 4: unable to get symbol from section [index 3]: invalid symbol index (4)
+# LLVM-BROKEN-SYM-NEXT:     To: <?> (4)
+# LLVM-BROKEN-SYM-NEXT:     Weight: 20
+# LLVM-BROKEN-SYM-NEXT:   }
+# LLVM-BROKEN-SYM-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .llvm.call-graph-profile
+    Type: SHT_LLVM_CALL_GRAPH_PROFILE
+    Entries:
+      - From:   1
+        To:     2
+        Weight: 10
+      - From:   2
+        To:     3
+        Weight: 20
+      - From:   0x0 ## Null symbol.
+        To:     0x4 ## This index goes past the end of the symbol table.
+        Weight: 20
+  - Name:    .strtab
+    Type:    SHT_STRTAB
+    Content: "0041004200" ## '\0', 'A', '\0', 'B', '\0'
+Symbols:
+  - StName: 1    ## 'A'
+  - StName: 0xFF ## An arbitrary currupted index in the string table.
+  - StName: 3    ## 'B'

llvm/tools/llvm-readobj/ELFDumper.cpp

void LLVMStyle<ELFT>::printHashHistograms(const ELFFile<ELFT> *Obj) {
   W.startLine() << "Hash Histogram not implemented!\n";
 }
 
 template <class ELFT>
 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) {
+
+  Expected<ArrayRef<Elf_CGProfile>> CGProfileOrErr =
+      Obj->template getSectionContentsAsArray<Elf_CGProfile>(
+          this->dumper()->getDotCGProfileSec());
+  if (!CGProfileOrErr) {
+    this->reportUniqueWarning(
+        createError("unable to dump the SHT_LLVM_CALL_GRAPH_PROFILE section: " +
+                    toString(CGProfileOrErr.takeError())));
+    return;
+  }
+
+  auto GetSymName = [&](uint32_t Index) -> std::string {
+    if (Expected<std::string> NameOrErr =
+            this->dumper()->getStaticSymbolName(Index))
+      return *NameOrErr;
+    else
+      this->reportUniqueWarning(
+          createError("unable to read the name of symbol with index " +
+                      Twine(Index) + ": " + toString(NameOrErr.takeError())));
+    return "<?>";
+  };
+
+  for (const Elf_CGProfile &CGPE : *CGProfileOrErr) {
     DictScope D(W, "CGProfileEntry");
-    W.printNumber(
-        "From",
-        unwrapOrError(this->FileName,
-                      this->dumper()->getStaticSymbolName(CGPE.cgp_from)),
-        CGPE.cgp_from);
-    W.printNumber(
-        "To",
-        unwrapOrError(this->FileName,
-                      this->dumper()->getStaticSymbolName(CGPE.cgp_to)),
-        CGPE.cgp_to);
+    W.printNumber("From", GetSymName(CGPE.cgp_from), CGPE.cgp_from);
+    W.printNumber("To", GetSymName(CGPE.cgp_to), 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();