[RFC] mips-tdep.c: Fix mips16 bit rot

[RFC] mips-tdep.c: Fix mips16 bit rot

[Kevin Buettner]

In the course of some recent mips testing, I noticed that GDB's mips16
support has been non-functional for quite a while.

A change which occurred between "2007-01-21 09:49" and
"2007-01-21 09:50" (use those strings as arguments to cvs's -D
switches...) caused all line number information in mips16 programs to
be marked using the mips16 convention of setting the low bit of the
address.  Prior to that patch, the behavior was mixed - function entry
points would have the mips16 bit stripped, whereas other lines would
have it in place.  This change caused a search for a function, e.g
main(), without the mips16 bit set, to be considered as part of
whatever function preceded main().  As a consequence,
skip_prologue_using_sal() would return the address of the first
instruction in main() rather than the address just past the prologue.

(I don't see anything wrong with the patch that was committed in Jan
2007, and, quite frankly, I'm surprised that it had this affect on
mips16 behavior at all!)

Note that gdbarch_addr_bits_remove() is invoked in
dwarf_decode_lines() when determining the address associated with a
line.  The fact that mips_addr_bits_remove(), in the patch below,
strips the low bit of the address is what causes the address to now be
stored in the line table without the low bit.

The changes to mips_read_pc() and mips_unwind_pc() were needed to
correct potential mismatches between addresses that GDB stores
internally and actual PC values.  (My recollection is that when GDB
was stopping at a breakpoint, it was comparing an non-mips16 address
with a mips16 address and then delivering a message to the user about
a SIGTRAP occurring.  (They didn't compare as equal due to being off
by one bit.)

And, likewise, when writing the PC, or when writing a function
argument for an inferior call, we must take care to set the mips16
address bit as needed.  If we do not do so, the processor attempts to
execute mips16 code as ordinary mips code, with predictably disastrous
results.

In the course of making these changes, I studied what arm-tdep.c
does for THUMB mode and made analogous changes to mips-tdep.c.

I've tested this patch using various mips16 multilibs and it vastly
improves the test results.  (There are roughly 1600 fewer failures
per multilib.)

Comments?

	* mips-tdep.c (make_mips16_addr): New function.
	(mips_elf_make_msymbol_special): Don't set the low bit in the
	symbol's address.
	(mips_read_pc, mips_unwind_pc, mips_addr_bits_remove): Strip bit
	indicating mips16 address, if present.
	(mips_write_pc): Set bit indicating mips16 address when in a mips16
	function.
	(mips_eabi_push_dummy_call, mips_o64_push_dummy_call): Likewise,
	but for each function pointer argument to inferior function call.

--- ../../../sourceware-mips/src/gdb/mips-tdep.c	2010-12-13 11:54:02.775400717 -0700
+++ mips-tdep.c	2010-12-13 15:49:30.386337794 -0700
@@ -205,6 +205,12 @@ unmake_mips16_addr (CORE_ADDR addr)
   return ((addr) & ~(CORE_ADDR) 1);
 }
 
+static CORE_ADDR
+make_mips16_addr (CORE_ADDR addr)
+{
+  return ((addr) | (CORE_ADDR) 1);
+}
+
 /* Return the MIPS ABI associated with GDBARCH.  */
 enum mips_abi
 mips_abi (struct gdbarch *gdbarch)
@@ -264,7 +270,6 @@ mips_elf_make_msymbol_special (asymbol *
   if (((elf_symbol_type *) (sym))->internal_elf_sym.st_other == STO_MIPS16)
     {
       MSYMBOL_TARGET_FLAG_1 (msym) = 1;
-      SYMBOL_VALUE_ADDRESS (msym) |= 1;
     }
 }
 
@@ -931,14 +936,21 @@ mips_read_pc (struct regcache *regcache)
   ULONGEST pc;
   int regnum = mips_regnum (get_regcache_arch (regcache))->pc;
   regcache_cooked_read_signed (regcache, regnum, &pc);
+  if (is_mips16_addr (pc))
+    pc = unmake_mips16_addr (pc);
   return pc;
 }
 
 static CORE_ADDR
 mips_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
 {
-  return frame_unwind_register_signed
-	   (next_frame, gdbarch_num_regs (gdbarch) + mips_regnum (gdbarch)->pc);
+  ULONGEST pc;
+
+  pc = frame_unwind_register_signed
+	 (next_frame, gdbarch_num_regs (gdbarch) + mips_regnum (gdbarch)->pc);
+  if (is_mips16_addr (pc))
+    pc = unmake_mips16_addr (pc);
+  return pc;
 }
 
 static CORE_ADDR
@@ -967,7 +979,10 @@ static void
 mips_write_pc (struct regcache *regcache, CORE_ADDR pc)
 {
   int regnum = mips_regnum (get_regcache_arch (regcache))->pc;
-  regcache_cooked_write_unsigned (regcache, regnum, pc);
+  if (mips_pc_is_mips16 (pc))
+    regcache_cooked_write_unsigned (regcache, regnum, make_mips16_addr (pc));
+  else
+    regcache_cooked_write_unsigned (regcache, regnum, pc);
 }
 
 /* Fetch and return instruction from the specified location.  If the PC
@@ -2450,6 +2465,10 @@ static CORE_ADDR
 mips_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR addr)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  if (is_mips16_addr (addr))
+    addr = unmake_mips16_addr (addr);
+
   if (mips_mask_address_p (tdep) && (((ULONGEST) addr) >> 32 == 0xffffffffUL))
     /* This hack is a work-around for existing boards using PMON, the
        simulator, and any other 64-bit targets that doesn't have true
@@ -2869,9 +2888,25 @@ mips_eabi_push_dummy_call (struct gdbarc
 			    "mips_eabi_push_dummy_call: %d len=%d type=%d",
 			    argnum + 1, len, (int) typecode);
 
+      /* Function pointer arguments to mips16 code need to be made into
+         mips16 pointers.  */
+      if (typecode == TYPE_CODE_PTR
+          && TYPE_CODE (TYPE_TARGET_TYPE (arg_type)) == TYPE_CODE_FUNC)
+	{
+	  CORE_ADDR addr = extract_signed_integer (value_contents (arg),
+						   len, byte_order);
+	  if (mips_pc_is_mips16 (addr))
+	    {
+	      store_signed_integer (valbuf, len, byte_order, 
+				    make_mips16_addr (addr));
+	      val = valbuf;
+	    }
+	  else
+	    val = value_contents (arg);
+	}
       /* The EABI passes structures that do not fit in a register by
          reference.  */
-      if (len > regsize
+      else if (len > regsize
 	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
 	{
 	  store_unsigned_integer (valbuf, regsize, byte_order,
@@ -4159,6 +4194,7 @@ mips_o64_push_dummy_call (struct gdbarch
   for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
+      gdb_byte valbuf[MAX_REGISTER_SIZE];
       struct value *arg = args[argnum];
       struct type *arg_type = check_typedef (value_type (arg));
       int len = TYPE_LENGTH (arg_type);
@@ -4171,6 +4207,21 @@ mips_o64_push_dummy_call (struct gdbarch
 
       val = value_contents (arg);
 
+      /* Function pointer arguments to mips16 code need to be made into
+         mips16 pointers.  */
+      if (typecode == TYPE_CODE_PTR
+          && TYPE_CODE (TYPE_TARGET_TYPE (arg_type)) == TYPE_CODE_FUNC)
+	{
+	  CORE_ADDR addr = extract_signed_integer (value_contents (arg),
+						   len, byte_order);
+	  if (mips_pc_is_mips16 (addr))
+	    {
+	      store_signed_integer (valbuf, len, byte_order, 
+				    make_mips16_addr (addr));
+	      val = valbuf;
+	    }
+	}
+
       /* Floating point arguments passed in registers have to be
          treated specially.  On 32-bit architectures, doubles
          are passed in register pairs; the even register gets

Re: [RFC] mips-tdep.c: Fix mips16 bit rot

[Joel Brobecker]

> Comments?
> 
> 	* mips-tdep.c (make_mips16_addr): New function.
> 	(mips_elf_make_msymbol_special): Don't set the low bit in the
> 	symbol's address.
> 	(mips_read_pc, mips_unwind_pc, mips_addr_bits_remove): Strip bit
> 	indicating mips16 address, if present.
> 	(mips_write_pc): Set bit indicating mips16 address when in a mips16
> 	function.
> 	(mips_eabi_push_dummy_call, mips_o64_push_dummy_call): Likewise,
> 	but for each function pointer argument to inferior function call.

I'm not a mips expert, but the changes look reasonable to me, and they
should only affect mips16 (at least in theory).  I'd probably wait a few
more days to give others a chance to comment, but it otherwise seems OK
to me.

-- 
Joel

Re: [RFC] mips-tdep.c: Fix mips16 bit rot

[Kevin Buettner]

On Mon, 13 Dec 2010 17:06:35 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> 	* mips-tdep.c (make_mips16_addr): New function.
> 	(mips_elf_make_msymbol_special): Don't set the low bit in the
> 	symbol's address.
> 	(mips_read_pc, mips_unwind_pc, mips_addr_bits_remove): Strip bit
> 	indicating mips16 address, if present.
> 	(mips_write_pc): Set bit indicating mips16 address when in a mips16
> 	function.
> 	(mips_eabi_push_dummy_call, mips_o64_push_dummy_call): Likewise,
> 	but for each function pointer argument to inferior function call.

Committed.

Kevin

Re: [RFC] mips-tdep.c: Fix mips16 bit rot

[Maciej W. Rozycki]

Hi Kevin,

 I'm trying to merge a number of MIPS16 fixes that we've long maintained 
locally to upstream GDB sources and came across this change of yours:

On Tue, 14 Dec 2010, Kevin Buettner wrote:

> In the course of some recent mips testing, I noticed that GDB's mips16
> support has been non-functional for quite a while.
> 
> A change which occurred between "2007-01-21 09:49" and
> "2007-01-21 09:50" (use those strings as arguments to cvs's -D
> switches...) caused all line number information in mips16 programs to
> be marked using the mips16 convention of setting the low bit of the
> address.  Prior to that patch, the behavior was mixed - function entry
> points would have the mips16 bit stripped, whereas other lines would
> have it in place.  This change caused a search for a function, e.g
> main(), without the mips16 bit set, to be considered as part of
> whatever function preceded main().  As a consequence,
> skip_prologue_using_sal() would return the address of the first
> instruction in main() rather than the address just past the prologue.
> 
> (I don't see anything wrong with the patch that was committed in Jan
> 2007, and, quite frankly, I'm surprised that it had this affect on
> mips16 behavior at all!)
> 
> Note that gdbarch_addr_bits_remove() is invoked in
> dwarf_decode_lines() when determining the address associated with a
> line.  The fact that mips_addr_bits_remove(), in the patch below,
> strips the low bit of the address is what causes the address to now be
> stored in the line table without the low bit.
> 
> The changes to mips_read_pc() and mips_unwind_pc() were needed to
> correct potential mismatches between addresses that GDB stores
> internally and actual PC values.  (My recollection is that when GDB
> was stopping at a breakpoint, it was comparing an non-mips16 address
> with a mips16 address and then delivering a message to the user about
> a SIGTRAP occurring.  (They didn't compare as equal due to being off
> by one bit.)
> 
> And, likewise, when writing the PC, or when writing a function
> argument for an inferior call, we must take care to set the mips16
> address bit as needed.  If we do not do so, the processor attempts to
> execute mips16 code as ordinary mips code, with predictably disastrous
> results.
> 
> In the course of making these changes, I studied what arm-tdep.c
> does for THUMB mode and made analogous changes to mips-tdep.c.
> 
> I've tested this patch using various mips16 multilibs and it vastly
> improves the test results.  (There are roughly 1600 fewer failures
> per multilib.)
> 
> Comments?
> 
> 	* mips-tdep.c (make_mips16_addr): New function.
> 	(mips_elf_make_msymbol_special): Don't set the low bit in the
> 	symbol's address.
> 	(mips_read_pc, mips_unwind_pc, mips_addr_bits_remove): Strip bit
> 	indicating mips16 address, if present.
> 	(mips_write_pc): Set bit indicating mips16 address when in a mips16
> 	function.
> 	(mips_eabi_push_dummy_call, mips_o64_push_dummy_call): Likewise,
> 	but for each function pointer argument to inferior function call.

 I think overall this change might be in the right direction (though I'm 
not fully convinced as yet), but I'm trying to understand the interaction 
between it and our changes to see what adjustments need to be made to your 
or our changes.  We've got mostly clear MIPS16 test suite runs -- 
certainly not significantly worse if at all than MIPS32 runs and we got to 
it in a different way.

 First of all this construct used in a few places:

+  if (is_mips16_addr (pc))
+    pc = unmake_mips16_addr (pc);

makes me a bit nervous you might be removing the ISA bit for code 
references where it is the only means of signalling GDB the address is a 
MIPS16 code address -- that would happen if pc pointed to a location that 
has no associated symbol information.  While in this case the 
functionality GDB provides is limited, it still has to work correctly up 
to expectations, e.g. instruction-level single-stepping has to work and 
where software stepping is used the MIPS16 BREAK instruction encoding has 
to be used rather than the MIPS32 one.  Have you verified this 
functionality has not regressed?  Similarly the MIPS16 heuristic frame 
unwinder may be affected.

 Otherwise I'd just be tempted to change all these cases into something 
functionally equivalent to:

+  if (msymbol_is_special (lookup_minimal_symbol_by_pc (pc)))
+    pc = unmake_mips16_addr (pc);

where the ISA bit is only stripped if there's symbol information 
indicating this is a piece of MIPS16 code so there's no information lost.

 Second, you only made corresponding adjustments to 
mips_eabi_push_dummy_call and mips_o64_push_dummy_call which are the least 
standard and probably the least used MIPS ABIs.  Any particular reason you 
did not make similar changes to mips_o32_push_dummy_call or 
mips_n32n64_push_dummy_call?

 Also I see you only adjust function pointers that are arguments on their 
own -- isn't a similar adjustment required for such pointers that are 
parts of aggregate types as well?

 Overall, what was the rationale behing your change? -- as it's unclear to 
me from your e-mail.  Did you just want to fix test results you discovered 
that were quite poor or does this change address problems you stumbled 
across during actual MIPS16 debugging?  I can't see any follow-up changes 
of yours to address MIPS16 problems.

 Original patch for reference below.  Thanks for your input.

  Maciej

> --- ../../../sourceware-mips/src/gdb/mips-tdep.c	2010-12-13 11:54:02.775400717 -0700
> +++ mips-tdep.c	2010-12-13 15:49:30.386337794 -0700
> @@ -205,6 +205,12 @@ unmake_mips16_addr (CORE_ADDR addr)
>    return ((addr) & ~(CORE_ADDR) 1);
>  }
>  
> +static CORE_ADDR
> +make_mips16_addr (CORE_ADDR addr)
> +{
> +  return ((addr) | (CORE_ADDR) 1);
> +}
> +
>  /* Return the MIPS ABI associated with GDBARCH.  */
>  enum mips_abi
>  mips_abi (struct gdbarch *gdbarch)
> @@ -264,7 +270,6 @@ mips_elf_make_msymbol_special (asymbol *
>    if (((elf_symbol_type *) (sym))->internal_elf_sym.st_other == STO_MIPS16)
>      {
>        MSYMBOL_TARGET_FLAG_1 (msym) = 1;
> -      SYMBOL_VALUE_ADDRESS (msym) |= 1;
>      }
>  }
>  
> @@ -931,14 +936,21 @@ mips_read_pc (struct regcache *regcache)
>    ULONGEST pc;
>    int regnum = mips_regnum (get_regcache_arch (regcache))->pc;
>    regcache_cooked_read_signed (regcache, regnum, &pc);
> +  if (is_mips16_addr (pc))
> +    pc = unmake_mips16_addr (pc);
>    return pc;
>  }
>  
>  static CORE_ADDR
>  mips_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
>  {
> -  return frame_unwind_register_signed
> -	   (next_frame, gdbarch_num_regs (gdbarch) + mips_regnum (gdbarch)->pc);
> +  ULONGEST pc;
> +
> +  pc = frame_unwind_register_signed
> +	 (next_frame, gdbarch_num_regs (gdbarch) + mips_regnum (gdbarch)->pc);
> +  if (is_mips16_addr (pc))
> +    pc = unmake_mips16_addr (pc);
> +  return pc;
>  }
>  
>  static CORE_ADDR
> @@ -967,7 +979,10 @@ static void
>  mips_write_pc (struct regcache *regcache, CORE_ADDR pc)
>  {
>    int regnum = mips_regnum (get_regcache_arch (regcache))->pc;
> -  regcache_cooked_write_unsigned (regcache, regnum, pc);
> +  if (mips_pc_is_mips16 (pc))
> +    regcache_cooked_write_unsigned (regcache, regnum, make_mips16_addr (pc));
> +  else
> +    regcache_cooked_write_unsigned (regcache, regnum, pc);
>  }
>  
>  /* Fetch and return instruction from the specified location.  If the PC
> @@ -2450,6 +2465,10 @@ static CORE_ADDR
>  mips_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR addr)
>  {
>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (is_mips16_addr (addr))
> +    addr = unmake_mips16_addr (addr);
> +
>    if (mips_mask_address_p (tdep) && (((ULONGEST) addr) >> 32 == 0xffffffffUL))
>      /* This hack is a work-around for existing boards using PMON, the
>         simulator, and any other 64-bit targets that doesn't have true
> @@ -2869,9 +2888,25 @@ mips_eabi_push_dummy_call (struct gdbarc
>  			    "mips_eabi_push_dummy_call: %d len=%d type=%d",
>  			    argnum + 1, len, (int) typecode);
>  
> +      /* Function pointer arguments to mips16 code need to be made into
> +         mips16 pointers.  */
> +      if (typecode == TYPE_CODE_PTR
> +          && TYPE_CODE (TYPE_TARGET_TYPE (arg_type)) == TYPE_CODE_FUNC)
> +	{
> +	  CORE_ADDR addr = extract_signed_integer (value_contents (arg),
> +						   len, byte_order);
> +	  if (mips_pc_is_mips16 (addr))
> +	    {
> +	      store_signed_integer (valbuf, len, byte_order, 
> +				    make_mips16_addr (addr));
> +	      val = valbuf;
> +	    }
> +	  else
> +	    val = value_contents (arg);
> +	}
>        /* The EABI passes structures that do not fit in a register by
>           reference.  */
> -      if (len > regsize
> +      else if (len > regsize
>  	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
>  	{
>  	  store_unsigned_integer (valbuf, regsize, byte_order,
> @@ -4159,6 +4194,7 @@ mips_o64_push_dummy_call (struct gdbarch
>    for (argnum = 0; argnum < nargs; argnum++)
>      {
>        const gdb_byte *val;
> +      gdb_byte valbuf[MAX_REGISTER_SIZE];
>        struct value *arg = args[argnum];
>        struct type *arg_type = check_typedef (value_type (arg));
>        int len = TYPE_LENGTH (arg_type);
> @@ -4171,6 +4207,21 @@ mips_o64_push_dummy_call (struct gdbarch
>  
>        val = value_contents (arg);
>  
> +      /* Function pointer arguments to mips16 code need to be made into
> +         mips16 pointers.  */
> +      if (typecode == TYPE_CODE_PTR
> +          && TYPE_CODE (TYPE_TARGET_TYPE (arg_type)) == TYPE_CODE_FUNC)
> +	{
> +	  CORE_ADDR addr = extract_signed_integer (value_contents (arg),
> +						   len, byte_order);
> +	  if (mips_pc_is_mips16 (addr))
> +	    {
> +	      store_signed_integer (valbuf, len, byte_order, 
> +				    make_mips16_addr (addr));
> +	      val = valbuf;
> +	    }
> +	}
> +
>        /* Floating point arguments passed in registers have to be
>           treated specially.  On 32-bit architectures, doubles
>           are passed in register pairs; the even register gets

Re: [RFC] mips-tdep.c: Fix mips16 bit rot

[Kevin Buettner]

Hi Maciej,

Sorry it's taken me so long to get back to you this.  I will try to
answer your questions to the best of my ability, but it's been a
while since I've looked at or thought about this code.  I fear
that I might not be of much help...

On Wed, 23 Nov 2011 12:40:37 +0000
"Maciej W. Rozycki" <macro@codesourcery.com> wrote:
>  First of all this construct used in a few places:
> 
> +  if (is_mips16_addr (pc))
> +    pc = unmake_mips16_addr (pc);
> 
> makes me a bit nervous you might be removing the ISA bit for code 
> references where it is the only means of signalling GDB the address is a 
> MIPS16 code address -- that would happen if pc pointed to a location that 
> has no associated symbol information.  While in this case the 
> functionality GDB provides is limited, it still has to work correctly up 
> to expectations, e.g. instruction-level single-stepping has to work and 
> where software stepping is used the MIPS16 BREAK instruction encoding has 
> to be used rather than the MIPS32 one.  Have you verified this 
> functionality has not regressed?  Similarly the MIPS16 heuristic frame 
> unwinder may be affected.
> 
>  Otherwise I'd just be tempted to change all these cases into something 
> functionally equivalent to:
> 
> +  if (msymbol_is_special (lookup_minimal_symbol_by_pc (pc)))
> +    pc = unmake_mips16_addr (pc);
> 
> where the ISA bit is only stripped if there's symbol information 
> indicating this is a piece of MIPS16 code so there's no information lost.

I see your point.  You proposed change looks reasonable to me.

>  Second, you only made corresponding adjustments to 
> mips_eabi_push_dummy_call and mips_o64_push_dummy_call which are the least 
> standard and probably the least used MIPS ABIs.  Any particular reason you 
> did not make similar changes to mips_o32_push_dummy_call or 
> mips_n32n64_push_dummy_call?
> 
>  Also I see you only adjust function pointers that are arguments on their 
> own -- isn't a similar adjustment required for such pointers that are 
> parts of aggregate types as well?

I don't remember enough about what I did roughly a year ago to be able
to answer this.  I think it's likely that changes should be made to the
areas that you've identified.

>  Overall, what was the rationale behing your change? -- as it's unclear to 
> me from your e-mail.  Did you just want to fix test results you discovered 
> that were quite poor or does this change address problems you stumbled 
> across during actual MIPS16 debugging?

The former - I was attempting to fix some very bad test results with respect
to mips16.

Kevin

Re: [RFC] mips-tdep.c: Fix mips16 bit rot

[Maciej W. Rozycki]

Hi Kevin,

> Sorry it's taken me so long to get back to you this.  I will try to
> answer your questions to the best of my ability, but it's been a
> while since I've looked at or thought about this code.  I fear
> that I might not be of much help...

 No worries, I keep being distracted by various stuff too.  Thanks for 
your time.

> >  First of all this construct used in a few places:
> > 
> > +  if (is_mips16_addr (pc))
> > +    pc = unmake_mips16_addr (pc);
> > 
> > makes me a bit nervous you might be removing the ISA bit for code 
> > references where it is the only means of signalling GDB the address is a 
> > MIPS16 code address -- that would happen if pc pointed to a location that 
> > has no associated symbol information.  While in this case the 
> > functionality GDB provides is limited, it still has to work correctly up 
> > to expectations, e.g. instruction-level single-stepping has to work and 
> > where software stepping is used the MIPS16 BREAK instruction encoding has 
> > to be used rather than the MIPS32 one.  Have you verified this 
> > functionality has not regressed?  Similarly the MIPS16 heuristic frame 
> > unwinder may be affected.
> > 
> >  Otherwise I'd just be tempted to change all these cases into something 
> > functionally equivalent to:
> > 
> > +  if (msymbol_is_special (lookup_minimal_symbol_by_pc (pc)))
> > +    pc = unmake_mips16_addr (pc);
> > 
> > where the ISA bit is only stripped if there's symbol information 
> > indicating this is a piece of MIPS16 code so there's no information lost.
> 
> I see your point.  You proposed change looks reasonable to me.
> 
> >  Second, you only made corresponding adjustments to 
> > mips_eabi_push_dummy_call and mips_o64_push_dummy_call which are the least 
> > standard and probably the least used MIPS ABIs.  Any particular reason you 
> > did not make similar changes to mips_o32_push_dummy_call or 
> > mips_n32n64_push_dummy_call?
> > 
> >  Also I see you only adjust function pointers that are arguments on their 
> > own -- isn't a similar adjustment required for such pointers that are 
> > parts of aggregate types as well?
> 
> I don't remember enough about what I did roughly a year ago to be able
> to answer this.  I think it's likely that changes should be made to the
> areas that you've identified.
> 
> >  Overall, what was the rationale behing your change? -- as it's unclear to 
> > me from your e-mail.  Did you just want to fix test results you discovered 
> > that were quite poor or does this change address problems you stumbled 
> > across during actual MIPS16 debugging?
> 
> The former - I was attempting to fix some very bad test results with respect
> to mips16.

 So essentially at this point, after some testing and lots of thinking, 
I'd like to revert your change entirely and make additional changes 
elsewhere, so that the ISA bit is actually preserved all the time.  This 
reflects the program's view of the world which I think GDB should mimic.  
Unless proved otherwise, I think any other approach is unworkable and is 
simply missing the point.

 I've opened a discussion and sent a proposal here:

http://sourceware.org/ml/gdb-patches/2012-05/msg00515.html

-- feel free to join if interested.

  Maciej