Page MenuHomePhabricator

[llvm-readobj] Add support for decoding FreeBSD ELF notes
ClosedPublic

Authored by arichardson on Feb 11 2020, 3:20 AM.

Details

Summary

The current support only printed coredump notes, but most binaries also
contain notes. This change adds names for four FreeBSD-specific notes and
pretty-prints three of them:

NT_FREEBSD_ABI_TAG:
This note holds a 32-bit (decimal) integer containing the value of the
__FreeBSD_version macro, which is defined in crt1.o and will hold a value
such as 1300076 for a binary build on a FreeBSD 13 system.

NT_FREEBSD_ARCH_TAG:
A string containing the value of the build-time MACHINE_ARCH

NT_FREEBSD_FEATURE_CTL: A 32-bit flag that indicates to the kernel that
the binary wants certain bevahiour. Examples include setting
NT_FREEBSD_FCTL_ASLR_DISABLE which tells the kernel to disable ASLR.

After this change llvm-readobj also no longer decodes coredump-only
FreeBSD notes in non-coredump files. I've also converted the
note-freebsd.s test to use yaml2obj instead of llvm-mc.

Diff Detail

Event Timeline

arichardson created this revision.Feb 11 2020, 3:20 AM
jhenderson added inline comments.Feb 14 2020, 1:34 AM
llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
2

binaries -> coredump(?)

3

Minor point, but we've started to tend to use -o %t.o instead of shell redirection in newer tests.

4–5

Two spurious lines?

11–14

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
20

Desc is optional - it emits an empty description if unspecified, so you can delete this line (and similar ones below).

21

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.

35

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.

36

Is Type 3 one of the non-core ones, or just undefined? Please add a comment explaining why "3".

llvm/test/tools/llvm-readobj/ELF/note-freebsd.test
2

binaries -> binary

3

Same comment re. -o, whitespace in the YAML, using enum strings etc also apply in this test.

4

Spurious line?

17–18

You should be able to delete the flag and address values here. They aren't relevant to the test.

21

I think there should be a test case for a core note in this file, showing the behaviour for this case.

34

Delete extra blank line.

llvm/tools/llvm-readobj/ELFDumper.cpp
4664

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?

4677

Ditto - this seems to be untested.

4915

Nit: missing trailing full stop.

4921

I don't see any testing for this and the other corrupt tag code path?

4936–4938

A non-zero Feature flags test would be nice.

6583

Nit: missing trailing full stops for all these comments.

arichardson planned changes to this revision.Feb 14 2020, 2:25 AM

Thanks for the detailed review! I'll update this patch early next week.
Many of the problems exist because I simply converted the existing assembly test to a YAML one instead of writing a new one.

emaste added a comment.Mon, Feb 1, 5:26 PM

@arichardson do you think you'll get back to this?

@arichardson do you think you'll get back to this?

I'll try and update it this afternoon.

arichardson marked 17 inline comments as done.

Address review comments and rebase on latest HEAD

arichardson edited the summary of this revision. (Show Details)Wed, Feb 3, 7:22 AM
arichardson edited reviewers, added: dim; removed: espindola.
arichardson added inline comments.
llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
35

Yes that makes sense, I just copied the value from the .s file.

36

Again this is a copy from the .s file, and it is a valid type.

arichardson marked 2 inline comments as done.Wed, Feb 3, 8:00 AM
arichardson marked an inline comment as done.
emaste added inline comments.Wed, Feb 3, 1:12 PM
llvm/test/tools/llvm-readobj/ELF/note-freebsd.test
95

0xfffffff0 looks like an ELF Tool Chain bug :(

llvm/tools/llvm-readobj/ELFDumper.cpp
6631–6632

yes, should fall back to raw data if no pretty printer available; GNU and ELF Tool Chain readelf do this

emaste added inline comments.Wed, Feb 3, 1:15 PM
llvm/test/tools/llvm-readobj/ELF/note-freebsd.test
95

Oh, nvm you set the field to 0xFFFFFFFF. ELF Tool Chain masks off the known bits as it prints them, and displays what's left.

rupprecht added inline comments.Wed, Feb 3, 2:09 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
4912–4917

Can you define this as an enum, like in llvm/include/llvm/BinaryFormat/ELF.h?

arichardson added inline comments.Wed, Feb 3, 2:12 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
4912–4917

Sounds good, I'll also move it to llvm/include/llvm/BinaryFormat/ELF.h

jhenderson added inline comments.Thu, Feb 4, 12:48 AM
llvm/include/llvm/BinaryFormat/ELF.h
1519

Nit: add trailing full stop (same below for core note types comment).

1526

Nit: I think adding a blank line before this would better match existing style.

llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
44

Nit: delete extra ProgramHeaders:

74

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.

llvm/tools/llvm-readobj/ELFDumper.cpp
5221–5225

I might have missed it, but is there testing for both halves of this code path (i.e. regular core notes + FreeBSD)?

arichardson marked 8 inline comments as done.

address comments and rebase on top of D74367

arichardson added inline comments.Thu, Feb 4, 3:38 AM
llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
74

I don't understand why this is happening, but it also appears to affect other tests. Possibly a bug in yaml2obj? Unfortunately I don't have time to investigate this.

llvm/test/tools/llvm-readobj/ELF/note-freebsd.test
95

Yeah I'm not sure what the best output format is. I think just printing the raw value in parens seems less confusing than printing the non-matching bits.

llvm/tools/llvm-readobj/ELFDumper.cpp
5221–5225

note-freebsd-core.test checks for NT_PRPSINFO (which is one of the generic core notes) with a FreeBSD owner.

jhenderson accepted this revision.Fri, Feb 5, 1:06 AM

LGTM, once the other related patches land. The <?> oddity can be sorted separately (unless a warning makes it obvious we're doing something silly in the YAML). I'm happy to investigate it myself, if you could provide the stderr (if any)?

llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
74

Is there anything on stderr? I'd expect a warning in this case.

This revision is now accepted and ready to land.Fri, Feb 5, 1:06 AM
arichardson added inline comments.Fri, Feb 5, 6:31 AM
llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
74

No stderr, but it appears that the name is printed if I change it from ET_CORE to ET_DYN.

jhenderson added inline comments.Mon, Feb 8, 12:18 AM
llvm/test/tools/llvm-readobj/ELF/note-freebsd-core.test
74

Ah, that makes sense. See the StartNotes lambda in the LLVM style printing of the notes data. It seems like we always print Section: for LLVM, even when there's no section name to be had. I haven't looked, but I suspect section names are ignored in ET_CORE files, since they aren't really expected to have sections. I've filed https://bugs.llvm.org/show_bug.cgi?id=49088 to suggest not to print the Section: line in such situations.

This revision was landed with ongoing or failed builds.Tue, Feb 9, 8:59 AM
This revision was automatically updated to reflect the committed changes.
arichardson marked 3 inline comments as done.