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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
Comment at: llvm/test/tools/llvm-readobj/ELF/mips-abiflags.test:352-353
 Sections:
+  - Type:   SHT_NULL
+    ShName: [[NULLNAME=0x0]]
   - Name:     .MIPS.abiflags
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/mips-options-sec.test:1
-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 that we are able to dump the SHT_MIPS_OPTIONS section using -A properly.
+
----------------
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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1275
+    if (!NameOrErr)
+      return createError("error locating the " + Name +
+                         " section: " + toString(NameOrErr.takeError()));
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84651/new/

https://reviews.llvm.org/D84651