This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
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
once.
> 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.
Maciej
2011-07-04 Maciej W. Rozycki <macro@codesourcery.com>
include/opcode/
* 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
table:
+
+ INSN_ISA3
+
INSN_UNCOND_BRANCH_DELAY
INSN_COND_BRANCH_DELAY
INSN_COND_BRANCH_LIKELY (never used)
@@ -1140,7 +1143,7 @@ extern int bfd_mips_num_opcodes;
INSN_WRITE_HI
INSN_WRITE_LO
INSN_TRAP
- INSN_ISA3
+ FP_D (never used)
*/
extern const struct mips_opcode mips16_opcodes[];