Support for branch instruction on armv7 with ptrace single step

[wma_semihalf.com]

Summary

Previous approach supported only "continuous" code without any kind of
branch instructions. To change that, new function was implemented,
which parses current instruction and returns an address where
the jump might happen (alternative addr). Mdthread structure
was extended to support two breakpoints (one directly below
current instruction and the second placed on the alternative
location). One of them must trigger regardless if the instruction
has or has not been executed due to condition field.
Upon cleanup, both software breakpoints are removed.

WARNING: this implementation parses only the most common instructions
that are present in the code (like 99.99% of all), but there
is a chance there are some left, not covered by the parsing routine.
Parsing is done only for 32-bit instruction, no Thumb nor Thumb-2
support is provided.

[andrew]

sys/arm/arm/machdep.c

676–677 ↗

(On Diff #9763)

Why not something like:

#define DB_ARM_INST_IS_BRANCH(i) (((i) & DB_ARM_INST_BRANCH_MASK) == DB_ARM_INST_BRANCH_MATCH)
..
if (DB_ARM_INST_IS_BRANCH(cur_instr)) {
...

or similar?

767 ↗

(On Diff #9763)

Missing space

779 ↗

(On Diff #9763)

You can write this without the try_alt label.

if (!error) {
  error = ptrace_write_int(...);
  if (error)
    td->td_md.md_ptrace_instr = 0;
  else
    td->td_md.md_ptrace_addr = ...;
}

[kib]

This is very good and needed change.

I have a reservations about the md_ptrace_instr mechanism as the whole. I think that it is too much wrong as it is currently implemented. The state modifyied by the single-stepper is global (it is the shared process virtual address space), while md_ptrace_instr is per-thread. If two threads step over the same instruction, they would either corrupt the program text, or corrupt other thread single-stepping state. IMO, the only sane design there is to have global breakpoint map, keyed by bp address, with each breakpoint having the ref counter.

sys/arm/arm/machdep.c

636 ↗

(On Diff #9763)

Arguably, this must be a panic instead.

668 ↗

(On Diff #9763)

cur_instr should have unsigned type, and I think that the explicitely sized uint32_t is the best there. Same for locals below, probably except offset.

Also, I do not like the fact that the function interface inlines error returns together with success. Since you selected 0 as the error indicator, you cannot handle branches to address zero, which is, in principle, allowed for userspace. You could change return to -1 to indicate no branch or error, but I would prefer to split them.

722 ↗

(On Diff #9763)

No need for line break.

[wma_semihalf.com]

Yep, you're right. Currently the ptrace stepping works only for very special case. The interface requires redesign, but I'd like to have this patch integrated first, just to let some of debugging be working.
There is also a problem, when you try to step over the code from a shared library. Modification of any instruction becomes global for all processes using the same lib.
However, I believe that someone trying to use this functionality now is aware of some of its limitations. Or at least should be...

[kib]

In D4021#84079, @wma_semihalf.com wrote:

Yep, you're right. Currently the ptrace stepping works only for very special case. The interface requires redesign, but I'd like to have this patch integrated first, just to let some of debugging be working.

I agree that the redesign is the independed endeavor.

There is also a problem, when you try to step over the code from a shared library. Modification of any instruction becomes global for all processes using the same lib.

Hm, are you sure about this ? Shared object's text is mapped private, same as the main binary text. The proc_rwmem() functionality, which backs ptrace_write_int(), correctly handles COW, or, at least it did when I last checked it.

However, I believe that someone trying to use this functionality now is aware of some of its limitations. Or at least should be...

Another argument could be that since ARMv7 hardware does not provide host-managed trace functionality, this code could be removed from the kernel and only live in the userspace debugger. At least, this would reduce the exposure of the bugs.

[wma_semihalf.com]

Hm, are you sure about this ? Shared object's text is mapped private, same as the main binary text. The proc_rwmem() functionality, which backs ptrace_write_int(), correctly handles COW, or, at least it did when I last checked it.

I though the shared libs pages are not mapped as COW... apparently I was mistaken. Sorry for confusion.

[wma_semihalf.com]

Argh... I must have been drunk I didn;t notice the parsing function is already there :) With few tweaks it looks usable.

[kib]

arm/db_interface.c is optional on option ddb.

I think that for your approach to be acceptable, you should move branch_taken*() into unconditionally compiled file and make ddb reference it.

sys/arm/arm/db_interface.c

299 ↗

(On Diff #9795)

What for is this ? You could use void *cookie __unused in the function declaration.

[kib]

Other than that small nit, the patch looks good to me.

sys/arm/arm/db_interface.c

323 ↗

(On Diff #9880)

It does not make much sense to panic from the ddb context. The panic was there before, but your change is the good opportunity to clear the mess. You can try to use kdb_reenter() to get out of the situation.

[andrew]

sys/arm/arm/machdep.c

636 ↗

(On Diff #9880)

Why panic over KASSERT? And what if reg is negative (it's signed)?

729–730 ↗

(On Diff #9880)

Extra newline

1193 ↗

(On Diff #9880)

In general I'm not a fan of magic numbers, especially when there are limited comments explaining what they are for. This is the case in this switch block.

1264–1267 ↗

(On Diff #9880)

Do we not have a function for this?

sys/arm/include/db_machdep.h

93 ↗

(On Diff #9880)

I don't understand this comment.