Hi,
On Fri, 04 Apr 2008 04:53:58 +0200, Roland McGrath wrote:
> I'd like to make sure this all seems coherent and there aren't
> holes in my logic you can think of before I send this upstream.
It all PASSes all now on x86_64 and i686 (except step-to-breakpoint) on (files
will vanish in several days as it is in /scratch/):
http://koji.fedoraproject.org/scratch/jkratoch/task_547885/
> Btw, I'd find it clearer to read the asm block if you just used
> "jnz instr3" and the like instead of 1b with multiple 1: labels
> at the same places you already have named labels.
Updated.
(It was intentional - if one inserts there a new instruction and renumbers the
labels one may miss updating also the jump target. But currently updating the
C code around for the renumbering would be already much worse than the jump
target label.)
> Many of the asserts would be nicer as messages telling you what
> the offending value of ip or stopsig or whatever was. Whenever
> I actually debug the kernel, I have to tweak the test programs
> by hand to display enough info to figure out what I'm doing.
Updated.
(It was intentional - the code was much shorter and I would like to keep there
a difference between full-blown debugger and a simple testcase - as one GDB
testsuite already exists. Still some simplifications make it possibly not
worth the insufficiencies - as the duplicated blocks in `user-regs-peekpoke.c',
also all the code duplications instead of a single include file may not be
worth it, particularly the peekuser/pokeuser triplicity in
`step-jump-cont-strict.c' being used in various forms in the other testcases.)
> > Also it fails later on:
> > PTRACE_SINGLEBLOCK should stop us at instr8 (not instr5).
...
> With the kernel fix I'm using, the test case now barfs because
> of hitting "instr12: hlt" (and so SIGSEGV instead of SIGTRAP).
> That's because of the known limitation that stepping "pushf"
> puts a false TF in the flags the inferior sees, so the instr10
> branch is not taken.
TF should be set there both intentionally and unintentionally.
Intentionally as the code already does `orl $0x100, %eflags' there and
unintentionally because PTRACE_SINGLEBLOCK also sets TF, thus the `instr10'
jump should not loop.
My mistake there was expecting the CPU stops in PTRACE_SINGLEBLOCK after
`orl $0x100, %eflags'. This does not happen as PTRACE_SINGLEBLOCK already has
TF set and uses WRMSR to differ from PTRACE_SINGLESTEP. In this respect
PTRACE_SINGLEBLOCK behaves more like PTRACE_SINGLESTEP than PTRACE_CONT.
> I think this adds up to all expected behavior. The limitations
> explained in the comments are probably never going to change.
> (At least, it's not worth addressing them for a synthetic test
> case before any real-world need arises.)
I find the wrong passing of ptrace-"forced" TF into pushf may be easily
workarounded in the current kernel ptrace code but sure nobody uses it now.
> It's especially mind-bending because this is testing everything
> at once. I think there are permutations it's not testing that
> were in fact wrong as well and should be fixed by my patch. I
> would be more confident of the fixes if we were to test some of
> the cases with few variables as well as the ones we test now.
> e.g. I think that even if no pushf/popf were involved recently,
> if we just single-stepped before single-step into popf, we'll
> fail to clear the "forced"; also, if we just single-stepped we
> will always fail to suppress block-step. I've been thinking
> about it too long now and can't quite keep it all straight.
Some of the cases could be tested with a more simple code but I did not expect
so before I saw the fix. Any specific testcase code suggestions are welcome.
I was thinking about splitting the `step-jump-cont-strict.c' code but it is
a single set of tested code (INSTR%d and SETTEST%d instruction blocks) cases
which make sense to run in the three modes
PTRACE_SINGLESTEP/PTRACE_CONT/PTRACE_SINGLEBLOCK so the separation would mean
a lot of code duplicity requiring starting to use include files.
Regards,
Jan
