[PATCH] D74393: [llvm-readobj] Add support for decoding FreeBSD ELF notes
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 14 01:36:37 PST 2020
jhenderson added inline comments.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:1
+## Test that note values are interpreted correctly for FreeBSD binaries files.
+# RUN: yaml2obj %s > %t.o
----------------
binaries -> coredump(?)
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:2
+## Test that note values are interpreted correctly for FreeBSD binaries files.
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-readelf --notes %t.o
----------------
Minor point, but we've started to tend to use `-o %t.o` instead of shell redirection in newer tests.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:3-4
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-readelf --notes %t.o
+# RUN: llvm-readobj --notes %t.o
+# RUN: llvm-readelf --notes %t.o | FileCheck %s --check-prefix=GNU --strict-whitespace
----------------
Two spurious lines?
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:10-13
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_CORE
+ Machine: EM_RISCV
----------------
Could you remove the excessive whitespace, please? I find it makes things more readable. I'd prefer it if all values line up, with the minimal amount of space possible, i.e. something like:
```
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_CORE
Machine: EM_RISCV
```
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:19
+ - Name: FreeBSD
+ Desc: ''
+ Type: 0x00000007 # NT_FREEBSD_THRMISC
----------------
`Desc` is optional - it emits an empty description if unspecified, so you can delete this line (and similar ones below).
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:20
+ Desc: ''
+ Type: 0x00000007 # NT_FREEBSD_THRMISC
+ - Name: FreeBSD
----------------
Could you make a small change to yaml2obj to allow it to take the NT_* string as well as hex numbers, if it's practical? We already do that for most other enum-style fields (e.g. dynamic entries, section types etc), so it should be straightforward enough to copy what has been done before.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:34
+ - Name: FreeBSD
+ Desc: '4c6f72656d20697073756d20646f6c6f722073697420616d65740a00'
+ Type: 0x00000003
----------------
Is this just arbitrary data? If so, can you make it look more arbitrary by reducing the size and maybe make it some sort of pattern like "aabbccdd00"? If not, please add a comment showing what this should represent.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test:35
+ Desc: '4c6f72656d20697073756d20646f6c6f722073697420616d65740a00'
+ Type: 0x00000003
+ProgramHeaders:
----------------
Is Type 3 one of the non-core ones, or just undefined? Please add a comment explaining why "3".
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd.test:1
+## Test that note values are interpreted correctly for FreeBSD binaries files.
+# RUN: yaml2obj %s > %t.o
----------------
binaries -> binary
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd.test:2
+## Test that note values are interpreted correctly for FreeBSD binaries files.
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-readobj --notes %t.o
----------------
Same comment re. -o, whitespace in the YAML, using enum strings etc also apply in this test.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd.test:3
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-readobj --notes %t.o
+# RUN: llvm-readelf --notes %t.o | FileCheck %s --check-prefix=GNU --strict-whitespace
----------------
Spurious line?
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd.test:16-17
+ Type: SHT_NOTE
+ Flags: [ SHF_ALLOC ]
+ Address: 0x0000000000010270
+ AddressAlign: 0x0000000000000004
----------------
You should be able to delete the flag and address values here. They aren't relevant to the test.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd.test:20
+ Notes:
+ - Name: FreeBSD
+ Desc: 6CD61300
----------------
I think there should be a test case for a core note in this file, showing the behaviour for this case.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-freebsd.test:33
+
+
+# GNU: Displaying notes found at file offset 0x00000040 with length 0x00000064:
----------------
Delete extra blank line.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4874
+ {ELF::NT_FREEBSD_PROCSTAT_FILES, "NT_PROCSTAT_FILES (files data)"},
+ {ELF::NT_FREEBSD_PROCSTAT_VMMAP, "NT_PROCSTAT_VMMAP (vmmap data)"},
+ {ELF::NT_FREEBSD_PROCSTAT_GROUPS, "NT_PROCSTAT_GROUPS (groups data)"},
----------------
Seems like we only test a small number of these, but we should test all of them. Could you extend the test accordingly (possibly as a separate change), please?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4887
+ {ELF::NT_FREEBSD_NOINIT_TAG, "NT_FREEBSD_NOINIT_TAG (no .init tag)"},
+ {ELF::NT_FREEBSD_ARCH_TAG, "NT_FREEBSD_ARCH_TAG (architecture tag)"},
+ {ELF::NT_FREEBSD_FEATURE_CTL,
----------------
Ditto - this seems to be untested.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5165
+ if (IsCore)
+ return false; // No pretty-printing yet
+ switch (NoteType) {
----------------
Nit: missing trailing full stop.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5171
+ if (Desc.size() != 4) {
+ OS << " <corrupt FREEBSD_ABI_TAG>\n";
+ return false;
----------------
I don't see any testing for this and the other corrupt tag code path?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5186-5188
+ OS << " Feature flags: 0x"
+ << utohexstr(
+ support::endian::read32<ELFT::TargetEndianness>(Desc.data()));
----------------
A non-zero Feature flags test would be nice.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6542
+ if (IsCore)
+ return false; // No pretty-printing yet
+ switch (NoteType) {
----------------
Nit: missing trailing full stops for all these comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74393/new/
https://reviews.llvm.org/D74393
More information about the llvm-commits
mailing list