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

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

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