Diagnose invalid pointer arithmetic on gdb.Value

  On 20/09/16 17:42 +0100, Pedro Alves wrote:
>On 09/20/2016 05:19 PM, Jonathan Wakely wrote:
>> On 20/09/16 16:41 +0100, Pedro Alves wrote:
>>> On 09/20/2016 03:46 PM, Jonathan Wakely wrote:
>>>> Instead of passing invalid arguments to value_binop and getting a
>>>> misleading error, raise a TypeError directly in valpy_binop_throw.
>>>
>>> Did you try changing value_binop instead?  The error string seems
>>> misleading even in C:
>>>
>>> (gdb) p ptr
>>> $1 = 0x601040 <buf> ""
>>> (gdb) p ptr + 1
>>> $2 = 0x601041 <buf+1> ""
>>> (gdb) p ptr + 1.0
>>> Argument to arithmetic operation not a number or boolean.
>>> (gdb)
>>
>> Ah, so it's not specific to the Python API, in which case changing
>> value_binop (or scalar_binop more accurately) might make sense.
>> The check in scalar_binop *does* check for numbers, so the error is
>> accurate. The argument that triggers the errors is actually the
>> pointer argument, not the float one:
>>
>>  if ((TYPE_CODE (type1) != TYPE_CODE_FLT
>>       && TYPE_CODE (type1) != TYPE_CODE_DECFLOAT
>>       && !is_integral_type (type1))
>>      || (TYPE_CODE (type2) != TYPE_CODE_FLT
>>       && TYPE_CODE (type2) != TYPE_CODE_DECFLOAT
>>       && !is_integral_type (type2)))
>>    error (_("Argument to arithmetic operation not a number or boolean."));
>>
>>
>> The problem is that pointer arithmetic with invalid operands ends up
>> here, because value_ptr{add,sub,diff} don't get called for invalid
>> arguments.
>>
>> So maybe a better fix is to first check if either argument is a
>> pointer and give a more specific error, as attached (untested).
>>
>
>Makes sense to me.

Here's a patch to do that. None of the existing tests needed to change
as far as I can see. gdb.base/pointers.exp and gdb.base/completion.exp
have tests that try to match the old error, but they're commented out.

I kept my additions to py-value.exp that do invalid pointer
arithmetic, updating the expected text. Should I move that to
gdb.base/pointers.exp instead? Or check it in both?
commit 39448eaecd5f2e97c6f2c1566b7bae40fa63b2ff
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Sep 20 15:39:44 2016 +0100

    gdb: Better error message for invalid pointer arithmetic
    
    The value_ptr{add,sub,diff} functions are only used when the arguments
    are suitable types, otherwise we end up in scalar_binop so add a more
    helpful error message there.
    
    gdb/ChangeLog:
    2016-09-21  Jonathan Wakely  <jwakely@redhat.com>
    
    	PR python/20622
    	* valarith.c (scalar_binop): If either argument is a pointer
    	give a more descriptive error message.
    
    gdb/testsuite/ChangeLog:
    2016-09-21  Jonathan Wakely  <jwakely@redhat.com>
    
    	PR python/20622
    	* gdb.python/py-value.exp: Test invalid pointer arithmetic.
  

On 09/21/2016 12:07 PM, Jonathan Wakely wrote:

> Here's a patch to do that. None of the existing tests needed to change
> as far as I can see. gdb.base/pointers.exp and gdb.base/completion.exp
> have tests that try to match the old error, but they're commented out.
> 
> I kept my additions to py-value.exp that do invalid pointer
> arithmetic, updating the expected text. Should I move that to
> gdb.base/pointers.exp instead? Or check it in both?
> 

If we keep only one set, then I think moving is appropriate.
Though I guess having both would be nice, as potentially Python
layer changes can mess this up too.  Belt and suspenders.
If you could do that, it'd be great.

+  gdb_test_multiple "python print ('result = ' + str(a+0.5))" "catch error in python type conversion" {
+      -re "Invalid arguments to pointer arithmetic operation..*$gdb_prompt $"   {pass "catch error in python type conversion"}
+      -re "result = .*$gdb_prompt $"		      {fail "catch error in python type conversion"}
+      -re "$gdb_prompt $"			      {fail "catch error in python type conversion"}
+  }
+
+  gdb_test_multiple "python print ('result = ' + str(0.5+a))" "catch error in python type conversion" {
+      -re "Invalid arguments to pointer arithmetic operation..*$gdb_prompt $"   {pass "catch error in python type conversion"}
+      -re "result = .*$gdb_prompt $"		      {fail "catch error in python type conversion"}
+      -re "$gdb_prompt $"			      {fail "catch error in python type conversion"}
+  }
+
+  gdb_test_multiple "python print ('result = ' + str(a-0.5))" "catch error in python type conversion" {
+      -re "Invalid arguments to pointer arithmetic operation..*$gdb_prompt $"   {pass "catch error in python type conversion"}
+      -re "result = .*$gdb_prompt $"		      {fail "catch error in python type conversion"}
+      -re "$gdb_prompt $"			      {fail "catch error in python type conversion"}
+  }
 }

BTW, I noticed that this using the same test message in all cases,
which is a problem we've been slowing fixing:

  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

I see that the test is already violating that, but I'd prefer to
not make it worse.  (see below).

Also, while at it, and I know its a preexisting pattern in the
test that you're copying, but there's really no reason to have the
three regexs per test.  The latter two in all cases are unnecessary,
as gdb_test_multiple has a fallback fail case for "$gdb_prompt $".
So if the first -re line doesn't match we'd get a fail anyway.
What this means is that the tests could be simplified to use gdb_test
instead.  Say, like:

gdb_test "python print ('result = ' + str(a+0.5))" \
   "Invalid arguments to pointer arithmetic operation..*" \
   "catch error in python type conversion: a+0.5"

gdb_test "python print ('result = ' + str(0.5+a))" \
   "Invalid arguments to pointer arithmetic operation..*" \
   "catch error in python type conversion: 0.5+a"

gdb_test "python print ('result = ' + str(a-0.5))" \
   "Invalid arguments to pointer arithmetic operation..*" \
   "catch error in python type conversion: a-0.5"

Thanks,
Pedro Alves