Page MenuHomeFreeBSD

Support for branch instruction on armv7 with ptrace single step
ClosedPublic

Authored by wma_semihalf.com on Oct 28 2015, 6:29 AM.

Details

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.

Diff Detail

Repository
rS 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

wma_semihalf.com retitled this revision from to Support for branch instruction on armv7 with ptrace single step.
wma_semihalf.com updated this object.
wma_semihalf.com edited the test plan for this revision. (Show Details)
wma_semihalf.com added reviewers: ian, jhb, imp, kib, andrew, zbb.
andrew added inline comments.Oct 28 2015, 9:22 AM
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 edited edge metadata.Oct 28 2015, 9:49 AM

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 edited edge metadata.

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 accepted this revision.Oct 29 2015, 8:18 AM
kib edited edge metadata.

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.

This revision is now accepted and ready to land.Oct 29 2015, 8:18 AM

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 edited edge metadata.

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

This revision now requires review to proceed.Oct 29 2015, 4:48 PM
kib added a comment.Oct 30 2015, 9:25 PM

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.

wma_semihalf.com edited edge metadata.
kib accepted this revision.Nov 2 2015, 12:03 PM
kib edited edge metadata.

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.

This revision is now accepted and ready to land.Nov 2 2015, 12:03 PM
andrew added inline comments.Nov 2 2015, 12:17 PM
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.

This revision was automatically updated to reflect the committed changes.