[PATCH] D73484: [llvm-readobj] - Improve error message reported by DynRegionInfo.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Should section sizes be in hex in the warnings? Not sure, but I feel like typical sections with entsize will have a more readable size (and entsize) when it is in hex.



================
Comment at: llvm/test/Object/invalid.test:140
 
-# INVALID-DYNSYM-SIZE: warning: '[[FILE]]': invalid section size (48) or entity size (32)
+# INVALID-DYNSYM-SIZE: warning: '[[FILE]]': invalid size of section with index 1 (48) or entity size (32)
 
----------------
I'm not sure how to work this into the way you've written the code, but this doesn't read well. It should really be "invalid size (48) or entity size (32) of section with index 1". Perhaps this wording might work too: "section with index 1 has invalid size (48) or entity size (32)".


================
Comment at: llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test:53
+
+## Show we print a warning when the size of the relocation table stored in the DT_RELASZ entry is invalid.
+# RUN: yaml2obj --docnum=2 %s -o %t2
----------------
This doesn't read well (the relocation table isn't stored in the DT_RELASZ entry...). I think it should be something like "Show we print a warning for an invalid relocation table size stored in a DT_RELASZ entry".


================
Comment at: llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test:90
+
+## Show we print a warning when the relocation table entity stored in the DT_RELAENT entry is invalid.
+# RUN: yaml2obj --docnum=3 %s -o %t3
----------------
Similar to above, how about: "Show we print a warning for an invalid relocation table entry size stored in a DT_RELAENT entry"?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test:127
+
+## Show we print a warning when the size of the relocation table stored in the DT_RELSZ entry is invalid.
+# RUN: yaml2obj --docnum=4 %s -o %t4
----------------
Same comment as above.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test:164
+
+## Show we print a warning when the size of the relocation table entity stored in the DT_RELENT entry is invalid.
+# RUN: yaml2obj --docnum=5 %s -o %t5
----------------
Same as above.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:358
+# RUN: llvm-readelf --dyn-symbols %t10 2>&1 | FileCheck %s -DFILE=%t10 --check-prefix=DYNSYM-SIZE-INVALID1
+## b) The same, but the DT_SYMTAB tag is present. In this case it has a priority over the information about
+##    a location and an entity size of the dynamic symbol table from the section header.
----------------
it has a priority -> the dynamic tag has priority


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:360
+##    a location and an entity size of the dynamic symbol table from the section header.
+##    The code uses sizeof(Elf_Sym) for an entity size, so it can't be incorrect and
+##    the message printed is a bit shorter.
----------------
Aside: why is it not using the DT_SYMENT dynamic tag?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:368
+
+# DYNSYM-SIZE-INVALID2: warning: '[[FILE]]': invalid size of section with index 2 (1){{$}}
+
----------------
This sort of message should be "invalid size (1) of section..." The current format isn't clear that the (1) is the size.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:404
+      - Section: .dynsym
\ No newline at end of file

----------------
Nit: no new line at EOF.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:140-141
+  std::string SizePrintName = "data size";
+  /// Entity size name. Used for error reporting. We do not print the text with
+  /// the assumption about an invalid entity size if this field is empty.
+  StringRef EntSizePrintName = "entity size";
----------------
ELF gabi refers to items in sections with non-zero sh_entsize as "entries", so  this should probably be "Entry size name".

Change the last sentence to "If this field is empty, errors will not mention the entry size."


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1938
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  for (const Elf_Shdr &Sec :
-       unwrapOrError(ObjF->getFileName(), Obj->sections())) {
+  typename ELFT::ShdrRange Sections = unwrapOrError(ObjF->getFileName(), Obj->sections());
+  for (const Elf_Shdr &Sec : Sections) {
----------------
clang-format


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

https://reviews.llvm.org/D73484