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

[Iain Buclaw <ibuclaw@gdcproject.org>]

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

> Just recently i fixed a bunch of bad practices in terms of test naming.
>
> I think using { continue } here to mean "just end the test" is one of those
> bad practices that we still carry around in our codebase. Until someone
> decides to clean those up, we still still be carrying those around.
>
> I agree with you we have examples in the codebase using exactly this
> confusing construct, but we want to make sure new code does things in more
> reasonable ways so our codebase stays clean and readable.
>
>

I'll make a note and if I remember, I'll send something through later.

>>>> +
>>>> +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.
>>
>