Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Thursday 20 May 2010 13:47:35, Jan Kratochvil wrote:
> On Thu, 20 May 2010 18:13:01 +0200, Sergio Durigan Junior wrote:
> > On Thursday 20 May 2010 12:29:42, Jan Kratochvil wrote:
> > > On Thu, 20 May 2010 07:10:26 +0200, Sergio Durigan Junior wrote:
> > > > +	case BINOP_ASSIGN:
> > > > +	case BINOP_ASSIGN_MODIFY:
> > > > +	case OP_FUNCALL:
> > > > +	case OP_OBJC_MSGCALL:
> > > > +	case OP_F77_UNDETERMINED_ARGLIST:
> > > > +	case UNOP_PREINCREMENT:
> > > > +	case UNOP_POSTINCREMENT:
> > > > +	case UNOP_PREDECREMENT:
> > > > +	case UNOP_POSTDECREMENT:
> > > 
> > > This is not a `const'/`pure' function, it has some side-effect of the
> > > assignment.  I do not thing they should be caught as constant.
> > 
> > Sorry, I didn't understand why they can't be considered constant in the
> > context of a watchpoint.
> 
> echo 'int v;main(){}'|gcc -o 1 -x c - -g;./gdb -nx -ex 'p v' -ex 'watch v=1' -ex start -ex 'p v' ./1
> 
> The output of last
> 	-ex 'p v'
> will change if you include / not include
> 	-ex 'watch v=1'
> so that 'watch v=1' is not a NOP without meaning.
> 
> Someone may use it in existing scripts to set variable `v' this way.
> 
> I understand it is very broken idea to modify variables from watchpoint
> expression.  Just I did not find it too useful to check here and when such
> patch could change the GDB behavior I find it more a disadvantage than an
> advantage.
> 
> But I do not have a strong opinion on it.

Ok, now I've got it.  I really wasn't considering this option because
I would never modify a variable's value this way, but you're right.

> > > > +	case BINOP_VAL:
> > > > +	case BINOP_INCL:
> > > > +	case BINOP_EXCL:
> > > > +	case UNOP_PLUS:
> > > > +	case UNOP_CAP:
> > > > +	case UNOP_CHR:
> > > > +	case UNOP_ORD:
> > > > +	case UNOP_ABS:
> > > > +	case UNOP_FLOAT:
> > > > +	case UNOP_MAX:
> > > > +	case UNOP_MIN:
> > > > +	case UNOP_ODD:
> > > > +	case UNOP_TRUNC:
> > > 
> > > I do not see implemented evaluation of these, also their processing should
> > > have been probably moved to some m2-* file.
> > 
> > Does it mean that I have to remove them from this list?
> 
> There is a problem these operators have no implementation in the current GDB
> sources.  Therefore one cannot verify what they do and if they are or they are
> not constant.  How did you verify BINOP_VAL does not depend on some possibly
> changing value?  The comments at their definition may not be always right.
> 
> As they are also not useful to be used in an expression when no processing of
> them is implemented I find more dangerous to make some - possibly invalid
> - assumptions about them (that they are constant - they possibly may be
> implemented in a non-constant way in the future).
> 
> The note about move to a different file was invalid, described in the very
> last comment of my mail.

Sorry, I just blindly added those types to the list based on their comments.
I will remove them, thanks for the explanation.

> > > > +	case OP_INTERNALVAR:
> > > 
> > > I would guess value of some of the internal variables can change.
> > 
> > Is the user allowed to put a watchpoint on an internal variable?
> 
> It seems to me so:
> 	(gdb) watch $a
> 	Watchpoint 2: $a
> Although the watchpoint does not get hit when $a gets modified during inferior
> run.  Unaware why.

I really didn't know you could ask GDB to trigger when an internal
variable changes.  But OK, living and learning ;-).

Will send the refreshed patch later today.

Thanks,

-- 
Sergio Durigan Junior
Red Hat