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

  Changes in v2:
 * Added a test case to cover this.

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.
---
 .../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
  

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
  

On Mon, Jun 15, 2015 at 8:17 AM, Pedro Alves <palves@redhat.com> wrote:
> 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.

Understood.

>> +# 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?

Yes, 'function' ends up inlined. If we try to set a breakpoint at a
line inside 'function', the breakpoint will be set at an address we'll
never hit, thus we need to advance using 'step' and 'next' instead.

>> +  /* 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.

Ok.

> 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.

What about "Attempt to take address of a synthetic pointer generated
from a C++ reference"? The users can see it's a synthetic pointer
anyways, so we could be as explicit as possible.

> 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.

I haven't been able to test this so far because the test requires an
x86-like target, which I don't have at hand. I'll keep you posted,
though.

Assuming gdb crashes there too, couldn't I just add a "print &j" test
there and discard the synthetic-pointer-reference.exp test altogether?

>  (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.

I might take a look into these issues in a later patch :)
  

On 06/20/2015 04:25 AM, Martin Galvan wrote:
> On Mon, Jun 15, 2015 at 8:17 AM, Pedro Alves <palves@redhat.com> wrote:
>> 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.
> 
> Understood.
> 
>>> +# 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?
> 
> Yes, 'function' ends up inlined. If we try to set a breakpoint at a
> line inside 'function', the breakpoint will be set at an address we'll
> never hit, thus we need to advance using 'step' and 'next' instead.

Then it sounds like you were putting a breakpoint at a line that was
optimized out.  If you put the breakpoint at the same line that you
"step" and "next" to, then it should work, I think.  You'll still
need a "step" to go from real stack frame to inline frame, as GDB always
presents you the stack frame on breakpoint hits.

> 
>>> +  /* 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.
> 
> Ok.
> 
>> 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.
> 
> What about "Attempt to take address of a synthetic pointer generated
> from a C++ reference"? The users can see it's a synthetic pointer
> anyways, so we could be as explicit as possible.

Hmm, that looks too long to me and still escaping out a detail
that isn't really that interesting to the user.  I was just thinking:

if (is reference)
 "Attempt to take address of a synthetic reference."
else
 "Attempt to take address of a synthetic pointer."

> 
>> 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.
> 
> I haven't been able to test this so far because the test requires an
> x86-like target, which I don't have at hand. I'll keep you posted,
> though.

Do you have an x86-64 box?  If so, you can just run the test with:

 make check RUNTESTFLAGS="--target_board=unix/-m32 gdb.dwarf2/implptr.exp"

> 
> Assuming gdb crashes there too, couldn't I just add a "print &j" test
> there and discard the synthetic-pointer-reference.exp test altogether?

Actually from your description, I assume it _doesn't_ crash, because it's
not a reference.  But it's not tested presently, so who know.  I think it's
worth it to have both pointers and references covered.

Thanks,
Pedro Alves
  

On Wed, Jun 24, 2015 at 8:13 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/20/2015 04:25 AM, Martin Galvan wrote:
>> Yes, 'function' ends up inlined. If we try to set a breakpoint at a
>> line inside 'function', the breakpoint will be set at an address we'll
>> never hit, thus we need to advance using 'step' and 'next' instead.
>
> Then it sounds like you were putting a breakpoint at a line that was
> optimized out.  If you put the breakpoint at the same line that you
> "step" and "next" to, then it should work, I think.  You'll still
> need a "step" to go from real stack frame to inline frame, as GDB always
> presents you the stack frame on breakpoint hits.

That's what I tried at the beginning. However, if I set the breakpoint
at the line I step to, using "continue" will either take me to an
address after the one I want to stop at, or just finish the program.

> Hmm, that looks too long to me and still escaping out a detail
> that isn't really that interesting to the user.  I was just thinking:
>
> if (is reference)
>  "Attempt to take address of a synthetic reference."
> else
>  "Attempt to take address of a synthetic pointer."

The case on which the variable is an actual synthetic pointer (as
opposed to a synthetic reference) doesn't seem to reach value_addr;
instead it prints the "Can't take address of foo which isn't an
lvalue" message from address_of_variable.

If you think it's ok, I'll print "Attempt to take address of a
synthetic reference" at value_addr, and move the check inside the
TYPE_CODE_REF path.

>> I haven't been able to test this so far because the test requires an
>> x86-like target, which I don't have at hand. I'll keep you posted,
>> though.
>
> Do you have an x86-64 box?  If so, you can just run the test with:
>
>  make check RUNTESTFLAGS="--target_board=unix/-m32 gdb.dwarf2/implptr.exp"

That worked alright. Is this documented somewhere?

>> Assuming gdb crashes there too, couldn't I just add a "print &j" test
>> there and discard the synthetic-pointer-reference.exp test altogether?
>
> Actually from your description, I assume it _doesn't_ crash, because it's
> not a reference.  But it's not tested presently, so who know.  I think it's
> worth it to have both pointers and references covered.

You're right, it doesn't crash. I'll still add a test to implptr.exp
to catch the "Can't take address of foo which isn't an lvalue"
message, though.

About not using printf inside synthetic-pointer-reference.cc, is there
any way to make sure gcc won't optimize 'arg' out completely other
than that? Declaring it as volatile doesn't seem to work.
  

On 06/28/2015 06:05 AM, Martin Galvan wrote:
> On Wed, Jun 24, 2015 at 8:13 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/20/2015 04:25 AM, Martin Galvan wrote:
>>> Yes, 'function' ends up inlined. If we try to set a breakpoint at a
>>> line inside 'function', the breakpoint will be set at an address we'll
>>> never hit, thus we need to advance using 'step' and 'next' instead.
>>
>> Then it sounds like you were putting a breakpoint at a line that was
>> optimized out.  If you put the breakpoint at the same line that you
>> "step" and "next" to, then it should work, I think.  You'll still
>> need a "step" to go from real stack frame to inline frame, as GDB always
>> presents you the stack frame on breakpoint hits.
> 
> That's what I tried at the beginning. However, if I set the breakpoint
> at the line I step to, using "continue" will either take me to an
> address after the one I want to stop at, or just finish the program.
> 
>> Hmm, that looks too long to me and still escaping out a detail
>> that isn't really that interesting to the user.  I was just thinking:
>>
>> if (is reference)
>>  "Attempt to take address of a synthetic reference."
>> else
>>  "Attempt to take address of a synthetic pointer."
> 
> The case on which the variable is an actual synthetic pointer (as
> opposed to a synthetic reference) doesn't seem to reach value_addr;
> instead it prints the "Can't take address of foo which isn't an
> lvalue" message from address_of_variable.
> 
> If you think it's ok, I'll print "Attempt to take address of a
> synthetic reference" at value_addr, and move the check inside the
> TYPE_CODE_REF path.

Sounds OK.

> 
>>> I haven't been able to test this so far because the test requires an
>>> x86-like target, which I don't have at hand. I'll keep you posted,
>>> though.
>>
>> Do you have an x86-64 box?  If so, you can just run the test with:
>>
>>  make check RUNTESTFLAGS="--target_board=unix/-m32 gdb.dwarf2/implptr.exp"
> 
> That worked alright. Is this documented somewhere?

Not sure.  I'd assume the dejagnu manual would say something about it,
but I if it's there, I can't find it right now.

Yao wrote a blog post about this a while ago:

 https://yaoqi.wordpress.com/2014/09/01/powerful-testing-with-dejagnu/

Guess it would be good to add something to:

 https://sourceware.org/gdb/wiki/TestingGDB


> About not using printf inside synthetic-pointer-reference.cc, is there
> any way to make sure gcc won't optimize 'arg' out completely other
> than that? Declaring it as volatile doesn't seem to work.

Odd.

But looking again, I see that all of implptr-optimized-out.exp,
implptrpiece.exp and implptrconst.exp already use the Dwarf::assemble
machinery to exercise  GNU_implicit_pointer related things.
It's probably not that hard to base a reference test case on one
of those.  I'd probably pick implptr-optimized-out.exp.
Could you give that a try?

The main advantage of the Dwarf::assemble machinery is that
then the test is both target and compiler independent.

Thanks,
Pedro Alves