arm64: extend struct db_reg to include watchpoint registers

[mhorne]

Summary

The motivation is to provide access to these registers from userspace
via ptrace(2).

As is, this is an ABI-breaking change. So far I have been unable to come
up with any ways of providing compatibility that are not completely
messy. The main difficulty of this comes from the fact that the
copyin/copyout is performed in the MI function sys_ptrace(), with length
sizeof(struct dbregs).

Fortunately, it does not appear there are any consumers of this
interface in base or in ports, on arm64. Two large potential consumers
of this interface, gdb and lldb, have not yet been taught to use it.
Since PT_GETDBREGS is FreeBSD-specific, we may be able to make this
change now without major consequence.

One alternative solution would be to add one or more arm64-specific
ptrace requests, whose purpose is to get or set the watchpoint
registers. This would preserve the ABI, but is slightly less convenient
for consumers.

PR: 252860

Test Plan

Only compile tested. Will validate further if we agree on this course of action.

[emaste]

I think the ABI incompatibility is no concern; as you point out gdb and lldb are the only expected consumers. IMO it's better that we get this into 13.0 than worry about hypothetical existing ABI issues.

[mgorny_gentoo.org]

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.

[mhorne]

In D28415#635669, @mgorny_gentoo.org wrote:

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.

This constitutes an API change, and I'm less inclined to provide this since amd64 is already using PT_GETDBREGS with data == 0. The kernel could safely pass back the size of the copied structure in data, but it would only help detect the buffer overflow, not prevent it.

[jhb]

If we need to support a variable size, then we could make it a regset instead in the future. I think this is fine to go ahead and break now. Debuggers can use uname() to handle the old ABI if they really need to.

[markj]

sys/arm64/include/reg.h

69

Is there any reason not to provide these as separate fields? If so, perhaps it would be worth providing accessors?

77

This looks like it should be called db_breakregs - is it a problem to rename since we're already adding fields and changing the value of db_info?

[mhorne]

Rename db_regs to db_breakregs. Split db_info into separate fields.

[mgorny_gentoo.org]

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

dbregs read: debug_ver=6, nbkpts=6, nwtpts=1

However, dmesg says:

Debug Features 0 = <2 CTX BKPTs,4 Watchpoints,6 Breakpoints,PMUv3,Debugv8>

[andrew]

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

[mgorny_gentoo.org]

In D28415#639223, @andrew wrote:

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

But why does it do that? According to dmesg, the register indicates support for 6 watchpoints.

[mhorne]

In D28415#639234, @mgorny_gentoo.org wrote:

In D28415#639223, @andrew wrote:

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

But why does it do that? According to dmesg, the register indicates support for 6 watchpoints.

Some fields are masked from userspace, this being one of them, because it was unused until now. I will upload a couple more required patches today (including the fix for this), so please sit tight for a little longer.

[mhorne]

Validate the input registers, as with D28560.

[mgorny_gentoo.org]

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.

[andrew]

It would pay to bump __FreeBSD_version when you commit this.