[rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

Hello,

I've been running into problems when using the new_objfile observer to
set a breakpoint on a newly loaded library on powerpc64-linux.

The observer gets called when ld.so hits the _dl_debug_state breakpoint.
At this point, my code was retrieving a symbol from the new library and
setting a breakpoint.  As this is powerpc64-linux, this involves resolving
function descriptors, i.e. reading the target address from the .opd section.

Now the current ppc64_linux_convert_from_func_ptr_addr simply reads the
.opd contents from target memory.  However, there may in fact be relocations
that need to be applied to .opd.  Usually, this is not a problem, because
the _dl_debug_state breakpoint is hit only after ld.so has applied those
relocations to target memory.

However, this is not the case if the library is loaded via dlopen.  In this
case, the _dl_debug_state breakpoint is hit *before* relocations are applied.
(I guess this might be considered a bug in glibc.  But we have to live with
existing glibc's in the field anyway ...)

This is somewhat unfortunate, as I now cannot even manually apply the 
relocation (because it might already have been applied, and I cannot
find out whether or not this has actually happened).

So to solve this I'm now completely ignoring contents of .opd in target
memory, and instead always retrieve the contents from the BFD.  Those
will certainly be non-relocated, so applying the relocation offset by
hand will always result in the correct target address.

Is this a reasonable thing to do?


Tested on powerpc64-linux.

Bye,
Ulrich

ChangeLog:

	* ppc-linux-tdep.c (ppc64_linux_convert_from_func_ptr_addr): Read
	and manually relocate .opd contents from BFD instead of reading
	them from target memory.


diff -urNp gdb-orig/gdb/ppc-linux-tdep.c gdb-head/gdb/ppc-linux-tdep.c
--- gdb-orig/gdb/ppc-linux-tdep.c	2008-05-14 20:28:48.244451000 +0200
+++ gdb-head/gdb/ppc-linux-tdep.c	2008-05-14 21:11:35.672042314 +0200
@@ -591,7 +591,31 @@ ppc64_linux_convert_from_func_ptr_addr (
 
   /* Check if ADDR points to a function descriptor.  */
   if (s && strcmp (s->the_bfd_section->name, ".opd") == 0)
-    return get_target_memory_unsigned (targ, addr, 8);
+    {
+      /* There may be relocations that need to be applied to the .opd 
+	 section.  Unfortunately, this function may be called at a time
+	 where these relocations have not yet been performed -- this can
+	 happen for example shortly after a library has been loaded with
+	 dlopen, but ld.so has not yet applied the relocations.
+
+	 To cope with both the case where the relocation has been applied,
+	 and the case where it has not yet been applied, we do *not* read
+	 the (maybe) relocated value from target memory, but we instead
+	 read the non-relocated value from the BFD, and apply the relocation
+	 offset manually.
+
+	 This makes the assumption that all .opd entries are always relocated
+	 by the same offset the section itself was relocated.  */
+
+      gdb_byte buf[8];
+      int res;
+
+      res = bfd_get_section_contents (s->bfd, s->the_bfd_section,
+				      &buf, addr - s->addr, 8);
+      if (res != 0)
+	return extract_unsigned_integer (buf, 8)
+		- bfd_section_vma (s->bfd, s->the_bfd_section) + s->addr;
+   }
 
   return addr;
 }
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Daniel Jacobowitz]

On Thu, May 15, 2008 at 01:02:49AM +0200, Ulrich Weigand wrote:
> However, this is not the case if the library is loaded via dlopen.  In this
> case, the _dl_debug_state breakpoint is hit *before* relocations are applied.
> (I guess this might be considered a bug in glibc.  But we have to live with
> existing glibc's in the field anyway ...)

Not that I disagree with your conclusions, but this is a sorry state
of affairs.  There's a status field in struct r_debug; what is it when
this happens?  RT_ADD or RT_CONSISTENT?  RT_ADD is supposed to happen
before the object is added to the list, and RT_CONSISTENT after it has
been relocated.  We can end up loading inconsistent shared libraries,
if we're between those two points and someone does "info shared", but
this happens rarely and it's not the case here.

I feel like we've discussed this problem before, in one of the various
revisions of the same patch we were discussing yesterday, but I can't
find it.

> So to solve this I'm now completely ignoring contents of .opd in target
> memory, and instead always retrieve the contents from the BFD.  Those
> will certainly be non-relocated, so applying the relocation offset by
> hand will always result in the correct target address.
> 
> Is this a reasonable thing to do?

We went round the choice of where to read memory from several times on
the previous patch, but I don't know the details.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Thu, May 15, 2008 at 01:02:49AM +0200, Ulrich Weigand wrote:
> > However, this is not the case if the library is loaded via dlopen.  In this
> > case, the _dl_debug_state breakpoint is hit *before* relocations are applied.
> > (I guess this might be considered a bug in glibc.  But we have to live with
> > existing glibc's in the field anyway ...)
> 
> Not that I disagree with your conclusions, but this is a sorry state
> of affairs.  There's a status field in struct r_debug; what is it when
> this happens?  RT_ADD or RT_CONSISTENT?  RT_ADD is supposed to happen
> before the object is added to the list, and RT_CONSISTENT after it has
> been relocated.  We can end up loading inconsistent shared libraries,
> if we're between those two points and someone does "info shared", but
> this happens rarely and it's not the case here.

Well, it seems that this field is set to RT_CONSISTENT *before* the 
objects are relocated  (from elf/dl-open.c):


  /* Notify the debugger all new objects are now ready to go.  */
  struct r_debug *r = _dl_debug_initialize (0, args->nsid);
  r->r_state = RT_CONSISTENT;
  _dl_debug_state ();

  /* Only do lazy relocation if `LD_BIND_NOW' is not set.  */
  lazy = (mode & RTLD_BINDING_MASK) == RTLD_LAZY && GLRO(dl_lazy);

  /* Relocate the objects loaded.  We do this in reverse order so that copy
     relocs of earlier objects overwrite the data written by later objects.  */

  struct link_map *l = new;
  while (l->l_next)
    l = l->l_next;
  while (1)
    {
      if (! l->l_real->l_relocated)
        {
#ifdef SHARED
          if (__builtin_expect (GLRO(dl_profile) != NULL, 0))
            {
              /* If this here is the shared object which we want to profile
                 make sure the profile is started.  We can find out whether
                 this is necessary or not by observing the `_dl_profile_map'
                 variable.  If was NULL but is not NULL afterwars we must
                 start the profiling.  */
              struct link_map *old_profile_map = GL(dl_profile_map);

              _dl_relocate_object (l, l->l_scope, 1, 1);

              if (old_profile_map == NULL && GL(dl_profile_map) != NULL)
                {
                  /* We must prepare the profiling.  */
                  _dl_start_profile ();

                  /* Prevent unloading the object.  */
                  GL(dl_profile_map)->l_flags_1 |= DF_1_NODELETE;
                }
            }
          else
#endif
            _dl_relocate_object (l, l->l_scope, lazy, 0);
        }

      if (l == new)
        break;
      l = l->l_prev;
    }


> > So to solve this I'm now completely ignoring contents of .opd in target
> > memory, and instead always retrieve the contents from the BFD.  Those
> > will certainly be non-relocated, so applying the relocation offset by
> > hand will always result in the correct target address.
> > 
> > Is this a reasonable thing to do?
> 
> We went round the choice of where to read memory from several times on
> the previous patch, but I don't know the details.

OK, thanks.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Daniel Jacobowitz]

On Thu, May 15, 2008 at 06:14:50PM +0200, Ulrich Weigand wrote:
> Well, it seems that this field is set to RT_CONSISTENT *before* the 
> objects are relocated  (from elf/dl-open.c):

Yuck.  I thought it was just before calling _dl_init, but I was
clearly wrong.  This makes relying on the opd contents from the target
unreliable whenever we are not in the midst of a call, I guess.

    This makes the assumption that all .opd entries are always relocated
    by the same offset the section itself was relocated.  */

Do Linux kernel modules have an opd section?  I'd recommend the
routine the dwarf reader uses to apply relocations except it would be
very inefficient unless we cached the result.

> > We went round the choice of where to read memory from several times on
> > the previous patch, but I don't know the details.
> 
> OK, thanks.

It looks like the main issue was making sure we did read from the
target if the target_ops provided said to; the exception being the use
of tmp_bfd_target in solib-svr4.c.  I don't see a problem with your
change other than the offset assumption I mentioned above.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

Daniel Jacobowitz wrote:

>     This makes the assumption that all .opd entries are always relocated
>     by the same offset the section itself was relocated.  */
> 
> Do Linux kernel modules have an opd section?  I'd recommend the
> routine the dwarf reader uses to apply relocations except it would be
> very inefficient unless we cached the result.

Kernel modules generally have an opd section; as in other object files,
these will carry a R_PPC64_ADDR64 relocation pointing to .text + some
offset.  (In shared libraries we see a R_PPC_RELATIVE instead.)

That means my heuristics will probably go wrong when applied to an
object file (or kernel module).  When would that actually happen?
Should we be using the ppc-linux-tdep.c gdbarch for that?

I guess we could cache the result of symfile_relocate_debug_section
on the .opd section for the objfile.  One minor issue would be that
this function currently refuses to operate on non-SEC_DEBUGGING
sections -- is there a reason for that?

As I understand symfile_relocate_debug_section, this would still *not*
take the load address of a shared library into account, so that part
would still need to be applied manually, right?


> > > We went round the choice of where to read memory from several times on
> > > the previous patch, but I don't know the details.
> > 
> > OK, thanks.
> 
> It looks like the main issue was making sure we did read from the
> target if the target_ops provided said to; the exception being the use
> of tmp_bfd_target in solib-svr4.c.I don't see a problem with your
> change other than the offset assumption I mentioned above.

Thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

I wrote:

> I guess we could cache the result of symfile_relocate_debug_section
> on the .opd section for the objfile.

Huh, that's not as simple as I though: we're using target sections
here (not obj_sections) -- these are not even associated with any
objfile at this point ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Daniel Jacobowitz]

On Thu, May 15, 2008 at 07:36:33PM +0200, Ulrich Weigand wrote:
> Kernel modules generally have an opd section; as in other object files,
> these will carry a R_PPC64_ADDR64 relocation pointing to .text + some
> offset.  (In shared libraries we see a R_PPC_RELATIVE instead.)
> 
> That means my heuristics will probably go wrong when applied to an
> object file (or kernel module).  When would that actually happen?

Generally, they are loaded with either add-symbol-file (by hand or
autogenerated) specifying each section.  The KGDB guys also have a GDB
patch to do it automatically.  That's one of my targetted applications
of Python scripting.

> Should we be using the ppc-linux-tdep.c gdbarch for that?

It's a toss-up.  I think yes; either that, or the function descriptor
code has to move somewhere else that it would still be used, since it
still applies.

> I guess we could cache the result of symfile_relocate_debug_section
> on the .opd section for the objfile.  One minor issue would be that
> this function currently refuses to operate on non-SEC_DEBUGGING
> sections -- is there a reason for that?

Not that I can remember.

> As I understand symfile_relocate_debug_section, this would still *not*
> take the load address of a shared library into account, so that part
> would still need to be applied manually, right?

I believe so - ANOFFSET?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> > As I understand symfile_relocate_debug_section, this would still *not*
> > take the load address of a shared library into account, so that part
> > would still need to be applied manually, right?
> 
> I believe so - ANOFFSET?

ANOFFSET is also a obj_section thing, but we have target sections
in the convert_from_func_ptr callback ...

Why do we need to keep section data in both the target and the
objfile?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Daniel Jacobowitz]

On Fri, May 16, 2008 at 03:09:32PM +0200, Ulrich Weigand wrote:
> ANOFFSET is also a obj_section thing, but we have target sections
> in the convert_from_func_ptr callback ...
> 
> Why do we need to keep section data in both the target and the
> objfile?

Nowadays, I don't think we do.  Pedro has a patch to remove some of
it, but I think we could go even further - probably by eliminating
99% of the distinction between exec-file and symbol-file.  Not all of
it, since some is still clearly useful.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Pedro Alves]

A Friday 16 May 2008 15:51:04, Daniel Jacobowitz wrote:
> On Fri, May 16, 2008 at 03:09:32PM +0200, Ulrich Weigand wrote:
> > ANOFFSET is also a obj_section thing, but we have target sections
> > in the convert_from_func_ptr callback ...
> >
> > Why do we need to keep section data in both the target and the
> > objfile?
>
> Nowadays, I don't think we do.  Pedro has a patch to remove some of
> it, but I think we could go even further - probably by eliminating
> 99% of the distinction between exec-file and symbol-file.  Not all of
> it, since some is still clearly useful.

Hmmm, I'll post my dormant patch as an RFC in a sec, as a new thread.

-- 
Pedro Alves

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Thu, May 15, 2008 at 07:36:33PM +0200, Ulrich Weigand wrote:
> > Kernel modules generally have an opd section; as in other object files,
> > these will carry a R_PPC64_ADDR64 relocation pointing to .text + some
> > offset.  (In shared libraries we see a R_PPC_RELATIVE instead.)
> > 
> > That means my heuristics will probably go wrong when applied to an
> > object file (or kernel module).  When would that actually happen?
> 
> Generally, they are loaded with either add-symbol-file (by hand or
> autogenerated) specifying each section.  The KGDB guys also have a GDB
> patch to do it automatically.  That's one of my targetted applications
> of Python scripting.

Thinking about this, it seems this would mean that function descriptors
cannot work in kernel modules even today: add-symbol-file solely adds
an objfile (with obj_sections and so on); it does not modify the target
and its section table.  Right?

However, ppc64_linux_convert_from_func_ptr_addr *by design* only consults
the section table of the target -- this means it will never see those
extra symbol files anyway.

Am I missing something here?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Daniel Jacobowitz]

On Fri, May 16, 2008 at 10:35:14PM +0200, Ulrich Weigand wrote:
> Thinking about this, it seems this would mean that function descriptors
> cannot work in kernel modules even today: add-symbol-file solely adds
> an objfile (with obj_sections and so on); it does not modify the target
> and its section table.  Right?

Sounds like you're right.  I have never tried it, though many of our
customers use KGDB; perhaps not the ones who use PPC64.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, May 16, 2008 at 10:35:14PM +0200, Ulrich Weigand wrote:
> > Thinking about this, it seems this would mean that function descriptors
> > cannot work in kernel modules even today: add-symbol-file solely adds
> > an objfile (with obj_sections and so on); it does not modify the target
> > and its section table.  Right?
> 
> Sounds like you're right.  I have never tried it, though many of our
> customers use KGDB; perhaps not the ones who use PPC64.

Another patch that I didn't get to work on for a while ...

Given that it seems object files / kernel modules will not work with the
current setup anyway, and the patch I originally proposed does fix a
serious problem with Linux shared libraries in some cases, I'd like to
just apply that patch.

Here's an updated version with additional comments to summarize this
discussion.   Retested on powerpc64-linux.

Any objections to installing this patch?

Bye,
Ulrich


ChangeLog:

	* ppc-linux-tdep.c (ppc64_linux_convert_from_func_ptr_addr): Read
	and manually relocate .opd contents from BFD instead of reading
	them from target memory.


diff -urNp gdb-orig/gdb/ppc-linux-tdep.c gdb-head/gdb/ppc-linux-tdep.c
--- gdb-orig/gdb/ppc-linux-tdep.c	2008-05-26 19:48:46.000000000 +0200
+++ gdb-head/gdb/ppc-linux-tdep.c	2008-08-14 18:00:17.398719879 +0200
@@ -601,7 +601,36 @@ ppc64_linux_convert_from_func_ptr_addr (
 
   /* Check if ADDR points to a function descriptor.  */
   if (s && strcmp (s->the_bfd_section->name, ".opd") == 0)
-    return get_target_memory_unsigned (targ, addr, 8);
+    {
+      /* There may be relocations that need to be applied to the .opd 
+	 section.  Unfortunately, this function may be called at a time
+	 where these relocations have not yet been performed -- this can
+	 happen for example shortly after a library has been loaded with
+	 dlopen, but ld.so has not yet applied the relocations.
+
+	 To cope with both the case where the relocation has been applied,
+	 and the case where it has not yet been applied, we do *not* read
+	 the (maybe) relocated value from target memory, but we instead
+	 read the non-relocated value from the BFD, and apply the relocation
+	 offset manually.
+
+	 This makes the assumption that all .opd entries are always relocated
+	 by the same offset the section itself was relocated.  This should
+	 always be the case for GNU/Linux executables and shared libraries.
+	 Note that other kind of object files (e.g. those added via
+	 add-symbol-files) will currently never end up here anyway, as this
+	 function accesses *target* sections only; only the main exec and
+	 shared libraries are ever added to the target.  */
+
+      gdb_byte buf[8];
+      int res;
+
+      res = bfd_get_section_contents (s->bfd, s->the_bfd_section,
+				      &buf, addr - s->addr, 8);
+      if (res != 0)
+	return extract_unsigned_integer (buf, 8)
+		- bfd_section_vma (s->bfd, s->the_bfd_section) + s->addr;
+   }
 
   return addr;
 }

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix problem with (maybe) non-relocated .opd section on powerpc64-linux

[Ulrich Weigand]

> 	* ppc-linux-tdep.c (ppc64_linux_convert_from_func_ptr_addr): Read
> 	and manually relocate .opd contents from BFD instead of reading
> 	them from target memory.

I've checked this in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com