Re: [PATCH 1/3] Check function is GC'ed

[Doug Evans <dje at google dot com>]

Yao Qi writes:
 > Doug Evans <dje@google.com> writes:
 > 
 > > I'd like to solve this for both partial syms and .gdb_index.
 > >
 > > We want to, essentially, discard the entry if address is outside the
 > > range of the cu (the range needn't be contiguous, but for the task
 > > at hand I don't think it matters).  I haven't dug into this too deeply,
 > > but if we have lowpc (which for example is that read_file_scope has
 > > before it calls handle_DW_AT_stmt_list (which calls dwarf_decode_lines),
 > > can we use that in the test?  Can we arrange for all callers of
 > > dwarf_decode_lines_1 pass lowpc down to it? [Or both lowpc,highpc
 > > for a more complete test, but for the task at hand we've only been
 > > checking, effectively, lowpc all along so I don't mind leaving it at that
 > > for now.]
 > 
 > Hi Doug,
 > Patch below implemented what you suggested, if I understand you correct
 > :)
 > 
 > A new argument 'lowpc' is added to both dwarf2_build_include_psymtabs
 > and handle_DW_AT_stmt_list, to keep the similarity between them.  I
 > thought about merging them into one function, but still need more
 > thinking.
 > 
 > -- 
 > Yao (  )
 > 
 > Subject: [PATCH] Check function is GC'ed
 > 
 > I see the following fail on arm-none-eabi target,
 > 
 > (gdb) b 24^M
 > Breakpoint 1 at 0x4: file
 > ../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
 > line 24.^M
 > (gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24
 > 
 > Currently, we are using flag has_section_at_zero to determine whether
 > address zero in debug info means the corresponding code has been
 > GC'ed, like this:
 > 
 > 	case DW_LNE_set_address:
 > 	  address = read_address (abfd, line_ptr, cu, &bytes_read);
 > 
 > 	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > 	    {
 > 	      /* This line table is for a function which has been
 > 		 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > 
 > However, this is incorrect on some bare metal targets, as .text
 > section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
 > is true.  If a function is GC'ed by linker, the address is zero.  GDB
 > thinks address zero is a function's address rather than this function
 > is GC'ed.
 > 
 > In this patch, we choose 'lowpc' got in read_file_scope to check
 > whether 'lowpc' is greater than zero.  If it isn't, address zero really
 > means the function is GC'ed.  In this patch, we pass 'lowpc' in
 > read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
 > and to dwarf_decode_lines_1 finally.
 > 
 > This patch fixes the fail above. This patch also covers the path that
 > partial symbol isn't used, which is tested by starting gdb with
 > --readnow option.
 > 
 > It is regression tested on x86-linux with
 > target_board=dwarf4-gdb-index, and arm-none-eabi.  OK to apply?
 > 
 > gdb:
 > 
 > 2014-08-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (dwarf_decode_lines): Update declaration.
 > 	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
 > 	comments.  Callers update.
 > 	(dwarf_decode_lines): Likewise.
 > 	(dwarf2_build_include_psymtabs): Likewise.
 > 	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
 > 	comments.  Skip the line table if  'lowpc' is greater than
 > 	'address'.
 > 
 > gdb/testsuite:
 > 
 > 2014-08-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
 > 	proc set_breakpoint_on_gcd_function.  Invoke
 > 	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
 > 	invoke set_breakpoint_on_gcd_function again.

Hi.
Thanks for the ping.

 > ---
 >  gdb/dwarf2read.c                                   | 35 ++++++++++++++--------
 >  .../gdb.base/break-on-linker-gcd-function.exp      | 24 +++++++++++----
 >  2 files changed, 41 insertions(+), 18 deletions(-)
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index be32309..8fdae74 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -1512,7 +1512,8 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 >  						     struct dwarf2_cu *cu);
 >  
 >  static void dwarf_decode_lines (struct line_header *, const char *,
 > -				struct dwarf2_cu *, struct partial_symtab *);
 > +				struct dwarf2_cu *, struct partial_symtab *,
 > +				CORE_ADDR);
 >  
 >  static void dwarf2_start_subfile (const char *, const char *, const char *);
 >  
 > @@ -4436,7 +4437,8 @@ dwarf2_create_include_psymtab (const char *name, struct partial_symtab *pst,
 >  static void
 >  dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
 >  			       struct die_info *die,
 > -			       struct partial_symtab *pst)
 > +			       struct partial_symtab *pst,
 > +			       CORE_ADDR lowpc)
 >  {
 >    struct line_header *lh = NULL;
 >    struct attribute *attr;
 > @@ -4448,7 +4450,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
 >      return;  /* No linetable, so no includes.  */
 >  
 >    /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
 > -  dwarf_decode_lines (lh, pst->dirname, cu, pst);
 > +  dwarf_decode_lines (lh, pst->dirname, cu, pst, lowpc);

We don't really need a "lowpc" argument to dwarf2_build_include_psymtabs.
Replace that with:

  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);

and remove the lowpc argument to dwarf2_build_include_psymtabs.

 >  
 >    free_line_header (lh);
 >  }
 > @@ -5954,7 +5956,7 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
 >  
 >    /* Get the list of files included in the current compilation unit,
 >       and build a psymtab for each of them.  */
 > -  dwarf2_build_include_psymtabs (cu, comp_unit_die, pst);
 > +  dwarf2_build_include_psymtabs (cu, comp_unit_die, pst, pst->textlow);
 >  
 >    if (dwarf2_read_debug)
 >      {
 > @@ -8967,11 +8969,12 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
 >  
 >  /* Handle DW_AT_stmt_list for a compilation unit.
 >     DIE is the DW_TAG_compile_unit die for CU.
 > -   COMP_DIR is the compilation directory.  */
 > +   COMP_DIR is the compilation directory.  LOWPC is passed to
 > +   dwarf_decode_lines.  See dwarf_decode_lines comments about it.  */
 >  
 >  static void
 >  handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 > -			const char *comp_dir) /* ARI: editCase function */
 > +			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 >  {
 >    struct attribute *attr;
 >  
 > @@ -8988,7 +8991,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 >  	{
 >  	  cu->line_header = line_header;
 >  	  make_cleanup (free_cu_line_header, cu);
 > -	  dwarf_decode_lines (line_header, comp_dir, cu, NULL);
 > +	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
 >  	}
 >      }
 >  }
 > @@ -9039,7 +9042,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
 >    /* Decode line number information if present.  We do this before
 >       processing child DIEs, so that the line header table is available
 >       for DW_AT_decl_file.  */
 > -  handle_DW_AT_stmt_list (die, cu, comp_dir);
 > +  handle_DW_AT_stmt_list (die, cu, comp_dir, lowpc);
 >  
 >    /* Process all dies in compilation unit.  */
 >    if (die->child != NULL)
 > @@ -17252,7 +17255,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 >  
 >  static void
 >  dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 > -		      struct dwarf2_cu *cu, const int decode_for_pst_p)
 > +		      struct dwarf2_cu *cu, const int decode_for_pst_p,
 > +		      CORE_ADDR lowpc)
 >  {
 >    const gdb_byte *line_ptr, *extended_end;
 >    const gdb_byte *line_end;
 > @@ -17375,7 +17379,9 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >  		case DW_LNE_set_address:
 >  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 >  
 > -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +		  if (address == 0
 > +		      && (!dwarf2_per_objfile->has_section_at_zero
 > +			  || lowpc > address))

I'm trying to decide whether to keep the
"!dwarf2_per_objfile->has_section_at_zero"
test here.  It feels wrong to keep it: What does it matter
whether any section has address zero?  It was only used as a proxy
for a better test.  Here we know we have an address
that is outside the CU in question, which is a far more specific test
than just checking whether any section is at address zero.
But maybe I'm missing something.

One could even argue that the "address == 0" test is also
now superfluous.

IOW, I'm wondering if we could just write the following here:

		  if (lowpc > address)

But if we write it that way then the code no longer readily expresses
what we're trying to do here which is handle the specific case expressed by
the comment that follows: "This line table is for a function which has been
GCd by the linker."  Instead we'd be now testing for a more general
case which would include bad debug info.

What do you think?

I'm leaning towards not changing things too much in this patch
and only handling GC'd functions.  Therefore I'm leaning towards
something like the following:

		  /* If address < lowpc then it's not a usable value, it's
		     outside the pc range of the CU.  However, we restrict
		     the test to only address values of zero to preserve
		     GDB's previous behaviour which is to handle the specific
		     case of a function being GC'd by the linker.  */
		  if (address == 0 && address < lowpc)

 >  		    {
 >  		      /* This line table is for a function which has been
 >  			 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > @@ -17595,17 +17601,20 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >     as the corresponding symtab.  Since COMP_DIR is not used in the name of the
 >     symtab we don't use it in the name of the psymtabs we create.
 >     E.g. expand_line_sal requires this when finding psymtabs to expand.
 > -   A good testcase for this is mb-inline.exp.  */
 > +   A good testcase for this is mb-inline.exp.
 > +
 > +   LOWPC is the lowest address in CU (or 0 if not known).  */
 >  
 >  static void
 >  dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 > -		    struct dwarf2_cu *cu, struct partial_symtab *pst)
 > +		    struct dwarf2_cu *cu, struct partial_symtab *pst,
 > +		    CORE_ADDR lowpc)
 >  {
 >    struct objfile *objfile = cu->objfile;
 >    const int decode_for_pst_p = (pst != NULL);
 >    struct subfile *first_subfile = current_subfile;
 >  
 > -  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p);
 > +  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 >  
 >    if (decode_for_pst_p)
 >      {
 > diff --git a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
 > index e593b51..b8de1cd 100644
 > --- a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
 > +++ b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
 > @@ -41,9 +41,23 @@ if {[build_executable_from_specs $testfile.exp $testfile \
 >  
 >  clean_restart $testfile
 >  
 > -# Single hex digit
 > -set xd {[0-9a-f]}
 > +proc set_breakpoint_on_gcd_function {} {
 > +    # Single hex digit
 > +    set xd {[0-9a-f]}
 > +
 > +    # This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
 > +    # but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
 > +    gdb_test "b [gdb_get_line_number "gdb break here"]" \
 > +	"Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
 > +}
 > +
 > +set_breakpoint_on_gcd_function
 >  
 > -# This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
 > -# but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
 > -gdb_test "b [gdb_get_line_number "gdb break here"]" "Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
 > +set saved_gdbflags $GDBFLAGS
 > +set GDBFLAGS "$GDBFLAGS --readnow"
 > +clean_restart ${testfile}
 > +set GDBFLAGS $saved_gdbflags
 > +
 > +with_test_prefix "readnow" {
 > +    set_breakpoint_on_gcd_function
 > +}