Bug 25235 - Forward-referencing ADR instructions generate wrong offsets in Thumb code
Summary: Forward-referencing ADR instructions generate wrong offsets in Thumb code
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.31
: P2 normal
Target Milestone: 2.34
Assignee: Tamar Christina
Depends on:
Reported: 2019-11-30 02:11 UTC by Max Marrone
Modified: 2020-01-13 14:16 UTC (History)
3 users (show)

See Also:
Target: arm-*
Last reconfirmed: 2020-01-06 00:00:00


Note You need to log in before you can comment on or make changes to this bug.
Description Max Marrone 2019-11-30 02:11:11 UTC
bx'ing or blx'ing to a Thumb instruction requires the least significant bit of the address to be set.  When the address of a label is taken with the ADR instruction, the GNU documentation says that this setting of the least significant bit happens automatically, provided -mthumb-interwork was passed.  See https://sourceware.org/binutils/docs/as/ARM-Opcodes.html.

However, despite this guarantee, adr instructions sometimes generates incorrect offsets that don't have the least significant bit set.

Backward references, assemble into subw instructions that subtract an odd constant from the pc, as expected.  Forward references assemble into add instructions that add an even constant to the pc, which is wrong.

I would expect adr to always generate correct addresses, regardless of the direction.  If the instruction encoding makes that impossible, I would expect gas to abort with a descriptive error.

There is some discussion of this at https://stackoverflow.com/questions/59110205/.

Test case:

    .syntax unified

    .align 2
    .type f1, %function
        b f1

    .align 2
    .type f2, %function
        adr r1, f1
        blx r1
        adr r3, f3
        blx r3

    .align 2
    .type f3, %function
        b f3

Compiling with:

    arm-none-eabi-as -mthumb-interwork TestCase.s

Gives the disassembly (arm-none-eabi-objdump -d):

    00000000 <f1>:
       0:   e7fe        b.n 0 <f1>
       2:   46c0        nop         ; (mov r8, r8)

    00000004 <f2>:
       4:   f2af 0107   subw    r1, pc, #7  ; <-- Note odd offset, as expected.
       8:   4788        blx r1
       a:   a301        add r3, pc, #4  ; (adr r3, 10 <f3>) <-- Note even (wrong) offset.
       c:   4798        blx r3
       e:   46c0        nop         ; (mov r8, r8)

    00000010 <f3>:
      10:   e7fe        b.n 10 <f3>
      12:   46c0        nop         ; (mov r8, r8)

Version I tested with:

    GNU assembler (GNU Tools for Arm Embedded Processors 8-2018-q4-major)
Comment 1 dwelch 2019-12-30 10:24:17 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?
Comment 2 Tamar Christina 2020-01-13 14:16:27 UTC
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.