public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/3] move entry point info to the per-BFD object
@ 2014-01-06 20:30 Tom Tromey
  2014-01-06 20:30 ` [PATCH 3/3] move the entry point info into the per-bfd object Tom Tromey
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Tom Tromey @ 2014-01-06 20:30 UTC (permalink / raw)
  To: gdb-patches

This series is part of the ongoing objfile splitting project.

Right now the entry info object in the objfile is relocated when it is
created.  This series applies the usual transform, changing gdb to
apply the offset at the point of use.  Then the entry_info object is
moved into the per-BFD.

See patch #2 for the possibly latent bug I think I uncovered.

Built and regtested on x86-64 Fedora 18.

Tom

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] change solib-frv to use entry_point_address_query
  2014-01-06 20:30 [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
  2014-01-06 20:30 ` [PATCH 3/3] move the entry point info into the per-bfd object Tom Tromey
@ 2014-01-06 20:30 ` Tom Tromey
  2014-01-08 13:23   ` Pedro Alves
  2014-01-06 20:30 ` [PATCH 2/3] relocate the entry point addess when used Tom Tromey
  2014-01-15 18:01 ` [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
  3 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2014-01-06 20:30 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

This is just a minor cleanup in advance of some other changes, that
modifies solib-frv.c to use entry_point_address_query.  I don't have a
good way to test this but I think it is obviously correct.

2014-01-06  Tom Tromey  <tromey@redhat.com>

	* solib-frv.c (enable_break): Use entry_point_address_query.
---
 gdb/ChangeLog   | 4 ++++
 gdb/solib-frv.c | 9 ++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index 71f2ebe..d257eea 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -721,6 +721,7 @@ static int
 enable_break (void)
 {
   asection *interp_sect;
+  CORE_ADDR entry_point;
 
   if (symfile_objfile == NULL)
     {
@@ -730,7 +731,7 @@ enable_break (void)
       return 0;
     }
 
-  if (!symfile_objfile->ei.entry_point_p)
+  if (!entry_point_address_query (&entry_point))
     {
       if (solib_frv_debug)
 	fprintf_unfiltered (gdb_stdlog,
@@ -751,15 +752,13 @@ enable_break (void)
       return 0;
     }
 
-  create_solib_event_breakpoint (target_gdbarch (),
-				 symfile_objfile->ei.entry_point);
+  create_solib_event_breakpoint (target_gdbarch (), 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));
+			hex_string_custom (entry_point, 8));
   return 1;
 }
 
-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] relocate the entry point addess when used
  2014-01-06 20:30 [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
  2014-01-06 20:30 ` [PATCH 3/3] move the entry point info into the per-bfd object Tom Tromey
  2014-01-06 20:30 ` [PATCH 1/3] change solib-frv to use entry_point_address_query Tom Tromey
@ 2014-01-06 20:30 ` Tom Tromey
  2014-01-08 13:22   ` Pedro Alves
  2014-01-15 18:01 ` [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
  3 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2014-01-06 20:30 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

This changes the entry point to be unrelocated in the objfile, and
instead applies the relocation when it is used.

I think the existing code here is wrong.  It computes the entry point
address directly from the BFD, not applying any runtime offsets.
However, then objfile_relocate1 passes this address to find_pc_section
-- which does use the offsets .  So, it seems to me that the current
code can only find the correct address by luck.

2014-01-06  Tom Tromey  <tromey@redhat.com>

	* objfiles.c (entry_point_address_query): Relocate entry point
	address.
	(objfile_relocate1): Do not relocate entry point address.
	* objfiles.h (struct entry_info) <entry_point>: Update comment.
	<the_bfd_section_index>: New field.
	* symfile.c (init_entry_point_info): Find the entry point's
	section.
---
 gdb/ChangeLog  | 10 ++++++++++
 gdb/objfiles.c | 20 +++-----------------
 gdb/objfiles.h |  5 ++++-
 gdb/symfile.c  | 18 ++++++++++++++++++
 4 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 73847bd..2604ae4 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -367,7 +367,9 @@ entry_point_address_query (CORE_ADDR *entry_p)
   if (symfile_objfile == NULL || !symfile_objfile->ei.entry_point_p)
     return 0;
 
-  *entry_p = symfile_objfile->ei.entry_point;
+  *entry_p = (symfile_objfile->ei.entry_point
+	      + ANOFFSET (symfile_objfile->section_offsets,
+			  symfile_objfile->ei.the_bfd_section_index));
 
   return 1;
 }
@@ -794,22 +796,6 @@ objfile_relocate1 (struct objfile *objfile,
      to be out of order.  */
   msymbols_sort (objfile);
 
-  if (objfile->ei.entry_point_p)
-    {
-      /* Relocate ei.entry_point with its section offset, use SECT_OFF_TEXT
-	 only as a fallback.  */
-      struct obj_section *s;
-      s = find_pc_section (objfile->ei.entry_point);
-      if (s)
-	{
-	  int idx = gdb_bfd_section_index (objfile->obfd, s->the_bfd_section);
-
-	  objfile->ei.entry_point += ANOFFSET (delta, idx);
-	}
-      else
-        objfile->ei.entry_point += ANOFFSET (delta, SECT_OFF_TEXT (objfile));
-    }
-
   {
     int i;
 
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index c2b6177..620d7e8 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -101,9 +101,12 @@ struct objfile_data;
 
 struct entry_info
   {
-    /* The relocated value we should use for this objfile entry point.  */
+    /* The unrelocated value we should use for this objfile entry point.  */
     CORE_ADDR entry_point;
 
+    /* The index of the section in which the entry point appears.  */
+    int the_bfd_section_index;
+
     /* Set to 1 iff ENTRY_POINT contains a valid value.  */
     unsigned entry_point_p : 1;
   };
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 4f0fea8..a0ff9db 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -895,6 +895,7 @@ init_entry_point_info (struct objfile *objfile)
 
   if (objfile->ei.entry_point_p)
     {
+      struct obj_section *osect;
       CORE_ADDR entry_point =  objfile->ei.entry_point;
 
       /* Make certain that the address points at real code, and not a
@@ -908,6 +909,23 @@ init_entry_point_info (struct objfile *objfile)
 	 symbol table.  */
       objfile->ei.entry_point
 	= gdbarch_addr_bits_remove (get_objfile_arch (objfile), entry_point);
+
+      ALL_OBJFILE_OSECTIONS (objfile, osect)
+	{
+	  struct bfd_section *sect = osect->the_bfd_section;
+
+	  if (entry_point >= bfd_get_section_vma (objfile->obfd, sect)
+	      && entry_point < (bfd_get_section_vma (objfile->obfd, sect)
+				+ bfd_get_section_size (sect)))
+	    {
+	      objfile->ei.the_bfd_section_index
+		= gdb_bfd_section_index (objfile->obfd, sect);
+	      break;
+	    }
+	}
+
+      if (osect == NULL)
+	objfile->ei.the_bfd_section_index = SECT_OFF_TEXT (objfile);
     }
 }
 
-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] move the entry point info into the per-bfd object
  2014-01-06 20:30 [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
@ 2014-01-06 20:30 ` Tom Tromey
  2014-01-08 13:30   ` Pedro Alves
  2014-01-06 20:30 ` [PATCH 1/3] change solib-frv to use entry_point_address_query Tom Tromey
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2014-01-06 20:30 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

This moves the entry point information into the per-BFD object and
arranges not to recompute it when it has already been computed.

2014-01-06  Tom Tromey  <tromey@redhat.com>

	* symfile.c (init_entry_point_info): Use new "initialized" field.
	Update.
	* objfiles.h (struct entry_point) <initialized>: New field.
	(struct objfile_per_bfd_storage) <ei>: New field, moved from...
	(struct objfile) <ei>: ...here.  Remove.
	* objfiles.c (entry_point_address_query): Update.
---
 gdb/ChangeLog  |  9 +++++++++
 gdb/objfiles.c |  6 +++---
 gdb/objfiles.h | 13 ++++++++-----
 gdb/symfile.c  | 24 ++++++++++++++----------
 4 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 2604ae4..c2238d1 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -364,12 +364,12 @@ get_objfile_arch (struct objfile *objfile)
 int
 entry_point_address_query (CORE_ADDR *entry_p)
 {
-  if (symfile_objfile == NULL || !symfile_objfile->ei.entry_point_p)
+  if (symfile_objfile == NULL || !symfile_objfile->per_bfd->ei.entry_point_p)
     return 0;
 
-  *entry_p = (symfile_objfile->ei.entry_point
+  *entry_p = (symfile_objfile->per_bfd->ei.entry_point
 	      + ANOFFSET (symfile_objfile->section_offsets,
-			  symfile_objfile->ei.the_bfd_section_index));
+			  symfile_objfile->per_bfd->ei.the_bfd_section_index));
 
   return 1;
 }
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 620d7e8..d448c9e 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -109,6 +109,9 @@ struct entry_info
 
     /* Set to 1 iff ENTRY_POINT contains a valid value.  */
     unsigned entry_point_p : 1;
+
+    /* Set to 1 iff this object was initialized.  */
+    unsigned initialized : 1;
   };
 
 /* Sections in an objfile.  The section offsets are stored in the
@@ -195,6 +198,11 @@ struct objfile_per_bfd_storage
      name, and the second is the demangled name or just a zero byte
      if the name doesn't demangle.  */
   struct htab *demangled_names_hash;
+
+  /* The per-objfile information about the entry point, the scope (file/func)
+     containing the entry point, and the scope of the user's main() func.  */
+
+  struct entry_info ei;
 };
 
 /* Master structure for keeping track of each file from which
@@ -318,11 +326,6 @@ struct objfile
 
     const struct sym_fns *sf;
 
-    /* The per-objfile information about the entry point, the scope (file/func)
-       containing the entry point, and the scope of the user's main() func.  */
-
-    struct entry_info ei;
-
     /* Per objfile data-pointers required by other GDB modules.  */
 
     REGISTRY_FIELDS;
diff --git a/gdb/symfile.c b/gdb/symfile.c
index a0ff9db..2a49379 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -868,6 +868,10 @@ read_symbols (struct objfile *objfile, int add_flags)
 static void
 init_entry_point_info (struct objfile *objfile)
 {
+  if (objfile->per_bfd->ei.initialized)
+    return;
+  objfile->per_bfd->ei.initialized = 1;
+
   /* Save startup file's range of PC addresses to help blockframe.c
      decide where the bottom of the stack is.  */
 
@@ -875,8 +879,8 @@ 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;
+      objfile->per_bfd->ei.entry_point = bfd_get_start_address (objfile->obfd);
+      objfile->per_bfd->ei.entry_point_p = 1;
     }
   else if (bfd_get_file_flags (objfile->obfd) & DYNAMIC
 	   && bfd_get_start_address (objfile->obfd) != 0)
@@ -884,19 +888,19 @@ init_entry_point_info (struct objfile *objfile)
       /* 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;
+      objfile->per_bfd->ei.entry_point = bfd_get_start_address (objfile->obfd);
+      objfile->per_bfd->ei.entry_point_p = 1;
     }
   else
     {
       /* Examination of non-executable.o files.  Short-circuit this stuff.  */
-      objfile->ei.entry_point_p = 0;
+      objfile->per_bfd->ei.entry_point_p = 0;
     }
 
-  if (objfile->ei.entry_point_p)
+  if (objfile->per_bfd->ei.entry_point_p)
     {
       struct obj_section *osect;
-      CORE_ADDR entry_point =  objfile->ei.entry_point;
+      CORE_ADDR entry_point =  objfile->per_bfd->ei.entry_point;
 
       /* Make certain that the address points at real code, and not a
 	 function descriptor.  */
@@ -907,7 +911,7 @@ init_entry_point_info (struct objfile *objfile)
 
       /* Remove any ISA markers, so that this matches entries in the
 	 symbol table.  */
-      objfile->ei.entry_point
+      objfile->per_bfd->ei.entry_point
 	= gdbarch_addr_bits_remove (get_objfile_arch (objfile), entry_point);
 
       ALL_OBJFILE_OSECTIONS (objfile, osect)
@@ -918,14 +922,14 @@ init_entry_point_info (struct objfile *objfile)
 	      && entry_point < (bfd_get_section_vma (objfile->obfd, sect)
 				+ bfd_get_section_size (sect)))
 	    {
-	      objfile->ei.the_bfd_section_index
+	      objfile->per_bfd->ei.the_bfd_section_index
 		= gdb_bfd_section_index (objfile->obfd, sect);
 	      break;
 	    }
 	}
 
       if (osect == NULL)
-	objfile->ei.the_bfd_section_index = SECT_OFF_TEXT (objfile);
+	objfile->per_bfd->ei.the_bfd_section_index = SECT_OFF_TEXT (objfile);
     }
 }
 
-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] relocate the entry point addess when used
  2014-01-06 20:30 ` [PATCH 2/3] relocate the entry point addess when used Tom Tromey
@ 2014-01-08 13:22   ` Pedro Alves
  2014-01-13 20:46     ` Tom Tromey
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2014-01-08 13:22 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 01/06/2014 08:30 PM, Tom Tromey wrote:
> This changes the entry point to be unrelocated in the objfile, and
> instead applies the relocation when it is used.
> 
> I think the existing code here is wrong.  It computes the entry point
> address directly from the BFD, not applying any runtime offsets.
> However, then objfile_relocate1 passes this address to find_pc_section
> -- which does use the offsets .  So, it seems to me that the current
> code can only find the correct address by luck.

It's twisted, but I don't think it's luck.  You can convince yourself
it works by debugging a PIE, and trying a backtrace past main
("set backtrace past-main" will then trigger the code to stop the
backtrace at the entry point), or doing "info files" (shows the entry).

Thing is, find_pc_section uses obj_section_addr/obj_section_endaddr
for comparison:

/* Bsearch comparison function.  */

static int
bsearch_cmp (const void *key, const void *elt)
{
  const CORE_ADDR pc = *(CORE_ADDR *) key;
  const struct obj_section *section = *(const struct obj_section **) elt;

  if (pc < obj_section_addr (section))
    return -1;
  if (pc < obj_section_endaddr (section))
    return 0;
  return 1;
}

And obj_section_addr indeed applies the offsets:

/* Relocation offset applied to S.  */
#define obj_section_offset(s)						\
  (((s)->objfile->section_offsets)->offsets[gdb_bfd_section_index ((s)->objfile->obfd, (s)->the_bfd_section)])

/* The memory address of section S (vma + offset).  */
#define obj_section_addr(s)				      		\
  (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)		\
   + obj_section_offset (s))

/* The one-passed-the-end memory address of section S
   (vma + size + offset).  */
#define obj_section_endaddr(s)						\
  (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)		\
   + bfd_get_section_size ((s)->the_bfd_section)			\
   + obj_section_offset (s))


But, objfile_relocate1 only sets the offsets to the obj_sections
_after_ looking up the entry point:

  if (objfile->ei.entry_point_p)
    {
      /* Relocate ei.entry_point with its section offset, use SECT_OFF_TEXT
	 only as a fallback.  */
      struct obj_section *s;
      s = find_pc_section (objfile->ei.entry_point);
      if (s)
	{
	  int idx = gdb_bfd_section_index (objfile->obfd, s->the_bfd_section);

	  objfile->ei.entry_point += ANOFFSET (delta, idx);
	}
      else
        objfile->ei.entry_point += ANOFFSET (delta, SECT_OFF_TEXT (objfile));
    }

  {
    int i;

    for (i = 0; i < objfile->num_sections; ++i)
      (objfile->section_offsets)->offsets[i] = ANOFFSET (new_offsets, i);
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  }

So find_pc_section is still using the unrelocated section addresses
when looking up the entry point's unrelocated address.

> +
> +      ALL_OBJFILE_OSECTIONS (objfile, osect)
> +	{
> +	  struct bfd_section *sect = osect->the_bfd_section;
> +
> +	  if (entry_point >= bfd_get_section_vma (objfile->obfd, sect)
> +	      && entry_point < (bfd_get_section_vma (objfile->obfd, sect)
> +				+ bfd_get_section_size (sect)))
> +	    {
> +	      objfile->ei.the_bfd_section_index
> +		= gdb_bfd_section_index (objfile->obfd, sect);
> +	      break;
> +	    }
> +	}
> +
> +      if (osect == NULL)

This is assuming osect ends up NULL after iterating over all.
It's violating the abstraction of the macro.  And, actually,
it's wrong, showing exactly why such assumptions are a bad idea:

#define ALL_OBJFILE_OSECTIONS(objfile, osect)	\
  for (osect = objfile->sections; osect < objfile->sections_end; osect++) \
    if (osect->the_bfd_section == NULL)					\
      {									\
	/* Nothing.  */							\
      }									\
    else

OSECT doesn't end up NULL at the end of the iteration at all.

It's better to, make the "break" above when the section is found
be a return instead.

> +	objfile->ei.the_bfd_section_index = SECT_OFF_TEXT (objfile);
>      }
>  }
>  
> 


-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] change solib-frv to use entry_point_address_query
  2014-01-06 20:30 ` [PATCH 1/3] change solib-frv to use entry_point_address_query Tom Tromey
@ 2014-01-08 13:23   ` Pedro Alves
  0 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2014-01-08 13:23 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 01/06/2014 08:30 PM, Tom Tromey wrote:
> This is just a minor cleanup in advance of some other changes, that
> modifies solib-frv.c to use entry_point_address_query.  I don't have a
> good way to test this but I think it is obviously correct.

Agreed.

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] move the entry point info into the per-bfd object
  2014-01-06 20:30 ` [PATCH 3/3] move the entry point info into the per-bfd object Tom Tromey
@ 2014-01-08 13:30   ` Pedro Alves
  2014-01-13 20:51     ` Tom Tromey
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2014-01-08 13:30 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

Looks good to me.

> @@ -875,8 +879,8 @@ 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;
> +      objfile->per_bfd->ei.entry_point = bfd_get_start_address (objfile->obfd);
> +      objfile->per_bfd->ei.entry_point_p = 1;

I think this has reached sufficient levels of indirection
that if I were writing this, I'd do:

  static void
  init_entry_point_info (struct objfile *objfile)
  {
+     struct entry_info *ei = &objfile->per_bfd->ei;

and then use ei-> throughout the function instead, like:

  if (ei->initialized)
    return;
  ei->initialized = 1;
...

  ei->entry_point = bfd_get_start_address (objfile->obfd);
  ei->entry_point_p = 1;
...
  etc.

Just a suggestion.

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] relocate the entry point addess when used
  2014-01-08 13:22   ` Pedro Alves
@ 2014-01-13 20:46     ` Tom Tromey
  2014-01-15 11:42       ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2014-01-13 20:46 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Tom> I think the existing code here is wrong.  It computes the entry point
Tom> address directly from the BFD, not applying any runtime offsets.
Tom> However, then objfile_relocate1 passes this address to find_pc_section
Tom> -- which does use the offsets .  So, it seems to me that the current
Tom> code can only find the correct address by luck.

Pedro> It's twisted, but I don't think it's luck.  You can convince yourself
Pedro> it works by debugging a PIE, and trying a backtrace past main
Pedro> ("set backtrace past-main" will then trigger the code to stop the
Pedro> backtrace at the entry point), or doing "info files" (shows the entry).

Well, what I don't understand is that most addresses in dwarf2read.c are
offset:

  baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));

... however this is not done for the entry point, which comes directly
from the BFD:

      objfile->per_bfd->ei.entry_point = bfd_get_start_address (objfile->obfd);

I suppose there is some other invariant ensuring that the entry point is
only computed when all the objfile offsets are zero.  This part is not
obvious to me.

[...]
Pedro> This is assuming osect ends up NULL after iterating over all.
Pedro> It's violating the abstraction of the macro.  And, actually,
Pedro> it's wrong, showing exactly why such assumptions are a bad idea:

Sorry about that, I'll fix it up locally.

Tom

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] move the entry point info into the per-bfd object
  2014-01-08 13:30   ` Pedro Alves
@ 2014-01-13 20:51     ` Tom Tromey
  0 siblings, 0 replies; 13+ messages in thread
From: Tom Tromey @ 2014-01-13 20:51 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I think this has reached sufficient levels of indirection
Pedro> that if I were writing this, I'd do:
Pedro>   static void
Pedro>   init_entry_point_info (struct objfile *objfile)
Pedro>   {
Pedro> +     struct entry_info *ei = &objfile->per_bfd->ei;
Pedro> and then use ei-> throughout the function instead, like:

Thanks, I made this change.

Tom

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] relocate the entry point addess when used
  2014-01-13 20:46     ` Tom Tromey
@ 2014-01-15 11:42       ` Pedro Alves
  2014-01-15 17:59         ` Tom Tromey
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2014-01-15 11:42 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 01/13/2014 08:46 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Tom> I think the existing code here is wrong.  It computes the entry point
> Tom> address directly from the BFD, not applying any runtime offsets.
> Tom> However, then objfile_relocate1 passes this address to find_pc_section
> Tom> -- which does use the offsets .  So, it seems to me that the current
> Tom> code can only find the correct address by luck.
> 
> Pedro> It's twisted, but I don't think it's luck.  You can convince yourself
> Pedro> it works by debugging a PIE, and trying a backtrace past main
> Pedro> ("set backtrace past-main" will then trigger the code to stop the
> Pedro> backtrace at the entry point), or doing "info files" (shows the entry).
> 
> Well, what I don't understand is that most addresses in dwarf2read.c are
> offset:
> 
>   baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
> 
> ... however this is not done for the entry point, which comes directly
> from the BFD:
> 
>       objfile->per_bfd->ei.entry_point = bfd_get_start_address (objfile->obfd);
> 
> I suppose there is some other invariant ensuring that the entry point is
> only computed when all the objfile offsets are zero.  This part is not
> obvious to me.

I think it's just that the entry point is only really used for the main
executable, and if that is loaded at some relocation offset, we'll
always go through objfile_relocate (either PIE handling, or RSP
qOffsets handling for embedded systems) after reading in symbols,
while shared libraries are read in with the offsets already handy
upfront.  Even though init_entry_point_info seemingly computes the
entry point for shared libraries, that's really still for the
main executable:
 https://sourceware.org/ml/gdb-patches/2008-05/msg00112.html
and is probably indeed wrong for real shared libraries not
the main executable.

I do think your patch makes things much clearer and sturdier.

> 
> [...]
> Pedro> This is assuming osect ends up NULL after iterating over all.
> Pedro> It's violating the abstraction of the macro.  And, actually,
> Pedro> it's wrong, showing exactly why such assumptions are a bad idea:
> 
> Sorry about that, I'll fix it up locally.

Thanks.


-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] relocate the entry point addess when used
  2014-01-15 11:42       ` Pedro Alves
@ 2014-01-15 17:59         ` Tom Tromey
  2014-01-15 18:02           ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2014-01-15 17:59 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I think it's just that the entry point is only really used for the main
Pedro> executable, and if that is loaded at some relocation offset, we'll
Pedro> always go through objfile_relocate (either PIE handling, or RSP
Pedro> qOffsets handling for embedded systems) after reading in symbols,
Pedro> while shared libraries are read in with the offsets already handy
Pedro> upfront.

Yeah, that makes sense.

Pedro> This is assuming osect ends up NULL after iterating over all.
Pedro> It's violating the abstraction of the macro.  And, actually,
Pedro> it's wrong, showing exactly why such assumptions are a bad idea:

Tom> Sorry about that, I'll fix it up locally.

Pedro> Thanks.

Here's the updated patch.

Tom

commit 9211b8474f76a95e993f4dc6e2ee873c6997caa4
Author: Tom Tromey <tromey@redhat.com>
Date:   Tue Dec 31 06:52:33 2013 -0700

    relocate the entry point address when used
    
    This changes the entry point to be unrelocated in the objfile, and
    instead applies the relocation when it is used.
    
    2014-01-15  Tom Tromey  <tromey@redhat.com>
    
    	* objfiles.c (entry_point_address_query): Relocate entry point
    	address.
    	(objfile_relocate1): Do not relocate entry point address.
    	* objfiles.h (struct entry_info) <entry_point>: Update comment.
    	<the_bfd_section_index>: New field.
    	* symfile.c (init_entry_point_info): Find the entry point's
    	section.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 01a4087..5d48363 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2014-01-15  Tom Tromey  <tromey@redhat.com>
 
+	* objfiles.c (entry_point_address_query): Relocate entry point
+	address.
+	(objfile_relocate1): Do not relocate entry point address.
+	* objfiles.h (struct entry_info) <entry_point>: Update comment.
+	<the_bfd_section_index>: New field.
+	* symfile.c (init_entry_point_info): Find the entry point's
+	section.
+
+2014-01-15  Tom Tromey  <tromey@redhat.com>
+
 	* solib-frv.c (enable_break): Use entry_point_address_query.
 
 2014-01-15  Pedro Alves  <palves@redhat.com>
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 9cc0054..a80d4c7 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -367,7 +367,9 @@ entry_point_address_query (CORE_ADDR *entry_p)
   if (symfile_objfile == NULL || !symfile_objfile->ei.entry_point_p)
     return 0;
 
-  *entry_p = symfile_objfile->ei.entry_point;
+  *entry_p = (symfile_objfile->ei.entry_point
+	      + ANOFFSET (symfile_objfile->section_offsets,
+			  symfile_objfile->ei.the_bfd_section_index));
 
   return 1;
 }
@@ -794,22 +796,6 @@ objfile_relocate1 (struct objfile *objfile,
      to be out of order.  */
   msymbols_sort (objfile);
 
-  if (objfile->ei.entry_point_p)
-    {
-      /* Relocate ei.entry_point with its section offset, use SECT_OFF_TEXT
-	 only as a fallback.  */
-      struct obj_section *s;
-      s = find_pc_section (objfile->ei.entry_point);
-      if (s)
-	{
-	  int idx = gdb_bfd_section_index (objfile->obfd, s->the_bfd_section);
-
-	  objfile->ei.entry_point += ANOFFSET (delta, idx);
-	}
-      else
-        objfile->ei.entry_point += ANOFFSET (delta, SECT_OFF_TEXT (objfile));
-    }
-
   {
     int i;
 
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index c2b6177..620d7e8 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -101,9 +101,12 @@ struct objfile_data;
 
 struct entry_info
   {
-    /* The relocated value we should use for this objfile entry point.  */
+    /* The unrelocated value we should use for this objfile entry point.  */
     CORE_ADDR entry_point;
 
+    /* The index of the section in which the entry point appears.  */
+    int the_bfd_section_index;
+
     /* Set to 1 iff ENTRY_POINT contains a valid value.  */
     unsigned entry_point_p : 1;
   };
diff --git a/gdb/symfile.c b/gdb/symfile.c
index d863282..bd12552 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -894,7 +894,9 @@ init_entry_point_info (struct objfile *objfile)
 
   if (objfile->ei.entry_point_p)
     {
+      struct obj_section *osect;
       CORE_ADDR entry_point =  objfile->ei.entry_point;
+      int found;
 
       /* Make certain that the address points at real code, and not a
 	 function descriptor.  */
@@ -907,6 +909,25 @@ init_entry_point_info (struct objfile *objfile)
 	 symbol table.  */
       objfile->ei.entry_point
 	= gdbarch_addr_bits_remove (get_objfile_arch (objfile), entry_point);
+
+      found = 0;
+      ALL_OBJFILE_OSECTIONS (objfile, osect)
+	{
+	  struct bfd_section *sect = osect->the_bfd_section;
+
+	  if (entry_point >= bfd_get_section_vma (objfile->obfd, sect)
+	      && entry_point < (bfd_get_section_vma (objfile->obfd, sect)
+				+ bfd_get_section_size (sect)))
+	    {
+	      objfile->ei.the_bfd_section_index
+		= gdb_bfd_section_index (objfile->obfd, sect);
+	      found = 1;
+	      break;
+	    }
+	}
+
+      if (!found)
+	objfile->ei.the_bfd_section_index = SECT_OFF_TEXT (objfile);
     }
 }
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] move entry point info to the per-BFD object
  2014-01-06 20:30 [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
                   ` (2 preceding siblings ...)
  2014-01-06 20:30 ` [PATCH 2/3] relocate the entry point addess when used Tom Tromey
@ 2014-01-15 18:01 ` Tom Tromey
  3 siblings, 0 replies; 13+ messages in thread
From: Tom Tromey @ 2014-01-15 18:01 UTC (permalink / raw)
  To: gdb-patches

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This series is part of the ongoing objfile splitting project.
Tom> Right now the entry info object in the objfile is relocated when it is
Tom> created.  This series applies the usual transform, changing gdb to
Tom> apply the offset at the point of use.  Then the entry_info object is
Tom> moved into the per-BFD.

I'm checking this in.

Tom

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] relocate the entry point addess when used
  2014-01-15 17:59         ` Tom Tromey
@ 2014-01-15 18:02           ` Pedro Alves
  0 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2014-01-15 18:02 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 01/15/2014 05:59 PM, Tom Tromey wrote:

> Here's the updated patch.

Thanks, looks good to me.

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-01-15 18:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-06 20:30 [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey
2014-01-06 20:30 ` [PATCH 3/3] move the entry point info into the per-bfd object Tom Tromey
2014-01-08 13:30   ` Pedro Alves
2014-01-13 20:51     ` Tom Tromey
2014-01-06 20:30 ` [PATCH 1/3] change solib-frv to use entry_point_address_query Tom Tromey
2014-01-08 13:23   ` Pedro Alves
2014-01-06 20:30 ` [PATCH 2/3] relocate the entry point addess when used Tom Tromey
2014-01-08 13:22   ` Pedro Alves
2014-01-13 20:46     ` Tom Tromey
2014-01-15 11:42       ` Pedro Alves
2014-01-15 17:59         ` Tom Tromey
2014-01-15 18:02           ` Pedro Alves
2014-01-15 18:01 ` [PATCH 0/3] move entry point info to the per-BFD object Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).