[PATCH] D92636: [llvm-readelf/obj] - Improve diagnostics when printing NT_FILE notes.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5338
+    return createError(
+        "unable to read the file mappings number: the note of size 0x" +
+        Twine::utohexstr(Desc.size()) + " is truncated");
----------------
rupprecht wrote:
> This is checking more than just the file mappings number, it also includes the page size (note the check above is for 2 bytes).
> How about: "the note of size 0x1 is too short, expected at least 0x2"?
Done. Note: it is not 2 bytes, but 2 addresses. I.e 8 or 16 bytes.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5341
   if (Desc.getData().back() != 0)
-    return createStringError(object_error::parse_failed,
-                             "malformed note: not NUL terminated");
+    return createError("the note is not NUL terminated");
 
----------------
Side note: I think this doesn't work when N = 0 ("# of file mappings", see function comment) case.
Seems we should move this check below and exit a bit eariler when there are no mappings.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5350
+        "unable to read start/end/offset tripples: the note of size 0x" +
+        Twine::utohexstr(Desc.size()) + " is truncated");
 
----------------
rupprecht wrote:
> Maybe this could also say how many bytes it expected it to be? It should be `(2 + 3 * FileCount) * Bytes` IIRC.
> 
> And maybe say what `FileCount` is? That may give indication to a developer debugging a bad note whether the note is really too short or if the number of files is bogus.
> Maybe this could also say how many bytes it expected it to be? It should be (2 + 3 * FileCount) * Bytes IIRC.

I think that `(2 + 3 * FileCount) * Bytes` is not the right value, because there should be at least N=FileCount NUL bytes in
filenames area it seems. I.e. here we have a check for ` (2 + 3 * FileCount)` bytes, but only because we check
file names data in the code below and not right here.
So, I am not sure that we should mention an expected data size here?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5576
+    for (const typename ELFT::Note Note : Obj.notes(P, Err)) {
+      ++Index;
+      if (Error E = ProcessNoteFn(Note))
----------------
rupprecht wrote:
> `Index` should probably be incremented at the end, otherwise it reports the first note as index 1, not index 0.
This is intentional. We enumerate program headers (few lines above) starting from 1 too.
And we do the same when printing warnings for relocations (in ` DumpStyle<ELFT>::forEachRelocationDo`).


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

https://reviews.llvm.org/D92636