Prevent inline function parameters from crashing the DWARF reader

  When reading DWARF information for an inlined function, new_symbol
assumes that the CU has a valid builder:

   case DW_TAG_formal_parameter:
     {
       /* If we are inside a function, mark this as an argument.  If
          not, we might be looking at an argument to an inlined function
          when we do not have enough information to show inlined frames;
          pretend it's a local variable in that case so that the user can
          still see it.  */
       struct context_stack *curr
         = cu->builder->get_current_context_stack ());
       if (curr != nullptr && curr->name != nullptr)
         SYMBOL_IS_ARGUMENT (sym) = 1;
       // ..

However, as demonstrated in this Fedora bugzilla:
  https://bugzilla.redhat.com/show_bug.cgi?id=1638798

the abstract_origin of the parameter DIE may be in an entirely different
CU.  This CU will have no builder defined for it, so the call to
get_current_context_stack is made on an uninitialized builder, and GDB
segfaults.

gdb/ChangeLog:
YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1638798
	* dwarf2read.c (new_symbol): Don't assume there is a valid
	builder when processing formal_parameter; it may have come
	from another CU.

gdb/testsuite/ChangeLog:
YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1638798
	* gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
---
 gdb/ChangeLog                                 |   7 +
 gdb/dwarf2read.c                              |   8 +-
 gdb/testsuite/ChangeLog                       |   5 +
 .../inline_subroutine-inheritance.exp         | 213 ++++++++++++++++++
 4 files changed, 232 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
  

On Friday, October 12 2018, Keith Seitz wrote:

> When reading DWARF information for an inlined function, new_symbol
> assumes that the CU has a valid builder:
>
>    case DW_TAG_formal_parameter:
>      {
>        /* If we are inside a function, mark this as an argument.  If
>           not, we might be looking at an argument to an inlined function
>           when we do not have enough information to show inlined frames;
>           pretend it's a local variable in that case so that the user can
>           still see it.  */
>        struct context_stack *curr
>          = cu->builder->get_current_context_stack ());
>        if (curr != nullptr && curr->name != nullptr)
>          SYMBOL_IS_ARGUMENT (sym) = 1;
>        // ..
>
> However, as demonstrated in this Fedora bugzilla:
>   https://bugzilla.redhat.com/show_bug.cgi?id=1638798
>
> the abstract_origin of the parameter DIE may be in an entirely different
> CU.  This CU will have no builder defined for it, so the call to
> get_current_context_stack is made on an uninitialized builder, and GDB
> segfaults.

Thanks for the patch, Keith.

Just FYI, <https://sourceware.org/bugzilla/show_bug.cgi?id=23773> has
just been filed (by a Fedora user).  You might want to assign it to
yourself and mention it in the ChangeLog.

> gdb/ChangeLog:
> YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
>
> 	https://bugzilla.redhat.com/show_bug.cgi?id=1638798
> 	* dwarf2read.c (new_symbol): Don't assume there is a valid
> 	builder when processing formal_parameter; it may have come
> 	from another CU.
>
> gdb/testsuite/ChangeLog:
> YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
>
> 	https://bugzilla.redhat.com/show_bug.cgi?id=1638798
> 	* gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
> ---
>  gdb/ChangeLog                                 |   7 +
>  gdb/dwarf2read.c                              |   8 +-
>  gdb/testsuite/ChangeLog                       |   5 +
>  .../inline_subroutine-inheritance.exp         | 213 ++++++++++++++++++
>  4 files changed, 232 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index b3cc64659f..18fcb671b4 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
> +
> +	https://bugzilla.redhat.com/show_bug.cgi?id=1638798
> +	* dwarf2read.c (new_symbol): Don't assume there is a valid
> +	builder when processing formal_parameter; it may have come
> +	from another CU.
> +
>  2018-10-11  Gary Benson <gbenson@redhat.com>
>  
>  	* interps.h (interp::m_name): Make private and mutable.
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 2a1b805c9a..c11b778e71 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -21565,8 +21565,14 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>  	       when we do not have enough information to show inlined frames;
>  	       pretend it's a local variable in that case so that the user can
>  	       still see it.  */
> +
> +	    /* If this inlined function parameter is inherited from another
> +	       CU, that CU may not have a builder associated with it.
> +	       Don't assume that it does.  */
>  	    struct context_stack *curr
> -	      = cu->builder->get_current_context_stack ();
> +	      = (cu->builder == nullptr
> +		 ? nullptr
> +		 : cu->builder->get_current_context_stack ());
>  	    if (curr != nullptr && curr->name != nullptr)
>  	      SYMBOL_IS_ARGUMENT (sym) = 1;
>  	    attr = dwarf2_attr (die, DW_AT_location, cu);
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index ba5fae99e7..9f5ab8f86a 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
> +
> +	https://bugzilla.redhat.com/show_bug.cgi?id=1638798
> +	* gdb.dwarf2/inline_subroutine-inheritance.exp: New file.
> +
>  2018-10-11  Sandra Loosemore  <sandra@codesourcery.com>
>  
>  	* gdb.base/solib-vanish.exp: Fix regexp not to require a POSIX
> diff --git a/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
> new file mode 100644
> index 0000000000..92c5d0529b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/inline_subroutine-inheritance.exp
> @@ -0,0 +1,213 @@
> +# Copyright 2018 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 tests a segfault that occurs when reading inline_subroutine DIEs
> +# with abstract_origins pointing to DIEs in other CUs.
> +#
> +# See https://bugzilla.redhat.com/show_bug.cgi?id=1638798 .
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF.
> +if {![dwarf2_support]} {
> +    return 0
> +}
> +
> +standard_testfile main.c .S
> +
> +# Create the DWARF.  This is derived from the reproducer in the Fedora
> +# bugzila mentioned above.  For clarity, some "superfluous" DIES have
> +# been left instead of simplifying/pruning the test further.
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    declare_labels Db D72f8 D736e
> +    declare_labels D266465 D266477 D266483 D266496 D266498 D266ad3 D266ad9 \
> +	D266ade D26b227 D26b237
> +    declare_labels D26d8b1 D26d8c3 D26d8cf D26d944 D26d946 D26e103 D26e145 \
> +	D26e415 D26e48c D26df00 D26df06 D26df0b D272519 D274c1a D274c42
> +
> +    cu {} {
> +	Db: compile_unit {
> +	    {language @DW_LANG_C99}
> +	    {name "<artificial>"}
> +	} {
> +	    D72f8: subprogram {
> +		{abstract_origin :$D272519}
> +		{low_pc 0xb9e20 addr}
> +		{high_pc 0x1f5 data4}
> +	    } {
> +		D736e: inlined_subroutine {
> +		    {abstract_origin :$D26b227}
> +		    {low_pc 0xb9efc addr}
> +		    {high_pc 0xc data4}
> +		} {
> +		    formal_parameter {
> +			{abstract_origin :$D274c42}
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +
> +    cu {} {
> +	D266465: compile_unit {
> +	    {language @DW_LANG_C99}
> +	} {
> +	    D266477: typedef {
> +		{name "size_t"}
> +		{type :$D266483}
> +	    }
> +
> +	    D266483: base_type {
> +		{byte_size 8 sdata}
> +		{encoding @DW_ATE_unsigned}
> +	    }
> +
> +	    D266496: pointer_type {
> +		{byte_size 8 sdata}
> +	    }
> +
> +	    D266498: restrict_type {
> +		{type :$D266496}
> +	    }
> +
> +	    D266ad3: pointer_type {
> +		{byte_size 8 sdata}
> +		{type :$D266ade}
> +	    }
> +
> +	    D266ad9: restrict_type {
> +		{type :$D266ad3}
> +	    }
> +
> +	    D266ade: const_type {}
> +
> +	    D26b227: subprogram {
> +		{external 1 flag}
> +		{name "memcpy"}
> +		{type :$D266496}
> +	    } {
> +		D26b237: formal_parameter {
> +		    {name "__dest"}
> +		    {type :$D266498}
> +		}
> +		formal_parameter {
> +		    {name "__src"}
> +		    {type :$D266ad9}
> +		}
> +		formal_parameter {
> +		    {name "__len"}
> +		    {type :$D266477}
> +		}
> +	    }
> +	}
> +    }
> +
> +    cu {} {
> +	D26d8b1: compile_unit {
> +	    {language @DW_LANG_C99}
> +	} {
> +	    D26d8c3: typedef {
> +		{name "size_t"}
> +		{type :$D26d8cf}
> +	    }
> +
> +	    D26d8cf: base_type {
> +		{byte_size 8 sdata}
> +		{encoding @DW_ATE_unsigned}
> +		{name "long unsigned int"}
> +	    }
> +
> +	    D26d944: pointer_type {
> +		{byte_size 8 sdata}
> +	    }
> +
> +	    D26d946: restrict_type {
> +		{type :$D26d944}
> +	    }
> +
> +	    D26e103: structure_type {
> +		{name "__object"}
> +		{byte_size 12 sdata}
> +	    } {
> +		member {
> +		    {name "__ob_next"}
> +		    {type :$D26e145}
> +		    {data_member_location 0 sdata}
> +		}
> +	    }
> +
> +	    D26e145: pointer_type {
> +		{byte_size 8 sdata}
> +		{type :$D26e103}
> +	    }
> +
> +	    D26e415: typedef {
> +		{name "PyObject"}
> +		{type :$D26e103}
> +	    }
> +
> +	    D26e48c: pointer_type {
> +		{byte_size 8 sdata}
> +		{type :$D26e415}
> +	    }
> +
> +	    D26df00: pointer_type {
> +		{byte_size 8 sdata}
> +		{type :$D26df0b}
> +	    }
> +
> +	    D26df06: restrict_type {
> +		{type :$D26df00}
> +	    }
> +
> +	    D26df0b: const_type {}
> +
> +	    D272519: subprogram {
> +		{name "bytes_repeat"}
> +		{type :$D26e48c}
> +	    }
> +
> +	    D274c1a: subprogram {
> +		{external 1 flag}
> +		{name "memcpy"}
> +		{type :$D26d944}
> +	    } {
> +		formal_parameter {
> +		    {name "__dest"}
> +		    {type :$D26d946}
> +		}
> +		formal_parameter {
> +		    {name "__src"}
> +		    {type :$D26df06}
> +		}
> +		D274c42: formal_parameter {
> +		    {name "__len"}
> +		    {type :$D26d8c3}
> +		}
> +	    }
> +	}
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile \
> +	 "${asm_file} ${srcfile}" {}]} {
> +    return -1
> +}
> +
> +# All we need to do is set a breakpoint, which causes the DWARF
> +# info to be read, to demonstrate the problem.
> +
> +gdb_breakpoint "bytes_repeat" message
> -- 
> 2.17.1
  

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> However, as demonstrated in this Fedora bugzilla:
Keith>   https://bugzilla.redhat.com/show_bug.cgi?id=1638798

Keith> the abstract_origin of the parameter DIE may be in an entirely different
Keith> CU.  This CU will have no builder defined for it, so the call to
Keith> get_current_context_stack is made on an uninitialized builder, and GDB
Keith> segfaults.

I was curious what happened before, so I checked out the commit
before 804d27291b2922f01fe5cc2d9ae759533d4a2f6f, and ran it on the
executable in that bug.

I set a breakpoint conditional on curr==0 and it never hit.

Now, I am not totally sure that this was correct before, but I suppose
what was going on was that the earlier code was installing these symbols
into the referencing CU's symtab.

Abstractly, at least, that seems reasonable.  So maybe that approach
should be taken again.

Also, when I look at new_symbol, I see a bunch of uses of cu->builder,
so I wonder if there are ways to make those spots crash as well.
So maybe this indicates some different approach is needed as well.

Sorry about this.  I didn't anticipate this at all.  I can try to take
it over if you'd like.

Maybe one idea would be to temporarily set the referenced CU's builder
to the referencing CU's builder somehow.

Tom
  

On 10/16/18 3:15 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> However, as demonstrated in this Fedora bugzilla:
> Keith>   https://bugzilla.redhat.com/show_bug.cgi?id=1638798
> 
> Keith> the abstract_origin of the parameter DIE may be in an entirely different
> Keith> CU.  This CU will have no builder defined for it, so the call to
> Keith> get_current_context_stack is made on an uninitialized builder, and GDB
> Keith> segfaults.
> 
> I was curious what happened before, so I checked out the commit
> before 804d27291b2922f01fe5cc2d9ae759533d4a2f6f, and ran it on the
> executable in that bug.
> 
> I set a breakpoint conditional on curr==0 and it never hit.
> 

What ends up happening, IIRC, is that we simply only ever checked (previously)
the depth level. It was 0.

> Now, I am not totally sure that this was correct before, but I suppose
> what was going on was that the earlier code was installing these symbols
> into the referencing CU's symtab.

Yeah, I was wondering about something like that, too, but had convinced myself
temporarily that this was appropriate. The immediate problem is that the
formal_parameter DIE lives in a partial_unit in its own CU.

After troubleshooting other similar problems, I'm now not so sure. [I
was coming around to another (better?) solution, see below.]

> Abstractly, at least, that seems reasonable.  So maybe that approach
> should be taken again.
> 
> Also, when I look at new_symbol, I see a bunch of uses of cu->builder,
> so I wonder if there are ways to make those spots crash as well.
> So maybe this indicates some different approach is needed as well.

It's not just new_symbol. It happens in other places, too, like
read_import_statement (where it calls using_directives).

> Sorry about this.  I didn't anticipate this at all.  I can try to take
> it over if you'd like.

So far, I'm still investigating, but from the Fedora side -- which seems
like it may be in really bad shape right now for some reason.

> Maybe one idea would be to temporarily set the referenced CU's builder
> to the referencing CU's builder somehow.

Yeah, that is an approach not too far off (I think?) from what I was
attempting: When we "import" DIEs from other CUs, I add them to an
sort of CU inheritance chain. Then we crawl up the chain looking for the
last valid builder. [This can happen frequently with DW_TAG_partial_unit
DIEs in the debuginfo.]

If I need more help, I will certainly let you know.

Let's consider this patch shelved for the moment.

Keith