Include process IDs in core dumps.

[jhb]

Summary

Include process IDs in core dumps.

When threads were added to the kernel, the pr_pid member of the
NT_PRSTATUS note was repurposed to store LWP IDs instead of process
IDs. However, the process ID was no longer recorded in core dumps.
This change adds a pr_pid field to prpsinfo (NT_PRSINFO) and bumps the
version of prpsinfo to 2.

Note: We could perhaps choose to leave the version at 1 and require
core dump parsers to use the size of the note data to determine if
pr_pid is present or not. I'm open to whatever folks think is best.

Test Plan

• Using patches from github/bsdjhb/gdb.git on branch psinfo2 which parses pr_pid from version 2 NT_PRSINFO notes, run gdb against a newer core dump and verify the PID is correct in 'info inferiors'

[kib]

Did you tried e.g. readelf on the new core files ?

sys/sys/procfs.h

81 ↗

(On Diff #18159)

Since you are extending the structure, might be add a padding right now as well ? Of course, when padding is used, the version number must be bumped anyway. but I think it is still easier for consumers.

[jhb]

readelf only parses the note name and type, it doesn't care about the note contents, so it doesn't care about the change.

There is 2 bytes of padding because the two char arrays end on a 16-bit boundary. I could add a uin16t_t pad0 explicitly if you think that is best.

This is a pointer to the change for devel/gdb (really binutils) to handle this btw:

https://github.com/bsdjhb/gdb/compare/psinfo2

[kib]

In D7117#148638, @jhb wrote:

readelf only parses the note name and type, it doesn't care about the note contents, so it doesn't care about the change.

Ok.

There is 2 bytes of padding because the two char arrays end on a 16-bit boundary. I could add a uin16t_t pad0 explicitly if you think that is best.

No, I mean adding reserved paddings for future structure extension.

[jhb]

In D7117#148648, @kib wrote:

In D7117#148638, @jhb wrote:

There is 2 bytes of padding because the two char arrays end on a 16-bit boundary. I could add a uin16t_t pad0 explicitly if you think that is best.

No, I mean adding reserved paddings for future structure extension.

Well, in theory we could just use the note data size and not have to bump the version number at all. (This would actually be more friendly to older binutils in some way.) That is, you could leave the version number unchanged so long as you just keep appending fields to the end and have consumers know to check the note size (similar to how struct ptrace_lwpinfo works). That was one of the reasons for my original question. I'm actually starting to think that is simpler, except it's a bit awkward to document in the comments in procfs.h compared to the current version annotations. The idea then would be to only bump the version if we were to change the meaning of existing fields.

[emaste]

sys/sys/procfs.h

64 ↗

(On Diff #18159)

What about LWP (Thread) ID as the comment? I think many folks these days won't know what an LWP is (even if they're unlikely to be parsing struct prstatus)

81 ↗

(On Diff #18159)

IMO not necessary. If you look at jhb's gdb patch, extra padding wouldn't have made this extension easier. LLDB's parsing is similar.

[emaste]

I'm actually starting to think that is simpler, except it's a bit awkward to document in the comments in procfs.h compared to the current version annotations. The idea then would be to only bump the version if we were to change the meaning of existing fields.

I'd be fine with that approach too and agree it probably makes backwards compatibility slightly easier. You could tag it as "1a" or such in the comment.

That said, if we do plan to bump the version only if/when the meaning of any existing fields change the comments on each struct element don't really make sense anyway.

[kib]

In D7117#148654, @emaste wrote:

I'm actually starting to think that is simpler, except it's a bit awkward to document in the comments in procfs.h compared to the current version annotations. The idea then would be to only bump the version if we were to change the meaning of existing fields.

I'd be fine with that approach too and agree it probably makes backwards compatibility slightly easier. You could tag it as "1a" or such in the comment.

Well, ok, my proposal/question was from my understanding of the interface, not from a real usage of it.

[jhb]

• Revert PRSTATUS_VERSION to 1. Consumers will depend on the note size.

[jhb]

• Note that LWPs == threads.

[jhb]

Rebase.