Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code

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

On Sat, 2 Jul 2011, Richard Sandiford wrote:

> >  Hmm, does it?  I did verify it both on a GNU/Linux and an ELF target.  It 
> > still passes for me, except that:
> >
> > +  Advance PC by 24 to 0x64
> >
> > is required instead as with my intended-to-be-sent-originally change.  
> > Likewise:
> >
> > +  Advance PC by 23 to 0x40
> >
> > for the MIPS16 dump.  Which target is failing for you?  Perhaps the cause 
> > was merely the outdated version of the change, sigh...
> 
> Ah, could be.  It was mips64-elf.  Maybe I'd got confused and the
> original didn't pass for GNU/Linux after all.  Sorry about that.

 I have checked mips64-elf to be sure and this test case passes in its 
updated form (there are some unrelated failures for this target that 
shouldn't happen though).

> >  I have regenerated and revalidated the change now and I'm ready to push 
> > it, but I'd prefer the final DWARF-2 instructions to be matched exactly.  
> > I made the effort to make the source alignment-agnostic and I'd prefer to 
> > keep the final offsets to be verified literally so that any unexpected 
> > discrepancies are caught.
> 
> Agreed.

 I'm glad to hear this for once. ;)

> > 2011-07-02  Maciej W. Rozycki  <macro@codesourcery.com>
> >
> > 	gas/
> > 	* config/tc-mips.c (append_insn): Make sure DWARF-2 location
> > 	information is properly adjusted for branches that get swapped.
> >
> > 	gas/testsuite/
> > 	* gas/mips/loc-swap.d: New test case for DWARF-2 location with
> > 	branch swapping.
> > 	* gas/mips/loc-swap-dis.d: Likewise.
> > 	* gas/mips/mips16\@loc-swap.d: Likewise, MIPS16 version.
> > 	* gas/mips/mips16\@loc-swap-dis.d: Likewise.
> > 	* gas/mips/loc-swap.s: Source for the new tests.
> > 	* gas/mips/mips.exp: Run the new tests.
> 
> OK.

 Applied now, thanks.

 BTW, I've been wondering about this construct:

  return mask & ~0;

you've introduced in a couple of places.  The masking operation seems a 
no-op to me -- given that both mask and the function's return type are 
defined as unsigned int.  Am I missing anything?

 Also why "can not" instead of "cannot"?  My understanding of the 
subtleties of English is this essentially means we "are allowed not to 
swap" rather than we "are not allowed to swap".  The usual disclaimer 
applies of course. ;)

 Finally I think the change below is needed as well.  We shouldn't be 
relying on the meaning of FP_D for MIPS16 code as opcode flags are assumed 
to have a different meaning in the MIPS16 table.  Granted, the location of 
the FP_D bit hasn't been assigned to anything (yet), so it's guaranteed to 
be zero and the check is harmless, but I think we better avoided this kind 
of overloading.  Do you agree?

 Alternatively, given that there's no FP support in the MIPS16 mode, the 
two functions could simply return straight away in that case and then let 
the rest of the bodies to stay outside a conditional block.

2011-07-04  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (fpr_read_mask, fpr_write_mask): Avoid the
	FP_D check in MIPS16 mode.

  Maciej

binutils-gas-mips-fp-d.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2011-07-02 19:30:04.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2011-07-02 19:32:22.000000000 +0100
@@ -2519,7 +2519,7 @@ fpr_read_mask (const struct mips_cl_insn
     }
   /* Conservatively treat all operands to an FP_D instruction are doubles.
      (This is overly pessimistic for things like cvt.d.s.)  */
-  if (HAVE_32BIT_FPRS && (pinfo & FP_D))
+  if (!mips_opts.mips16 && HAVE_32BIT_FPRS && (pinfo & FP_D))
     mask |= mask << 1;
   return mask;
 }
@@ -2548,7 +2548,7 @@ fpr_write_mask (const struct mips_cl_ins
     }
   /* Conservatively treat all operands to an FP_D instruction are doubles.
      (This is overly pessimistic for things like cvt.s.d.)  */
-  if (HAVE_32BIT_FPRS && (pinfo & FP_D))
+  if (!mips_opts.mips16 && HAVE_32BIT_FPRS && (pinfo & FP_D))
     mask |= mask << 1;
   return mask;
 }