Re: [PATCH 2/3] Check data is GC'ed

[Doug Evans <dje@google.com>]

Hi. Comments inline.

Yao Qi writes:
 > We see the following fail on arm-none-eabi target,
 > 
 > (gdb) print &var^M
 > $1 = (int *) 0x0 <_ftext>^M
 > (gdb) FAIL: gdb.dwarf2/dw2-var-zero-addr.exp: print &var
 > 
 > Test dw2-var-zero-addr.exp is intended to test whether GDB can
 > correctly know variable var is GC'ed by linker.  Currently, the
 > heuristic GDB is using is (see add_partial_symbol)
 > 
 >  	  && addr == 0
 > 	  && !dwarf2_per_objfile->has_section_at_zero
 > 
 > however, it doesn't work for bare metal targets, on which certain
 > section is located at address zero.  In this patch, we improve the
 > heuristic that if the variable address is zero, and section at address
 > zero is executable, we think the variable is GC'ed by linker, because
 > there can't be a variable in an executable section.  In order to know
 > this, we replace flag has_section_at_zero with a pointer
 > section_at_zero.
 > 
 > gdb:
 > 
 > 2014-08-20  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (struct dwarf2_per_objfile) <has_section_at_zero>:
 > 	Remove.
 > 	<section_at_zero>: New field.  Callers update.
 > 	(add_partial_symbol): Extend the condition to check whether the
 > 	section at zero is executable.
 > 	(new_symbol_full): Check whether the section at zero is
 > 	executable.
 > ---
 >  gdb/dwarf2read.c | 32 +++++++++++++++++++-------------
 >  1 file changed, 19 insertions(+), 13 deletions(-)
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index cf2ce76..f5b6341 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -278,9 +278,8 @@ struct dwarf2_per_objfile
 >       original data was compressed using 'dwz -m'.  */
 >    struct dwz_file *dwz_file;
 >  
 > -  /* A flag indicating wether this objfile has a section loaded at a
 > -     VMA of 0.  */
 > -  int has_section_at_zero;
 > +  /* The section of this objfile loaded at a VMA of 0.  */
 > +  asection *section_at_zero;

If overlays are in use, could multiple sections be at zero?
[I think so, but I haven't used overlays in awhile.]
Storing a vector of sections_at_zero seems like massive
overkill to solve this problem, maybe there's a simpler solution,
I don't know.  But I'm not opposed to it if it's the only
solution we have at the moment.

 >  
 >    /* True if we are using the mapped index,
 >       or we are faking it for OBJF_READNOW's sake.  */
 > @@ -2186,7 +2185,7 @@ dwarf2_locate_sections (bfd *abfd, asection *sectp, void *vnames)
 >  
 >    if ((bfd_get_section_flags (abfd, sectp) & SEC_LOAD)
 >        && bfd_section_vma (abfd, sectp) == 0)
 > -    dwarf2_per_objfile->has_section_at_zero = 1;
 > +    dwarf2_per_objfile->section_at_zero = sectp;
 >  }
 >  
 >  /* A helper function that decides whether a section is empty,
 > @@ -6851,7 +6850,13 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 >  
 >        if (pdi->d.locdesc
 >  	  && addr == 0
 > -	  && !dwarf2_per_objfile->has_section_at_zero)
 > +	  /* When the address is 0, if the object file doesn't have
 > +	     section at zero or the section at zero is executable,
 > +	     we think address 0 means the corresponding variable is
 > +	     removed by linker, instead of there is a data at address
 > +	     0.  */
 > +	  && (dwarf2_per_objfile->section_at_zero == NULL
 > +	      || dwarf2_per_objfile->section_at_zero->flags & SEC_CODE))

Could there be a DW_TAG_variable in SEC_CODE?
Someone might put one there for a particular reason.

 >  	{
 >  	  /* A global or static variable may also have been stripped
 >  	     out by the linker if unused, in which case its address
 > @@ -7341,8 +7346,8 @@ dwarf2_read_symtab (struct partial_symtab *self,
 >  	    = objfile_data (objfile->separate_debug_objfile_backlink,
 >  			    dwarf2_objfile_data_key);
 >  
 > -	  dwarf2_per_objfile->has_section_at_zero
 > -	    = dpo_backlink->has_section_at_zero;
 > +	  dwarf2_per_objfile->section_at_zero
 > +	    = dpo_backlink->section_at_zero;
 >  	}
 >  
 >        dwarf2_per_objfile->reading_partial_symbols = 0;
 > @@ -11758,7 +11763,7 @@ dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
 >        /* A not-uncommon case of bad debug info.
 >  	 Don't pollute the addrmap with bad data.  */
 >        if (range_beginning + baseaddr == 0
 > -	  && !dwarf2_per_objfile->has_section_at_zero)
 > +	  && !dwarf2_per_objfile->section_at_zero)

Convention is to write "ptr == NULL" instead of "!ptr".

 >  	{
 >  	  complaint (&symfile_complaints,
 >  		     _(".debug_ranges entry has start address of zero"
 > @@ -11871,7 +11876,7 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
 >       labels are not in the output, so the relocs get a value of 0.
 >       If this is a discarded function, mark the pc bounds as invalid,
 >       so that GDB will ignore it.  */
 > -  if (low == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +  if (low == 0 && !dwarf2_per_objfile->section_at_zero)
 >      return 0;
 >  
 >    *lowpc = low;
 > @@ -12095,7 +12100,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 >  
 >  	      /* A not-uncommon case of bad debug info.
 >  		 Don't pollute the addrmap with bad data.  */
 > -	      if (start == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +	      if (start == 0 && !dwarf2_per_objfile->section_at_zero)
 >  		{
 >  		  complaint (&symfile_complaints,
 >  			     _(".debug_ranges entry has start address of zero"
 > @@ -15668,7 +15673,7 @@ read_partial_die (const struct die_reader_specs *reader,
 >  	 labels are not in the output, so the relocs get a value of 0.
 >  	 If this is a discarded function, mark the pc bounds as invalid,
 >  	 so that GDB will ignore it.  */
 > -      if (part_die->lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +      if (part_die->lowpc == 0 && !dwarf2_per_objfile->section_at_zero)
 >  	{
 >  	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
 >  
 > @@ -17297,7 +17302,7 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >  		    pst = cu->per_cu->v.psymtab;
 >  
 >  		  if (address == 0
 > -		      && (!dwarf2_per_objfile->has_section_at_zero
 > +		      && (!dwarf2_per_objfile->section_at_zero
 >  			  || (pst != NULL && pst->textlow > address)))
 >  		    {
 >  		      /* This line table is for a function which has been
 > @@ -17870,7 +17875,8 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 >  
 >  	      if (SYMBOL_CLASS (sym) == LOC_STATIC
 >  		  && SYMBOL_VALUE_ADDRESS (sym) == 0
 > -		  && !dwarf2_per_objfile->has_section_at_zero)
 > +		  && (!dwarf2_per_objfile->section_at_zero
 > +		      || (dwarf2_per_objfile->section_at_zero->flags & SEC_CODE)))

The SEC_CODE test doesn't feel right.
I understand the intent, but someone might put a variable in .text
for a reason.

 >  		{
 >  		  /* When a static variable is eliminated by the linker,
 >  		     the corresponding debug information is not stripped
 > -- 
 > 1.9.3
 >