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

[Luis Machado <lgustavo at codesourcery dot com>]

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.

> +
> +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'm assuming no testsuite regressions?