[PATCH] Make enable reset disposition

[PATCH] Make enable reset disposition

[Stan Shebs]

In the process of developing an additional enablement option (to be 
posted soon), I ran across this little bit of behavior that seems wrong; 
if you do "enable once" and then "enable" on a breakpoint, the 
disposition is unchanged - the breakpoint is still going to get disabled 
after being hit.  (Similarly for "enable delete" breakpoints.)

While one could argue that this is good, because you can toggle a 
breakpoint's enablement independently of its ultimate disposition, the 
downside is that you're stuck with your original choice; once you've set 
a breakpoint's disposition to delete for instance, there is no way to 
undo that, and when the breakpoint is hit, it's gone, conditions and 
command list and all.

Having "enable" reset dispositions has its own fault, namely that if you 
do just "enable" to enable all breakpoints, and they have different 
dispositions, then all the dispositions are reset en masse, and you 
would have to manually do a combination of "enable once", "enable 
delete", etc to get those back to desired values.

A more complicated solution might be to introduce an additional flavor 
or option of enable command ("enable always"?), but I wouldn't like to 
try to explain the different flavors to users, and chances are that 
nobody would remember it anyway.

I couldn't see anything in the manual that addressed the point either way.

Stan

2012-01-26  Stan Shebs <stan@codesourcery.com>

     * breakpoint.c (enable_breakpoint): Set disposition instead
     of copying it.

     * gdb.base/ena-dis-br.exp: Add a check of disposition reset.


Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.648
diff -u -p -r1.648 breakpoint.c
--- breakpoint.c    25 Jan 2012 15:57:04 -0000    1.648
+++ breakpoint.c    26 Jan 2012 23:42:32 -0000
@@ -12945,7 +12945,7 @@ enable_breakpoint_disp (struct breakpoin
  void
  enable_breakpoint (struct breakpoint *bpt)
  {
-  enable_breakpoint_disp (bpt, bpt->disposition);
+  enable_breakpoint_disp (bpt, disp_donttouch);
  }

  static void
Index: testsuite/gdb.base/ena-dis-br.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/ena-dis-br.exp,v
retrieving revision 1.19
diff -u -p -r1.19 ena-dis-br.exp
--- testsuite/gdb.base/ena-dis-br.exp    16 Jan 2012 16:21:44 -0000    1.19
+++ testsuite/gdb.base/ena-dis-br.exp    26 Jan 2012 23:42:32 -0000
@@ -78,6 +78,9 @@ proc break_at { breakpoint where } {

  set bp [break_at "marker1" " line ($bp_location15|$bp_location16)"]

+# Do this first to check that regular enable resets the disposition.
+gdb_test_no_output "enable once $bp" "enable once break marker1"
+
  gdb_test_no_output "enable $bp" "enable break marker1"

  gdb_test "info break $bp" \

Re: [PATCH] Make enable reset disposition

[Doug Evans]

On Thu, Jan 26, 2012 at 4:23 PM, Stan Shebs <stanshebs@earthlink.net> wrote:
> In the process of developing an additional enablement option (to be posted
> soon), I ran across this little bit of behavior that seems wrong; if you do
> "enable once" and then "enable" on a breakpoint, the disposition is
> unchanged - the breakpoint is still going to get disabled after being hit.
>  (Similarly for "enable delete" breakpoints.)
>
> While one could argue that this is good, because you can toggle a
> breakpoint's enablement independently of its ultimate disposition, the
> downside is that you're stuck with your original choice; once you've set a
> breakpoint's disposition to delete for instance, there is no way to undo
> that, and when the breakpoint is hit, it's gone, conditions and command list
> and all.
>
> Having "enable" reset dispositions has its own fault, namely that if you do
> just "enable" to enable all breakpoints, and they have different
> dispositions, then all the dispositions are reset en masse, and you would
> have to manually do a combination of "enable once", "enable delete", etc to
> get those back to desired values.
>
> A more complicated solution might be to introduce an additional flavor or
> option of enable command ("enable always"?), but I wouldn't like to try to
> explain the different flavors to users, and chances are that nobody would
> remember it anyway.

Hi.
I don't know that I would call it *more* complicated.
IIUC, there two independent(/orthogonal) attributes of a breakpoint
(once vs delete vs always, and enabled vs disabled) and they are
controlled by the same command.
Oops.

From a u/i perspective keeping the orthogonality feels right.
Also, having "enable" reset dispositions en masse does bother me.
Does this affect tbreak-created breakpoints?

It feels like adding "always" doesn't muddy the waters any more than
they already are. :-)
If one were do do this again, having a new command instead of "enable"
may be easier for user's to digest and remember.

Re: [PATCH] Make enable reset disposition

[Tom Tromey]

>>>>> "Stan" == Stan Shebs <stanshebs@earthlink.net> writes:

Stan> In the process of developing an additional enablement option (to be
Stan> posted soon), I ran across this little bit of behavior that seems
Stan> wrong; if you do "enable once" and then "enable" on a breakpoint, the
Stan> disposition is unchanged - the breakpoint is still going to get
Stan> disabled after being hit.

Nice find.

Stan> A more complicated solution might be to introduce an additional flavor
Stan> or option of enable command ("enable always"?), but I wouldn't like to
Stan> try to explain the different flavors to users, and chances are that
Stan> nobody would remember it anyway.

If you want to go this way, I think explaining it doesn't have to be too
hard.  You could just have the "enable once" docs say, "if you did
`enable once' and then changed your mind, you can `enable always' to
undo it"; and then in the `enable always' paragraph (semi-redundantly)
mention that this command exists just to undo the effect of enable once
and enable delete.

FWIW I didn't even remember the existence of enable once or enable delete.
I think I have never used them.

Stan> I couldn't see anything in the manual that addressed the point either way.

Doug> It feels like adding "always" doesn't muddy the waters any more than
Doug> they already are. :-)
Doug> If one were do do this again, having a new command instead of "enable"
Doug> may be easier for user's to digest and remember.

What Doug said; but with the caveat that I am actually ok with either
approach, as I consider this to be a fairly minor issue; however, if you
choose the simpler route then I think a manual update is in order.

Tom