Re: [PATCH] D: Fix crash when expression debugging

Iain Buclaw
2017-01-04T19:38:15Z

On 4 January 2017 at 17:08, Luis Machado <lgustavo@codesourcery.com> wrote:
> On 01/03/2017 06:02 PM, Iain Buclaw wrote:
>>
>> This patch fixes a crash found on "p *(type *)0x1234" when using "set
>> debug expression 1".
>>
>> While casting works as expected with expression debugging turned off,
>> this seems to be an indication that I'm infact doing something wrong
>> in the building of the expression.
>>
>> (gdb) print *(int*)(0x0)
>> Dump of expression @ 0x12db320, before conversion to prefix form:
>> Language d, 11 elements, 16 bytes each.
>> Index Opcode Hex Value String Value
>> 0 OP_TYPE 87 W...............
>> 1 <unknown 20114800> 20114800 p.2.............
>> 2 OP_TYPE 87 W...............
>> 3 OP_LONG 38 &...............
>> 4 <unknown 19696640> 19696640 ..,.............
>> 5 OP_NULL 0 ................
>> 6 OP_LONG 38 &...............
>> 7 UNOP_CAST 51 3...............
>> 8 <unknown 20114800> 20114800 p.2.............
>> 9 UNOP_CAST 51 3...............
>> 10 UNOP_IND 61 =...............
>> Dump of expression @ 0x12db320, after conversion to prefix form:
>> Expression: `*(int *) 0'
>> Language d, 11 elements, 16 bytes each.
>>
>>
>> 0 UNOP_IND
>> 1 UNOP_CAST Type @0x132ed70 (int *)
>> 4 OP_LONG Type @0x12c8c00 (int), value 0 (0x0)
>> 8 <unknown 20114800> Unknown format
>> 9 UNOP_CAST Type @0x3d (Segmentation fault
>>
>>
>> Looks like using UNOP_CAST_TYPE is the right thing to do here, as the
>> TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes.
>>
>>
>> dlang-debug-expr.patch
>>
>>
>> 2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org>
>>
>> gdb/ChangeLog:
>>
>> * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE.
>>
>> gdb/testsuite/ChangeLog:
>>
>> * gdb.dlang/debug-expr.exp: New file.
>>
>> ---
>> diff --git a/gdb/d-exp.y b/gdb/d-exp.y
>> index 077e645..91d15f2 100644
>> --- a/gdb/d-exp.y
>> +++ b/gdb/d-exp.y
>> @@ -321,15 +321,12 @@ UnaryExpression:
>>
>> CastExpression:
>> CAST_KEYWORD '(' TypeExp ')' UnaryExpression
>> - { write_exp_elt_opcode (pstate, UNOP_CAST);
>> - write_exp_elt_type (pstate, $3);
>> - write_exp_elt_opcode (pstate, UNOP_CAST); }
>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); }
>> /* C style cast is illegal D, but is still recognised in
>> the grammar, so we keep this around for convenience. */
>> | '(' TypeExp ')' UnaryExpression
>> - { write_exp_elt_opcode (pstate, UNOP_CAST);
>> - write_exp_elt_type (pstate, $2);
>> - write_exp_elt_opcode (pstate, UNOP_CAST); }
>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); }
>> +
>> ;
>>
>> PowExpression:
>> diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp
>> b/gdb/testsuite/gdb.dlang/debug-expr.exp
>> new file mode 100644
>> index 0000000..3bb2c09
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp
>> @@ -0,0 +1,40 @@
>> +# Copyright 2017 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test "set debug expr 1" on d expressions.
>> +
>> +if { [skip_d_tests] } { continue }
>
>
> We should output a message:
>
> untested "skipping d language tests"
>
> It may be more reasonable to just return instead of continuing? The effect
> will probably be the same, but it is a bit confusing to read "continue"
> without a visible loop.
>

I did a quick grep, and it seems like everyone is skippingtests in
this way except for gdb.ada and gdb.btrace which are doing { return -1
}

>> +
>> +gdb_start
>> +gdb_test_no_output "set language d"
>> +gdb_test_no_output "set debug expression 1"
>> +
>> +# Test whether the expression debug machinery accepts the expression.
>> +
>> +proc test_debug_expr { cmd output } {
>> + global gdb_prompt
>> +
>> + gdb_test_multiple $cmd "" {
>> + -re ".*Invalid expression.*\r\n$gdb_prompt $" {
>> + fail $cmd
>> + }
>> + -re ".*\[\r\n\]$output\r\n$gdb_prompt $" {
>> + pass $cmd
>> + }
>> + }
>> +}
>> +
>> +# This caused gdb to segfault.
>> +test_debug_expr "print *(int*)(0)" "Cannot access memory at address 0x0"
>>
>
> Otherwise the test looks good. I don't have much state on the fix itself,
> but it seems reasonable if it fixes a crash.
>

I had a look at other language frontends for inspiration. At least
C/C++ language does it this way. Others that use UNOP_CAST don't
write out OP_TYPE. The other way to do it would be to change the
grammar to '(' TYPE ')' UnaryExpression. But I don't want to do
that.

> I'm assuming no testsuite regressions?

None at all.