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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/note-core-ntfile.test:7
+
+## llvm-mc doesn't support generating ET_CORE files; the following section data
+## was generated with the following steps:
----------------
> llvm-mc doesn't support generating ET_CORE files; the following section data was generated with the following steps:

May be worth mentioning that the `Content` field of .note.foo` was derived from the following assembly.

The comment make me think that other fields of the file are derived from llvm-mc output, but they aren't according to "following steps".

(Not necessarily mentioned in the comment: llvm-readobj doesn't give hex pairs you can immediately use, but `llvm-objcopy --dump-section=.note.foo=>(xxd -p) tmp.o /dev/null` (`>(..)` is zsh syntax) does.)


================
Comment at: llvm/test/tools/llvm-readobj/note-core-ntfile.test:14
+#       .align 4
+#       .long 5 /* namesz */
+#       .long end - begin /* descsz */
----------------
Nit: `/* */` -> `#`

`#` is probably a bit easier to read.


================
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';
----------------
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"`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65832