gdb: report specific stop reason for watchpoints

[mhorne]

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.

[cem]

Generally seems good to me.

sys/gdb/gdb_main.c

735

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

70

Maybe add unimplemented comments here and for PPC?

[jhb]

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

735

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

735

770

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

[mhorne]

Handle review notes.

[mhorne]

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.

[mhorne]

In D29174#658702, @mhorne wrote:

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

[mhorne]

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

[jhb]

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.

[mhorne]

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.

[mhorne]

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.