Re: [PING^2][PATCH] in_plt_section: support alternate stub section names

[Pedro Alves <palves at redhat dot com>]

On 06/22/2013 12:32 AM, Maciej W. Rozycki wrote:
> On Fri, 21 Jun 2013, Pedro Alves wrote:
> 
>>>  Well, I have focused here, perhaps mistakenly, on the intended use of the 
>>> call -- to determine whether the PC is in a dynamic function call 
>>> trampoline.  Contrary to the description we currently have at 
>>> in_plt_section, .plt is not -- per SVR4 ABI -- a standard name of the 
>>> trampoline section.  The name (and the presence of any such section in the 
>>> first place) is actually left to processor-specific ABI supplements.
>>>
>>>  For many processors .plt has indeed been the choice, but for MIPS .plt 
>>> has only recently been added as an ABI extension.  The original MIPS SVR4 
>>> processor-specific ABI supplement defined no specific section name to be 
>>> used for its .plt equivalent.  I can't easily check what IRIX tools chose 
>>> for this section's name (if anything; in a final executable you can have 
>>> ELF segments whose contents are not mapped to any section).  Binutils 
>>> chose .stubs long ago and more recently switched to .MIPS.stubs.  This may 
>>> well be the same names IRIX used in different versions (compare .reginfo 
>>> vs .MIPS.options standard MIPS SVR4 psABI sections).
>>
>> Right, but extending in_plt_section torwards a general "in trampoline
>> section" would imply to me hiding platform details underneath, through
>> e.g., recording the section name in gdbarch, as calls in common
>> code wouldn't known about such target details.
> 
>  FWIW, this has been the original design of the internal API considered 
> here, that I excavated and quoted for the purpose of the original 
> submission:
> 
> http://sourceware.org/ml/gdb-patches/2013-06/msg00150.html
> 
> Not that we should repeat or maintain old mistakes, that is.

Hmm.  Using macros rather than gdbarch is something we've
moved away from since and we don't want to go back to,
of course.

But something is looking backwards to me.  Not so sure I'd
call it an old mistake.  For these in_plt_section calls:

nto-tdep.c:321:  if (in_plt_section (pc, NULL))
solib-dsbt.c:767:         || in_plt_section (pc, NULL));
solib-frv.c:451:          || in_plt_section (pc, NULL));
solib-svr4.c:1535:        || in_plt_section (pc, NULL)
solib-target.c:479:  return in_plt_section (pc, NULL);

it sounds like we really want to also check if the PC is in the
MIPS stubs section.  These are all in
target_solib_ops->in_dynsym_resolve_code implementations, used by
this bit of infrun:

  if (execution_direction != EXEC_REVERSE
      && ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE
      && in_solib_dynsym_resolve_code (stop_pc))
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    {
      CORE_ADDR pc_after_resolver =
	gdbarch_skip_solib_resolver (gdbarch, stop_pc);

      if (debug_infrun)
	 fprintf_unfiltered (gdb_stdlog,
			     "infrun: stepped into dynsym resolve code\n");

      if (pc_after_resolver)
	{
	  /* Set up a step-resume breakpoint at the address
	     indicated by SKIP_SOLIB_RESOLVER.  */
	  struct symtab_and_line sr_sal;

	  init_sal (&sr_sal);
	  sr_sal.pc = pc_after_resolver;
	  sr_sal.pspace = get_frame_program_space (frame);

	  insert_step_resume_breakpoint_at_sal (gdbarch,
						sr_sal, null_frame_id);
	}

      keep_going (ecs);
      return;
    }

in order to skip resolve code.  In the SVR4 case, that's:

/* Return 1 if PC lies in the dynamic symbol resolution code of the
   SVR4 run time loader.  */

int
svr4_in_dynsym_resolve_code (CORE_ADDR pc)
{
  struct svr4_info *info = get_svr4_info ();

  return ((pc >= info->interp_text_sect_low
	   && pc < info->interp_text_sect_high)
	  || (pc >= info->interp_plt_sect_low
	      && pc < info->interp_plt_sect_high)
	  || in_plt_section (pc, NULL)
             ^^^^^^^^^^^^^^^^^^^^^^^^^
	  || in_gnu_ifunc_stub (pc));
}

So it sounds like MIPS is still missing out a bit on this
optimization, single-stepping while in the .MIPS.stubs section,
even with your change.

I think it'd be better to reinstate IN_SOLIB_TRAMPOLINE, modernized
as a gdbarch hook.  The default gdbarch_in_solib_trampoline
would then be in_plt_section.  But MIPS would install its
own version, based on the existing mips_linux_in_dynsym_resolve_code,
with your changes:

static int
mips_linux_in_solib_trampoline (CORE_ADDR pc)
{
  /* Check whether the PC is in the .plt section, used by non-PIC
     executables.  */
  if (in_plt_section (pc))
     return 1;

  /* Likewise for the stubs.  They live in the .MIPS.stubs section these
     days, so we check if the PC is within, than fall back to a pattern
     match.  */
  if (mips_linux_in_dynsym_stub (pc))
     return 1;

  /* Pattern match for the stub.  It would be nice if there were a
     more efficient way to avoid this check.  */
  if (mips_linux_in_dynsym_stub (pc, NULL))
    return 1;

  return 0;
}


svr4_in_dynsym_resolve_code would then be adjusted like this:

int
svr4_in_dynsym_resolve_code (CORE_ADDR pc)
{
  struct svr4_info *info = get_svr4_info ();

  return ((pc >= info->interp_text_sect_low
	   && pc < info->interp_text_sect_high)
	  || (pc >= info->interp_plt_sect_low
	      && pc < info->interp_plt_sect_high)
-	  || in_plt_section (pc)
+	  || gdbarch_in_solib_trampoline (pc)
	  || in_gnu_ifunc_stub (pc));
}

Likewise for the other target_so_ops in_dynsym_resolve_code
hooks I pointed out at the top of the email.

As bonus, we'd no longer need the mips_svr4_so_ops ugly
hack in mips-linux-tdep.c at all:

  /* Initialize this lazily, to avoid an initialization order
     dependency on solib-svr4.c's _initialize routine.  */
  if (mips_svr4_so_ops.in_dynsym_resolve_code == NULL)
    {
      mips_svr4_so_ops = svr4_so_ops;
      mips_svr4_so_ops.in_dynsym_resolve_code
	= mips_linux_in_dynsym_resolve_code;
    }
  set_solib_ops (gdbarch, &mips_svr4_so_ops);


(the fact that mips_svr4_so_ops exists at all is what I'm
calling a hack.)

>> static inline is fine with me.  However, what I really dislike is the
>> inclusion of solib-svr4.h in parts of the debugger that have nothing
>> to do with SVR4, or even are implementing a different shared library
>> support backend, like solib-frv.c solib-dsbt.c, solib-target.c, etc.
>> That's really an abstraction violation, there bits have nothing to so
>> with SVR4.
> 
>  Umm, perhaps we have a file naming confusion in our sources 

GDB supports various different shared library mechanisms.
Each is abstracted out behind a "struct target_so_ops" instance,
and implemented on a solib-*.c file:

$ ls solib-*.c
solib-aix.c     solib-dsbt.c  solib-ia64-hpux.c  solib-osf.c   solib-som.c
solib-sunos.c  solib-target.c  solib-darwin.c  solib-frv.c   solib-irix.c
solib-pa64.c  solib-spu.c  solib-svr4.c

Some of these modules need to export functions to other modules, and
therefore have a corresponding .h file.

Not sure what isn't clear in the naming.

> or I am
> missing something.  The thing is all of the ELF stuff and its shared 
> library features such as the PLT are inherently SVR4 concepts.  Now there 
> are I think two possible options -- either our solib-svr4.h stuff is not 
> entirely SVR4 and includes things beyond or we support some pre-SVR4 
> systems (SunOS perhaps?) that already included early ELF support.  The 
> latter unfortunately I cannot comment on as I lack background information 
> here.  So what's the story behind it?

Not sure exactly what you're asking, but the history I believe was
that solib.c originally started out as supporting SunOS and then SVR4
came later, an then over time target_so_ops was invented, and SVR4 ended
up in solib-svr4.c, SunOS aged, and ended up split in solib-sunos.c.
The solib-target.c came along, and that one is (supposedly) entirely target_ops
driven.  IOW, the (remote) target feeds GDB the list of shared libraries,
reports events for load/unload, etc, unlike with svr4 and others, where GDB
coordinates with the loader by reading the loaders globals, and gets reported
of events through breakpoints and similars.

>  That written, given that this patch is blocking a cascade of changes 
> finding the exactly right location for in_plt_section seems secondary to 
> me and the objfiles module is where it already resides.  Moving it to 
> objfiles.h as a static inline function will already be some progress as it 
> won't be compiled in cases where it's not actually used (e.g. plain COFF).
> 
>  So here's a version you've suggested.  Even with this change applied we 
> can continue looking for a better place for in_plt_section.

Alright.

>  OK to apply?

Please mention the inclusion of "objfiles.h" in the ChangeLog,
in the files that needed it.  Okay with that change, but I'd
like the idea above to be explored.

-- 
Pedro Alves