[PATCH] D64470: [Object/ELF] - Improve error reporting for notes.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/Object/ELF.h:205
+    if (Phdr.p_type != ELF::PT_NOTE)
+      llvm_unreachable("Phdr is not of type PT_NOTE");
+
----------------
I'm not sure making this llvm_unreachable is correct: if you are using libObject as a library, you could pass in an invalid Phdr, which should report an error, not an assertion.


================
Comment at: include/llvm/Object/ELF.h:225
+    if (Shdr.sh_type != ELF::SHT_NOTE)
+      llvm_unreachable("Shdr is not of type SHT_NOTE");
+
----------------
Same comment about llvm_unreachable applies here.


================
Comment at: test/tools/llvm-readobj/gnu-notes.test:1
-# RUN: yaml2obj %s > %t.so
-# RUN: llvm-readelf --notes %t.so | FileCheck %s --check-prefix=GNU
-# RUN: llvm-readobj --elf-output-style LLVM --notes %t.so | FileCheck %s --check-prefix=LLVM
+## Test tools are able to dump different types of the notes.
 
----------------
of the notes -> of notes


================
Comment at: test/tools/llvm-readobj/gnu-notes.test:24
 # LLVM-NEXT:   NoteSection {
-# LLVM-NEXT:     Offset: 0x340
+# LLVM-NEXT:     Offset: 0x200
 # LLVM-NEXT:     Size: 0x20
----------------
Any particular reason you've changed these Offset values?


================
Comment at: test/tools/llvm-readobj/gnu-notes.test:105
+
+# ERR2: error: SHT_NOTE section [index 1] has invalid offset (0x180) or size (4294901760)
+
----------------
I don't know what consistency says here, but it was very off-putting to see size listed as decimal, given offset is in hex. I'd prefer both to be hex if possible.


================
Comment at: test/tools/llvm-readobj/gnu-notes.test:149
+
+# ERR4: error: PT_NOTE header has invalid offset (0x1b8) or size (4294901760)
+
----------------
Same comment here.


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

https://reviews.llvm.org/D64470