Re: [PATCH v2] Fix gdb crash when trying to print the address of a synthetic pointer.

[Pedro Alves <palves at redhat dot com>]

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  <martin.galvan@tallertechnologies.com>
> 
> 	* valops.c (value_addr): Don't try to get the address
> 	of a synthetic pointer.
> 
> gdb/testsuite/ChangeLog:
> 2015-06-14  Martin Galvan  <martin.galvan@tallertechnologies.com>
> 
> 	* 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 <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +
> +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 <http://www.gnu.org/licenses/>.
> +# 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=<synthetic pointer>.*" \
> +         "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\\) <synthetic pointer>" "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=<synthetic pointer>.*" \
> +         "stepped inside 'function'; 'arg' is a synthetic pointer"

Specifically, the "arg=<synthetic pointer>" bit.  So is looks like
GDB doesn't handle synthetic references all that well:

 (gdb) p arg
 $2 = (int &) <synthetic pointer>

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 &) @<synthetic pointer>: 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