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

[Luis Machado <lgustavo@codesourcery.com>]

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. :-)