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

Luis Machado
2017-01-04T20:54:09Z

On 01/04/2017 02:38 PM, Iain Buclaw wrote:
> On 4 January 2017 at 20:48, Luis Machado <lgustavo@codesourcery.com> wrote:
>> On 01/04/2017 01:38 PM, Iain Buclaw wrote:
>>>
>>> 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
>>> }
>>>
>>>
>>
>> That's a bit of a stretch. Take, for example, a few of the examples in
>> gdb.base. You will see a number of them returning.
>>
>> The problem here is inheriting past confusing practices when we use some
>> existing files to create new ones, which is not your fault really. I'm
>> guilty myself. :-)
>>
>
> Yes indeed. I wasn't disagreeing, just questioning the two competing
> ways of returning.
>
> I will update to use return and push this in then if there's no
> disagreement. :-)
>

Hopefully someone will chime in for a second opinion. :-)