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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/note-core-ntfile.test:14
+#       .align 4
+#       .long 5 /* namesz */
+#       .long end - begin /* descsz */
----------------
MaskRay wrote:
> Nit: `/* */` -> `#`
> 
> `#` is probably a bit easier to read.
I don't really have a preference, but this is consistent with all the other tests like this in this package.


================
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:
> rupprecht wrote:
> > rupprecht wrote:
> > > 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.
> > Actually, I do notice a change after testing this locally:
> > When I run a dummy crashing program and then run `llvm-readelf -n core`, there's a note with type "LINUX" that says "NT_X86_XSTATE (x86 XSAVE extended state)" in GNU readelf that now shows up as "Unknown note type: (0x00000202)"
> > 
> > So, I'll have to undo this part of the change and update the diff in a bit. We should consider separating the note type vs description printing similarly.
> > (Aside: there is very little llvm-readobj test coverage for the FreeBSD/AMD/AMDGPU note types above, I'll add some in a separate commit)
> Verified. `|| Obj->getHeader()->e_type == ELF::ET_CORE` dumps `"LINUX"`. This makes me think if we should use more specific `Name == "CORE" || Name == "LINUX"`.
> 
> I think this should be sufficient. See `Linux/fs/binfmt_elf{,_fdpic}.c`, fill_note() is only called on `"CORE"` and `"LINUX"`.
I think it's safer to check for the `ET_CORE` since the name seems vendor/kernel specific.

Actually, there's a bigger issue... the conditional logic for printing types vs description data seems to be orthogonal, so I've gone ahead and split it up. I've included my next (unmailed) patch for printing raw description bytes to show a stronger motivation, e.g. FreeBSD has custom note types defined (and so has a method to get those types), but goes through the fallback logic for printing raw bytes for the description. That's difficult/error prone to do with just one loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65832