[llvm-readobj, libSupport] - Refine the implementation of the code that dumps build attributes.

[grimar]

Summary

This implementation of ELFDumper<ELFT>::printAttributes() in llvm-readobj has issues:

1. It crashes when the content of the attribute section is empty.
2. It uses unwrapOrError and reportWarning calls, though ideally we want to use reportUniqueWarning.
3. It contains a TODO about redundant format version check.

lib/Support/ELFAttributeParser.cpp uses a hardcoded constant instead of the named constant.

This patch fixes all these issues.

[grimar]

• Rebased. Used the new overload of reportUniqueWarning

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

2907

I'm not sure you need the "the content of" bit of this message. The section itself is the thing that is empty/not empty.

2918–2928

I think you can avoid this dance using the Error as output parameter idiom. See ErrorAsOutParameter.

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

2918–2928

I am not sure I understood tthe idea. Are you saying that parse should accept an Error argument?
ErrorAsOutParameter clears the checked bit in destructor. I wonder how it might help here.

I've replaced consumeError with cantFail.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

2918–2928

Never mind, I misread the code. However, I have an alternative idea now: factor the if (Machine == EM_ARM) ... if statement and its corresponding else into a separat function/lambda, then return the Error so that you can do:

Error E = parseAttributes(...);
if (E)
  reportUniqueWarning(...);

What do you think?

[grimar]

• Addressed review comment.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

2918–2928

Done.

[jhenderson]

LGTM, with one nit.

llvm/tools/llvm-readobj/ELFDumper.cpp

2918–2927

Do you need the trailing return type? I don't think there's any conversion involved, so it should work without it?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

2918–2927

I think so. I'll remove.

llvm/lib/Support/ELFAttributeParser.cpp

Error ELFAttributeParser::parse(ArrayRef<uint8_t> section,
   // cursor.
   struct ClearCursorError {
     DataExtractor::Cursor &cursor;
     ~ClearCursorError() { consumeError(cursor.takeError()); }
   } clear{cursor};
 
   // Unrecognized format-version.
   uint8_t formatVersion = de.getU8(cursor);
-  if (formatVersion != 'A')
+  if (formatVersion != ELFAttrs::Format_Version)
     return createStringError(errc::invalid_argument,
                              "unrecognized format-version: 0x" +
                                  utohexstr(formatVersion));
 
   while (!de.eof(cursor)) {
     uint32_t sectionLength = de.getU32(cursor);
     if (!cursor)
       return cursor.takeError();

llvm/test/tools/llvm-readobj/ELF/RISCV/attributes-invalid.test

 ## Check how we dump invalid SHT_RISCV_ATTRIBUTES sections.
 
 ## This test case is used to ensure llvm-readobj checks the version of
 ## attribute sections correctly. The only supported format is 'A' (41),
 ## here we use 'B' (42).
 
 # RUN: yaml2obj %s -D BITS=32 -DCONTENT=42 -o %t1.32.o
-# RUN: llvm-readobj -A %t1.32.o 2>&1 | FileCheck -DFILE=%t1 %s --check-prefix=ERR-FORMAT
+# RUN: llvm-readobj -A %t1.32.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t1 %s --implicit-check-not=warning: --check-prefix=ERR-FORMAT
 # RUN: yaml2obj %s -D BITS=64 -DCONTENT=42 -o %t1.64.o
-# RUN: llvm-readobj -A %t1.64.o 2>&1 | FileCheck -DFILE=%t1 %s --check-prefix=ERR-FORMAT
+# RUN: llvm-readobj -A %t1.64.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t1 %s --implicit-check-not=warning: --check-prefix=ERR-FORMAT
 
-# ERR-FORMAT: warning: '[[FILE]].{{32|64}}.o': unrecognised FormatVersion: 0x42
+# ERR-FORMAT:      BuildAttributes {
+# ERR-FORMAT-NEXT:   FormatVersion: 0x42
+# ERR-FORMAT-NEXT: warning: '[[FILE]].{{32|64}}.o': unable to dump attributes from the SHT_RISCV_ATTRIBUTES section with index 1: unrecognized format-version: 0x42
+# ERR-FORMAT-NEXT: }
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS[[BITS=64]]
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_RISCV
 Sections:
-  - Name:    .riscv.attributes
-    Type:    SHT_RISCV_ATTRIBUTES
-    Content: [[CONTENT]]
+  - Name:     .riscv.attributes
+    Type:     SHT_RISCV_ATTRIBUTES
+    Content:  [[CONTENT]]
+    ShOffset: [[SHOFFSET=<none>]]
 
 ## Check we report a warning when we are unable to parse the attribute section data.
+## FIXME: this test case documents that we don't close the "Section X" clause of
+##        the output properly when a warning is reported by the attributes parser.
 
 # RUN: yaml2obj %s -D BITS=32 -DCONTENT=4100000000 -o %t2.32.o
-# RUN: llvm-readobj -A %t2.32.o 2>&1 | FileCheck -DFILE=%t2 %s --check-prefix=ERR-LENGTH
+# RUN: llvm-readobj -A %t2.32.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t2 %s --implicit-check-not=warning: --check-prefix=ERR-LENGTH
 # RUN: yaml2obj %s -D BITS=64 -DCONTENT=4100000000 -o %t2.64.o
-# RUN: llvm-readobj -A %t2.64.o 2>&1 | FileCheck -DFILE=%t2 %s --check-prefix=ERR-LENGTH
+# RUN: llvm-readobj -A %t2.64.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t2 %s --implicit-check-not=warning: --check-prefix=ERR-LENGTH
 
-# ERR-LENGTH: warning: '[[FILE]].{{32|64}}.o': invalid section length 0 at offset 0x1
+# ERR-LENGTH:      BuildAttributes {
+# ERR-LENGTH-NEXT:   FormatVersion: 0x41
+# ERR-LENGTH-NEXT:   Section 1 {
+# ERR-LENGTH-NEXT: warning: '[[FILE]].{{32|64}}.o': unable to dump attributes from the SHT_RISCV_ATTRIBUTES section with index 1: invalid section length 0 at offset 0x1
+# ERR-LENGTH-NEXT:   }
+# ERR-LENGTH-NOT: {{.}}
+
+## Check that we don't report a warning when the SHT_RISCV_ATTRIBUTES section contains the
+## valid format version byte and no other data.
+
+# RUN: yaml2obj %s -DCONTENT=41 -o %t3.o
+# RUN: llvm-readobj -A %t3.o 2>&1 | \
+# RUN:   FileCheck %s --implicit-check-not=warning: --check-prefix=NO-CONTENT
+
+# NO-CONTENT:      BuildAttributes {
+# NO-CONTENT-NEXT:   FormatVersion: 0x41
+# NO-CONTENT-NEXT: }
+
+## Check we report a warning when we are unable to read the content of the SHT_RISCV_ATTRIBUTES section.
+
+# RUN: yaml2obj %s -DCONTENT="''" -DSHOFFSET=0xffffffff -o %t4.o
+# RUN: llvm-readobj -A %t4.o 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t4.o --implicit-check-not=warning: --check-prefix=BROKEN-CONTENT
+
+# BROKEN-CONTENT:      BuildAttributes {
+# BROKEN-CONTENT-NEXT: warning: '[[FILE]]': unable to read the content of the SHT_RISCV_ATTRIBUTES section with index 1: section [index 1] has a sh_offset (0xffffffff) + sh_size (0x0) that is greater than the file size (0x168)
+# BROKEN-CONTENT-NEXT: }
+
+# RUN: yaml2obj %s -DCONTENT="''" -o %t5.o
+# RUN: llvm-readobj -A %t5.o 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t5.o --implicit-check-not=warning: --check-prefix=EMPTY-CONTENT
+
+# EMPTY-CONTENT:      BuildAttributes {
+# EMPTY-CONTENT-NEXT: warning: '[[FILE]]': the SHT_RISCV_ATTRIBUTES section with index 1 is empty
+# EMPTY-CONTENT-NEXT: }

llvm/tools/llvm-readobj/ELFDumper.cpp

assert((Machine == EM_ARM || Machine == EM_RISCV) &&
          "Attributes not implemented.");
 
   DictScope BA(W, "BuildAttributes");
   for (const Elf_Shdr &Sec : cantFail(Obj.sections())) {
     if (Sec.sh_type != ELF::SHT_ARM_ATTRIBUTES &&
         Sec.sh_type != ELF::SHT_RISCV_ATTRIBUTES)
       continue;
 
-    ArrayRef<uint8_t> Contents =
-        unwrapOrError(ObjF.getFileName(), Obj.getSectionContents(Sec));
-    if (Contents[0] != ELFAttrs::Format_Version) {
-      reportWarning(createError(Twine("unrecognised FormatVersion: 0x") +
-                                Twine::utohexstr(Contents[0])),
-                    ObjF.getFileName());
+    ArrayRef<uint8_t> Contents;
+    if (Expected<ArrayRef<uint8_t>> ContentOrErr =
+            Obj.getSectionContents(Sec)) {
+      Contents = *ContentOrErr;
+      if (Contents.empty()) {
+        reportUniqueWarning("the " + describe(Sec) + " is empty");
         continue;
       }
-    W.printHex("FormatVersion", Contents[0]);
-    if (Contents.size() == 1)
+    } else {
+      reportUniqueWarning("unable to read the content of the " + describe(Sec) +
+                          ": " + toString(ContentOrErr.takeError()));
       continue;
-
-    // TODO: Delete the redundant FormatVersion check above.
-    if (Machine == EM_ARM) {
-      if (Error E = ARMAttributeParser(&W).parse(Contents, support::little))
-        reportWarning(std::move(E), ObjF.getFileName());
-    } else if (Machine == EM_RISCV) {
-      if (Error E = RISCVAttributeParser(&W).parse(Contents, support::little))
-        reportWarning(std::move(E), ObjF.getFileName());
     }
+
+    W.printHex("FormatVersion", Contents[0]);
+
+    Error E = Error::success();
+    cantFail(std::move(E));
+    if (Machine == EM_ARM)
+      E = ARMAttributeParser(&W).parse(Contents, support::little);
+    else
+      E = RISCVAttributeParser(&W).parse(Contents, support::little);
+
+    if (E)
+      reportUniqueWarning("unable to dump attributes from the " +
+                          describe(Sec) + ": " + toString(std::move(E)));
   }
 }
 
 namespace {
 
 template <class ELFT> class MipsGOTParser {
 public:
   TYPEDEF_ELF_TYPES(ELFT)
   using Entry = typename ELFO::Elf_Addr;