Diagnose invalid pointer arithmetic on gdb.Value
Commit Message
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.
Comments
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
@@ -144,6 +144,24 @@ proc test_value_numeric_ops {} {
-re "result = .*$gdb_prompt $" {fail "catch throw of GDB error"}
-re "$gdb_prompt $" {fail "catch throw of GDB error"}
}
+
+ 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"}
+ }
}
proc test_value_boolean {} {
@@ -951,7 +951,9 @@ scalar_binop (struct value *arg1, struct value *arg2, enum exp_opcode op)
type1 = check_typedef (value_type (arg1));
type2 = check_typedef (value_type (arg2));
- if ((TYPE_CODE (type1) != TYPE_CODE_FLT
+ if (TYPE_CODE (type1) == TYPE_CODE_PTR || TYPE_CODE (type2) == TYPE_CODE_PTR)
+ error (_("Invalid arguments to pointer arithmetic operation."));
+ else if ((TYPE_CODE (type1) != TYPE_CODE_FLT
&& TYPE_CODE (type1) != TYPE_CODE_DECFLOAT
&& !is_integral_type (type1))
|| (TYPE_CODE (type2) != TYPE_CODE_FLT