From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18885 invoked by alias); 4 Jan 2017 20:38:20 -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 18872 invoked by uid 89); 4 Jan 2017 20:38:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 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=competing, H*f:sk:ef39b7b, H*i:sk:ef39b7b, january X-HELO: mail-vk0-f67.google.com Received: from mail-vk0-f67.google.com (HELO mail-vk0-f67.google.com) (209.85.213.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 04 Jan 2017 20:38:09 +0000 Received: by mail-vk0-f67.google.com with SMTP id y197so17305843vky.0 for ; Wed, 04 Jan 2017 12:38:09 -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=RzI58mZ7lolciXf7zFU6iQamPrutSzRMJJTulYV6kwk=; b=oBEvW6zHPYjTPDL0YSycCceRzt3Zoj+ACAW3GwmDlKn3V67S+2lkC1G/mmdZezwG1C zSTsiy+b4p1WBjd6RZSshKyndeCuoo4xsanHPTS1aioRH4ScJ1BIEmZOv2kOqDnHIIIC U4pM+0ylpVeOqiJzVNP/U/BNcC374CU+8JyMgyXg9Zdfyt7TkqvBVmzhVPX9u+P9/fFV RCuuI5raHJOfsjgoST92FB5Zwz2PpM0v5qPE5ajxJmb/eqglqTsCnRReo6F3VDj7YWQo ULQqxdXddWFdJiFxEjqZHUQmVqEIDXLZWJ+WXHDr7EW5lHdAeg9UugmtP2gUEtDcd+b8 GkLg== X-Gm-Message-State: AIkVDXIJJmteFCe7pSv6GymLkPTH+uNodgtUwtkDgo4WMchRyNcAgeovlgcsmFMRRvwRM3xFsoECNDMVx/IdBg== X-Received: by 10.31.137.18 with SMTP id l18mr21789887vkd.54.1483562288001; Wed, 04 Jan 2017 12:38:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.176.16.7 with HTTP; Wed, 4 Jan 2017 12:38:07 -0800 (PST) In-Reply-To: References: <5806a075-dc30-b664-a834-a633b3b27256@codesourcery.com> From: Iain Buclaw Date: Wed, 04 Jan 2017 20: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/msg00063.txt.bz2 On 4 January 2017 at 20:48, Luis Machado wrote: > On 01/04/2017 01:38 PM, Iain Buclaw wrote: >> >> 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 >> } >> >> > > 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. >> >