microblaze: add BFD_RELOC_8 and BFD_RELOC_16 cases
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
From: Samuel Price <thesamprice@gmail.com>
md_apply_fix has cases for BFD_RELOC_32, BFD_RELOC_64 and for the
MicroBlaze-specific relocations, but none for BFD_RELOC_8 or BFD_RELOC_16.
Those fall through to the default and nothing is written, so a byte or
halfword datum whose value is only known at fixup time assembles to zero.
The fixup is then resolved and turned into BFD_RELOC_NONE, so no relocation
survives for the linker to correct it either, and the zero is permanent.
The two new cases replicate the BFD_RELOC_32 case immediately below them in
the same switch -- the same S_IS_DEFINED guard and the same target_big_endian
byte order -- narrowed to one and two bytes.
Fixes gas/all/simple-forward. It also fixes gas/all/forward, which was
xfailed for microblaze-*-*, so drop it from that xfail list. The gas
testsuite on microblaze-elf goes from 327 passes and 1 unexpected failure to
329 passes and 0 failures.
gas/
* config/tc-microblaze.c (md_apply_fix): Handle BFD_RELOC_8 and
BFD_RELOC_16.
* testsuite/gas/all/forward.d: Remove the microblaze-*-* xfail.
Signed-off-by: Sam Price <thesamprice@gmail.com>
Assisted-by: Claude (Anthropic)
---
I considered writing the two new cases the way aarch64 and mips do instead --
case BFD_RELOC_16:
case BFD_RELOC_8:
if (fixP->fx_done)
md_number_to_chars (buf, *valP, fixP->fx_size);
break;
-- which is shorter, drops the endianness branch, and guards on fx_done
rather than S_IS_DEFINED, which is arguably what is really meant here. The
comment on that case in tc-mips.c describes this exact failure mode: "If we
are deleting this reloc entry, we must fill in the value now."
I kept the local form so the patch stays minimal and consistent with its
neighbour, but I am happy to send the md_number_to_chars version instead if
you would rather have it. Or if you want the neighbors converted to
aarch64 / mips and tested I can do that also.
gas/config/tc-microblaze.c | 23 +++++++++++++++++++++++
gas/testsuite/gas/all/forward.d | 2 +-
2 files changed, 24 insertions(+), 1 deletion(-)
Comments
AMD General
Hi Sam,
> md_apply_fix has cases for BFD_RELOC_32, BFD_RELOC_64 and for the
> MicroBlaze-specific relocations, but none for BFD_RELOC_8 or BFD_RELOC_16.
> Those fall through to the default and nothing is written, so a byte or
> halfword datum whose value is only known at fixup time assembles to zero.
> The fixup is then resolved and turned into BFD_RELOC_NONE, so no relocation
> survives for the linker to correct it either, and the zero is permanent.
> The two new cases replicate the BFD_RELOC_32 case immediately below them in
> the same switch -- the same S_IS_DEFINED guard and the same target_big_endian
> byte order -- narrowed to one and two bytes.
> Fixes gas/all/simple-forward. It also fixes gas/all/forward, which was
> xfailed for microblaze-*-*, so drop it from that xfail list. The gas
> testsuite on microblaze-elf goes from 327 passes and 1 unexpected failure to
> 329 passes and 0 failures.
> gas/
> * config/tc-microblaze.c (md_apply_fix): Handle BFD_RELOC_8 and
> BFD_RELOC_16.
> * testsuite/gas/all/forward.d: Remove the microblaze-*-* xfail.
> Signed-off-by: Sam Price <thesamprice@gmail.com>
> Assisted-by: Claude (Anthropic)
Thank you for this!
Reviewed-by: Neal Frager <neal.frager@amd.com>
> ---
> I considered writing the two new cases the way aarch64 and mips do instead --
> case BFD_RELOC_16:
> case BFD_RELOC_8:
> if (fixP->fx_done)
> md_number_to_chars (buf, *valP, fixP->fx_size);
> break;
> -- which is shorter, drops the endianness branch, and guards on fx_done
> rather than S_IS_DEFINED, which is arguably what is really meant here. The
> comment on that case in tc-mips.c describes this exact failure mode: "If we
> are deleting this reloc entry, we must fill in the value now."
> I kept the local form so the patch stays minimal and consistent with its
> neighbour, but I am happy to send the md_number_to_chars version instead if
> you would rather have it. Or if you want the neighbors converted to
> aarch64 / mips and tested I can do that also.
> gas/config/tc-microblaze.c | 23 +++++++++++++++++++++++
> gas/testsuite/gas/all/forward.d | 2 +-
> 2 files changed, 24 insertions(+), 1 deletion(-)
> diff --git a/gas/config/tc-microblaze.c b/gas/config/tc-microblaze.c
> index 3e3dca81921..36b98bbd8fc 100644
> --- a/gas/config/tc-microblaze.c
> +++ b/gas/config/tc-microblaze.c
> @@ -2145,6 +2145,29 @@ md_apply_fix (fixS * fixP,
> }
> }
> break;
> + case BFD_RELOC_8:
> + /* Don't do anything if the symbol is not defined. */
> + if (fixP->fx_addsy == NULL || S_IS_DEFINED (fixP->fx_addsy))
> + buf[0] |= (val & 0xff);
> + break;
> +
> + case BFD_RELOC_16:
> + /* Don't do anything if the symbol is not defined. */
> + if (fixP->fx_addsy == NULL || S_IS_DEFINED (fixP->fx_addsy))
> + {
> + if (target_big_endian)
> + {
> + buf[0] |= ((val >> 8) & 0xff);
> + buf[1] |= (val & 0xff);
> + }
> + else
> + {
> + buf[1] |= ((val >> 8) & 0xff);
> + buf[0] |= (val & 0xff);
> + }
> + }
> + break;
> +
> case BFD_RELOC_32:
> case BFD_RELOC_RVA:
> case BFD_RELOC_32_PCREL:
> diff --git a/gas/testsuite/gas/all/forward.d b/gas/testsuite/gas/all/forward.d
> index d921ec517be..360ab778ebb 100644
> --- a/gas/testsuite/gas/all/forward.d
> +++ b/gas/testsuite/gas/all/forward.d
> @@ -11,7 +11,7 @@
> # mep and microblaze use complex relocs and don't resolve the relocs.
Since this is now fixed for microblaze, can we remove this reference to
microblaze in the comment? This line should now be:
# mep uses complex relocs and doesn't resolve the relocs.
> # one of the relocs references minus_one, which is a bug, but no one
> # seems bothered enough to fix this.
> -#xfail: mep-*-* microblaze-*-*
> +#xfail: mep-*-*
> .*: .*
> --
> 2.39.5 (Apple Git-154)
Best regards,
Neal Frager
AMD
@@ -2145,6 +2145,29 @@ md_apply_fix (fixS * fixP,
}
}
break;
+ case BFD_RELOC_8:
+ /* Don't do anything if the symbol is not defined. */
+ if (fixP->fx_addsy == NULL || S_IS_DEFINED (fixP->fx_addsy))
+ buf[0] |= (val & 0xff);
+ break;
+
+ case BFD_RELOC_16:
+ /* Don't do anything if the symbol is not defined. */
+ if (fixP->fx_addsy == NULL || S_IS_DEFINED (fixP->fx_addsy))
+ {
+ if (target_big_endian)
+ {
+ buf[0] |= ((val >> 8) & 0xff);
+ buf[1] |= (val & 0xff);
+ }
+ else
+ {
+ buf[1] |= ((val >> 8) & 0xff);
+ buf[0] |= (val & 0xff);
+ }
+ }
+ break;
+
case BFD_RELOC_32:
case BFD_RELOC_RVA:
case BFD_RELOC_32_PCREL:
@@ -11,7 +11,7 @@
# mep and microblaze use complex relocs and don't resolve the relocs.
# one of the relocs references minus_one, which is a bug, but no one
# seems bothered enough to fix this.
-#xfail: mep-*-* microblaze-*-*
+#xfail: mep-*-*
.*: .*