add 'struct ptrace_lwpinfo' to a corefile note and support in procstat to view it

[tychon]

Summary

This patch adds support for capturing 'struct ptrace_lwpinfo' for signals resulting in a process dumping core in the corefile of the process.

Also included is some extension to procstat to view select members of 'struct ptrace_lwpinfo' from the contents of the note.

Test Plan

Terminate a process with SIGBUS, SIGILL, SIGQUIT and SIGSEGV and view the contents of the note.

[jhb]

So while this is what Linux does (a single siginfo note), the idea I've been kicking around is replacing the NT_THRMISC note with a per-thread note that stores the entire 'struct ptrace_lwpinfo'. This would include both the thread name as well as the siginfo_t for each thread. It is almost an expanded NT_PRSTATUS (though not quite). But in particular we have slowly added more things to 'struct ptrace_lwpinfo' over the last few years and having a note that includes it would mean that each new thing we add in the future would automatically be included in core dumps. This is not a bad patch though, and if kib@ is ok with just having a single siginfo_t note I won't object. (Not sure what others think of a NT_LWPINFO or the like as I don't think I've mentioned this idea before.)

[kib]

In D9995#206488, @jhb wrote:

So while this is what Linux does (a single siginfo note), the idea I've been kicking around is replacing the NT_THRMISC note with a per-thread note that stores the entire 'struct ptrace_lwpinfo'. This would include both the thread name as well as the siginfo_t for each thread. It is almost an expanded NT_PRSTATUS (though not quite). But in particular we have slowly added more things to 'struct ptrace_lwpinfo' over the last few years and having a note that includes it would mean that each new thing we add in the future would automatically be included in core dumps. This is not a bad patch though, and if kib@ is ok with just having a single siginfo_t note I won't object. (Not sure what others think of a NT_LWPINFO or the like as I don't think I've mentioned this idea before.)

Sure I prefer the approach you described, but somebody has to implement it. If the alternatives are between not having anything and this patch, lets go with the patch. But might be the patch' author consider extending and modifying it to implement your proposal ?

[tychon]

In D9995#206504, @kib wrote:

In D9995#206488, @jhb wrote:

So while this is what Linux does (a single siginfo note), the idea I've been kicking around is replacing the NT_THRMISC note with a per-thread note that stores the entire 'struct ptrace_lwpinfo'. This would include both the thread name as well as the siginfo_t for each thread. It is almost an expanded NT_PRSTATUS (though not quite). But in particular we have slowly added more things to 'struct ptrace_lwpinfo' over the last few years and having a note that includes it would mean that each new thing we add in the future would automatically be included in core dumps. This is not a bad patch though, and if kib@ is ok with just having a single siginfo_t note I won't object. (Not sure what others think of a NT_LWPINFO or the like as I don't think I've mentioned this idea before.)

Sure I prefer the approach you described, but somebody has to implement it. If the alternatives are between not having anything and this patch, lets go with the patch. But might be the patch' author consider extending and modifying it to implement your proposal ?

It seems reasonable to rework this patch to include the suggested approach of using a per-thread 'struct ptrace_lwpinfo' instead of just an interim per-process siginfo_t. I'll go ahead and do that and update this review.

I'll also sheepishly inquire if anyone can point out why Differential is claiming these changes have failed to build. I'm perusing the logs and can't discern anything obvious and furthermore took the patch from a copy of the code which was building for me.

[kib]

In D9995#206560, @tychon wrote:

I'll also sheepishly inquire if anyone can point out why Differential is claiming these changes have failed to build. I'm perusing the logs and can't discern anything obvious and furthermore took the patch from a copy of the code which was building for me.

I am sure that this is because the phabricator has no way to perform the build, not because the build was tried and your patch broke it.

[tychon]

This needs a bit more polish (and testing) but at a higher level how's this?

[tychon]

Mark noted my diff was missing some context. I've generated with "svnlite diff -x -U9999".

[markj]

lib/libprocstat/Symbol.map

24

I believe that symbols new in FreeBSD 12 should belong to FBSD_1.5.

lib/libprocstat/core.c

435

Style is to have a space before the open paren.

461

This could be for (n = 0; offset < eoffset; n++) {

sys/kern/imgact_elf.c

2031

Why include the size with each element? ptrace_lwpinfo has a fixed size which is already included in the note header.

2042

This value might be stale if the thread received a signal while running under ptrace.

sys/kern/kern_sig.c

1351

I think it's ok to overload td_dbgksi this way, but its name and annotation should probably be changed to reflect the additional use.

An explicit ksiginfo_copy() would be preferable here, since not all fields of ksi can be meaningfully copied. Ditto below.

3009

Perhaps just make ksi = &td->td_dbgksi at the beginning of the function?

[kib]

lib/libprocstat/core.c

182–183

Remove this switch at all ? Fill all types into the psc_type_info array, and use an indicative value for 'variable size'.

435

Same.

sys/kern/imgact_elf.c

2031

No, at least not on the long run. New elements are added to struct ptrace_lwpinfo at the end with some frequency, and size is used as cheap way to specify version. So providing explicit version by size is useful. OTOH, there is already size of the note, so might be explicitly passed size is simply redundant.

2033

Style: move local var declaration into common block. No initialization at declaration.

[tychon]

Thanks for the reviews. I will incorporate the feedback; some of which I have replied to 'inline'. Mark, I'll also take another pass at the usage and initialization of 'td_dbgksi'.

Did anyone look at the xo_emit() stuff? There must be a better way of determining the width of a pointer printed in hexedmial than "2 * sizeof(void *) + 2" but nothing obvious sprung to mind.

[tychon]

Missed inline comment and the structsize being part of the note itself.

[tychon]

Sorry to be so noisy that's twice now that Differential threw away my inline note. To belabor the point, the structsize is part of the note itself to support procstat -- because that's how 'procstat' notes are formatted. If we don't want to use procstat to view the note I can omit the structsize but it does seem like a nice to have. That's assuming that the size of the note is sufficient for rudimentary versioning otherwise this is just a reinforcement of that.

[markj]

In D9995#208023, @tychon wrote:

Sorry to be so noisy that's twice now that Differential threw away my inline note.

Hm, you still need to hit "submit" after saving an inline note in order to post it. I can't think of any other gotchas.

To belabor the point, the structsize is part of the note itself to support procstat -- because that's how 'procstat' notes are formatted. If we don't want to use procstat to view the note I can omit the structsize but it does seem like a nice to have. That's assuming that the size of the note is sufficient for rudimentary versioning otherwise this is just a reinforcement of that.

Thanks, I see.

In D9995#208017, @tychon wrote:

Did anyone look at the xo_emit() stuff? There must be a better way of determining the width of a pointer printed in hexedmial than "2 * sizeof(void *) + 2" but nothing obvious sprung to mind.

I don't see anything significantly better in e.g., usr.bin/netstat/unix.c, which prints the PCB address for each unix socket. It uses sizeof(void *) to select a format string.

[tychon]

In D9995#208051, @markj wrote:

In D9995#208023, @tychon wrote:

Sorry to be so noisy that's twice now that Differential threw away my inline note.

Hm, you still need to hit "submit" after saving an inline note in order to post it. I can't think of any other gotchas.

To belabor the point, the structsize is part of the note itself to support procstat -- because that's how 'procstat' notes are formatted. If we don't want to use procstat to view the note I can omit the structsize but it does seem like a nice to have. That's assuming that the size of the note is sufficient for rudimentary versioning otherwise this is just a reinforcement of that.

Thanks, I see.

Sorry, I reread my reply and it came off unintentionally negative. I was frustrated having written a reply 3x and having it gone amiss each time. I'll experiment a bit more with differential as it's can probably just something I'm doing incorrectly.

On topic, I'm assuming retaining the ability to extract the note via procstat is indeed desirable?

In D9995#208017, @tychon wrote:

Did anyone look at the xo_emit() stuff? There must be a better way of determining the width of a pointer printed in hexedmial than "2 * sizeof(void *) + 2" but nothing obvious sprung to mind.

I don't see anything significantly better in e.g., usr.bin/netstat/unix.c, which prints the PCB address for each unix socket. It uses sizeof(void *) to select a format string.

Okay. Then I'll stick with what I have. I haven't any previous experience with xo_emit() so while it seemed to work it may not be ideal.

[markj]

In D9995#208064, @tychon wrote:

On topic, I'm assuming retaining the ability to extract the note via procstat is indeed desirable?

Oh, I think so. I had just been comparing with the NT_THRMISC implementation and failed to note that procstat made use of the struct size.

In D9995#208017, @tychon wrote:

Okay. Then I'll stick with what I have. I haven't any previous experience with xo_emit() so while it seemed to work it may not be ideal.

Seems reasonable to me. I find libxo format strings to be pretty inscrutable regardless. :(

[jhb]

Most core notes do not include an explicit struct size but instead depend on the note size field in the note header. This is certainly true of all the per-thread notes like NT_PRSTATUS and NT_PRPSINFO as well as additional notes for alternate register sets like XSAVE state, powerpc vector register state, etc. When I added a new field to NT_PRPSINFO I depended on the note size in gdb to determine if the new field was available.

In general I'd say that the struct size thing is mostly useful for notes that contain an array of 1 or more entries rather than notes that always contain exactly 1 entry. It is slightly more work to skip over it in gdb when reading it or to prepend it if I update 'gcore' to generate these notes, but not a lot. Reading this info from procstat is probably useful (you could also include things like with PL_FLAGS are set, syscall number for SCE/SCX, child pid for FORKED, etc.).

Note that you will also want to patch usr.bin/gcore/elfcore.c. The code is somewhat similar-ish to imgact_elf.c. For gcore you can just use ptrace(PT_LWPINFO) to extract the struct that you write into the note.

[tychon]

I think I've incorporated all the feedback I received with the exception of adding to the procstat output the additional data collected in the 'gcore' case. In that case, the core file contains more data than I'm printing but I'm struggling on how to format it in a useful way. That could make a reasonable follow on commit.

[markj]

In D9995#208711, @tychon wrote:

I think I've incorporated all the feedback I received with the exception of adding to the procstat output the additional data collected in the 'gcore' case. In that case, the core file contains more data than I'm printing but I'm struggling on how to format it in a useful way. That could make a reasonable follow on commit.

That sounds reasonable to me - this diff is pretty large as it is. :)

lib/libprocstat/libprocstat.c

2488

Use calloc() instead?

2494

sizeof(*pl), as above?

2501

Style: space before open paren.

sys/kern/imgact_elf.c

2029

Style: pl's declaration should come before the others.

2042

It still seems as though this is an issue? One way to handle it might be to have sigexit() clear td_si for the other threads in the calling thread's proc.

2058

Why order the threads this way? That is, why not just loop with:

p = td->td_proc;
TAILQ_FOREACH(td, &p->p_threads, td_plist) {
....

?

usr.bin/gcore/elfcore.c

709

Hm, shouldn't p be of type char * in order to do this arithmetic?

713

Style: continuation lines should be indented by four spaces.

usr.bin/procstat/procstat_ptlwpinfo.c

27

.c files use

#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");

to get the ID string.

[jhb]

sys/kern/imgact_elf.c

2058

The existing code is correct here. For other thread state (e.g. register notes) we right out the thread that died due to sigexit() first followed by the remaining threads. Debuggers use this to pick the "default" thread to show when opening a core. It is probaby less important for this particular note type, but I think it's better to keep all the per-thread notes grouped together in the same order.

Oh. You are only writing out one note? Please write out per-thread notes. It will be far, far simpler to work with in debuggers like gdb as per-thread notes ala thrmisc. That probably makes procstat unhappy as procstat only deals with a single note, but I'd rather we fix procstat. This new note should work like NT_THRMISC and eventually replace it. If you'd rather not spend more time on this I can work on doing that, but this note should be per-thread.

[tychon]

sys/kern/imgact_elf.c

2042

Rather than clearing it for all other threads, I'll only dump the signify_t if thread being dumped is the one which triggered the core dump i.e. received the signal. That should address the stale issue.

2058

Originally I was writing more than one note but then I interpreted your comment about the structsize being useful for an array to consolidating the notes into one. I'll revert to the previous version and address this last batch of comments. I think it's getting close.

[tychon]

I've reverted back to a note-per-thread and (hopefully) addressed some review comments.

[markj]

sys/kern/imgact_elf.c

2042

It looks like you went the route of clearing them after all? Please consider moving that code into a subroutine.

[tychon]

sys/kern/imgact_elf.c

2042

Yes, after going back to multiple notes per core file, I realized that what I had said about discerning the thread that received the signal from within the note generating function no longer made sense.

I'll create a subroutine for this.

[tychon]

Addressed Mark's request to create a function to capture the siginfo_t for the specific thread, and clear the stale siginfo_t for other threads, in the process receiving the signal.

[tychon]

Deleted debug drivel which snuck in.

[tychon]

Is everyone reasonably content with this?

[jhb]

At some point we should fix the core dump to include a fully populated lwpinfo. (Might be worth adding a /* TODO */ to that effect).

[markj]

This was committed in r316286.