From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22861 invoked by alias); 4 Oct 2003 06:44:04 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 22854 invoked from network); 4 Oct 2003 06:44:02 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 4 Oct 2003 06:44:02 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id h946i2123433 for ; Sat, 4 Oct 2003 02:44:02 -0400 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id h946i1c17600; Sat, 4 Oct 2003 02:44:01 -0400 Received: from localhost.localdomain (vpn50-46.rdu.redhat.com [172.16.50.46]) by pobox.corp.redhat.com (8.12.8/8.12.8) with ESMTP id h946hxbe012811; Sat, 4 Oct 2003 02:44:00 -0400 Received: (from kev@localhost) by localhost.localdomain (8.11.6/8.11.6) id h946hrg25457; Fri, 3 Oct 2003 23:43:53 -0700 Date: Sat, 04 Oct 2003 06:44:00 -0000 From: Kevin Buettner Message-Id: <1031004064353.ZM25456@localhost.localdomain> To: msnyder@redhat.com, gdb-patches@sources.redhat.com Subject: [RFA,RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 3 of 4 Cc: Eli Zaretskii MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-10/txt/msg00073.txt.bz2 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 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 : subicc.p gr8,0,gr0,icc0 0x10414 : add gr9,gr10,gr9 0x10418 : ckeq icc0,cc4 0x1041c : cadd gr11,gr12,gr9,cc4,0x1 0x10420 : ori.p gr9,0,gr8 0x10424 : 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 and ends at 0x1041c . 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;