[PATCH] D74393: [llvm-readobj] Add support for decoding FreeBSD ELF notes

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:1514
 
+// FreeBSD note types
+enum {
----------------
Nit: add trailing full stop (same below for core note types comment).


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:1521
+};
+// FreeBSD core note types
+enum {
----------------
Nit: I think adding a blank line before this would better match existing style.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:43
+        Type: NT_PRPSINFO
+ProgramHeaders:
+ProgramHeaders:
----------------
Nit: delete extra `ProgramHeaders:`


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:73
+# LLVM-NEXT:   NoteSection {
+# LLVM-NEXT:     Name: <?>
+# LLVM-NEXT:     Offset: 0xB0
----------------
Why the `<?>` for the name? Sounds like something odd is going on if I follow it correctly (there are perfectly good section headers as far as I can see from yaml). If it's expected, add a comment explaining why.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5220-5224
+      // FreeBSD also places the generic core notes in the FreeBSD namespace.
+      StringRef Result = FindNote(FreeBSDCoreNoteTypes);
+      if (!Result.empty())
+        return Result;
+      return FindNote(CoreNoteTypes);
----------------
I might have missed it, but is there testing for both halves of this code path (i.e. regular core notes + FreeBSD)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74393