This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

On Mon, 4 Jul 2011, Richard Sandiford wrote:

> >  BTW, I've been wondering about this construct:
> >
> >   return mask & ~0;
> >
> > you've introduced in a couple of places.
> Oops.  Fixed below.

 Hmm, makes sense indeed, thanks.  Except that it didn't pop up in testing 
which is worrisome and means that all this branch swapping stuff is not 
really terribly well covered.  I think we should cook up something more 
systematic than the couple of scattered cases we already have.

> >  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. ;)
> Those comments were already there.  I just moved them around.

 Fair enough.  I got a notion of being otherwise, but now that I have 
double-checked you are right.  Sorry about the confusion.

 In that case though I'm tempted to adjust the comments I'll be tweaking 
anyway with the microMIPS changes, if you don't mind that is.

> >  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,
> Yes.  I checked that before writing it this way.  It's guaranteed to be
> harmless anyway, because "until" MIPS16 gets hardware FP support, we'd
> never have any set bits.

 Actually, I was more worried about this bit being reused for something 
else as a distinct MIPS16_* flag and then hitting this condition 
inadvertently.  There is a place in append_insn() using the FPR mask 
unconditionally even in the MIPS16 mode (quite rightfully IMO).  Granted, 
it's only used for the calculation of .fmask, that is less of importance 
now that we have DWARF-2, but still.

 How about making a suitable comment in include/opcode/mips.h, preferably 
following all the other MIPS16_* flags, that the bit position used for the 
FP_D flag is already reserved for MIPS16 code too?  There's a terse 
description already there covering the shared flags.  I think a change 
like below should be enough.

 I think INSN_ISA3 should be moved to the beginning and made clearly 
distinct.  It took me a while to notice it's there.  I have decided these 
changes are too trivial to be kept separate, so I've made them both at 

> I structured the code to be consistent with the GPR case.  If you feel
> strongly about it though, feel free to move the conditions inside the
> existing "!mips_opts.mips16" check.

 I just feel no confidence that the notion of such an assumption will 
stand for the next half a dozen years or so.  The chance for the MIPS16 
ASE being extended any further is probably slim, but the lifespan of 
binutils is long enough to be careful about any assumptions and about 
long-term maintenance of any piece of code.  We seem to keep discovering 
obscure oddities all the time despite quite a strict maintenance regime.

 As long as include/opcode/mips.h is updated accordingly I don't insist on 
my previous change though.


2011-07-04  Maciej W. Rozycki  <>

	* mips.h: Document the use of FP_D in MIPS16 mode.  Adjust the
	order of flags documented.

Index: include/opcode/mips.h
RCS file: /cvs/src/src/include/opcode/mips.h,v
retrieving revision 1.73
diff -u -p -r1.73 mips.h
--- include/opcode/mips.h	28 Feb 2011 16:06:51 -0000	1.73
+++ include/opcode/mips.h	4 Jul 2011 22:57:01 -0000
@@ -1132,6 +1132,9 @@ extern int bfd_mips_num_opcodes;
 /* The following flags have the same value for the mips16 opcode
    INSN_COND_BRANCH_LIKELY (never used)
@@ -1140,7 +1143,7 @@ extern int bfd_mips_num_opcodes;
+   FP_D (never used)
 extern const struct mips_opcode mips16_opcodes[];

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]