Re: [PATCH] Fix range end handling of inlined subroutines

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-02-09 21:07:32 +0000]:

> This patch itself is preliminary, since the is-stmt patch
> needs to be rebased after the refactoring of
> dwarf2read.c from yesterday, so I will have to rebase
> this patch as well, but decided to wait for Andrew.

I've been busy with other projects for the last few days, but plan to
look at this series tomorrow.

Thanks,
Andrew


> 
> 
> Thanks
> Bernd.

> From d15f3346feb1ed5cbbc14e708f3f6b58d88bc0fe Mon Sep 17 00:00:00 2001
> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Date: Sun, 9 Feb 2020 21:13:17 +0100
> Subject: [PATCH] Fix range end handling of inlined subroutines
> 
> Since the is_stmt is now handled, it became
> possible to locate dubious is_stmt line entries
> at the end of an inlined function, in the same
> subfile.
> 
> If any is-stmt line is followed by a non-is-stmt,
> at the same PC as the inlined subroutine ends,
> clear the is-stmt there (except the line = 0
> end marker, of course).
> 
> This is about the best we can do at the moment,
> unless location view information are added to the
> block ranges debug info structure, and location
> views are implemented in gdb in general.
> 
> gdb:
> 2020-02-09  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* buildsym.c (buildsym_compunit::record_inline_range_end):
> 	New function.
> 	* buildsym.h (buildsym_compunit::record_inline_range_end): Declare.
> 	* dwarf2read.c (dwarf2_rnglists_process,
> 	dwarf2_ranges_process): Don't ignore empty ranges here.
> 	(dwarf2_ranges_read): Ignore empty ranges here.
> 	(dwarf2_record_block_ranges): Pass end of range PC to
> 	record_inline_range_end for inline functions.
> 
> gdb/testsuite:
> 2020-02-09  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* gdb.cp/next-inline.exp: Adjust test.
> ---
>  gdb/buildsym.c                       | 23 +++++++++++++++++++++++
>  gdb/buildsym.h                       |  2 ++
>  gdb/dwarf2read.c                     | 22 +++++++++++++---------
>  gdb/testsuite/gdb.cp/next-inline.exp |  9 ---------
>  4 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index 7fd256f..381a7c8 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -732,6 +732,29 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
>  }
>  
>  \f
> +/* Record a PC where a inlined subroutine ends.  */
> +
> +void
> +buildsym_compunit::record_inline_range_end (CORE_ADDR end)
> +{
> +  struct subfile *subfile;
> +
> +  for (subfile = m_subfiles; subfile != NULL; subfile = subfile->next)
> +    {
> +      if (subfile->line_vector != NULL)
> +	{
> +	  struct linetable_entry *items = subfile->line_vector->item;
> +	  int i;
> +
> +	  for (i = subfile->line_vector->nitems - 1; i > 0; i--)
> +	    if (items[i].pc == end && items[i - 1].pc == end
> +		&& !items[i].is_stmt && items[i - 1].line != 0)
> +	      items[i - 1].is_stmt = 0;
> +	}
> +    }
> +}
> +
> +\f
>  /* Subroutine of end_symtab to simplify it.  Look for a subfile that
>     matches the main source file's basename.  If there is only one, and
>     if the main source file doesn't have any symbol or line number
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index c768a4c..3cf0f8b 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -190,6 +190,8 @@ struct buildsym_compunit
>    void record_line (struct subfile *subfile, int line, CORE_ADDR pc,
>  		    bool is_stmt);
>  
> +  void record_inline_range_end (CORE_ADDR end);
> +
>    struct compunit_symtab *get_compunit_symtab ()
>    {
>      return m_compunit_symtab;
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index cbee4ab..6919bf1 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -14407,10 +14407,6 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
>  	  return false;
>  	}
>  
> -      /* Empty range entries have no effect.  */
> -      if (range_beginning == range_end)
> -	continue;
> -
>        range_beginning += base;
>        range_end += base;
>  
> @@ -14521,10 +14517,6 @@ dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu,
>  	  return 0;
>  	}
>  
> -      /* Empty range entries have no effect.  */
> -      if (range_beginning == range_end)
> -	continue;
> -
>        range_beginning += base;
>        range_end += base;
>  
> @@ -14564,6 +14556,10 @@ dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
>    retval = dwarf2_ranges_process (offset, cu,
>      [&] (CORE_ADDR range_beginning, CORE_ADDR range_end)
>      {
> +      /* Empty range entries have no effect.  */
> +      if (range_beginning == range_end)
> +	return;
> +
>        if (ranges_pst != NULL)
>  	{
>  	  CORE_ADDR lowpc;
> @@ -14801,6 +14797,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>    struct gdbarch *gdbarch = get_objfile_arch (objfile);
>    struct attribute *attr;
>    struct attribute *attr_high;
> +  bool inlined_subroutine = (die->tag == DW_TAG_inlined_subroutine);
>  
>    attr_high = dwarf2_attr (die, DW_AT_high_pc, cu);
>    if (attr_high)
> @@ -14816,7 +14813,10 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>  
>  	  low = gdbarch_adjust_dwarf2_addr (gdbarch, low + baseaddr);
>  	  high = gdbarch_adjust_dwarf2_addr (gdbarch, high + baseaddr);
> -	  cu->get_builder ()->record_block_range (block, low, high - 1);
> +	  if (inlined_subroutine)
> +	    cu->get_builder ()->record_inline_range_end (high);
> +	  if (low < high)
> +	    cu->get_builder ()->record_block_range (block, low, high - 1);
>          }
>      }
>  
> @@ -14841,6 +14841,10 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>  	  end += baseaddr;
>  	  start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
>  	  end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
> +	  if (inlined_subroutine)
> +	    cu->get_builder ()->record_inline_range_end (end);
> +	  if (start == end)
> +	    return;
>  	  cu->get_builder ()->record_block_range (block, start, end - 1);
>  	  blockvec.emplace_back (start, end);
>  	});
> diff --git a/gdb/testsuite/gdb.cp/next-inline.exp b/gdb/testsuite/gdb.cp/next-inline.exp
> index 0b2b22d..11d9e2e 100644
> --- a/gdb/testsuite/gdb.cp/next-inline.exp
> +++ b/gdb/testsuite/gdb.cp/next-inline.exp
> @@ -42,24 +42,15 @@ proc do_test { use_header } {
>      gdb_test "step" ".*" "step into get_alias_set"
>      gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
>  	"not in inline 1"
> -    # It's possible that this first failure (when not using a header
> -    # file) is GCC's fault, though the remaining failures would best
> -    # be fixed by adding location views support (though it could be
> -    # that some easier heuristic could be figured out).  Still, it is
> -    # not certain that the first failure wouldn't also be fixed by
> -    # having location view support, so for now it is tagged as such.
> -    if {!$use_header} { setup_kfail "*-*-*" symtab/25507 }
>      gdb_test "next" ".*TREE_TYPE.*" "next step 1"
>      gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
>  	"not in inline 2"
>      gdb_test "next" ".*TREE_TYPE.*" "next step 2"
>      gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
>  	"not in inline 3"
> -    if {!$use_header} { setup_kfail "*-*-*" symtab/25507 }
>      gdb_test "next" ".*TREE_TYPE.*" "next step 3"
>      gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
>  	"not in inline 4"
> -    if {!$use_header} { setup_kfail "*-*-*" symtab/25507 }
>      gdb_test "next" "return 0.*" "next step 4"
>      gdb_test "bt" \
>  	"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
> -- 
> 1.9.1
>