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

[Iain Buclaw <ibuclaw at gdcproject dot org>]

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.