Use a while loop.

I don't have commit access. Could you push it for me, please?

I've opened https://github.com/freebsd/freebsd-src/pull/570 for that.

Could you push this for me, as I don't have push access?

It works for me, thanks!

Superseded by D29955.

Disable coredump compression for the time being.

Switched to td_dbgflags. Removed unnecessary OOM handling. Added locking for TDF_ASTPENDING update.

Hmm, this makes me wonder if we're not going to hang if we're trying to get a coredump from a thread that's PT_SUSPEND-ed.

Thanks for the input, I'll do that in a minute. In the meantime, I've been thinking how to unsuspend one thread only. A primitive solution I can think of is using both process and thread flags, and assuming that if P2_COREDUMP is set, then only threads having TDF_COREDUMP are resumed.

Update keep state per thread, on the thread explicitly requested via ptrace(2) and malloc-ed rather than inline.

I'm sorry, I've had a newer version locally but I've forgotten to update the diff here. Lemme implement the newest comments, retest and update.

Enable enabling coredump per LWP. It mostly works — when passed PID, or LWPIDs of two spawned threads. However, if I pass the LWPID of the initial thread, the test program hangs (haven't verified if it's hanging in ptrace() yet or somewhere else).

In D29691#670121, @kib wrote:

I suspect that my code might cause more than one thread to observe P2_COREDUMP, BTW. I believe something more involved needs to be done: either the flag should be set for specific thread, or the first thread that observes the flag in AST should clear it, and then you need some other flag (?) to sync this thread with debugger.

First working version, with ability to set coredump size limit.

Ok, so apparently I'm hitting this condition:

Thanks for your suggestions. I've had to make a few modifications, particularly regarding addr and data use.

Ok. I'd use a few more pointers. I think I'm supposed to add a flag to td_flags (reusing one of the values marked as unused?), add new fields to struct proc to pass the necessary data, and then do the work inside ast() function. What I can't figure out is how to block the ptrace(2) call and trigger the context switch.

In D29691#668838, @kib wrote:

And what is the panic message? I have to guess?

Ok, so I was trying to reimplement it to write into a fd. I've managed to make it successfully dump the core... of the debugger ;-). However, when I tried to make it dump the right core, my guesswork lead me to this version… which unfortunately panics:

In D29691#667586, @kib wrote:

In D29691#667582, @emaste wrote:

I'm personally not a fan of doing more in kernel space than is absolutely necessary

Agreed, but we already have the ability to compress core dumps in the kernel so it is probably straightforward to plumb it through into this interface.

It does not require any plumbing, compression just happens if elf sv_coredump is called and compression is enabled. It indeed should be connected to some argument if we want to manage it manually for PT_COREDUMP.

May be more interesting option is the control over errors when dumping segments. Right now code exits the dump if some segment cannot be read (this could happen if e.g. file was mapped and then truncated). It is reasonable to add a tweak to skip the rest of the segment and move ahead.

In D29691#667524, @kib wrote:

In D29691#667173, @mgorny_gentoo.org wrote:

@kib, so if I agree with the FD approach, is there anything left to decide? If not, could you start giving me some pointers?

I suppose the first thing to do would be splitting the 'core' of coredump() function dealing with the fd, correct?

I do not think that passing fd as data is a wise idea. It probably makes sense to define a structure that gets a pack of arguments for the call, one of them fd.

@kib, so if I agree with the FD approach, is there anything left to decide? If not, could you start giving me some pointers?

Passing of a file descriptor was something I've been considering initially but using the path was an easier way to get a working prototype. I suppose we can switch to using an fd (passed via data, I guess?) to make things simpler and more robust. Since we lose any compatibility with NetBSD, we can also rename it to PT_COREDUMP.

In D29691#665860, @kib wrote:

coredump(9) cannot work this way. It assumes that td == curthread, and that all other threads are stopped.

In D29496#661068, @emaste wrote:

Do you know what Linux does?

In D29496#661038, @kib wrote:

Clearing %db registers seems to be the right thing to do for forked process.

What is the opinion about new thread? Should we inherit or clear db state? [Patch currently clears]

Thanks, I was able to get working hardware watchpoints with this patch.

Thank you. I get the correct number of watchpoints now ;-). I'm going to test them fully in the next few days and report back.

In D28415#639223, @andrew wrote:

That's because extract_user_id_field returns a default value of 0 for the watchpoint count field.

Ok, I'm starting to test this applied on top of 13.x and something seems off:

It seems to work for me. Thanks!

Since you're breaking ABI anyway, could you maybe future-proof it by using data to pass sizeof(dbreg)? This would permit to add extra fields in the future without risking buffer overflows in clients.

FYI, I believe that this commit broke the kernel on MIPS64, at least via qemu: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=253051

Updated per @ygy/@emaste's suggestions.

Does this look fine?

Sure, this makes sense.

In D27524#615445, @bcr wrote:

In D27524#615441, @mgorny_gentoo.org wrote:

Updated as requested. Note that this shouldn't be probably pushed yet, as it could confuse users before LLVM 12.0.0 is released.

But it does say "... starting with LLDB 12.0.0", so the confusion would be minimal, right?

Updated as requested. Note that this shouldn't be probably pushed yet, as it could confuse users before LLVM 12.0.0 is released.

Thank you!

Fixed per review comments.

That said:

1. I'm wondering if there's a common place we could move this function too not to duplicate it.
2. I suppose we could use a packed struct to look at the float80 instead of pointer hacks.

I can confirm that the patch fixes the LLDB problem too!

I'm currently building the kernel with the new version and I should have some test results in an hour or so.

Sure. Here you are:

To be honest, I think you can call 32-bit version for 32-bit programs unconditionally, independently of CPUID_STDEXT_NFPUSG. After all, 32-bit programs use 32-bit addresses, so they always fit in the shorter form.