[PATCH] D84854: [llvm-readobj/readelf] - Refine the implementation of printMipsOptions().

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/mips-options-sec.test:80
+# RUN: llvm-readobj -A %t4 2>&1 | FileCheck %s -DFILE=%t4 -DTAG="ODK_EXCEPTIONS (2)" --check-prefix=KIND
+# RUN: llvm-readobj -A %t4 2>&1 | FileCheck %s -DFILE=%t4 -DTAG="ODK_EXCEPTIONS (2)" --check-prefix=KIND
+
----------------
jhenderson wrote:
> Here and below, why are you running the llvm-readobj command twice? Did you mean for it to be llvm-readelf?
Right. Fixed.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3348
+    return createError(
+        "the descriptior of size 0x" + Twine::utohexstr(O->size) +
+        " goes past the end of the .MIPS.options section of size 0x" +
----------------
jhenderson wrote:
> the descriptior -> a descriptor
> 
> Don't we need something about the descriptor's offset in here somewhere? If I'm not mistaken, the sizes here mgiht be a bit misleading (e.g. the section size might be the remaining section size rather than the total section size, despite saying the section size is 0x1234)
I've added offset and fixed the calculation of the size.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3359
+      return createError(
+          "the .MIPS.options section of kind " +
+          Twine(getElfMipsOptionsOdkType(O->kind)) + " has a wrong size (0x" +
----------------
jhenderson wrote:
> If there can be more than one entry in the section, this message is misleading too, since it's saying the whole section has a single kind. Should it be "a .MIPS.options entry of kind"?
Fixed.


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

https://reviews.llvm.org/D84854