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.)

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 ?

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.

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.

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

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.

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.

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.

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. :(

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.

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. :)