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

Iain Buclaw
2017-01-04T20:38:20Z

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