[PATCH] D65832: [llvm-readelf] Implement NT_FILE core file parsing

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4339
+    return createStringError(object_error::parse_failed,
+                             "Malformed note - too short for header");
+
----------------
grimar wrote:
> Other errors in this file are lowercase, I think the new errors should follow.
These are just lifted directly from GNU readelf, but we probably don't need to match them -- reformatted/reworded all of them to be more consistent. I'm open to different wording for any of them.

(especially "too short for supplied file count", I've rewritten it as "too short for number of files", but it still sounds a little awkward)


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4363
+        StringRef(reinterpret_cast<const char *>(Filenames.data()));
+    // Advance by size() + 1 due to null delimiter.
+    Filenames = Filenames.drop_front(Filename.size() + 1);
----------------
MaskRay wrote:
> (I think it is NUL, not null.)
Wow, TIL about the difference between NUL and null


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4442
         OS << "    " << N.Type << ":\n        " << N.Value << '\n';
+    } else if (Name == "CORE" || Obj->getHeader()->e_type == ELF::ET_CORE) {
+      OS << getCoreNoteTypeName(Type) << '\n';
----------------
MaskRay wrote:
> ` || Obj->getHeader()->e_type == ELF::ET_CORE` can be deleted.
> 
> readelf -n doesn't seem to perform the test, either.
Removed -- but I should elaborate, readelf does perform this check. Note that GNU readelf actually prints notes in two stages:
* First it tries to print the note type. It starts by looking at the name, although the fallback (if the name is empty, doesn't match any of the known names, or in some (but not all) cases matches the name but doesn't match any known constants) will look at the file type (see `process_note` and `get_note_type`).
 * Note: none of the "names" it checks is `"CORE"`, it _only_ prints out the `NT_*` core descriptions if the actual elf type is `ET_CORE`
* Second, it prints note data, this time only looking at the name, i.e. if name is `"CORE"` (and it ignores the elf type) -- this is what you're talking about, but not the first.

I think these points don't matter that much. I think the only thing this would break is if someone is expecting a ET_REL/ET_EXEC file with CORE notes to print non-core NT_* fields. But still, I think we should figure out if/how we want to match GNU readelf for printing notes.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5556
+    return;
+  }
+
----------------
grimar wrote:
> This part is common and can be reused for both LLVM and GNU styles:
> 
> ```
>   if (NoteType != ELF::NT_FILE)
>     return;
> 
>   Expected<CoreNote<ELFT>> Note = readCoreNote<ELFT>(Desc);
>   if (!Note) {
>     warn(Note.takeError());
>     return;
>   }
> ```
> 
> I.e. I would try either add a one more helper or try to combine `printCoreNoteLLVMStyle` and `printCoreNote`.
I moved some of this into the GNU/LLVMStyle::printNotes methods and it's a bit more succinct now, PTAL -- I'm not sure if it's what you're going for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65832