Summary: | Forward-referencing ADR instructions generate wrong offsets in Thumb code | ||
---|---|---|---|
Product: | binutils | Reporter: | Max Marrone <max> |
Component: | gas | Assignee: | Tamar Christina <tnfchris> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | dwelch, max, petemoore, tnfchris |
Priority: | P2 | ||
Version: | 2.31 | ||
Target Milestone: | 2.37 | ||
Host: | Target: | arm-* | |
Build: | Last reconfirmed: | 2020-01-06 00:00:00 |
Description
Max Marrone
2019-11-30 02:11:11 UTC
Both in do_adr and do_t_adr there are checks for S_IS_DEFINED and THUMB_IS_FUNC before applying the required offset (to match the defined behavior of the tool in its documentation). But at the time of those checks a forward referenced label is not yet defined so the correct offset is not computed. Later when handled as an add or subtract the implementation does not check for thumb nor compute the correct offset. do_t_adr for example: if (inst.relocs[0].exp.X_op == O_symbol && inst.relocs[0].exp.X_add_symbol != NULL && S_IS_DEFINED (inst.relocs[0].exp.X_add_symbol) <---- here && THUMB_IS_FUNC (inst.relocs[0].exp.X_add_symbol)) <---- and here inst.relocs[0].exp.X_add_number += 1; <---- make |= 1 for cleanliness There are further ADR thumb issues with respect to T16 and subtraction even though this pseudo instruction can be handled in the same number of bytes as a T32 (add rd,pc,#0, sub rd,#offset) so no excuse there for this error in general (if the range is too far for either A32, T16, T32 solutions then sure error out). if (subtract || value & ~0x3fc) as_bad_where (fixP->fx_file, fixP->fx_line, _("invalid immediate for address calculation (value = 0x%08lX)"), (unsigned long) (subtract ? - value : value)); This is in the same area of code where the fix for this bug would want/need to live. If this needs to be filed as a separate bug to get this fixed then fine, but might as well fix the whole ADR issue at once rather than in pieces, yes? I'm still working on this but it won't make it for the 2.34 cut off. I will backport the fix to 2.34 later. The issue is I'm not convinced the relaxations are correct for all cases so I am busy writing tests and diving into the current behaviour before I start changing the code. Was this ever resolved? The master branch has been updated by Tamar Christina <tnfchris@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d3e52e120b68bf19552743fbc078e0a759f48cb7 commit d3e52e120b68bf19552743fbc078e0a759f48cb7 Author: Tamar Christina <tamar.christina@arm.com> Date: Tue May 25 16:04:04 2021 +0100 Arm: Fix forward thumb references [PR gas/25235] When assembling a forward reference the symbol will be unknown and so during do_t_adr we cannot set the thumb bit. The bit it set so early to prevent relaxations that are invalid. i.e. relaxing a Thumb2 to Thumb1 insn when the symbol is Thumb. But because it's done so early we miss the case for forward references. This patch changes it so that we additionally check the thumb bit during the internal relocation processing. In principle we should be able to only set the bit during reloc processing but that would require changes to the other relocations that the instruction could be relaxed to. This approach still allows early relaxations (which means that we have less iteration of internal reloc processing) while still fixing the forward reference case. gas/ChangeLog: 2021-05-24 Tamar Christina <tamar.christina@arm.com> PR gas/25235 * config/tc-arm.c (md_convert_frag): Set LSB when Thumb symbol. (relax_adr): Thumb symbols 4 bytes. * testsuite/gas/arm/pr25235.d: New test. * testsuite/gas/arm/pr25235.s: New test. The binutils-2_36-branch branch has been updated by Tamar Christina <tnfchris@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=eaf6d3b360d2d4bd68acaebb80e40d5bb69a734d commit eaf6d3b360d2d4bd68acaebb80e40d5bb69a734d Author: Tamar Christina <tamar.christina@arm.com> Date: Tue May 25 16:04:04 2021 +0100 Arm: Fix forward thumb references [PR gas/25235] When assembling a forward reference the symbol will be unknown and so during do_t_adr we cannot set the thumb bit. The bit it set so early to prevent relaxations that are invalid. i.e. relaxing a Thumb2 to Thumb1 insn when the symbol is Thumb. But because it's done so early we miss the case for forward references. This patch changes it so that we additionally check the thumb bit during the internal relocation processing. In principle we should be able to only set the bit during reloc processing but that would require changes to the other relocations that the instruction could be relaxed to. This approach still allows early relaxations (which means that we have less iteration of internal reloc processing) while still fixing the forward reference case. gas/ChangeLog: 2021-05-24 Tamar Christina <tamar.christina@arm.com> PR gas/25235 * config/tc-arm.c (md_convert_frag): Set LSB when Thumb symbol. (relax_adr): Thumb symbols 4 bytes. * testsuite/gas/arm/pr25235.d: New test. * testsuite/gas/arm/pr25235.s: New test. (cherry picked from commit d3e52e120b68bf19552743fbc078e0a759f48cb7) Fixed in master (2.37) and backported to 2.36 Many thanks Tamar! :-) |