Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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.


> > > +	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.


> > > +	case UNOP_LOWER:
> > > +	case UNOP_UPPER:
> > > +	case UNOP_LENGTH:
> > > +	case UNOP_CARD:
> > > +	case UNOP_CHMAX:
> > > +	case UNOP_CHMIN:
> > 
> > I do not see implemented evaluation of these, also their processing should
> > have been probably moved to ... the already deleted Chill support files.
> 
> Likewise.

Likewise.


> > > +	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.


> > > +	case UNOP_HIGH:
> > 
> > If it really should be here it could be moved into m2-* but this separation is
> > already not strictly followed.
> 
> Sorry, I'm afraid I didn't understand your comment.

UNOP_HIGH can be probably included as I see now, sorry.


There is ada-lang.c:ada_operator_check which contains language-specific (for
ada in this case) operators.  I thought processing of a language-specific
operators should be in their specific language file.

But this currently applies only to Ada operators from ada-operator.inc as they
have OP_* numerical values possibly overlapping (*) other language-specific
OP_* numerical values.  Currently all the non-Ada operators are defined
globally unique so it is safe enough to process them in a general file like
you did in breakpoint.c.

(*) As Ada is currently the only language with OP_* numerical values starting
    at OP_EXTENDED0 sure currently it does not overlap them.



Thanks,
Jan