This is the mail archive of the gdb-patches@sources.redhat.com 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]

[RFA,RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 3 of 4


Below -- *way* below -- is patch 3, the breakpoint.[hc] related
patches for my current set of TARGET_ADJUST_BREAKPOINT address patch
submissions.

To review the criticism of the previous TARGET_ADJUST_BREAKPOINT
patch (from three years ago!), see:

    http://sources.redhat.com/ml/gdb-patches/2000-06/msg00260.html
    http://sources.redhat.com/ml/gdb-patches/2000-06/msg00270.html
    http://sources.redhat.com/ml/gdb-patches/2000-06/msg00274.html

I'll eventually quote bits and pieces of that old mail, but before
doing so I'll start by (re)describing the problem and then showing an
example of the behavior obtained by this current patch set.  After
that I will discuss how this compares with the proposals put forward
when I last submitted the patch.

The FR-V is a VLIW architecture in which a number of RISC-like
instructions are grouped together in one macro instruction.  The
Fujitsu documents describing the FR-V refer to these macro
instructions as VLIW instructions, but I find such terminology
confusing.  I'll refer to the group of instructions which comprise a
"VLIW instruction" as a "bundle" and continue referring to the
subinstructions comprising the bundle as "instructions".  (My
vocabulary on this matter is colored by my previous IA-64 experience,
however, I think such terminology will be less confusing even to those
who do not have previous VLIW experience.)

When the processor executes a bundle, it executes each of its
instructions in parallel.  This point is *very* important and
must not be forgotten.

The FR-V architecture has various constraints on the types of
instructions that may be grouped together in a bundle.  The
constraint that's relevant to this discussion is that a break
instruction *must* be the first instruction in a bundle.  But
recall that all instructions in the bundle are executed in parallel,
so the term "first" simply means that instruction which resides
at the lowest address and has no meaning with regard to which
instruction is executed first -- all of them are!  (Or none of
them are, depending upon how you look at it.)

In the course of optimization, the compiler may group instructions
from distinct source statements into the same bundle.  The line number
information associated with one of the latter statements will likely
refer to some instruction other than the first one in the bundle.  So,
if the user attempts to place a breakpoint on one of these latter
statements, GDB must be careful to NOT place the break instruction on
any instruction other than the first one in the bundle.  This is the
motivation behind this patch set.

It is tempting to try to think of alternate solutions.  One obvious
(but flawed) solution that most people think of when they first hear
about this problem is that GDB should attempt to split a bundle up
into 2 or more pieces that are executed sequentially, so that the
architectural constraints are not violated.  Unfortunately, this is
not possible in general without risk of changing the meaning of the
bundle.  The example that Michael Meissner provided me with some time
ago is as follows:

	/* gr4 == 4, gr5 == 5, gr6 == 6, gr7 == 7 */
	add.p gr4, gr5, gr6
	add gr6, gr7, gr8

Note the .p suffix on the "first" instruction.  It indicates that it
and the following instruction are bundled together.  (Instructions
ending with .p are bundled with the next until one not containing .p
is found.) These instructions are executed in parallel, so after
executing this bundle, gr8 will contain 13.  If these instructions
were split up (unbundled) and executed sequentially, gr8 would contain
16 (4+5+7).  Since these two results are different, it is now obvious
that splitting the bundle apart and executing its component
instructions sequentially would result in incorrect behavior.  (It's a
shame though because implementing it would have been as simple as
toggling a packing bit.)

Another vastly more complicated solution would be to stop on the
beginning of the bundle, save the register state somewhere, execute
instructions sequentially up to the desired one, being careful to
restore the inital state prior to the sequential execution of each
instruction in the bundle whilst also maintaining a "merged" state of
the register file after executing each instruction.  This merged state
would become the state of the register file after execution of the
entire bundle.  This scheme may indeed be implemented some day (though
hopefully by someone other than myself), but to do so would still
require the use of the much simpler mechanism presented in this patch
set.

Now let's consider an actual example and see how GDB behaves.  Below
is the program.  We'll be placing a breakpoint on line 18 and will
find that GDB adjusts the breakpoint to instead be on line 17.  (Though
this is not immediately obvious from the warning.)

    1	#include <stdio.h>
    2
    3	int csum (int cond, int a, int b, int c, int d);
    4
    5   int
    6   main (int argc, char **argv)
    7   {
    8     printf ("%d %d\n", csum (1, 3, 4, 5, 6), csum (0, 3, 4, 5, 6));
    9     return 0;
    10  }
    11
    12  int
    13  csum (int cond, int a, int b, int c, int d)
    14  {
    15    int total;
    16
    17    if (cond)
    18      total = a + b;
    19    else
    20      total = c + d;
    21    
    22    return total;
    23  }

Now for the sample GDB session...  First we'll set a couple of
breakpoints:

    (gdb) b main
    Breakpoint 1 at 0x103ac: file tst.c, line 8.
    (gdb) b 18
    warning: Breakpoint address adjusted from 0x00010414 to 0x00010410.
    Breakpoint 2 at 0x10410: file tst.c, line 18.

Note the warning on breakpoint 2 indicating that the breakpoint has
been adjusted.  Running the program to main provides no surprises:

    (gdb) run
    Starting program: /ocotillo2/devo-frv/frv-elf/bld/gdb/tst/tst 

    Breakpoint 1, main (argc=0, argv=0x5c210) at tst.c:8
    8         printf ("%d %d\n", csum (1, 3, 4, 5, 6), csum (0, 3, 4, 5, 6));

But running to the breakpoint that we had attempted to place on line 18
will show the following:

    (gdb) c
    Continuing.
    warning: Breakpoint 2 address previously adjusted from 0x00010414
    to 0x00010410.

    Breakpoint 2, csum (cond=1, a=3, b=4, c=5, d=6) at tst.c:17
    17        if (cond)

Note that we stopped not at line 18 as requested, but line 17 instead!

Let's disassemble csum to see what it looks like:

    (gdb) x/6i csum
    0x10410 <csum>: subicc.p gr8,0,gr0,icc0
    0x10414 <csum+4>:       add gr9,gr10,gr9
    0x10418 <csum+8>:       ckeq icc0,cc4
    0x1041c <csum+12>:      cadd gr11,gr12,gr9,cc4,0x1
    0x10420 <csum+16>:      ori.p gr9,0,gr8
    0x10424 <csum+20>:      bralr

Note that the '.p' suffix on subicc.  This means that 0x10410 and 10414
have been bundled together.

Here is the location of line 18:

    (gdb) info line 18
    Line 18 of "tst.c" starts at address 0x10414 <csum+4>
       and ends at 0x1041c <csum+12>.

And the program counter...

    (gdb) p/x $pc
    $1 = 0x10410

So we've stopped on the very first instruction of csum(), but line 18
starts on the second instruction which happens to be in the same
instruction bundle as the first line.  Since this is rather surprising
-- especially if the user didn't expect to break until that particular
condition had been met -- I feel that it definitely merits a warning.

In his criticism of my earlier TARGET_ADJUST_BREAKPOINT_ADDRESS
patch, Eli Zaretskii said:

    Printing a message is a good idea, but making it a warning IMHO is
    not.  Warning is for some potential trouble, while in this case GDB is
    doing The Right Thing.  Printing a warning will confuse users.

    Perhaps we should make the information about moving the breakpoint
    part of the "Breakpoint 1 at ..." message.

In later email, Eli proposed:

    [...] It's also possible that the
    "adjusted" part should be moved to a separate line, like so:

      Breakpoint 1 at 0x0620008c: file bar.c, line 21
      (breakpoint address adjusted from 0x6200090).

For the record, I *was* able to achieve this result.  (I can post a
patch if anyone's interested.)  There were two things I didn't like
about it.

    1) Causing this additional information to be output at the
       location suggested by Eli was immensely difficult.  When Eli
       proposed this, I had expected to be able to localize the
       changes to breakpoint.c, but unfortunately, the control
       for printing the "Breakpoint NUM,..." message is split across
       at least three files.  (breakpoint.c, infrun.c, and stack.c
       IIRC.) I was able to come up with a solution which only touched
       infrun.c and breakpoint.[hc], but this solution would have
       affected the way that ALL other ports print breakpoints.  I
       believe that my attempted solution would generate equivalent
       output, but I doubt I'd ever be able to prove it.

    2) The above difficulties aside, while playing around with some
       test cases, I became convinced that the adjustment of
       breakpoints really did represent potential trouble, making it
       preferable to print warnings.  Moreover, I became convinced
       that it was desirable to print a warning at both breakpoint
       creation time and when the breakpoint is hit.  I'll discuss
       this in greater detail below.

For these reasons, I abandoned Eli's proposal of adding additional
information after the message that's printed when a breakpoint is
hit.

Returning to the matter of warnings vs. informational messages,
consider the following scenarios:

    - The user attempts to place a breakpoint in a "then" block
      of an if-then-else statement, expecting to only see the
      breakpoint hit when that particular "if" condition is
      met.  If the breakpoint is moved so that it will *always* be
      hit (as illustrated in my real life example above), that's
      cause for concern.  Certainly, this merits a warning.

      [ BTW, this is subtly different from an optimizer performing
      code motion which moves some invariant out of a block.  In
      such a case, the line number associated with that particular
      instruction is preserved.  Such is not the case when GDB
      adjusts a breakpoint's address.  I will admit that the net
      effect, i.e, not hitting the instruction at the expected
      location is the same, but the code motion behavior is more
      comprehensible due to the fact that the line number has
      been preserved. ]

      The warning will allow the user to (attempt to) set a different
      breakpoint which won't be adjusted out of the block.  Another
      (less efficient) way around this problem might be to reset the
      breakpoint as a conditional breakpoint.  In either case, the use
      of a warning presents the user with an early opportunity to take
      remedial action.

    - In the course of stepping through a program, GDB creates
      internal breakpoints to assist in the stepping process.  At
      present, for FR-V, there are situations where this adjustment
      causes GDB to go into an infinite loop when attempting to step. 
      Clearly I have some more work to do elsewhere in GDB, but the
      point is that adjusting one of these internal breakpoints alters
      the expections of what might happen for both GDB and the user. 
      GDB's use of these internal breakpoints is usually invisible to
      the user.  I argue that the user must be made aware of the fact
      that the thing that he expects to happen might not happen when
      one of these internal breakpoints has been adjusted.  E.g, I
      believe it is possible for an adjustment to occur which would
      cause an internal breakpoint to *never* be hit that would
      normally be hit on more conventional architectures.  Certainly
      this sort of (possible) deviation from normal behavior merits a
      warning.

    - For user settable breakpoints, the user may not immediately
      realize the ramifications associated with the adjustment of a
      breakpoint.  So, in addition to warning the user as early as
      possible about possible problems, I also think it's also wise
      to remind the user again when the breakpoint is actually hit,
      because the behavior, due to the movement of the breakpoint,
      may not match the behavior that the user expects.

                             .......

This portion of the patch (below) is very similar to my earlier
attempt.  The only real change is the addition of the warning that'll
be printed when an adjusted breakpoint has been hit.  I seem to recall
Michael Snyder giving me private approval for my earlier attempt, but
given that three years have passed since my earlier patch, I think
it's prudent for me to allow both Michael the chance to look this over
again and give his thumbs up or thumbs down.

I would also like to hear from Eli.  If it's not clear from my words
above, I'll state again that I feel very strongly that a warning is
more appropriate than a mere informational message which might be
missed by the user.  The adjustment of a breakpoint *should* cause the
user to be alarmed since the expected breakpoint behavior could be
significantly altered.

So...

Michael S: Okay?

Eli: Comments?

Kevin

	* breakpoint.h (struct breakpoint): Add new member
	``requested_address''.
	* breakpoint.c (breakpoint_adjustment_warning)
	(adjust_breakpoint_address): New static functions.
	(print_it_typical): Issue warning if breakpoint's address is different
	from its requested address.
	(set_raw_breakpoint, set_longjmp_resume_breakpoint, watch_command_1)
	(breakpoint_re_set_one):  Set breakpoint's
	``requested_address'' field.  Set ``address'' field to the
	result of calling adjust_breakpoint_address() on the requested
	address.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.130
diff -u -p -r1.130 breakpoint.c
--- breakpoint.c	21 Sep 2003 01:26:44 -0000	1.130
+++ breakpoint.c	3 Oct 2003 23:38:25 -0000
@@ -97,6 +97,10 @@ struct breakpoint *set_raw_breakpoint (s
 
 static void check_duplicates (struct breakpoint *);
 
+static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
+
+static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr);
+
 static void describe_other_breakpoints (CORE_ADDR, asection *);
 
 static void breakpoints_info (char *, int);
@@ -2023,6 +2027,10 @@ print_it_typical (bpstat bs)
     {
     case bp_breakpoint:
     case bp_hardware_breakpoint:
+      if (bs->breakpoint_at->address != bs->breakpoint_at->requested_address)
+	breakpoint_adjustment_warning (bs->breakpoint_at->requested_address,
+	                               bs->breakpoint_at->address,
+				       bs->breakpoint_at->number, 1);
       annotate_breakpoint (bs->breakpoint_at->number);
       ui_out_text (uiout, "\nBreakpoint ");
       if (ui_out_is_mi_like_p (uiout))
@@ -3840,6 +3848,38 @@ check_duplicates (struct breakpoint *bpt
     }
 }
 
+static void
+breakpoint_adjustment_warning (CORE_ADDR from_addr, CORE_ADDR to_addr,
+                               int bnum, int have_bnum)
+{
+  char astr1[40];
+  char astr2[40];
+
+  strcpy (astr1, local_hex_string_custom ((unsigned long) from_addr, "08l"));
+  strcpy (astr2, local_hex_string_custom ((unsigned long) to_addr, "08l"));
+  if (have_bnum)
+    warning ("Breakpoint %d address previously adjusted from %s to %s.",
+             bnum, astr1, astr2);
+  else
+    warning ("Breakpoint address adjusted from %s to %s.", astr1, astr2);
+}
+
+/* Call TARGET_ADJUST_BREAKPOINT_ADDRESS and print warning if an address
+   adjustment was made.  Returns the adjusted address. */
+
+static CORE_ADDR
+adjust_breakpoint_address (CORE_ADDR bpaddr)
+{
+  CORE_ADDR adjusted_bpaddr;
+
+  adjusted_bpaddr = TARGET_ADJUST_BREAKPOINT_ADDRESS (bpaddr);
+
+  if (adjusted_bpaddr != bpaddr)
+    breakpoint_adjustment_warning (bpaddr, adjusted_bpaddr, 0, 0);
+
+  return adjusted_bpaddr;
+}
+
 /* set_raw_breakpoint() is a low level routine for allocating and
    partially initializing a breakpoint of type BPTYPE.  The newly
    created breakpoint's address, section, source file name, and line
@@ -3862,7 +3902,8 @@ set_raw_breakpoint (struct symtab_and_li
 
   b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
   memset (b, 0, sizeof (*b));
-  b->address = sal.pc;
+  b->requested_address = sal.pc;
+  b->address = adjust_breakpoint_address (b->requested_address);
   if (sal.symtab == NULL)
     b->source_file = NULL;
   else
@@ -4353,7 +4394,8 @@ set_longjmp_resume_breakpoint (CORE_ADDR
   ALL_BREAKPOINTS (b)
     if (b->type == bp_longjmp_resume)
     {
-      b->address = pc;
+      b->requested_address = pc;
+      b->address = adjust_breakpoint_address (b->requested_address);
       b->enable_state = bp_enabled;
       b->frame_id = frame_id;
       check_duplicates (b);
@@ -5468,7 +5510,9 @@ watch_command_1 (char *arg, int accessfl
 	  scope_breakpoint->frame_id = get_frame_id (prev_frame);
 
 	  /* Set the address at which we will stop.  */
-	  scope_breakpoint->address = get_frame_pc (prev_frame);
+	  scope_breakpoint->requested_address = get_frame_pc (prev_frame);
+	  scope_breakpoint->address =
+	    adjust_breakpoint_address (scope_breakpoint->requested_address);
 
 	  /* The scope breakpoint is related to the watchpoint.  We
 	     will need to act on them together.  */
@@ -6832,7 +6876,8 @@ breakpoint_re_set_one (void *bint)
 		  savestring (sals.sals[i].symtab->filename,
 			      strlen (sals.sals[i].symtab->filename));
 	      b->line_number = sals.sals[i].line;
-	      b->address = sals.sals[i].pc;
+	      b->requested_address = sals.sals[i].pc;
+	      b->address = adjust_breakpoint_address (b->requested_address);
 
 	      /* Used to check for duplicates here, but that can
 	         cause trouble, as it doesn't check for disabled
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.21
diff -u -p -r1.21 breakpoint.h
--- breakpoint.h	27 Apr 2003 01:11:10 -0000	1.21
+++ breakpoint.h	3 Oct 2003 23:38:25 -0000
@@ -228,6 +228,14 @@ struct breakpoint
        field.  */
     CORE_ADDR address;
 
+    /* Address at which breakpoint was requested, either by the user or
+       by GDB for internal breakpoints.  This will usually be the same
+       as ``address'' (above) except for cases in which
+       TARGET_ADJUST_BREAKPOINT_ADDRESS has computed a different
+       address at which to place the breakpoint in order to comply
+       with a processor's architectual constraints.  */
+    CORE_ADDR requested_address;
+
     /* Line number of this address.  */
 
     int line_number;


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