From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16216 invoked by alias); 18 Dec 2009 11:28:20 -0000 Received: (qmail 16206 invoked by uid 22791); 18 Dec 2009 11:28:19 -0000 X-SWARE-Spam-Status: No, hits=-0.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_JMF_BL,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 18 Dec 2009 11:28:13 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0KUU00C00I9LL900@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Fri, 18 Dec 2009 13:27:57 +0200 (IST) Received: from HOME-C4E4A596F7 ([87.70.160.137]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0KUU00C3TIIKRD00@a-mtaout20.012.net.il>; Fri, 18 Dec 2009 13:27:57 +0200 (IST) Date: Fri, 18 Dec 2009 11:28:00 -0000 From: Eli Zaretskii Subject: Re: [PATCH 1/2] Support the new PPC476 processor -- Arch Independent In-reply-to: <200912161847.19433.sergiodj@linux.vnet.ibm.com> To: =?iso-8859-1?q?S=E9rgio_Durigan_J=FAnior?= Cc: gdb-patches@sourceware.org, bauerman@br.ibm.com, luisgpm@linux.vnet.ibm.com, tyrlik@us.ibm.com Reply-to: Eli Zaretskii Message-id: <83vdg45xkx.fsf@gnu.org> MIME-version: 1.0 Content-type: text/plain; charset=iso-8859-1 Content-transfer-encoding: 8BIT References: <200912161847.19433.sergiodj@linux.vnet.ibm.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-12/txt/msg00248.txt.bz2 > From: Sérgio_Durigan_Júnior > Date: Wed, 16 Dec 2009 18:47:19 -0200 > Cc: Thiago Jung Bauermann , Luis Machado , Matt Tyrlik > > This is the patch that modifies the architecture-independent files. It is > responsible for the implementation of the new commands (such as hbreak-range > and watch-range), and the logic that handles the new types of hardware > breakpoints/watchpoints. Thanks. > + /* We've got to save some special fields before updating > + this watchpoint. */ > + switch (b->hw_point_flag) > + { > + case HW_POINT_MASKED_WATCH: > + mask = b->loc->hw_wp_mask; > + break; > + case HW_POINT_RANGED_WATCH: > + range = b->loc->ranged_hw_bp_length; > + break; What's the need for saving and later restoring these fields? > + enum enable_state e; > + > + /* We have to temporary disable this watchpoint, otherwise > + we will count it twice (once as being inserted, and once > + as a watchpoint that we want to insert). */ > + e = b->enable_state; > + b->enable_state = bp_disabled; What's the story behind the need to temporarily disabling this watchpoint? > + if (b->type == bp_hardware_watchpoint > + && TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_COND_HW_ACCEL) > + && TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P (b->loc)) > + { > + TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR (b->loc, > + &b->loc->cond_hw_addr); > + b->hw_point_flag = HW_POINT_COND_HW_ACCEL; > + } > + else > + b->hw_point_flag = HW_POINT_NONE; Instead of having all this target-dependent logic here, can't we have it inside target-specific code? > - val = target_insert_watchpoint (bpt->address, > - bpt->length, > - bpt->watchpoint_type); > + if (bpt->owner->hw_point_flag == HW_POINT_MASKED_WATCH) > + val = target_insert_mask_watchpoint (bpt->address, > + bpt->length, > + bpt->watchpoint_type, > + bpt->hw_wp_mask); > + else if (bpt->owner->hw_point_flag == HW_POINT_COND_HW_ACCEL) > + val = target_insert_cond_accel_watchpoint (bpt->address, > + bpt->length, > + bpt->watchpoint_type, > + bpt->cond_hw_addr); > + else > + val = target_insert_watchpoint (bpt->address, > + bpt->length, > + bpt->watchpoint_type); Again, why cannot this be on the target side? > - && breakpoint_address_match (bpt->pspace->aspace, bpt->address, > - aspace, pc)) > + && ((breakpoint_address_match (bpt->pspace->aspace, bpt->address, > + aspace, pc) && ((bpt->address == pc))) > + || (bpt->owner->hw_point_flag == HW_POINT_RANGED_BREAK > + && breakpoint_address_match_range (bpt->pspace->aspace, > + bpt->address, > + bpt->ranged_hw_bp_length, > + aspace, pc) > + && pc >= bpt->address > + && pc < (bpt->address + bpt->ranged_hw_bp_length)))) Wouldn't it be better to just extend the current breakpoint_address_match function, so that it supports ranges as well? > + if (b->hw_point_flag == HW_POINT_MASKED_WATCH) > + ui_out_text (uiout, "\nCheck the underlying instruction \ > +at PC for address and value related to this watchpoint trigger.\n"); Sorry, I don't understand what does this message mean. Can you explain? > -/* Check watchpoint condition. */ > +/* Check watchpoint condition. We can't use value_equal because it coerces > + an array to a pointer, thus comparing just the address of the array instead > + of its contents. This is not what we want. */ > + > +static int > +value_equal_watchpoint (struct value *arg1, struct value *arg2) > +{ > + struct type *type1, *type2; > + > + type1 = check_typedef (value_type (arg1)); > + type2 = check_typedef (value_type (arg2)); > + > + return TYPE_CODE (type1) == TYPE_CODE (type2) > + && TYPE_LENGTH (type1) == TYPE_LENGTH (type2) > + && memcmp (value_contents (arg1), value_contents (arg2), > + TYPE_LENGTH (type1)) == 0; > +} > > static int > watchpoint_check (void *p) > @@ -3246,7 +3388,7 @@ watchpoint_check (void *p) > > fetch_watchpoint_value (b->exp, &new_val, NULL, NULL); > if ((b->val != NULL) != (new_val != NULL) > - || (b->val != NULL && !value_equal (b->val, new_val))) > + || (b->val != NULL && !value_equal_watchpoint (b->val, new_val))) Can you elaborate the need for this change? It seems to change the semantics of watchpoint_check, so I wonder why it is done. > + else if (b->hw_point_flag == HW_POINT_MASKED_WATCH) > + /* Since we don't the exact trigger address (from stopped_data_address) > + Just tell the user we've triggered a mask watchpoint. */ > + return WP_VALUE_CHANGED; The sentence in the comment is missing something ("know" after "Since we"?). > @@ -5771,7 +5959,12 @@ hw_breakpoint_used_count (void) > ALL_BREAKPOINTS (b) > { > if (b->type == bp_hardware_breakpoint && breakpoint_enabled (b)) > - i++; > + { > + i++; > + /* Special types of hardware breakpoints can use more than > + one slot. */ > + i += target_hw_point_extra_slot_count (b->hw_point_flag); > + } Wouldn't it be more elegant to have target_hw_point_slot_count instead that would return the number of used slots, instead of incrementing by one and then call target_hw_point_extra_slot_count to get the extra slots? > @@ -5789,10 +5982,15 @@ hw_watchpoint_used_count (enum bptype type, int *other_type_used) > if (breakpoint_enabled (b)) > { > if (b->type == type) > - i++; > - else if ((b->type == bp_hardware_watchpoint > - || b->type == bp_read_watchpoint > - || b->type == bp_access_watchpoint)) > + { > + i++; > + /* Special types of hardware watchpoints can use more > + than one slot. */ > + i += target_hw_point_extra_slot_count (b->hw_point_flag); Same here. > + /* Does the target support masked watchpoints? */ > + if (!TARGET_CAN_USE_SPECIAL_HW_POINT_P (HW_POINT_MASKED_WATCH)) > + error (_("This target does not support the usage of masks \ > +with hardware watchpoints.")); Can't the functionality be emulated by GDB's application code? > + len_addr = addr_p - tokp; > + > + strtol (tokp, &endp, 16); > + /* Check if the user provided a valid numeric value for the > + mask address. */ > + if (*endp != ' ' && *endp != '\t' && *endp != '\0') > + error (_("Invalid mask address specification `%s'."), > + tokp); > + > + if (len_addr <= 0) > + error (_("The mask address is invalid.")); How can the last error condition happen? What would the luser need to type to trigger that? > + mem_cnt = can_use_hardware_watchpoint (val); > + if (mem_cnt < 0) > + error (_("Provided range can't be watched.")); This error message does not tell anything about the reason. Can we tell the user something more specific here about what can she do to alleviate the problem? > + if (can_use_wp < 0) > + error (_("Not enough available hardware watchpoints.")); "Not enough hardware resources for specified watchpoints" is more accurate, I think. > + /* Verifying if the lines make sense. We need to check if > + the first address in the range is smaller than the second, > + and also compute the length. */ > + if (sal1.pc > sal2.pc) > + error (_("Invalid search space, end preceeds start.")); First, the message should better be something like "Invalid range: start address is greater than end address." More importantly, would it make sense to reverse the addresses in this case, instead of erroring out? > + if (length == 0) > + { > + /* This range is simple enough to be treated by > + the `hbreak' command. */ > + printf_unfiltered (_("Range is too small, using `hbreak' \ > +instead.\n")); And why do we need to announce that? Perhaps do that only under verbose operation. > + if (can_use_bp < 0) > + error (_("Not enough available hardware breakpoints.")); See the comment above about a similar message. > + /* Make sure that the two addresses are the same. */ > + if (exp_address != cond_address) > + { > + printf_filtered ("Addresses in location and condition are different.\n"); > + return 0; > + } Why do we need this message? (If we need it, please put it in _().) > + /* Make sure that the two addresses are the same. */ > + if (exp_address != cond_address) > + { > + printf_filtered ("Addresses in location and condition are different.\n"); > + return 0; > + } Same here. And also, the user could have typed a variable, not an address, so the message text, if it is needed, might mislead. > + /* Make sure that the two variables' names are the same. */ > + if (strcmp (name_cond, name_exp) != 0) > + { > + printf_filtered ("Addresses in location and condition are different.\n"); > + return 0; > + } And here. Btw, two different names can eventually evaluate to the same addresses, no? > + c = add_com ("watch-range", class_breakpoint, watch_range_command, _("\ > +Set a WRITE hardware watchpoint for an address range.\n\ > +The address range should be specified in one of the following formats:\n\ > +\n\ > + start-address, end-address\n\ > + start-address, +length\n\ > +\n\ > +The watchpoint will stop execution of your program whenever the inferior\n\ > +writes to any address within that range.")); It would be good to tell if the address is inclusive or exclusive. Anyway, this command (and all the other command changes and additions) need a suitable change to the user manual (although I would suggest to wait with that until we resolve the UI issues I raised in my previous message in this thread.) > + if (start_addr > end_addr) > + error (_("Invalid search space, end preceeds start.")); See the comment above about a similar message. Thanks for working on this.