From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32105 invoked by alias); 4 Jan 2017 19:38:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 32084 invoked by uid 89); 4 Jan 2017 19:38:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS,URIBL_RED autolearn=no version=3.3.2 spammy=H*f:sk:5806a07, H*i:sk:5806a07, january, January X-HELO: mail-ua0-f193.google.com Received: from mail-ua0-f193.google.com (HELO mail-ua0-f193.google.com) (209.85.217.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 04 Jan 2017 19:38:04 +0000 Received: by mail-ua0-f193.google.com with SMTP id 2so3151614uax.3 for ; Wed, 04 Jan 2017 11:38:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=9zCyxFLd0lQNdMw+F3XL/Gm611COG4sDgw38vMf8fCI=; b=bGV0HvlFNwre6PwQ2CnEEXm2vRMuX2IDlciS/PeM18n/1g+5dF3aywoXkcByl1JI19 6vCuPQ76nVMII3OzT/jRqdQQuHxnKnbrsJ9Alcr1W7bq0hQuqEkeM49RIaEv41wgO7v1 WO4Se3xaowRdaOtExLn+alLpNR/gs0XnAZw5s2ZiW0jFCoFighFalQVo3aAiICc4PR1h GPZoj62AznfT1uoudNpjAtqnxqYh7xyQdWkw0P1tF/uQvKurRQTdG3W6qF8bqakCzUcv 6UXXw3xg0prT4aaT2N9qeQlne9Um9IRFhHiSEETkdfYV078Vmo0ZuBQtzdlZHmi+10so SnLg== X-Gm-Message-State: AIkVDXL5J0K3zCFNSwIdatDLUCPlQLHLyp7j0c6RrkG/m5x5YXovwVfb6RPn8vDs2zdnQNra+Gru0wQELAVcYg== X-Received: by 10.159.33.166 with SMTP id 35mr45016298uac.150.1483558682777; Wed, 04 Jan 2017 11:38:02 -0800 (PST) MIME-Version: 1.0 Received: by 10.176.16.7 with HTTP; Wed, 4 Jan 2017 11:38:02 -0800 (PST) In-Reply-To: <5806a075-dc30-b664-a834-a633b3b27256@codesourcery.com> References: <5806a075-dc30-b664-a834-a633b3b27256@codesourcery.com> From: Iain Buclaw Date: Wed, 04 Jan 2017 19:38:00 -0000 Message-ID: Subject: Re: [PATCH] D: Fix crash when expression debugging To: Luis Machado Cc: GDB Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00060.txt.bz2 On 4 January 2017 at 17:08, Luis Machado 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 20114800 p.2............. >> 2 OP_TYPE 87 W............... >> 3 OP_LONG 38 &............... >> 4 19696640 ..,............. >> 5 OP_NULL 0 ................ >> 6 OP_LONG 38 &............... >> 7 UNOP_CAST 51 3............... >> 8 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 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 >> >> 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 . >> + >> +# 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.