[llvm-readobj] - Don't call `unwrapOrErr` in `findSectionByName`.

[grimar]

Summary

We have a findSectionByName helper that tries to find a section
by it name. It is used in a few places, but never tested.

I'd like to reuse this helper for a different place.
For this, I've changed it to return Expected<> and now it
doesn't use unwrapOrErr anymore too.

[jhenderson]

It seems to me like findSectionByName should still be able to find a section even if an earlier section had an invalid name. Imagine the following two cases:

object1
+- sec1 (invalid sh_name)
+- sec2 (won't be findable)

object2
+- sec1 (can be found)
+- sec2 (invalid sh_name)

The only difference in these two cases is the section order. The actual section to be found can still be found in both cases, if the invalid name were to just be ignored (probably warned about in the first case).

Maybe that indicates the right thing to do here is just print the warning in findSectionByName instead of this change. What do you think?

llvm/test/tools/llvm-readobj/ELF/mips-abiflags.test

352–353

Setting the name of the SHT_NULL section made it look to me like the null section was significant in some way. Perhaps this could just be an arbitrary SHT_PROGBITS section instead. Same goes for the other tests.

llvm/test/tools/llvm-readobj/ELF/mips-options-sec.test

1

All the changes in mips-reginfo.test amd here look good to me, but perhaps you should split the comment and comment-marker additions into a separate NFC commit. Happy for that to be done without review.

llvm/tools/llvm-readobj/ELFDumper.cpp

1275

Maybe "unable to locate the" would be a better string here? That way, you don't have error repeated in the error message, and we won't have a situation in the future where the word "error" appears in a warning.

[grimar]

• Addressed review comments.
• Update findSectionByName as suggested (now it tries to find a section by name even if unable to read other section names properly).

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

1275

Done.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/mips-abiflags.test

381–383

I think these additional "section not found" errors/warnings are confusing the issue of what's important in the test. I'd add the respective missing sections, so that the errors aren't emitted.

Alternatively, I'm not 100% convinced we should have this kind of warning/error at all, since the earlier warning(s) might be sufficient to say there are problems reading the section names. I might be incined to just fallback to the "not found" behaviour in that case, though like I say, I could be persuaded otherwise.

Same goes for all the other test cases.

[grimar]

• Addressed review comments.

llvm/test/tools/llvm-readobj/ELF/mips-abiflags.test

381–383

Alternatively, I'm not 100% convinced we should have this kind of warning/error at all, since the earlier warning(s) might be sufficient to say there are problems reading the section names. I might be incined to just fallback to the "not found" behaviour

OK. This is simpler and works for me. I've changed the behavior to this one.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/mips-options-sec.test

29

.MIPS.options?

llvm/test/tools/llvm-readobj/ELF/mips-reginfo.test

11

Somewhat tangential, and definitely not part of this patch but "has a wrong size" sounds wrong to me (although I can't explain why). If you get a moment, changing it to "has an incorrect size" would be nice.

[jhenderson]

LGTM, with nit fixed.

[grimar]

llvm/test/tools/llvm-readobj/ELF/mips-reginfo.test

11

I have a plan to cleanup ELFDumper<ELFT>::printMipsReginfo() + refine the corresponding test file similar to as what D84854 did for printMipsOptions(). I'll change the message there.

D84854 , btw, prints:

warning: '[[FILE]]': the .MIPS.options section has a wrong size (0x1)

I guess I'll change it to "the .MIPS.options section has an incorrect size (0x1)" then too.

llvm/test/tools/llvm-readobj/ELF/mips-abiflags.test

 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_MIPS
 Sections:
+  - Type:   SHT_NULL
+    ShName: [[NULLNAME=0x0]]
   - Name:     .MIPS.abiflags
     Type:     SHT_MIPS_ABIFLAGS
     ISA:      MIPS32
     Offset:   0x100
     ShOffset: [[SHOFFSET=0x100]]
     ShSize:   [[SHSIZE=24]]
 
 ## Check we report a warning when the .MIPS.abiflags section has an unexpected size.
 # RUN: yaml2obj --docnum=3 -DSHSIZE=23 %s -o %t.err2
 # RUN: llvm-readelf -A %t.err2 2>&1 | FileCheck %s -DFILE=%t.err2 --check-prefix=SIZE-ERR
 # RUN: llvm-readobj -A %t.err2 2>&1 | FileCheck %s -DFILE=%t.err2 --check-prefix=SIZE-ERR
 
 # SIZE-ERR:      warning: '[[FILE]]': unable to read the .MIPS.abiflags section: it has a wrong size (23)
 # SIZE-ERR-NEXT: There is no .MIPS.options section in the file.
 # SIZE-ERR-NEXT: There is no .reginfo section in the file.
+
+## Check we report a warning when we are unable to find the .MIPS.abiflags section due to an error.
+# RUN: yaml2obj --docnum=3 -DNULLNAME=0xffff %s -o %t.err3
+# RUN: not llvm-readelf -A %t.err3 2>&1 | FileCheck %s -DFILE=%t.err3 --check-prefix=FIND-ERR --implicit-check-not=warning:
+# RUN: not llvm-readobj -A %t.err3 2>&1 | FileCheck %s -DFILE=%t.err3 --check-prefix=FIND-ERR --implicit-check-not=warning:
+
+# FIND-ERR: warning: '[[FILE]]': error locating the .MIPS.abiflags section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: warning: '[[FILE]]': error locating the .MIPS.options section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: warning: '[[FILE]]': error locating the .reginfo section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: error: '[[FILE]]': error locating the .got section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table

llvm/test/tools/llvm-readobj/ELF/mips-got.test

- Name: .dynamic
     Entries:
       - Tag:   DT_MIPS_LOCAL_GOTNO
         Value: 0
       - Tag:   DT_MIPS_GOTSYM
         Value: [[VAL1=0]]
       - Tag:   DT_PLTGOT
         Value: [[VAL2=0]]
 DynamicSymbols: []
+
+## Check we report an error when we are unable to find the .got section due to an error.
+# RUN: yaml2obj --docnum=3 -DNULLNAME=0xffff %s -o %t.err6.o
+# RUN: not llvm-readobj -A %t.err6.o 2>&1 | FileCheck %s -DFILE=%t.err6.o -check-prefix=FIND-ERR
+# RUN: not llvm-readelf -A %t.err6.o 2>&1 | FileCheck %s -DFILE=%t.err6.o -check-prefix=FIND-ERR
+
+# FIND-ERR: error: '[[FILE]]': error locating the .got section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_MIPS
+Sections:
+  - Type:   SHT_NULL
+    ShName: [[NULLNAME=0x0]]
+  - Name: .got
+    Type: SHT_PROGBITS

llvm/test/tools/llvm-readobj/ELF/mips-options-sec.test

-RUN: llvm-readobj -A %p/Inputs/options.obj.elf-mipsel | FileCheck %s
+## Check that we are able to dump the SHT_MIPS_OPTIONS section using -A properly.
 
-CHECK:      MIPS Options {
-CHECK-NEXT:   ODK_REGINFO {
-CHECK-NEXT:     GP: 0x0
-CHECK-NEXT:     General Mask: 0xF2000017
-CHECK-NEXT:     Co-Proc Mask0: 0x0
-CHECK-NEXT:     Co-Proc Mask1: 0x0
-CHECK-NEXT:     Co-Proc Mask2: 0x0
-CHECK-NEXT:     Co-Proc Mask3: 0x0
-CHECK-NEXT:   }
-CHECK-NEXT: }
+# RUN: llvm-readobj -A %p/Inputs/options.obj.elf-mipsel | FileCheck %s
+
+# CHECK:      MIPS Options {
+# CHECK-NEXT:   ODK_REGINFO {
+# CHECK-NEXT:     GP: 0x0
+# CHECK-NEXT:     General Mask: 0xF2000017
+# CHECK-NEXT:     Co-Proc Mask0: 0x0
+# CHECK-NEXT:     Co-Proc Mask1: 0x0
+# CHECK-NEXT:     Co-Proc Mask2: 0x0
+# CHECK-NEXT:     Co-Proc Mask3: 0x0
+# CHECK-NEXT:   }
+# CHECK-NEXT: }
+
+## Check we report a warning when we are unable to find the .MIPS.options section due to an error.
+# RUN: yaml2obj --docnum=1 -DNULLNAME=0xffff %s -o %t.err1
+# RUN: not llvm-readelf -A %t.err1 2>&1 | FileCheck %s -DFILE=%t.err1 --check-prefix=FIND-ERR --implicit-check-not=warning:
+# RUN: not llvm-readobj -A %t.err1 2>&1 | FileCheck %s -DFILE=%t.err1 --check-prefix=FIND-ERR --implicit-check-not=warning:
+
+# FIND-ERR: warning: '[[FILE]]': error locating the .MIPS.abiflags section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: warning: '[[FILE]]': error locating the .MIPS.options section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: warning: '[[FILE]]': error locating the .reginfo section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: error: '[[FILE]]': error locating the .got section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_MIPS
+Sections:
+  - Type:   SHT_NULL
+    ShName: [[NULLNAME=0x0]]
+  - Name: .MIPS.options
+    Type: SHT_MIPS_OPTIONS

llvm/test/tools/llvm-readobj/ELF/mips-reginfo.test

-RUN: llvm-readobj -A %p/Inputs/reginfo.obj.elf-mipsel | FileCheck %s
+## Check that we are able to dump the SHT_MIPS_REGINFO section using -A properly.
 
-CHECK:      MIPS RegInfo {
-CHECK-NEXT:   GP: 0x7FEF
-CHECK-NEXT:   General Mask: 0xB00001F6
-CHECK-NEXT:   Co-Proc Mask0: 0x0
-CHECK-NEXT:   Co-Proc Mask1: 0x0
-CHECK-NEXT:   Co-Proc Mask2: 0x0
-CHECK-NEXT:   Co-Proc Mask3: 0x0
-CHECK-NEXT: }
+# RUN: llvm-readobj -A %p/Inputs/reginfo.obj.elf-mipsel | FileCheck %s
+
+# CHECK:      MIPS RegInfo {
+# CHECK-NEXT:   GP: 0x7FEF
+# CHECK-NEXT:   General Mask: 0xB00001F6
+# CHECK-NEXT:   Co-Proc Mask0: 0x0
+# CHECK-NEXT:   Co-Proc Mask1: 0x0
+# CHECK-NEXT:   Co-Proc Mask2: 0x0
+# CHECK-NEXT:   Co-Proc Mask3: 0x0
+# CHECK-NEXT: }
+
+## Check we report a warning when we are unable to find the .reginfo section due to an error.
+# RUN: yaml2obj --docnum=1 -DNULLNAME=0xffff %s -o %t.err1
+# RUN: not llvm-readelf -A %t.err1 2>&1 | FileCheck %s -DFILE=%t.err1 --check-prefix=FIND-ERR --implicit-check-not=warning:
+# RUN: not llvm-readobj -A %t.err1 2>&1 | FileCheck %s -DFILE=%t.err1 --check-prefix=FIND-ERR --implicit-check-not=warning:
+
+# FIND-ERR: warning: '[[FILE]]': error locating the .MIPS.abiflags section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: warning: '[[FILE]]': error locating the .MIPS.options section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: warning: '[[FILE]]': error locating the .reginfo section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+# FIND-ERR: error: '[[FILE]]': error locating the .got section: a section [index 0] has an invalid sh_name (0xffff) offset which goes past the end of the section name string table
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_MIPS
+Sections:
+  - Type:   SHT_NULL
+    ShName: [[NULLNAME=0x0]]
+  - Name: .reginfo
+    Type: SHT_MIPS_REGINFO

llvm/tools/llvm-readobj/ELFDumper.cpp

findNotEmptySectionByAddress(const ELFO *Obj, StringRef FileName,
                              uint64_t Addr) {
   for (const typename ELFO::Elf_Shdr &Shdr : cantFail(Obj->sections()))
     if (Shdr.sh_addr == Addr && Shdr.sh_size > 0)
       return &Shdr;
   return nullptr;
 }
 
 template <class ELFO>
-static const typename ELFO::Elf_Shdr *
+static Expected<const typename ELFO::Elf_Shdr *>
 findSectionByName(const ELFO &Obj, StringRef FileName, StringRef Name) {
-  for (const typename ELFO::Elf_Shdr &Shdr : cantFail(Obj.sections()))
-    if (Name == unwrapOrError(FileName, Obj.getSectionName(&Shdr)))
+  for (const typename ELFO::Elf_Shdr &Shdr : cantFail(Obj.sections())) {
+    Expected<StringRef> NameOrErr = Obj.getSectionName(&Shdr);
+    if (!NameOrErr)
+      return createError("error locating the " + Name +
+                         " section: " + toString(NameOrErr.takeError()));
+    if (Name == *NameOrErr)
       return &Shdr;
+  }
   return nullptr;
 }
 
 static const EnumEntry<unsigned> ElfClass[] = {
   {"None",   "none",   ELF::ELFCLASSNONE},
   {"32-bit", "ELF32",  ELF::ELFCLASS32},
   {"64-bit", "ELF64",  ELF::ELFCLASS64},
 };

 Error MipsGOTParser<ELFT>::findGOT(Elf_Dyn_Range DynTable,
                                    Elf_Sym_Range DynSyms) {
   // See "Global Offset Table" in Chapter 5 in the following document
   // for detailed GOT description.
   // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
 
   // Find static GOT secton.
   if (IsStatic) {
-    GotSec = findSectionByName(*Obj, FileName, ".got");
+    Expected<const Elf_Shdr *> GotOrErr =
+        findSectionByName(*Obj, FileName, ".got");
+    if (!GotOrErr)
+      return GotOrErr.takeError();
+    else
+      GotSec = *GotOrErr;
+
     if (!GotSec)
       return Error::success();
 
     ArrayRef<uint8_t> Content =
         unwrapOrError(FileName, Obj->getSectionContents(GotSec));
     GotEntries = Entries(reinterpret_cast<const Entry *>(Content.data()),
                          Content.size() / sizeof(Entry));
     LocalNum = GotEntries.size();
static void printMipsReginfoData(ScopedPrinter &W,
   W.printHex("Co-Proc Mask0", Reginfo.ri_cprmask[0]);
   W.printHex("Co-Proc Mask1", Reginfo.ri_cprmask[1]);
   W.printHex("Co-Proc Mask2", Reginfo.ri_cprmask[2]);
   W.printHex("Co-Proc Mask3", Reginfo.ri_cprmask[3]);
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printMipsReginfo() {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  const Elf_Shdr *Shdr = findSectionByName(*Obj, ObjF->getFileName(), ".reginfo");
-  if (!Shdr) {
+  Expected<const Elf_Shdr *> RegInfoOrErr =
+      findSectionByName(*Obj, ObjF->getFileName(), ".reginfo");
+  if (!RegInfoOrErr) {
+    reportUniqueWarning(RegInfoOrErr.takeError());
+    return;
+  }
+
+  if ((*RegInfoOrErr) == nullptr) {
     W.startLine() << "There is no .reginfo section in the file.\n";
     return;
   }
-  ArrayRef<uint8_t> Sec =
-      unwrapOrError(ObjF->getFileName(), Obj->getSectionContents(Shdr));
+
+  ArrayRef<uint8_t> Sec = unwrapOrError(ObjF->getFileName(),
+                                        Obj->getSectionContents(*RegInfoOrErr));
   if (Sec.size() != sizeof(Elf_Mips_RegInfo<ELFT>)) {
     W.startLine() << "The .reginfo section has a wrong size.\n";
     return;
   }
 
   DictScope GS(W, "MIPS RegInfo");
   auto *Reginfo = reinterpret_cast<const Elf_Mips_RegInfo<ELFT> *>(Sec.data());
   printMipsReginfoData(W, *Reginfo);
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printMipsOptions() {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  const Elf_Shdr *Shdr =
+  Expected<const Elf_Shdr *> MipsOptOrErr =
       findSectionByName(*Obj, ObjF->getFileName(), ".MIPS.options");
-  if (!Shdr) {
+  if (!MipsOptOrErr) {
+    reportUniqueWarning(MipsOptOrErr.takeError());
+    return;
+  }
+
+  if ((*MipsOptOrErr) == nullptr) {
     W.startLine() << "There is no .MIPS.options section in the file.\n";
     return;
   }
 
   DictScope GS(W, "MIPS Options");
 
-  ArrayRef<uint8_t> Sec =
-      unwrapOrError(ObjF->getFileName(), Obj->getSectionContents(Shdr));
+  ArrayRef<uint8_t> Sec = unwrapOrError(ObjF->getFileName(),
+                                        Obj->getSectionContents(*MipsOptOrErr));
   while (!Sec.empty()) {
     if (Sec.size() < sizeof(Elf_Mips_Options<ELFT>)) {
       W.startLine() << "The .MIPS.options section has a wrong size.\n";
       return;
     }
     auto *O = reinterpret_cast<const Elf_Mips_Options<ELFT> *>(Sec.data());
     DictScope GS(W, getElfMipsOptionsOdkType(O->kind));
     switch (O->kind) {
if (!Parser.getPltEntries().empty()) {
     }
   }
 }
 
 template <class ELFT>
 Expected<const Elf_Mips_ABIFlags<ELFT> *>
 getMipsAbiFlagsSection(const ELFObjectFile<ELFT> *ObjF) {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  const typename ELFT::Shdr *Shdr =
+  Expected<const typename ELFT::Shdr *> SecOrErr =
       findSectionByName(*Obj, ObjF->getFileName(), ".MIPS.abiflags");
-  if (!Shdr)
+  if (!SecOrErr)
+    return SecOrErr.takeError();
+  if (!*SecOrErr)
     return nullptr;
 
   constexpr StringRef ErrPrefix = "unable to read the .MIPS.abiflags section: ";
-  Expected<ArrayRef<uint8_t>> DataOrErr = Obj->getSectionContents(Shdr);
+  Expected<ArrayRef<uint8_t>> DataOrErr = Obj->getSectionContents(*SecOrErr);
   if (!DataOrErr)
     return createError(ErrPrefix + toString(DataOrErr.takeError()));
 
   if (DataOrErr->size() != sizeof(Elf_Mips_ABIFlags<ELFT>))
     return createError(ErrPrefix + "it has a wrong size (" +
         Twine(DataOrErr->size()) + ")");
   return reinterpret_cast<const Elf_Mips_ABIFlags<ELFT> *>(DataOrErr->data());
 }