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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Mon, 24 Jun 2013, Pedro Alves wrote:

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

 Absolutely, I just referred to passing an extra optional NAME argument 
(that was originally used) as the old mistake.

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

 Well, the hack you've mentioned below takes care of this for MIPS/Linux 
with the use of mips_linux_in_dynsym_resolve_code, however other MIPS 
targets using ELF shared libraries (NetBSD for example) are indeed left 
unhandled.  Not sure why MIPS/IRIX defines its irix_in_dynsym_resolve_code 
to return 0 in all cases, IRIX certainly uses MIPS stubs (though obviously 
not PLT).

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

 You obviously mean in_mips_stubs_section here, don't you?

> 
>   /* 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.)

 Indeed, mips_linux_in_dynsym_resolve_code will then go in its current 
form, becoming gdbarch's in_solib_trampoline backend.  It sounds like a 
plan to me, thanks for the hint, I'll give it a shot.

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

 Well, it may be my view of things -- I see SVR4 (as far as binary file 
formats are concerned; there's certainly a lot beyond that in SVR4) as the 
set of features as defined back in mid 1990s, starting from the ELF format 
itself and including the way ELF shared libraries are handled in 
particular.  So for me solib-svr4.c would then be the base feature set 
common to all ELF shared library systems, and then any features beyond 
that, added as newer systems invented them, included in their respective 
add-on modules.

 E.g. if an ELF shared library system FrobOS running on an odd processor 
added a capability to map segments of a library to memory in the reverse 
direction, but supported all the other usual library features, then it 
would use solib-svr4.c for the usual services and additionally solib-rev.c 
for the reverse-mapping features.  But as I say, perhaps it's my view of 
things that is reversed. ;)

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

 Hmm, I never really dug into this area, but from your description I 
gather that the line between the individual ELF solib-*.c modules is not 
as clear as it probably should be.  I.e. I don't know exactly what the 
relationship between SunOS and a generic SVR4 system wrt shared libraries 
is, but assuming SVR4 is a strict superset of SunOS, then I'd expect 
solib-svr4.c to implement the SVR4 additions only and rely on 
solib-sunos.c for the rest.  All the SVR4 systems and ones that are a 
superset of SVR4 features would useboth solib-svr4.c and solib-sunos.c, 
and obviously SunOS would only use solib-sunos.c.

 Contrariwise if a generic SVR4 system was actually not a superset of 
SunOS and the latter had some extra features beyond a common ELF shared 
library feature set, then a base ELF shared library module, say 
solib-elf.c would handle the common feature set.  Than solib-sunos.c would 
handle the SunOS additions and consequently SunOS would use solib-elf.c 
and solib-sunos.c.  Similarly a generic SVR4 system would use solib-elf.c 
and solib-svr4.c.  And that dreaded FrobOS would use all of solib-elf.c, 
solib-svr4.c and solib-rev.c.

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

 Good!

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

 OK, thanks.  Here's the final ChangeLog entry I've committed.  I'll look 
into redesigning the pieces you've mentioned above, but I'll ask for 
assistance with testing as I have no access to non-Linux MIPS ELF shared 
library systems.

2013-06-24  Maciej W. Rozycki  <macro@codesourcery.com>

	* objfiles.h (pc_in_section): New prototype.
	(in_plt_section): Remove name argument, replace prototype with
	static inline function.
	* mips-tdep.h: Include "objfiles.h".
	(in_mips_stubs_section): New function.
	* hppa-tdep.h (gdbarch_tdep): Remove name argument of
	in_solib_call_trampoline member.
	(hppa_in_solib_call_trampoline): Remove name argument.
	* objfiles.c (pc_in_section): New function.
	(in_plt_section): Remove function.
	* mips-linux-tdep.c: Include "objfiles.h".
	(mips_linux_in_dynsym_stub): Call in_mips_stubs_section.  Remove
	name argument.  Return 1 rather than the low 16-bit halfword of
	any instruction examined.
	(mips_linux_in_dynsym_resolve_code): Update
	mips_linux_in_dynsym_stub call accordingly.
	* mips-tdep.c (mips_stub_frame_sniffer): Use in_mips_stubs_section
	rather than an equivalent hand-coded sequence.
	* hppa-hpux-tdep.c (in_opd_section): Remove function.
	(hppa32_hpux_in_solib_call_trampoline): Remove name argument.
	(hppa64_hpux_in_solib_call_trampoline): Likewise.
	(hppa64_hpux_find_global_pointer): Use pc_in_section rather than
	in_opd_section.
	* hppa-tdep.c (hppa_stub_unwind_sniffer): Remove name argument
	on call to tdep->in_solib_call_trampoline.
	(hppa_in_solib_call_trampoline): Remove name argument, update
	according to in_plt_section change.
	(hppa_skip_trampoline_code): Update according to in_plt_section
	change.
	* aarch64-tdep.c (aarch64_stub_unwind_sniffer): Likewise.
	* arm-symbian-tdep.c (arm_symbian_skip_trampoline_code):
	Likewise.
	* arm-tdep.c (arm_stub_unwind_sniffer): Likewise.
	* hppa-linux-tdep.c (hppa_linux_find_global_pointer): Likewise.
	* hppabsd-tdep.c (hppabsd_find_global_pointer): Likewise.
	* nios2-tdep.c (nios2_stub_frame_sniffer): Likewise.
	* nto-tdep.c (nto_relocate_section_addresses): Likewise.
	* s390-tdep.c (s390_stub_frame_sniffer): Likewise.
	* sh-tdep.c (sh_stub_unwind_sniffer): Likewise.
	* solib-dsbt.c (dsbt_in_dynsym_resolve_code): Likewise.
	* solib-frv.c (frv_in_dynsym_resolve_code): Likewise.
	* solib-svr4.c (svr4_in_dynsym_resolve_code): Likewise.
	* solib-target.c (solib_target_in_dynsym_resolve_code): Likewise.
	* sparc-tdep.c (sparc_analyze_prologue): Likewise.
	* tic6x-tdep.c (tic6x_stub_unwind_sniffer): Likewise.

  Maciej