diff mbox

[1/3] Check function is GC'ed

Message ID 87ioko1ec0.fsf@codesourcery.com
State New, archived
Headers

Commit Message

Yao Qi Sept. 16, 2014, 2:36 a.m. UTC 
  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.

Comments

Yao Qi Sept. 19, 2014, 9:04 a.m. UTC | #1 
Doug Evans <dje@google.com> writes:

>  > 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.

Thanks for the review.  Patch is pushed in.
diff mbox

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index be32309..9d0ee13 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 *);
 
@@ -4448,7 +4449,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, pst->textlow);
 
   free_line_header (lh);
 }
@@ -8967,11 +8968,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 +8990,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 +9041,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 +17254,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 +17378,12 @@  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 < 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 +17603,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
+}