Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: "Ulrich Weigand" <uweigand@de.ibm.com>
To: bauerman@br.ibm.com (Thiago Jung Bauermann)
Cc: gdb-patches@sourceware.org (gdb-patches ml)
Subject: Re: [RFA 2/3] Demote to sw watchpoint only in update_watchpoint
Date: Fri, 29 Apr 2011 17:26:00 -0000	[thread overview]
Message-ID: <201104291726.p3THQVaC029608@d06av02.portsmouth.uk.ibm.com> (raw)
In-Reply-To: <1303161745.1969.220.camel@hactar> from "Thiago Jung Bauermann" at Apr 18, 2011 06:22:25 PM

Thiago Jung Bauermann wrote:

> I was trying to adapt the logic in both places for masked watchpoints,
> but it's hard to anticipate in watch_command_1 what update_watchpoint
> will ultimately do without duplicating a good portion of the latter. The
> code in watch_command_1 was getting complicated and hard to get right in
> all situations for something which would be done again later anyway.

Agreed, that's a good idea.

> I decided that it was cleaner if watch_command_1 just delegated
> everything to update_watchpoint. This patch makes it create a
> watchpoint, call update_watchpoint and then delete it if some error is
> hit. The only drawback I can think of is that the aborted watchpoint
> will "consume" one breakpoint number, and thus GDB will skip one number
> when creating the next breakpoint/watchpoint. This doesn't seem
> important to me.

Hmm, I don't really like that change.  However, it should be easy to
fix by just moving the set_breakpoint_number call to down below where
update_watchpoint has succeeded, shouldn't it?

> 	(delete_breakpoint): Add announce argument to control whether
> 	observers are notified of the deletion.  Update all callers.

Ugh.  Adding an extra argument that everybody must be aware of just
to take care of this special case isn't really nice ...  I'd prefer
if you'd either split off the parts of delete_breakpoint that are
needed to undo partial creation into a separate function, or else,
if you use the set_breakpoint_number trick, identify partial
breakpoints by the fact that their b->number is still zero.

> @@ -1432,8 +1436,22 @@ update_watchpoint (struct breakpoint *b, int reparse)
>  	      target_resources_ok = target_can_use_hardware_watchpoint
>  		    (bp_hardware_watchpoint, i, other_type_used);
This should now use b->type instead of hardcoding bp_hardware_watchpoint,
shouldn't it?

Otherwise looks good to me.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com


  reply	other threads:[~2011-04-29 17:26 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-13 20:55 [RFA] Implement support for PowerPC BookE masked watchpoints Thiago Jung Bauermann
2011-01-31 20:09 ` Thiago Jung Bauermann
2011-02-17 15:10 ` Ulrich Weigand
2011-04-18 21:22   ` [RFA 1/3] Change watchpoint's enable state in do_enable_breakpoint Thiago Jung Bauermann
2011-04-29 17:21     ` Ulrich Weigand
2011-05-04  0:11       ` Thiago Jung Bauermann
2011-04-18 21:22   ` [RFA 2/3] Demote to sw watchpoint only in update_watchpoint Thiago Jung Bauermann
2011-04-29 17:26     ` Ulrich Weigand [this message]
2011-05-03  4:56       ` Thiago Jung Bauermann
2011-05-03  6:05         ` Eli Zaretskii
2011-05-03  9:58           ` Pedro Alves
2011-05-03 16:57             ` Eli Zaretskii
2011-05-03 17:41               ` Pedro Alves
2011-05-03 18:03                 ` Eli Zaretskii
2011-05-03 18:12                   ` Pedro Alves
2011-05-03 20:30                     ` Eli Zaretskii
2011-05-04  0:03                       ` Thiago Jung Bauermann
2011-05-04  3:07                         ` Eli Zaretskii
2011-05-04 22:21                           ` Thiago Jung Bauermann
2011-05-05  3:09                             ` Eli Zaretskii
2011-05-05  8:15                             ` Pedro Alves
2011-05-05 10:28                               ` Eli Zaretskii
2011-05-05 15:27                                 ` Pedro Alves
2011-05-05 16:27                                   ` Eli Zaretskii
2011-05-05 11:10                               ` Ulrich Weigand
2011-05-05 15:21                                 ` Pedro Alves
2011-05-04 19:12           ` Thiago Jung Bauermann
2011-05-04 20:31             ` Eli Zaretskii
2011-05-04 22:22               ` Thiago Jung Bauermann
2011-05-05 11:04         ` Ulrich Weigand
2011-04-18 21:24   ` [RFA 3/3] Implement support for PowerPC BookE masked watchpoints Thiago Jung Bauermann
2011-04-29 17:46     ` Ulrich Weigand
2011-05-03  4:56       ` [needs doc review] " Thiago Jung Bauermann
2011-05-03  6:24         ` Eli Zaretskii
2011-05-05 21:57           ` Thiago Jung Bauermann
2011-05-06 10:28             ` Eli Zaretskii
2011-05-06 20:35               ` Thiago Jung Bauermann
2011-05-05 11:07         ` Ulrich Weigand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201104291726.p3THQVaC029608@d06av02.portsmouth.uk.ibm.com \
    --to=uweigand@de.ibm.com \
    --cc=bauerman@br.ibm.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox