This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Yao Qi writes:
 > Doug Evans <dje@google.com> writes:
 > 
 > Thanks for the review, Doug.
 > 
 > >  >    /* 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.
 > >
 > 
 > That is fine to me.  The reason I add "lowpc" argument to
 > dwarf2_build_include_psymtabs is that I'd like to keep the similarity
 > between dwarf2_build_include_psymtabs and handle_DW_AT_stmt_list.
 > 
 > >  > @@ -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.
 > 
 > I thought about this when I wrote the patch.  I keep it in order to
 > preserve GDB's behaviour.  It should be OK to remove it.
 > 
 > >
 > > 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)
 > 
 > I agree with you on this.  This patch is to fix a GDB bug when a
 > function is GC'ed by linker, we can concentrate on this at first.  We
 > can remove "address == 0" if it fixes some bugs we find in the future.
 > 
 > Patch below is updated to address your comments above.
 > 
 > -- 
 > Yao (  )
 > 
 > From: Yao Qi <yao@codesourcery.com>
 > 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-09-16  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.
 > 	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
 > 	comments.  Skip the line table if  'lowpc' is greater than
 > 	'address'.  Don't check
 > 	dwarf2_per_objfile->has_section_at_zero.
 > 
 > gdb/testsuite:
 > 
 > 2014-09-16  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.

LGTM.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]