Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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.


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


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


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


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


> +	case OP_LAST:

For values <=0 it will change, it is not a constant.


> +	case OP_INTERNALVAR:

I would guess value of some of the internal variables can change.


> +	/* 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).


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


> +	case UNOP_HIGH:

If it really should be here it could be moved into m2-* but this separation is
already not strictly followed.



Thanks,
Jan