Page MenuHomeFreeBSD

gdb: report specific stop reason for watchpoints
ClosedPublic

Authored by mhorne on Mar 9 2021, 9:14 PM.

Details

Summary

The remote protocol allows for implementations to report more specific
reasons for the break in execution back to the client.

As doing this is entirely optional, it is only implemented on amd64 and
i386. It is not as simple to obtain the address that triggered the
watchpoint on other architectures, so simply avoid doing so for now.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mhorne requested review of this revision.Mar 9 2021, 9:14 PM

Generally seems good to me.

sys/gdb/gdb_main.c
730

Do we need any delimiter between the watch and thread data?

sys/i386/i386/gdb_machdep.c
120

Would this make sense in a x86/gdb_machdep? It looks identical to amd64.

sys/mips/include/gdb_machdep.h
69

Maybe add unimplemented comments here and for PPC?

You could also perhaps instead of 'gdb_is_watchpoint' just have this be a 'gdb_cpu_trap_reason(type)' or the like that is the MD function. For example, x86 might report 'swbreak' or 'hwbreak' in the future and it's probably easiest to let the logic for picking the reason be in a single MD routine.

sys/gdb/gdb_main.c
730

Yes, looks like there should be a ';' here.

730
746

I might be tempted to only do the 'watch:' part in the reason function and leave the "thread" bit here FWIW.

In D29174#657098, @jhb wrote:

You could also perhaps instead of 'gdb_is_watchpoint' just have this be a 'gdb_cpu_trap_reason(type)' or the like that is the MD function. For example, x86 might report 'swbreak' or 'hwbreak' in the future and it's probably easiest to let the logic for picking the reason be in a single MD routine.

Thanks, I've applied this suggestion.

I was thinking that if we expanded the code parameter of kdb_trap() to a register_t or equivalent, it would be a good way to convey some trap-specific information to gdb_cpu_stop_reason(), e.g. the far register on arm64. This seems like the field's original purpose, though it is almost entirely unused at the present. I would only do this as a future change, but we could include some of the plumbing in this review if it seems like a good idea.

sys/i386/i386/gdb_machdep.c
120

Normally yes, but I don't think it's worth introducing a new file for this one function. I'd be more tempted just to leave it unimplemented on i386 if that feels better than duplicating.

I was thinking that if we expanded the code parameter of kdb_trap() to a register_t or equivalent, it would be a good way to convey some trap-specific information to gdb_cpu_stop_reason(), e.g. the far register on arm64. This seems like the field's original purpose, though it is almost entirely unused at the present. I would only do this as a future change, but we could include some of the plumbing in this review if it seems like a good idea.

Then again, there's nothing to prevent reading far directly within gdb...

Report watch:addr for arm64 -- it was simpler than I originally thought. Add a missing semicolon.

I think this is probably fine. I would probably let the MD hook actually output whatever it wanted instead of requiring it to only do a single name and returning a value. That is I would just use gdb_tx_* directly in the new MD routine and not have the MI gdb_stop_reason intermediary, but I'm fine with it either way.

In D29174#659113, @jhb wrote:

I think this is probably fine. I would probably let the MD hook actually output whatever it wanted instead of requiring it to only do a single name and returning a value. That is I would just use gdb_tx_* directly in the new MD routine and not have the MI gdb_stop_reason intermediary, but I'm fine with it either way.

I also don't think it matters much. The only reason I didn't do it this way is because none of the MD routines currently call gdb_tx*, but there's not much of an argument to prohibit that.

Eliminate intermediary gdb_stop_reason().

Plumb in code in addition to type. This is required for amd64 and i386, where the contents of dr6 have been overwritten.

This revision is now accepted and ready to land.Mon, Mar 29, 5:24 PM