This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro
On Mon, 25 Oct 2010, Richard Sandiford wrote:
> > Tested with the usual set of targets: mips-linux, mips64-linux,
> > mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their respective
> > little-endian counterparts. At least a smoke test for the regression
> > caused here would be a good idea; I'll think of something.
>
> Yeah, I think the patch does need a testcase. Like you say, it doesn't
> have to be anything fancy.
Given the current state of affairs I'll post the testcase I have in mind
separately later on. Note that the LD failures spotted by Alan are
indirect only -- we seem to be lacking a direct test, so one will be good
to have IMO.
> > 2010-10-24 Maciej W. Rozycki <macro@linux-mips.org>
> >
> > gas/
> > * config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use any offset
> > reloc supplied.
>
> Looks good, but I think it would be easier to make the 'o' case in
> mips_ip set offset_expr appropriately, so that we can use it
> unconditionally. I.e.:
>
> case 'o': /* 16 bit offset */
> offset_reloc[0] = BFD_RELOC_LO16;
> offset_reloc[1] = BFD_RELOC_UNUSED;
> offset_reloc[2] = BFD_RELOC_UNUSED;
>
> This won't affect non-macro uses, since 'o' only accepts non-reloc
> constants that don't overflow a LO16.
Yes, that has a potential for making changes to other macros simpler.
> (As far as setting [1] and [2] goes: I think it would be better if all
> the cases in mips_ip set all three reloc elements, rather than just the
> first as some do. That's a separate change though.)
Agreed.
> With the change to 'o', let's leave "r" as a scalar.
(Un)done now.
> Also, while we're here, would you mind fixing the unaligned load/store
> macros in the same way?
And l.d, etc. presumably too? I'll post a separate change, but not
tonight, sorry (all this stuff took longer than I had thought it would due
to numerous testsuite adjustments).
> This may be a known bug, and is certainly nothing to do with your patches,
> but I notice:
>
> ld $4,0x7ffc($5)
>
> fails to work correctly (or trigger a diagnostic) in o32 mode.
> 0x8000 works fine of course.
You didn't like my patch addressing this issue back here:
http://sourceware.org/ml/binutils/2005-02/msg00610.html
(originally here: http://sourceware.org/ml/binutils/2004-06/msg00530.html)
but I've kept maintaining it locally over the years (and got it up to
2.20; obviously with the recent changes it'll require an update, but I
planned to do that anyway while upgrading the RPM packages I maintain).
If you'd like me to get it refreshed and resubmitted, then I am all for
it.
Note that at the moment we are a little bit inconsistent here -- when
executed for an unaligned address (such as 4 mod 8) 64-bit LD, etc. will
trigger an address error exception that will either be emulated (such as
under Linux) or presumably trigger diagnostics (such as with bare-iron
programs). On o32 our macro however will silently transfer corrupt data.
Also note that I don't think an offset of 4 mod 8 is inherently illegal
for 64-bit data quantities as MIPS IV ISA's alignment restriction on
offsets was never enforced and has been lifted since, and when combined
with a base address of 4 mod 8, the resulting sum will be correctly
aligned.
> Ah, the fun of macros.
Indeed.
Here's a new version -- tested as previously (plus for binutils and LD).
OK?
2010-10-31 Maciej W. Rozycki <macro@linux-mips.org>
gas/
* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use the offset
reloc supplied.
(mips_ip)['o']: Initialise offset_reloc.
Maciej
binutils-2.20.51-20100925-mips-gas-ld-reloc.patch
Index: binutils-2.20.51/gas/config/tc-mips.c
===================================================================
--- binutils-2.20.51.orig/gas/config/tc-mips.c
+++ binutils-2.20.51/gas/config/tc-mips.c
@@ -7350,12 +7350,15 @@ macro (struct mips_cl_insn *ip)
case M_SD_OB:
s = HAVE_64BIT_GPRS ? "sd" : "sw";
sd_ob:
- macro_build (&offset_expr, s, "t,o(b)", treg, BFD_RELOC_LO16, breg);
+ macro_build (&offset_expr, s, "t,o(b)", treg,
+ -1, offset_reloc[0], offset_reloc[1], offset_reloc[2],
+ breg);
if (!HAVE_64BIT_GPRS)
{
offset_expr.X_add_number += 4;
macro_build (&offset_expr, s, "t,o(b)", treg + 1,
- BFD_RELOC_LO16, breg);
+ -1, offset_reloc[0], offset_reloc[1], offset_reloc[2],
+ breg);
}
break;
@@ -10001,6 +10004,10 @@ do_msbd:
continue;
case 'o': /* 16 bit offset */
+ offset_reloc[0] = BFD_RELOC_LO16;
+ offset_reloc[1] = BFD_RELOC_UNUSED;
+ offset_reloc[2] = BFD_RELOC_UNUSED;
+
/* Check whether there is only a single bracketed expression
left. If so, it must be the base register and the
constant must be zero. */