Re: [PATCH] microMIPS support (Linux signal trampolines)

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

On Mon, 21 May 2012, Maciej W. Rozycki wrote:

> On Fri, 18 May 2012, Mark Kettenis wrote:
> 
> > >  To see if we need to check whether the execution mode selected matches 
> > > the given trampoline I have checked what the bit patterns of all the 
> > > trampoline sequences decode to in the opposite instruction set.  This 
> > > produced useless or at least unusual code in most cases, for example:
> > > 
> > > microMIPS/EB, o32 sigreturn, decoded as MIPS:
> > > 	30401017 	andi	zero,v0,0x1017
> > > 	00008b7c 	dsll32	s1,zero,0xd
> > > 
> > > MIPS/EL, o32 sigreturn, decoded as microMIPS:
> > > 	1017 2402 	addi	zero,s7,9218
> > > 	000c 0000 	sll	zero,t0,0x0
> > > 
> > > However in some corner cases reasonable code can mimic a trampoline, for 
> > > example:
> > > 
> > > MIPS/EB, n32 rt_sigreturn, decoded as microMIPS:
> > > 	2402      	sll	s0,s0,1
> > > 	1843 0000 	sb	v0,0(v1)
> > > 	000c 0f3c 	jr	t0
> > > 
> > > -- here the first instruction is a 16-bit one making things nastier even 
> > > as there are some other microMIPS instructions whose first 16-bit halfword 
> > > is 0x000c and therefore matches this whole trampoline pattern.
> > 
> > On some OSes the signal trampolines are guaranteed to have a certain
> > alignment.  Is that the case for MIPS Linux as well perhaps?  Or would
> > that not help you?
> 
>  The alignment is always the same -- 4 bytes, I would even say that the 
> page offsets are the same too.  The structure used by the kernel is the 
> same in both cases and also in both cases two 4-byte instructions are 
> used.  The ISA bit is the only difference.
> 
>  This is how it looks in the kernel sources, BTW:
> 
> #ifdef CONFIG_32BIT
> struct mips_vdso {
> 	u32 signal_trampoline[2];
> 	u32 rt_signal_trampoline[2];
> };
> #else  /* !CONFIG_32BIT */
> struct mips_vdso {
> 	u32 o32_signal_trampoline[2];
> 	u32 o32_rt_signal_trampoline[2];
> 	u32 rt_signal_trampoline[2];
> 	u32 n32_rt_signal_trampoline[2];
> };
> #endif /* CONFIG_32BIT */
> 
> the instructions for each trampoline are filled by the kernel at 
> bootstrap, using the kernel built-in "assembler" -- originally contributed 
> by Thiemo Seufer and recently updated for microMIPS support -- like this:
> 
> static void __init install_trampoline(u32 *tramp, unsigned int sigreturn)
> {
> 	uasm_i_addiu(&tramp, 2, 0, sigreturn);	/* li v0, sigreturn */
> 	uasm_i_syscall(&tramp, 0);
> }
> 
> -- uasm_i_addiu and uasm_i_syscall produce standard MIPS or microMIPS 
> machine instructions in standard MIPS and microMIPS kernel binaries 
> respectively.
> 
>  The VDSO itself is currently placed beyond the stack.  The VDSO is a page 
> and therefore its size and base address depend on the processor's page 
> size that is at the moment a kernel build-time configuration option, one 
> of 4kB, 16kB or 64kB (MIPS processors themselves can support page sizes 
> from 1kB up to 256TB; usually multiple page sizes are supported in a given 
> chip and are run-time selectable on a per-TLB-entry basis, i.e. a mixture 
> of page mappings of different sizes is possible, but Linux only supports 
> uniform page sizes; unlike IRIX IIUC).  AFAIK the presence and the 
> location of the VDSO is an internal implementation detail and does not 
> constitute a part of the Linux ABI.
> 
>  In older kernels these trampolines used to be located on user stack, so I 
> am even more convinced that we cannot assume anything special about the 
> VDSO in this context (obviously those older kernels did not support 
> microMIPS processors either).
> 
>  References:
> 
> http://sourceware.org/ml/gdb-patches/2010-02/msg00546.html
> http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=1266538385-29088-1-git-send-email-ddaney%40caviumnetworks.com
> 
>  Besides, the ISA bit is what hardware uses to switch between modes, so I 
> insist that it is our best bet to rely on it rather than trying to invent 
> smart ways to avoid referring to it that are guaranteed to fail once the 
> indirect dependencies have outsmarted us.
> 
> > > Index: gdb-fsf-trunk-quilt/gdb/tramp-frame.c
> > > ===================================================================
> > > --- gdb-fsf-trunk-quilt.orig/gdb/tramp-frame.c	2012-02-24 15:23:42.000000000 +0000
> > > +++ gdb-fsf-trunk-quilt/gdb/tramp-frame.c	2012-05-18 20:03:53.775469792 +0100
> > > @@ -87,6 +87,12 @@ tramp_frame_start (const struct tramp_fr
> > >    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > >    int ti;
> > >  
> > > +  /* Check if we can use this trampoline.  */
> > > +  if (tramp->validate)
> > > +    pc = tramp->validate (tramp, this_frame, pc);
> > > +  if (pc == 0)
> > > +    return 0;
> > 
> > I suppose chances are small we'll ever have a platform with
> > trampolines at address 0, but nevertheless, wouldn't it be more
> > correct to write
> > 
> >   if (tramp->validate)
> >     {
> >       pc = tramp->validate (tramp, this_frame, pc);
> >       if (pc == 0)
> >         return 0;
> >     }
> > 
> > as you're checking for the magic return value?
> 
>  Then the value of zero suddenly won't work if a platform where zero is a 
> valid PC needs to provide a validator.
> 
>  If you think the special treatment of a zero PC is a real issue, then I'd 
> rather trivially rewrite the change making the result of ->validate 
> boolean and passing the PC by reference, to be modified by the callee if 
> needed.  And after some thinking I have concluded there's no harm in doing 
> this anyway, there's no performance hit for platforms that do not need to 
> tweak the PC (to say nothing of these that do not need to validate at 
> all), and the hit for ones that do is probably negligible.
> 
>  Here's the result -- diff to previous and a new version.  As a side 
> effect it neatly avoids the problem of an overwide prototype I have 
> otherwise found no way to solve.

 I'd like to get back to this change and review.  Here's the same code 
regenerated against current trunk.  Can we please get consensus on changes 
to tramp-frame.[ch]?

 Regression-tested with the mips-linux-gnu target and the following 
multilibs:

-EB
-EB -msoft-float
-EB -mips16
-EB -mips16 -msoft-float
-EB -mmicromips
-EB -mmicromips -msoft-float
-EB -mabi=n32
-EB -mabi=n32 -msoft-float
-EB -mabi=64
-EB -mabi=64 -msoft-float

and the -EL variants of same with no regressions.

2014-09-28  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* tramp-frame.h (tramp_frame): Add validate member.
	* tramp-frame.c (tramp_frame_start): Validate trampoline before
	scanning.
	* mips-linux-tdep.c (MICROMIPS_INST_LI_V0): New macro.
	(MICROMIPS_INST_POOL32A, MICROMIPS_INST_SYSCALL): Likewise.
	(mips_linux_o32_sigframe): Initialize validate member.
	(mips_linux_o32_rt_sigframe): Likewise.
	(mips_linux_n32_rt_sigframe): Likewise.
	(mips_linux_n64_rt_sigframe): Likewise.
	(micromips_linux_o32_sigframe): New variable.
	(micromips_linux_o32_rt_sigframe): Likewise.
	(micromips_linux_n32_rt_sigframe): Likewise.
	(micromips_linux_n64_rt_sigframe): Likewise.
	(mips_linux_o32_sigframe_init): Handle microMIPS trampolines.
	(mips_linux_n32n64_sigframe_init): Likewise.
	(mips_linux_sigframe_validate): New function.
	(micromips_linux_sigframe_validate): Likewise.
	(mips_linux_init_abi): Install microMIPS trampoline unwinders.

  Maciej

gdb-micromips-linux-sigtramp.diff
Index: gdb-fsf-trunk-quilt/gdb/mips-linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-linux-tdep.c	2014-08-23 01:11:20.568972186 +0100
+++ gdb-fsf-trunk-quilt/gdb/mips-linux-tdep.c	2014-08-23 01:17:50.608926711 +0100
@@ -840,6 +840,14 @@ static void mips_linux_n32n64_sigframe_i
 					     struct trad_frame_cache *this_cache,
 					     CORE_ADDR func);
 
+static int mips_linux_sigframe_validate (const struct tramp_frame *self,
+					 struct frame_info *this_frame,
+					 CORE_ADDR *pc);
+
+static int micromips_linux_sigframe_validate (const struct tramp_frame *self,
+					      struct frame_info *this_frame,
+					      CORE_ADDR *pc);
+
 #define MIPS_NR_LINUX 4000
 #define MIPS_NR_N64_LINUX 5000
 #define MIPS_NR_N32_LINUX 6000
@@ -855,6 +863,10 @@ static void mips_linux_n32n64_sigframe_i
 #define MIPS_INST_LI_V0_N32_RT_SIGRETURN 0x24020000 + MIPS_NR_N32_rt_sigreturn
 #define MIPS_INST_SYSCALL 0x0000000c
 
+#define MICROMIPS_INST_LI_V0 0x3040
+#define MICROMIPS_INST_POOL32A 0x0000
+#define MICROMIPS_INST_SYSCALL 0x8b7c
+
 static const struct tramp_frame mips_linux_o32_sigframe = {
   SIGTRAMP_FRAME,
   4,
@@ -863,7 +875,8 @@ static const struct tramp_frame mips_lin
     { MIPS_INST_SYSCALL, -1 },
     { TRAMP_SENTINEL_INSN, -1 }
   },
-  mips_linux_o32_sigframe_init
+  mips_linux_o32_sigframe_init,
+  mips_linux_sigframe_validate
 };
 
 static const struct tramp_frame mips_linux_o32_rt_sigframe = {
@@ -873,7 +886,8 @@ static const struct tramp_frame mips_lin
     { MIPS_INST_LI_V0_RT_SIGRETURN, -1 },
     { MIPS_INST_SYSCALL, -1 },
     { TRAMP_SENTINEL_INSN, -1 } },
-  mips_linux_o32_sigframe_init
+  mips_linux_o32_sigframe_init,
+  mips_linux_sigframe_validate
 };
 
 static const struct tramp_frame mips_linux_n32_rt_sigframe = {
@@ -884,7 +898,8 @@ static const struct tramp_frame mips_lin
     { MIPS_INST_SYSCALL, -1 },
     { TRAMP_SENTINEL_INSN, -1 }
   },
-  mips_linux_n32n64_sigframe_init
+  mips_linux_n32n64_sigframe_init,
+  mips_linux_sigframe_validate
 };
 
 static const struct tramp_frame mips_linux_n64_rt_sigframe = {
@@ -895,7 +910,63 @@ static const struct tramp_frame mips_lin
     { MIPS_INST_SYSCALL, -1 },
     { TRAMP_SENTINEL_INSN, -1 }
   },
-  mips_linux_n32n64_sigframe_init
+  mips_linux_n32n64_sigframe_init,
+  mips_linux_sigframe_validate
+};
+
+static const struct tramp_frame micromips_linux_o32_sigframe = {
+  SIGTRAMP_FRAME,
+  2,
+  {
+    { MICROMIPS_INST_LI_V0, -1 },
+    { MIPS_NR_sigreturn, -1 },
+    { MICROMIPS_INST_POOL32A, -1 },
+    { MICROMIPS_INST_SYSCALL, -1 },
+    { TRAMP_SENTINEL_INSN, -1 }
+  },
+  mips_linux_o32_sigframe_init,
+  micromips_linux_sigframe_validate
+};
+
+static const struct tramp_frame micromips_linux_o32_rt_sigframe = {
+  SIGTRAMP_FRAME,
+  2,
+  {
+    { MICROMIPS_INST_LI_V0, -1 },
+    { MIPS_NR_rt_sigreturn, -1 },
+    { MICROMIPS_INST_POOL32A, -1 },
+    { MICROMIPS_INST_SYSCALL, -1 },
+    { TRAMP_SENTINEL_INSN, -1 } },
+  mips_linux_o32_sigframe_init,
+  micromips_linux_sigframe_validate
+};
+
+static const struct tramp_frame micromips_linux_n32_rt_sigframe = {
+  SIGTRAMP_FRAME,
+  2,
+  {
+    { MICROMIPS_INST_LI_V0, -1 },
+    { MIPS_NR_N32_rt_sigreturn, -1 },
+    { MICROMIPS_INST_POOL32A, -1 },
+    { MICROMIPS_INST_SYSCALL, -1 },
+    { TRAMP_SENTINEL_INSN, -1 }
+  },
+  mips_linux_n32n64_sigframe_init,
+  micromips_linux_sigframe_validate
+};
+
+static const struct tramp_frame micromips_linux_n64_rt_sigframe = {
+  SIGTRAMP_FRAME,
+  2,
+  {
+    { MICROMIPS_INST_LI_V0, -1 },
+    { MIPS_NR_N64_rt_sigreturn, -1 },
+    { MICROMIPS_INST_POOL32A, -1 },
+    { MICROMIPS_INST_SYSCALL, -1 },
+    { TRAMP_SENTINEL_INSN, -1 }
+  },
+  mips_linux_n32n64_sigframe_init,
+  micromips_linux_sigframe_validate
 };
 
 /* *INDENT-OFF* */
@@ -1015,7 +1086,8 @@ mips_linux_o32_sigframe_init (const stru
   const struct mips_regnum *regs = mips_regnum (gdbarch);
   CORE_ADDR regs_base;
 
-  if (self == &mips_linux_o32_sigframe)
+  if (self == &mips_linux_o32_sigframe
+      || self == &micromips_linux_o32_sigframe)
     sigcontext_base = frame_sp + SIGFRAME_SIGCONTEXT_OFFSET;
   else
     sigcontext_base = frame_sp + RTSIGFRAME_SIGCONTEXT_OFFSET;
@@ -1216,7 +1288,8 @@ mips_linux_n32n64_sigframe_init (const s
   CORE_ADDR sigcontext_base;
   const struct mips_regnum *regs = mips_regnum (gdbarch);
 
-  if (self == &mips_linux_n32_rt_sigframe)
+  if (self == &mips_linux_n32_rt_sigframe
+      || self == &micromips_linux_n32_rt_sigframe)
     sigcontext_base = frame_sp + N32_SIGFRAME_SIGCONTEXT_OFFSET;
   else
     sigcontext_base = frame_sp + N64_SIGFRAME_SIGCONTEXT_OFFSET;
@@ -1286,6 +1359,22 @@ mips_linux_n32n64_sigframe_init (const s
   trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
 }
 
+static int
+mips_linux_sigframe_validate (const struct tramp_frame *self,
+			      struct frame_info *this_frame,
+			      CORE_ADDR *pc)
+{
+  return mips_pc_is_mips (*pc);
+}
+
+static int
+micromips_linux_sigframe_validate (const struct tramp_frame *self,
+				   struct frame_info *this_frame,
+				   CORE_ADDR *pc)
+{
+  return mips_pc_is_micromips (get_frame_arch (this_frame), *pc);
+}
+
 /* Implement the "write_pc" gdbarch method.  */
 
 static void
@@ -1569,6 +1658,9 @@ mips_linux_init_abi (struct gdbarch_info
 					mips_linux_get_longjmp_target);
 	set_solib_svr4_fetch_link_map_offsets
 	  (gdbarch, svr4_ilp32_fetch_link_map_offsets);
+	tramp_frame_prepend_unwinder (gdbarch, &micromips_linux_o32_sigframe);
+	tramp_frame_prepend_unwinder (gdbarch,
+				      &micromips_linux_o32_rt_sigframe);
 	tramp_frame_prepend_unwinder (gdbarch, &mips_linux_o32_sigframe);
 	tramp_frame_prepend_unwinder (gdbarch, &mips_linux_o32_rt_sigframe);
 	set_xml_syscall_file_name ("syscalls/mips-o32-linux.xml");
@@ -1584,6 +1676,8 @@ mips_linux_init_abi (struct gdbarch_info
 	   except that the quiet/signalling NaN bit is reversed (GDB
 	   does not distinguish between quiet and signalling NaNs).  */
 	set_gdbarch_long_double_format (gdbarch, floatformats_ia64_quad);
+	tramp_frame_prepend_unwinder (gdbarch,
+				      &micromips_linux_n32_rt_sigframe);
 	tramp_frame_prepend_unwinder (gdbarch, &mips_linux_n32_rt_sigframe);
 	set_xml_syscall_file_name ("syscalls/mips-n32-linux.xml");
 	break;
@@ -1598,6 +1692,8 @@ mips_linux_init_abi (struct gdbarch_info
 	   except that the quiet/signalling NaN bit is reversed (GDB
 	   does not distinguish between quiet and signalling NaNs).  */
 	set_gdbarch_long_double_format (gdbarch, floatformats_ia64_quad);
+	tramp_frame_prepend_unwinder (gdbarch,
+				      &micromips_linux_n64_rt_sigframe);
 	tramp_frame_prepend_unwinder (gdbarch, &mips_linux_n64_rt_sigframe);
 	set_xml_syscall_file_name ("syscalls/mips-n64-linux.xml");
 	break;
Index: gdb-fsf-trunk-quilt/gdb/tramp-frame.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/tramp-frame.c	2014-08-23 01:11:20.568972186 +0100
+++ gdb-fsf-trunk-quilt/gdb/tramp-frame.c	2014-08-23 01:17:50.608926711 +0100
@@ -86,6 +86,10 @@ tramp_frame_start (const struct tramp_fr
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int ti;
 
+  /* Check if we can use this trampoline.  */
+  if (tramp->validate && !tramp->validate (tramp, this_frame, &pc))
+    return 0;
+
   /* Search through the trampoline for one that matches the
      instruction sequence around PC.  */
   for (ti = 0; tramp->insn[ti].bytes != TRAMP_SENTINEL_INSN; ti++)
Index: gdb-fsf-trunk-quilt/gdb/tramp-frame.h
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/tramp-frame.h	2014-08-23 01:00:49.498973998 +0100
+++ gdb-fsf-trunk-quilt/gdb/tramp-frame.h	2014-08-23 01:17:50.608926711 +0100
@@ -69,6 +69,13 @@ struct tramp_frame
 		struct frame_info *this_frame,
 		struct trad_frame_cache *this_cache,
 		CORE_ADDR func);
+  /* Return non-zero if the tramp-frame is valid for the PC requested.
+     Adjust the PC to point to the address to check the instruction
+     sequence against if required.  If this is NULL, then the tramp-frame
+     is valid for any PC.  */
+  int (*validate) (const struct tramp_frame *self,
+		   struct frame_info *this_frame,
+		   CORE_ADDR *pc);
 };
 
 void tramp_frame_prepend_unwinder (struct gdbarch *gdbarch,