[patch] cleanup: Remove INVALID_ENTRY_POINT (+FR-V modification)

[patch] cleanup: Remove INVALID_ENTRY_POINT (+FR-V modification)

[Jan Kratochvil]

Hi,

this patch has no (longer) relationship to other patches of mine, I just
thought when I accidentally wrote it it is worth to submit this cleanup.

CORE_ADDR width is now a bit problematic in GDB builds supporting for 32bit
and 64bit targets (to be described elsewhere).  Therefore INVALID_ENTRY_POINT
caught my eye as it should have type `CORE_ADDR' but it is `int'.  Which still
works in common cases due to automatic signed extension but it looks
dangerous.

I do not have FR-V to test, enable_break has been slightly updated over the
point of using new EI.ENTRY_POINT_P.

Regression tested on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


2009-10-31  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Remove INVALID_ENTRY_POINT.
	* objfiles.c (init_entry_point_info): Stop using INVALID_ENTRY_POINT.
	Initialize EI.ENTRY_POINT_P.
	(entry_point_address, objfile_relocate): Use EI.ENTRY_POINT_P.
	* objfiles.h (struct entry_info): Simplify entry_point comment.  New
	field entry_point_p.
	(INVALID_ENTRY_POINT): Remove.
	* solib-frv.c (enable_break): Check for NULL SYMFILE_OBJFILE and its
	EI.ENTRY_POINT_P.  Return 0 if ".interp" is not found.

--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -294,17 +294,21 @@ init_entry_point_info (struct objfile *objfile)
       /* Executable file -- record its entry point so we'll recognize
          the startup file because it contains the entry point.  */
       objfile->ei.entry_point = bfd_get_start_address (objfile->obfd);
+      objfile->ei.entry_point_p = 1;
     }
   else if (bfd_get_file_flags (objfile->obfd) & DYNAMIC
 	   && bfd_get_start_address (objfile->obfd) != 0)
-    /* Some shared libraries may have entry points set and be
-       runnable.  There's no clear way to indicate this, so just check
-       for values other than zero.  */
-    objfile->ei.entry_point = bfd_get_start_address (objfile->obfd);    
+    {
+      /* Some shared libraries may have entry points set and be
+	 runnable.  There's no clear way to indicate this, so just check
+	 for values other than zero.  */
+      objfile->ei.entry_point = bfd_get_start_address (objfile->obfd);    
+      objfile->ei.entry_point_p = 1;
+    }
   else
     {
       /* Examination of non-executable.o files.  Short-circuit this stuff.  */
-      objfile->ei.entry_point = INVALID_ENTRY_POINT;
+      objfile->ei.entry_point_p = 0;
     }
 }
 
@@ -316,7 +320,7 @@ entry_point_address (void)
   struct gdbarch *gdbarch;
   CORE_ADDR entry_point;
 
-  if (symfile_objfile == NULL)
+  if (symfile_objfile == NULL || !symfile_objfile->ei.entry_point_p)
     return 0;
 
   gdbarch = get_objfile_arch (symfile_objfile);
@@ -702,7 +706,7 @@ objfile_relocate (struct objfile *objfile, struct section_offsets *new_offsets)
      to be out of order.  */
   msymbols_sort (objfile);
 
-  if (objfile->ei.entry_point != ~(CORE_ADDR) 0)
+  if (objfile->ei.entry_point_p)
     {
       /* Relocate ei.entry_point with its section offset, use SECT_OFF_TEXT
 	 only as a fallback.  */
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -100,15 +100,11 @@ struct objfile_data;
 
 struct entry_info
   {
-
-    /* The value we should use for this objects entry point.
-       The illegal/unknown value needs to be something other than 0, ~0
-       for instance, which is much less likely than 0. */
-
+    /* The relocated value we should use for this objfile entry point.  */
     CORE_ADDR entry_point;
 
-#define INVALID_ENTRY_POINT (~0)	/* ~0 will not be in any file, we hope.  */
-
+    /* Set to 1 iff ENTRY_POINT contains a valid value.  */
+    unsigned entry_point_p : 1;
   };
 
 /* Sections in an objfile.  The section offsets are stored in the
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -822,30 +822,43 @@ enable_break (void)
      may have changed since the last time we ran the program.  */
   remove_solib_event_breakpoints ();
 
-  /* Check for the presence of a .interp section.  If there is no
-     such section, the executable is statically linked.  */
-
-  interp_sect = bfd_get_section_by_name (exec_bfd, ".interp");
-
-  if (interp_sect)
+  if (symfile_objfile == NULL)
     {
-      enable_break1_done = 1;
-      create_solib_event_breakpoint (target_gdbarch,
-				     symfile_objfile->ei.entry_point);
+      if (solib_frv_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "enable_break: No symbol file found.\n");
+      return 0;
+    }
 
+  if (!symfile_objfile->ei.entry_point_p)
+    {
       if (solib_frv_debug)
 	fprintf_unfiltered (gdb_stdlog,
-			    "enable_break: solib event breakpoint placed at entry point: %s\n",
-			    hex_string_custom
-			      (symfile_objfile->ei.entry_point, 8));
+			    "enable_break: Symbol file has no entry point.\n");
+      return 0;
     }
-  else
+
+  /* Check for the presence of a .interp section.  If there is no
+     such section, the executable is statically linked.  */
+
+  interp_sect = bfd_get_section_by_name (exec_bfd, ".interp");
+
+  if (interp_sect == NULL)
     {
       if (solib_frv_debug)
 	fprintf_unfiltered (gdb_stdlog,
-	                    "enable_break: No .interp section found.\n");
+			    "enable_break: No .interp section found.\n");
+      return 0;
     }
 
+  enable_break1_done = 1;
+  create_solib_event_breakpoint (target_gdbarch,
+				 symfile_objfile->ei.entry_point);
+
+  if (solib_frv_debug)
+    fprintf_unfiltered (gdb_stdlog,
+			"enable_break: solib event breakpoint placed at entry point: %s\n",
+			hex_string_custom (symfile_objfile->ei.entry_point, 8));
   return 1;
 }
 

Re: [patch] cleanup: Remove INVALID_ENTRY_POINT (+FR-V modification)

[Pedro Alves]

On Saturday 31 October 2009 15:08:29, Jan Kratochvil wrote:
> @@ -316,7 +320,7 @@ entry_point_address (void)
>    struct gdbarch *gdbarch;
>    CORE_ADDR entry_point;
>  
> -  if (symfile_objfile == NULL)
> +  if (symfile_objfile == NULL || !symfile_objfile->ei.entry_point_p)
>      return 0;

Previously, when we had a main symbol file, but the entry point was
invalid (how common is that?), we'd return ~0 here, but now we'll
return 0.  Is there potential for breakage?  inside_entry_func for
instance could trigger false positives more often.  Makes me wonder about
exposing the invalid-entry-point-ity to callers.  Say, by adding a new
entry_point_address_p(), or better, adjusting the interface to:

 /* If there's a valid and known entry point, fills ENTRY_P with it
    and return true; otherwise return false.  */
 int entry_point_address (CORE_ADDR *entry_p);

Otherwise looks ok to me.

-- 
Pedro Alves

Re: [patch] cleanup: Remove INVALID_ENTRY_POINT (+FR-V modification)

[Jan Kratochvil]

Hi Pedro,

On Sat, 31 Oct 2009 16:42:13 +0100, Pedro Alves wrote:
> Previously, when we had a main symbol file, but the entry point was
> invalid 

> (how common is that?),

If you load an object file but it would have to also run for the callers of
entry_point_address() so I am unable to reproduce such case.


> we'd return ~0 here, but now we'll return 0.  Is there potential for
> breakage?  inside_entry_func for instance could trigger false positives more
> often.

I agree there is a regression risk, fixed.


>  /* If there's a valid and known entry point, fills ENTRY_P with it
>     and return true; otherwise return false.  */
>  int entry_point_address (CORE_ADDR *entry_p);

Used this one + an error()-calling wrapper.


Regression tested on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


2009-10-31  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Remove INVALID_ENTRY_POINT.
	* frame.c (inside_entry_func): New variable entry_point.  Return 0 if
	the entry point is not known.
	* solib-irix.c (enable_break): Likewise.
	* objfiles.c (init_entry_point_info): Stop using INVALID_ENTRY_POINT.
	Initialize EI.ENTRY_POINT_P.
	(entry_point_address): Rename to ...
	(entry_point_address_query): ... a new function.  Use EI.ENTRY_POINT_P.
	(entry_point_address): New function.
	(objfile_relocate): Use EI.ENTRY_POINT_P.
	* objfiles.h (struct entry_info): Simplify entry_point comment.  New
	field entry_point_p.
	(INVALID_ENTRY_POINT): Remove.
	(entry_point_address_query): New prototype.
	* solib-frv.c (enable_break): Check for NULL SYMFILE_OBJFILE and its
	EI.ENTRY_POINT_P.  Return 0 if ".interp" is not found.

--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1650,7 +1650,12 @@ inside_main_func (struct frame_info *this_frame)
 static int
 inside_entry_func (struct frame_info *this_frame)
 {
-  return (get_frame_func (this_frame) == entry_point_address ());
+  CORE_ADDR entry_point;
+
+  if (!entry_point_address_query (&entry_point))
+    return 0;
+
+  return get_frame_func (this_frame) == entry_point;
 }
 
 /* Return a structure containing various interesting information about
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -294,29 +294,34 @@ init_entry_point_info (struct objfile *objfile)
       /* Executable file -- record its entry point so we'll recognize
          the startup file because it contains the entry point.  */
       objfile->ei.entry_point = bfd_get_start_address (objfile->obfd);
+      objfile->ei.entry_point_p = 1;
     }
   else if (bfd_get_file_flags (objfile->obfd) & DYNAMIC
 	   && bfd_get_start_address (objfile->obfd) != 0)
-    /* Some shared libraries may have entry points set and be
-       runnable.  There's no clear way to indicate this, so just check
-       for values other than zero.  */
-    objfile->ei.entry_point = bfd_get_start_address (objfile->obfd);    
+    {
+      /* Some shared libraries may have entry points set and be
+	 runnable.  There's no clear way to indicate this, so just check
+	 for values other than zero.  */
+      objfile->ei.entry_point = bfd_get_start_address (objfile->obfd);    
+      objfile->ei.entry_point_p = 1;
+    }
   else
     {
       /* Examination of non-executable.o files.  Short-circuit this stuff.  */
-      objfile->ei.entry_point = INVALID_ENTRY_POINT;
+      objfile->ei.entry_point_p = 0;
     }
 }
 
-/* Get current entry point address.  */
+/* If there is a valid and known entry point, function fills *ENTRY_P with it
+   and returns non-zero; otherwise it returns zero.  */
 
-CORE_ADDR
-entry_point_address (void)
+int
+entry_point_address_query (CORE_ADDR *entry_p)
 {
   struct gdbarch *gdbarch;
   CORE_ADDR entry_point;
 
-  if (symfile_objfile == NULL)
+  if (symfile_objfile == NULL || !symfile_objfile->ei.entry_point_p)
     return 0;
 
   gdbarch = get_objfile_arch (symfile_objfile);
@@ -332,7 +337,21 @@ entry_point_address (void)
      symbol table.  */
   entry_point = gdbarch_addr_bits_remove (gdbarch, entry_point);
 
-  return entry_point;
+  *entry_p = entry_point;
+  return 1;
+}
+
+/* Get current entry point address.  Call error if it is not known.  */
+
+CORE_ADDR
+entry_point_address (void)
+{
+  CORE_ADDR retval;
+
+  if (!entry_point_address_query (&retval))
+    error (_("Entry point address is not known."));
+
+  return retval;
 }
 
 /* Create the terminating entry of OBJFILE's minimal symbol table.
@@ -702,7 +721,7 @@ objfile_relocate (struct objfile *objfile, struct section_offsets *new_offsets)
      to be out of order.  */
   msymbols_sort (objfile);
 
-  if (objfile->ei.entry_point != ~(CORE_ADDR) 0)
+  if (objfile->ei.entry_point_p)
     {
       /* Relocate ei.entry_point with its section offset, use SECT_OFF_TEXT
 	 only as a fallback.  */
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -100,15 +100,11 @@ struct objfile_data;
 
 struct entry_info
   {
-
-    /* The value we should use for this objects entry point.
-       The illegal/unknown value needs to be something other than 0, ~0
-       for instance, which is much less likely than 0. */
-
+    /* The relocated value we should use for this objfile entry point.  */
     CORE_ADDR entry_point;
 
-#define INVALID_ENTRY_POINT (~0)	/* ~0 will not be in any file, we hope.  */
-
+    /* Set to 1 iff ENTRY_POINT contains a valid value.  */
+    unsigned entry_point_p : 1;
   };
 
 /* Sections in an objfile.  The section offsets are stored in the
@@ -447,6 +443,8 @@ extern struct gdbarch *get_objfile_arch (struct objfile *);
 
 extern void init_entry_point_info (struct objfile *);
 
+extern int entry_point_address_query (CORE_ADDR *entry_p);
+
 extern CORE_ADDR entry_point_address (void);
 
 extern int build_objfile_section_table (struct objfile *);
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -822,30 +822,43 @@ enable_break (void)
      may have changed since the last time we ran the program.  */
   remove_solib_event_breakpoints ();
 
-  /* Check for the presence of a .interp section.  If there is no
-     such section, the executable is statically linked.  */
-
-  interp_sect = bfd_get_section_by_name (exec_bfd, ".interp");
-
-  if (interp_sect)
+  if (symfile_objfile == NULL)
     {
-      enable_break1_done = 1;
-      create_solib_event_breakpoint (target_gdbarch,
-				     symfile_objfile->ei.entry_point);
+      if (solib_frv_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "enable_break: No symbol file found.\n");
+      return 0;
+    }
 
+  if (!symfile_objfile->ei.entry_point_p)
+    {
       if (solib_frv_debug)
 	fprintf_unfiltered (gdb_stdlog,
-			    "enable_break: solib event breakpoint placed at entry point: %s\n",
-			    hex_string_custom
-			      (symfile_objfile->ei.entry_point, 8));
+			    "enable_break: Symbol file has no entry point.\n");
+      return 0;
     }
-  else
+
+  /* Check for the presence of a .interp section.  If there is no
+     such section, the executable is statically linked.  */
+
+  interp_sect = bfd_get_section_by_name (exec_bfd, ".interp");
+
+  if (interp_sect == NULL)
     {
       if (solib_frv_debug)
 	fprintf_unfiltered (gdb_stdlog,
-	                    "enable_break: No .interp section found.\n");
+			    "enable_break: No .interp section found.\n");
+      return 0;
     }
 
+  enable_break1_done = 1;
+  create_solib_event_breakpoint (target_gdbarch,
+				 symfile_objfile->ei.entry_point);
+
+  if (solib_frv_debug)
+    fprintf_unfiltered (gdb_stdlog,
+			"enable_break: solib event breakpoint placed at entry point: %s\n",
+			hex_string_custom (symfile_objfile->ei.entry_point, 8));
   return 1;
 }
 
--- a/gdb/solib-irix.c
+++ b/gdb/solib-irix.c
@@ -369,11 +369,13 @@ enable_break (void)
     {
       struct frame_info *frame = get_current_frame ();
       struct address_space *aspace = get_frame_address_space (frame);
+      CORE_ADDR entry_point;
 
-      base_breakpoint
-	= deprecated_insert_raw_breakpoint (target_gdbarch,
-					    aspace,
-					    entry_point_address ());
+      if (!entry_point_address_query (&entry_point))
+	return 0;
+
+      base_breakpoint = deprecated_insert_raw_breakpoint (target_gdbarch,
+							  aspace, entry_point);
 
       if (base_breakpoint != NULL)
 	return 1;

Re: [patch] cleanup: Remove INVALID_ENTRY_POINT (+FR-V modification)

[Pedro Alves]

On Saturday 31 October 2009 19:50:53, Jan Kratochvil wrote:
> >  /* If there's a valid and known entry point, fills ENTRY_P with it
> >     and return true; otherwise return false.  */
> >  int entry_point_address (CORE_ADDR *entry_p);
> 
> Used this one + an error()-calling wrapper.

Nice, thanks.

> 2009-10-31  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         Remove INVALID_ENTRY_POINT.
>         * frame.c (inside_entry_func): New variable entry_point.  Return 0 if
>         the entry point is not known.
>         * solib-irix.c (enable_break): Likewise.
>         * objfiles.c (init_entry_point_info): Stop using INVALID_ENTRY_POINT.
>         Initialize EI.ENTRY_POINT_P.
>         (entry_point_address): Rename to ...
>         (entry_point_address_query): ... a new function.  Use EI.ENTRY_POINT_P.
>         (entry_point_address): New function.
>         (objfile_relocate): Use EI.ENTRY_POINT_P.
>         * objfiles.h (struct entry_info): Simplify entry_point comment.  New
>         field entry_point_p.
>         (INVALID_ENTRY_POINT): Remove.
>         (entry_point_address_query): New prototype.
>         * solib-frv.c (enable_break): Check for NULL SYMFILE_OBJFILE and its
>         EI.ENTRY_POINT_P.  Return 0 if ".interp" is not found.

Looks OK to me.

-- 
Pedro Alves

Re: [patch] cleanup: Remove INVALID_ENTRY_POINT (+FR-V modification)

[Jan Kratochvil]

On Sat, 31 Oct 2009 22:08:28 +0100, Pedro Alves wrote:
> > 2009-10-31  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> >         Remove INVALID_ENTRY_POINT.
> >         * frame.c (inside_entry_func): New variable entry_point.  Return 0 if
> >         the entry point is not known.
> >         * solib-irix.c (enable_break): Likewise.
> >         * objfiles.c (init_entry_point_info): Stop using INVALID_ENTRY_POINT.
> >         Initialize EI.ENTRY_POINT_P.
> >         (entry_point_address): Rename to ...
> >         (entry_point_address_query): ... a new function.  Use EI.ENTRY_POINT_P.
> >         (entry_point_address): New function.
> >         (objfile_relocate): Use EI.ENTRY_POINT_P.
> >         * objfiles.h (struct entry_info): Simplify entry_point comment.  New
> >         field entry_point_p.
> >         (INVALID_ENTRY_POINT): Remove.
> >         (entry_point_address_query): New prototype.
> >         * solib-frv.c (enable_break): Check for NULL SYMFILE_OBJFILE and its
> >         EI.ENTRY_POINT_P.  Return 0 if ".interp" is not found.
> 
> Looks OK to me.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-11/msg00006.html


Thanks,
Jan