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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/mips-options-sec.test:48
+                    0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, ## ODK_REGINFO: gp register value.
+## A descriptor for a one more arbirtary supported option.
+                    0x1, 0x28, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
----------------
for a one -> for one


================
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
+
----------------
Here and below, why are you running the llvm-readobj command twice? Did you mean for it to be llvm-readelf?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3342
+  if (From.size() < sizeof(Elf_Mips_Options<ELFT>))
+    return createError("the .MIPS.options section has a wrong size (0x" +
+                       Twine::utohexstr(From.size()) + ")");
----------------
a wrong size -> an invalid size


================
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" +
----------------
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)


================
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" +
----------------
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"?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3360
+          "the .MIPS.options section of kind " +
+          Twine(getElfMipsOptionsOdkType(O->kind)) + " has a wrong size (0x" +
+          Twine::utohexstr(From.size()) + "), the expected size is 0x" +
----------------
a wrong size -> an invalid size


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

https://reviews.llvm.org/D84854