From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67634 invoked by alias); 15 Jun 2015 11:17:47 -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 67616 invoked by uid 89); 15 Jun 2015 11:17:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 15 Jun 2015 11:17:45 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id DE81C347A56; Mon, 15 Jun 2015 11:17:43 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5FBHge8010889; Mon, 15 Jun 2015 07:17:42 -0400 Message-ID: <557EB455.5040909@redhat.com> Date: Mon, 15 Jun 2015 11:17:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Martin Galvan , gdb-patches@sourceware.org Subject: Re: [PATCH v2] Fix gdb crash when trying to print the address of a synthetic pointer. References: <1434257323-18553-1-git-send-email-martin.galvan@tallertechnologies.com> In-Reply-To: <1434257323-18553-1-git-send-email-martin.galvan@tallertechnologies.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-06/txt/msg00295.txt.bz2 On 06/14/2015 05:48 AM, Martin Galvan wrote: > Changes in v2: > * Added a test case to cover this. Thanks. > > Trying to print the address of a synthetic pointer (such as a C++ reference > after O3 optimization) will cause gdb to crash with the following message: > > ../gdb/dwarf2loc.c:1625: internal-error: Should not be able to create a lazy value with an enclosing type > > This patch fixes that by doing a check for synthetic pointers in value_addr > and printing an error message. > > Ok to commit? > > gdb/ChangeLog: > 2015-06-14 Martin Galvan > > * valops.c (value_addr): Don't try to get the address > of a synthetic pointer. > > gdb/testsuite/ChangeLog: > 2015-06-14 Martin Galvan > > * synthetic-pointer-reference.exp: New file. > * synthetic-pointer-reference.cc: New file. Should be: * gdb.dwarf2/synthetic-pointer-reference.exp: New file. * gdb.dwarf2/synthetic-pointer-reference.cc: New file. Also, we can't/shouldn't assume that gcc -O3 will always generate the expected dwarf info. That's why tests under gdb.dwarf2/ either include the compiled .S file, or preferably, use the Dwarf::assemble machinery. > --- > .../gdb.dwarf2/synthetic-pointer-reference.cc | 32 ++++++++++++++++++ > .../gdb.dwarf2/synthetic-pointer-reference.exp | 39 ++++++++++++++++++++++ > gdb/valops.c | 6 ++++ > 3 files changed, 77 insertions(+) > create mode 100644 gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.cc > create mode 100644 gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.exp > > diff --git a/gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.cc b/gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.cc > new file mode 100644 > index 0000000..36d775e > --- /dev/null > +++ b/gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.cc > @@ -0,0 +1,32 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2015 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 . */ > + > +#include > + > +static void function(int& arg) > +{ > + /* We do this to ensure that neither function nor arg are optimized out */ > + printf("%d\n", arg); > +} > + > +int main() > +{ > + int var = 42; > + function(var); > + > + return 0; > +} Unless there's a good reason otherwise, please make the test follow the GDB coding conventions. Line breaks before function name in definitions. Space before parens. Period and double-space in comments. Indentation. Etc. Also, it'd be preferable to make the test not depend on stdio/printf, so it runs everywhere. > diff --git a/gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.exp b/gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.exp > new file mode 100644 > index 0000000..9e327f8 > --- /dev/null > +++ b/gdb/testsuite/gdb.dwarf2/synthetic-pointer-reference.exp > @@ -0,0 +1,39 @@ > +# Copyright (C) 2015 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 . > +# This file is part of the gdb testsuite. > + > +# Test taking the address of a synthetic pointer created from a C++ reference. > + > +if { [skip_cplus_tests] } { continue } > + > +standard_testfile .cc > + > +set options {debug c++ optimize=-O3} > + > +if {[prepare_for_testing $testfile.exp $testfile $srcfile $options]} { > + return -1 > +} > + > +runto_main > + > +# If we tried to set a breakpoint and then 'continue' up to it, the program > +# would exit without us hitting it because of the optimizations performed > +# by gcc. We must advance using 'next' and 'step' instead. > + "optimizations performed by gcc" is a bit vague. Is it that "function" ends up inlined? What exactly doesn't work? > +gdb_test "next" ".*" "next" > +gdb_test "step" ".*function.*arg=.*" \ > + "stepped inside 'function'; 'arg' is a synthetic pointer" > +gdb_test "print &arg" "Attempt to take address of a synthetic pointer." \ > + "Can't take the address of a synthetic pointer" > diff --git a/gdb/valops.c b/gdb/valops.c > index 66c63c1..1174a5e 100644 > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -1474,6 +1474,12 @@ value_addr (struct value *arg1) > struct value *arg2; > struct type *type = check_typedef (value_type (arg1)); > > + /* The TYPE_CODE_REF path below causes gdb to crash for synthetic pointers > + created from C++ references. Catch those cases here. */ This "causes gdb to crash" really isn't that helpful for future readers. The immediate question readers will ask is: "Why would it crash? Because of a GDB bug? Or because it doesn't make sense because $some_reason?" So the comment should instead be in terms of $some_reason. Also, double space after periods. > + if (value_bits_synthetic_pointer (arg1, value_embedded_offset (arg1), > + TARGET_CHAR_BIT * TYPE_LENGTH (type))) > + error (_("Attempt to take address of a synthetic pointer.")); > + Indentation doesn't look right here. I wonder whether the error message should really talk about pointers, when the user tried to look at a reference? That looks like a bit of implementation detail escaping out. Also, gdb.dwarf2/implptr.exp has: # Test various pointer depths in bar. proc implptr_test_bar {} { global csrcfile set line [gdb_get_line_number "bar breakpoint" $csrcfile] gdb_test "break implptr.c:$line" "Breakpoint 2.*" \ "set bar breakpoint for implptr" gdb_continue_to_breakpoint "continue to bar breakpoint for implptr" gdb_test "print j" " = \\(intp\\) " "print j in implptr:bar" Sounds like a "print &j" test should be added here too. And does that crash too? Judging from the patch alone, I'd think it does. If it doesn't, why doesn't it? Something else in the test caught my eye: > +gdb_test "next" ".*" "next" > +gdb_test "step" ".*function.*arg=.*" \ > + "stepped inside 'function'; 'arg' is a synthetic pointer" Specifically, the "arg=" bit. So is looks like GDB doesn't handle synthetic references all that well: (gdb) p arg $2 = (int &) Without optimization we would get: (gdb) p arg $1 = (int &) @0x7fffffffd71c: 42 So I'd expect to get something like: (gdb) p arg $1 = (int &) @: 42 Because as is it seems like there's no way to get the referenced value, even thought it's right there in the debug info. (If it weren't there, there'd be no point on describing it as an implicit/synthetic pointer.) I noticed more breakage while trying to get at the reference's value, knowing it's an integer: (gdb) p arg + 0 Cannot access memory at address 0x0 These issues don't have to be handled by this patch, thought thinking through them may point at fixing the crash at hand differently. Or not, I really haven't thought about it much. But I thought I'd point it out regardless. Thanks, Pedro Alves