Re: [PATCH] Forbid watchpoint on a constant value

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

On Thursday 20 May 2010 12:29:42, Jan Kratochvil wrote:
> On Thu, 20 May 2010 07:10:26 +0200, Sergio Durigan Junior wrote:
> > I also added other constant types to that switch statement.  Please take a look
> > and see if you agree.
> [...]
> > +	/* The user could provide something like:
> > +
> > +	   `watch *0xdeadbeef + 4'
> > +
> > +	   In this case, we need to check the remaining elements
> > +	   of this expression.  */
> > +	case BINOP_ADD:
> 
> If you have overloaded operator '+' of some class cannot this operation
> execute an inferior function via value_x_binop()?  Maybe it is not
> exploitable, I am not sure.

I don't know.  I will see if I can make some tests to check.

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

> Offtopic here: they could be rather somehow forbidden from a watchpoint
> expression, moreover if it gets evaluated as a hardware watchpoint but that is
> already broken by incorrect/naive assumptions as filed in:
> 	PR breakpoints/11613: hardware watchpoint missed for -O2 -g inferior
> 
> 
> > +	case BINOP_SUBSCRIPT:
> 
> This is a regression:
> 	./gdb -nx -ex 'p &line' -ex 'watch $0[0]' -ex r ./gdb
> now prints:
> 	Cannot watch constant value $0[0].
> but it was a valid watchpoint, hit at:
> 	captured_main (data=0x7fffffffd1c0) at ./main.c:322

Sorry, removed.

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

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

> > +	case OP_LAST:
> 
> For values <=0 it will change, it is not a constant.

I think I didn't understand OP_LAST, then.  Sorry about that, removed.

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

> > +	/* UNOP_IND and UNOP_ADDR are not in this list becase
> > +	   they can be used in expressions like:
> > +
> > +	   (gdb) watch *0x12345678
> > +
> > +	   or
> > +
> > +	   (gdb) watch &some_var
> > +	   */
> 
> I do not see why UNOP_ADDR should not be listed here (but sure not a problem).

You're right.

> > +	case UNOP_SIZEOF:
> 
> UNOP_SIZEOF on OP_TYPE where the type is TYPE_DYNAMIC from the VLA patchset
> would be a regression; but that is not in FSF GDB so it is OK now.

Ok, thanks for letting me know.  I will remove UNOP_SIZEOF.

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

Thanks,

-- 
Sergio Durigan Junior
Red Hat