This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Program Breakpoints


target_stopped_by_trap_instruction

Thanks for your feedback. Detailed reply in-line below.

Pedro Alves wrote:

Okay, first try. Sorry for the delay...

On Tuesday 19 May 2009 18:28:35, Ross Morley wrote:


+/* Returns non-zero if we were stopped by a trap or break instruction,
+   whether or not it is a software break planted by GDB.  If size != 0,
+   sets *size to that of the instruction or 0 if it need not be skipped.  */
+
+#ifndef STOPPED_BY_TRAP_INSTRUCTION
+#define STOPPED_BY_TRAP_INSTRUCTION(size) \
+  (*current_target.to_stopped_by_trap_instruction) (size)
+#endif
#define target_get_ada_task_ptid(lwp, tid) \
     (*current_target.to_get_ada_task_ptid) (lwp,tid)




Please rename this to target_stopped_by_trap_instruction, and
make it unconditionally defined. We don't support overriding
of these target macros anymore.


OK.



+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
+ {
+ struct thread_info *tp = inferior_thread ();
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+ && count > 0)



Sorry, `read_pc' is now gone from the sources. Replace that by


regcache_read_pc (get_current_regcache ()), or get_frame_pc if there's a
frame handy.


OK.

+ /* "stepi" off program breakpoint: the first step is to just increment
+ the PC past the break, then there are count-1 steps to go.
+ Note proceed() won't be called the first time, and on subsequent
+ steps the PC will already be off the break, so the entire handling
+ of "stepi" off a program breakpoint is done here. If stopping after + the break, display location information as for normal_stop. */
+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
+ {
+ struct thread_info *tp = inferior_thread ();
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+ && count > 0)
+ {
+ count--;
+ write_pc (read_pc () + tp->program_breakpoint_size);
+ if (count == 0)
+ {
+ reinit_frame_cache ();
+ if (ui_out_is_mi_like_p (uiout))
+ {
+ ui_out_field_string + (uiout, "reason", + async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE));
+ ui_out_field_int (uiout, "thread-id",
+ pid_to_thread_id (inferior_ptid));
+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
+ }
+ else + print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE);
+ }
+ }
+ }
+


This is broken for the !single_inst cases (step/next). step N is mishandled.


You are right, it can mess up with step/next where count is
lines and not insns. In case of step/next, I need to decrement
count only if the PC increment would change the line number.

Stepping over a program breakpoint is primarily handled in resume.
This fixup just handles a special case in which a stepi/nexti would
step one instruction farther than expected. It re-"executes" the
trap as a no-op when in fact the inferior has not run at all.
Without this code a stepi would execute the instruction following:

   1:   program breakpoint
   2:   insn2
   3:   insn3

Starting at 1, stepi/nexti (with no arg or 1) should result
in PC at 2, but without this fixup it ends up at 3. Of course,
this does not apply for decr_pc_after_break targets. If you
think ending up at 3 is OK, we can just omit this whole chunk.

It also doesn't take into account that there may be a
breakpoint at ORIG_PC + program_breakpoint_size.


I hadn't noticed this before, that GDB reports a breakpoint
hit when it stops with PC at a breakpoint location, even it
hasn't yet tried to execute at that location. So a breakpoint
immediately after the program breakpoint would need to be
reported when the program breakpoint is stepped over, to be
consistent (also when a program breakpoint is first hit on a
decr_pc_after_break target). There may be other cases.

When I first did program breakpoints I studied bpstat, and
it seemed much harder and less appropriate. bpstat is very
tied to the breakpoint table, but program breakpoints are
encountered (not known in advance) and are not in the table.
A bpstat has a bp_location which has an "owner" breakpoint in
the table. bpstat_stop_status scans the breakpoint table to
find possible reasons for the stop (it won't find a program
breakpoint). So I'm not sure it would be appropriate to use
bpstat, and it would likely amount to a much bigger change.

I also noticed that x86 native is not entirely consistent in
this case. It does not report a breakpoint if it hit while
stepping over another. Take the following example transcript:

(gdb) b main
Breakpoint 1 at 0x8048384: file breakop.c, line 14.
(gdb) r
Starting program: /opt/local/ross/test/breakop/breakop

Breakpoint 1, main (argc=0x1, argv=0xbff6a674) at breakop.c:14
14          printf("About to execute 'break'...\n");
(gdb) disass
Dump of assembler code for function main:
0x08048368 <main+0>:    push   %ebp
0x08048369 <main+1>:    mov    %esp,%ebp
0x0804836b <main+3>:    sub    $0x8,%esp
0x0804836e <main+6>:    and    $0xfffffff0,%esp
0x08048371 <main+9>:    mov    $0x0,%eax
0x08048376 <main+14>:   add    $0xf,%eax
0x08048379 <main+17>:   add    $0xf,%eax
0x0804837c <main+20>:   shr    $0x4,%eax
0x0804837f <main+23>:   shl    $0x4,%eax
0x08048382 <main+26>:   sub    %eax,%esp
0x08048384 <main+28>:   sub    $0xc,%esp
0x08048387 <main+31>:   push   $0x80484a0
0x0804838c <main+36>:   call   0x80482b0 <_init+56>
0x08048391 <main+41>:   add    $0x10,%esp
0x08048394 <main+44>:   mov    0x80495f0,%eax
0x08048399 <main+49>:   incl   0x80495f0
0x0804839f <main+55>:   sub    $0x4,%esp
0x080483a2 <main+58>:   push   %eax
0x080483a3 <main+59>:   pushl  0x80495f0
0x080483a9 <main+65>:   push   $0x80484c0
0x080483ae <main+70>:   call   0x80482b0 <_init+56>
0x080483b3 <main+75>:   add    $0x10,%esp
0x080483b6 <main+78>:   mov    $0x0,%eax
0x080483bb <main+83>:   leave
0x080483bc <main+84>:   ret
End of assembler dump.
(gdb) p/x $pc
$1 = 0x8048384
(gdb) b * 0x08048387
Breakpoint 2 at 0x8048387: file breakop.c, line 14.
(gdb) b * 0x0804838c
Breakpoint 3 at 0x804838c: file breakop.c, line 14.
(gdb) si
0x08048387      14          printf("About to execute 'break'...\n");

-------> Note the breakpoint was not reported.

(gdb) si
0x0804838c      14          printf("About to execute 'break'...\n");

-------> Nor was this one.

(gdb) si
0x080482b0 in ?? ()
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /opt/local/ross/test/breakop/breakop

Breakpoint 1, main (argc=0x1, argv=0xbffd1164) at breakop.c:14
14          printf("About to execute 'break'...\n");
(gdb) dis 2
(gdb) si
0x08048387      14          printf("About to execute 'break'...\n");
(gdb) si

-------> With the preceding BP diabled, this one is reported.

Breakpoint 3, 0x0804838c in main (argc=0x1, argv=0xbffd1164) at breakop.c:14
14          printf("About to execute 'break'...\n");
(gdb)

So I don't have an ideal way to handle these corner cases.
I lean toward a philosophy that a breakpoint is not hit until
there is an attempt to execute at that location, so I'm not
sure it really makes sense for GDB to report a breakpoint when
it stops (for some other reason) just before it. Perhaps in
this case it doesn't matter because on resume or continued
stepping you will certainly hit the subsequent breakpoint and
GDB should report it then. Keep in mind the goal here is to
make sure GDB stops for a program breakpoint and (secondarily)
can resume afterward. If a GDB breakpoint immediately following
is not reported until resuming, that's not so bad.

What if a "program breakpoint" is reported for a GDB breakpoint? You shouldn't
just move the PC in that case. I don't see where that is handled.


I gather you are talking about the coincident case (GDB breakpoint
at the same location as a program breakpoint). They cannot both hit
at the same time. One could think of a GDB breakpoint conceptually
lying just before an instruction, to be hit before it is executed.
A program breakpoint is reported only when a trap is executed and
there is no GDB s/w breakpoint inserted at that location. A h/w
breakpoint would be hit before the trap is actually executed.

Here is my understanding of what happens for a s/w breakpoint
coincident with a program breakpoint. GDB first hits the s/w
breakpoint by executing the inserted trap. It stops and reports
a GDB breakpoint (and if decr_pc_after_break, backs up the PC).
On resume it replaces the program breakpoint trap and hits it
while stepping over the GDB breakpoint, and reports a program
breakpoint. On resume from that it increments the PC over the
program breakpoint (if size != 0), reinserts the GDB breakpoint
and continues as usual.

Here is my understanding of what happens for a h/w breakpoint
coincident with a program breakpoint. GDB first hits the h/w
breakpoint by triggering on the fetch PC. It stops and reports
a GDB breakpoint. On resume it disables the h/w breakpoint to
step over it, executes the program breakpoint, and reports it.
On resume from there it increments the PC over the program
breakpoint (if size != 0), enables the h/w breakpoint and
continues as normal.

Is that a reasonable assessment of the coincident case?

See comment
on next hunk. I suspect this short circuit should be done elsewhere, somewhere
where it will be possible to pass through handle_inferior_event after
moving the PC forward.


It seems it might be possible to arrange for step_1 or
step_once to call normal_stop directly. I'm not deeply
familiar with the infrun control flow however, and I don't
have the time to study it that much. I wonder if this is
something that could be improved later by an expert, but
in the meantime my less than perfect implementation would
improve current behavior and not lock us into implementation
details. The only things we'd be stuck with would be the
interface function to_stopped_by_trap_instruction(size)
and the remote protocol stop-reply extension "TNNtrap:size".
Improving the way infrun handles things would be possible
without changing how hitting a trap is reported to infrun.
The critical thing is to make sure no regression tests are
broken by this, espeically for targets that don't yet support
reporting the trap (and I did run the testsuite with no
regressions on x86/linux native).

Please read on for comments on your other suggestions.

Shouldn't you do something in prepare_to_proceed as well?

Use case is:

  <thread 1 hit program breakpoint>
  (gdb) thread 2
  (gdb) continue

 GDB is expected to make progress here, instead of reporting
 the same program breakpoint again.

Thread support was added since I originally did program breakpoints and I am unfamiliar with it (it's not supported on our target). In merging with cvs head, I tried to guess what would be needed, and I moved the program_breakpoint_{hit,size} variables into the thread structure (previously they were global like some other things that moved to the thread structure).

Now I wonder if I should have left it as it was. The stop state seems more associated with the inferior than with the GDB thread
(which is only which thread GDB is looking at). Would it be correct
then to just put it back how it was, with those flags global? Both?


Otherwise, I will have to add a test in prepare_to_proceed to switch back to the original thread like for a GDB breakpoint,
ie.
if (breakpoint_here_p (regcache_read_pc (regcache))
|| (tp->program_breakpoint_hit
&& tp->program_breakpoint_size != 0
&& regcache_read_pc (regcache) == stop_pc))
{
:
}


I'd have to compute tp and stop_pc because they're not passed
into prepare_to_proceed.

Once we get to resume, stepping over a program breakpoint is done
at the same point skipping a permanent breakpoint is done. It's the
same problem except skip_permanent_breakpoint has implicit knowledge
of the trap size. I've previously suggested making a single function
that could be used for both cases, with a size parameter. But for
now I did not want to interfere with permanent breakpoints, so kept
it simple thinking could be a future enhancement after program
breakpoints are accepted, if anyone cares.


+ /* Handle case of hitting a program breakpoint (break instruction
+ in target program, not planted by or known to GDB). */
+ + if (ecs->event_thread->program_breakpoint_hit)
+ {
+ stop_print_frame = 1;
+ + /* We are about to nuke the step_resume_breakpoint and
+ through_sigtramp_breakpoint via the cleanup chain, so
+ no need to worry about it here. */
+ + stop_stepping (ecs);
+ return;
+ }
+



Above was copied from other cases in infrun where we need to stop.
I don't mind if that comment gets nuked.


Hmmm, confused: can't the target report a program breakpoint hit
for a PC where there's a gdb breakpoint inserted as well?

Not if it's inserted - the program breakpoint trap won't be seen. GDB will see the trap instruction it inserted (which
may or may not be the same instruction). If it's a h/w
breakpoint, GDB will stop before it executes the trap.


Wouldn't
it better to have program breakpoints checked from inside
stop_bpstat/bpstop_status/bpstat_explains_signal/bpstat_print?
Then you wouldn't need to add
those if (program_breakpoint) do_this; else handle bpstat;

I thought about this, but it seemed bpstat is not well suited to this. See earlier discussion.

+static int
+linux_stopped_by_trap_instruction (int *size)
+{
+ CORE_ADDR stop_pc = get_stop_pc();
+ int trap_size = 0;
+
+ if (the_low_target.trap_size_at != NULL)
+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
+ else if (the_low_target.breakpoint_at != NULL
+ && (*the_low_target.breakpoint_at) (stop_pc))
+ trap_size = the_low_target.breakpoint_len;
+
+ if (trap_size != 0)
+ {
+ *size = (the_low_target.get_pc != NULL + && (*the_low_target.get_pc) () == stop_pc)
+ ? trap_size : 0;


I think I got this bit, but it would be nice to have a
comment here.

OK. Something like: /* if we hit a trap insn and the PC still points to it, report the size of the trap so GDB can step over it. */ ?

+      return 1;
+    }
+  else
+    return 0;
+}


Before we stick with this forever, it would be very important to have tests covering corner cases like:

single-stepping through consecutive
     "program breakpoint"->gdb breakpoint

single-stepping through consecutive
     "program breakpoint"->"program breakpoint"

single- stepping through consecutive
      gdb breakpoint->"program breakpoint"

(step, stepi, stepi n)


I agree we need test cases. I'll develop some before
submitting a revised patch (which I hope to do only
once after we hash out the basic issues).

    insn1
    "program breakpoint"
foo: gdb breakpoint
    insn2
    insn3
    jmp foo

"step" over this:

function ()

line 1:
    insn1
    "program breakpoint"
    insn2
    insn3
line 2:
    ...

... and have "program breakpoint" be reported, instead
of end-stepping-range at line 2.

From the looks of it, you don't have to do much, if anything
else at all, to have these cases covered on x86; I'd really
like to make sure this is sane on decr_pc_after_break archs, as
this is the way to fix the "step over range with trap
doesn't stop at trap" "bug".

I think that's the same bug I'm trying to fix, but it
manifests differently for a non decr_pc_after_break
arch. It keeps executing the trap, never advancing. I guess on x86 it just keeps going.


I think decr_pc_after_break archs will just fall out
except perhaps for a few corner cases. But not being
familiar with the x86 I'd have to learn now to implement
target_ops.to_stopped_by_trap_instruction for x86 native.
It would be better if someone else could implement that,
and I could test it.

Certainly I will run the test suite on native linux/x86
to make sure nothing breaks without that function.

Each target/arch can implement it if/when someone cares.
GDB behavior should not change until that is implemented
for any given target. Meanwhile the program breakpoint
test cases would be expected failures.

If I read the code correctly, on x86, if you continue
this:

0001     insn1
0002     "program breakpoint"

... you'll report a "program breakpoint" hit at 0003,

I'll report it hit, and the PC will be 0003. It was actually hit at 0002, but on this arch the PC is incremented.

with
program breakpoint size 0.

Yes.


If you instead stepi through that same
sequence, you'll get a "program breakpoint" hit at 0002, with
program breakpoint size 1.

That's up to target_ops.to_stopped_by_trap_instruction. I gather on x86, when stepping the PC is not incremented after a trap, and the trap size is 1?


That sounds good, but, if there's a
GDB breakpoint installed at "0002", gdb shouldn't be moving the PC
forward magically, without executing the instruction that
was under the breakpoint originally.

The instruction under the breakpoint is the program breakpoint trap insn. The program breakpoint won't be executed while the GDB breakpoint is inserted.

So GDB will first hit the GDB breakpoint. If it's a s/w
breakpoint it will be removed and the program breakpoint
replaced (the latter has not yet been executed/hit).
If it's a h/w breakpoint, I'm guessing the target stopped
before the program breakpoint was executed (else the target
would have undo the effects of any insn at a h/w breakpoint).

On continuing from there, GDB will remove the GDB breakpoint
and single-step. Target will execute the program breakpoint,
stop at 0002 (since it single-stepped), and report being
stopped by a trap with size 1. Since the GDB breakpoint was
not inserted, GDB will interpret it as a program breakpoint.

I'm assuming now that GDB forgets it was trying to step over
a GDB breakpoint, and reinserts it as usual on continue. Now
it knows it hit a program breakpoint and the PC has not changed.
so it will increment the PC by the size (1) to 0003 and continue.

Suppose a decr_pc_after_break arch also increments the pc after single-stepping a trap insn. In that case the program
breakpoint will be reported with size 0 when pc is at 0003,
and no step-over will be done.


So, if I get this correctly, adjust_pc_after_break should
never try to roll back if program size > 0, even on
decr_pc_after_break archs, but we don't need to check this,
because this will only happen if single-stepping, and it
already does nothing in that case.  Right?

adjust_pc_after_break does exactly the right thing already.
It should only roll back the PC if decr_pc_after_break and
software_breakpoint_inserted_here_p (breakpoint_pc) . The
only reason to roll back the PC is to execute the instruction
under a GDB s/w breakpoint.

In summary, if you agree, I need to make the following changes:
- Rename STOPPED_BY_TRAP_INSTRUCTION to target_stopped_by_trap_instruction (unconditionally defined).
- Replace read_pc () with regcache_read_pc (get_current_regcache ()).
- Move program_breakpoint_{hit,size} out of the thread structure
back to global variables?
- In step_1, !single_inst cases, decrement count only if
the line number would change.
- Write generic test cases, passing in the trap insn at compile.
- Test on Xtensa.
- Test on x86/native (program breakpoint tests would XFAIL but
all others that passed before would still pass).
- Add the cross-ref to the doc that Eli requested.
- Re-base and resubmit patch.


Thanks,
Ross



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]