[PATCH] arm/strlen-thumb2-Os.S: Correct assembly syntax for ldrb instruction

[PATCH] arm/strlen-thumb2-Os.S: Correct assembly syntax for ldrb instruction

[Keith Packard]

We want to use a post-indexed addressing mode (which means use the
original register contents as the address, then increment that
register) which is only valid in Encoding T3 of the LDRB instruction.

According to the ARMv7-M Architecture Reference Manual, the assembly
syntax for Encoding T3 does not include the '.W' width specifier as
that is used to specify Encoding T2, presumably to provide a wider
immediate field for possible relocations.

GAS allows the .W specifier for this addressing mode and generates
identical output with and without it. clang does not allow the .W
specifier for this addressing mode, so removing it offers wider
portability and closer adherance to the ARM assembly syntax
specification.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libc/machine/arm/strlen-thumb2-Os.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/newlib/libc/machine/arm/strlen-thumb2-Os.S b/newlib/libc/machine/arm/strlen-thumb2-Os.S
index 961f41a0a..aed8adf33 100644
--- a/newlib/libc/machine/arm/strlen-thumb2-Os.S
+++ b/newlib/libc/machine/arm/strlen-thumb2-Os.S
@@ -45,7 +45,7 @@
 
 def_fn	strlen p2align=1
 	mov     r3, r0
-1:	ldrb.w  r2, [r3], #1
+1:	ldrb    r2, [r3], #1
 	cmp     r2, #0
 	bne	1b
 	subs    r0, r3, r0
-- 
2.26.2

Re: [PATCH] arm/strlen-thumb2-Os.S: Correct assembly syntax for ldrb instruction

[Richard Earnshaw]

On 12/05/2020 18:58, Keith Packard via Newlib wrote:
> We want to use a post-indexed addressing mode (which means use the
> original register contents as the address, then increment that
> register) which is only valid in Encoding T3 of the LDRB instruction.
> 
> According to the ARMv7-M Architecture Reference Manual, the assembly
> syntax for Encoding T3 does not include the '.W' width specifier as
> that is used to specify Encoding T2, presumably to provide a wider
> immediate field for possible relocations.
> 
> GAS allows the .W specifier for this addressing mode and generates
> identical output with and without it. clang does not allow the .W
> specifier for this addressing mode, so removing it offers wider
> portability and closer adherance to the ARM assembly syntax
> specification.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  newlib/libc/machine/arm/strlen-thumb2-Os.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/newlib/libc/machine/arm/strlen-thumb2-Os.S b/newlib/libc/machine/arm/strlen-thumb2-Os.S
> index 961f41a0a..aed8adf33 100644
> --- a/newlib/libc/machine/arm/strlen-thumb2-Os.S
> +++ b/newlib/libc/machine/arm/strlen-thumb2-Os.S
> @@ -45,7 +45,7 @@
>  
>  def_fn	strlen p2align=1
>  	mov     r3, r0
> -1:	ldrb.w  r2, [r3], #1
> +1:	ldrb    r2, [r3], #1
>  	cmp     r2, #0
>  	bne	1b
>  	subs    r0, r3, r0
> 

IIRC the .w was deliberate to keep the alignment right for some
subsequent instructions.

LLVM's assembler needs fixing if it doesn't accept '.w'.

R.

Re: [PATCH] arm/strlen-thumb2-Os.S: Correct assembly syntax for ldrb instruction

[Emmanuel Blot]

On 15 May 2020, at 15:25, Richard Earnshaw wrote:

> IIRC the .w was deliberate to keep the alignment right for some
> subsequent instructions.
>
> LLVM's assembler needs fixing if it doesn't accept '.w'.

https://bugs.llvm.org//show_bug.cgi?id=43382

HTH,
Emmanuel.

Re: [PATCH] arm/strlen-thumb2-Os.S: Correct assembly syntax for ldrb instruction

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:

> IIRC the .w was deliberate to keep the alignment right for some
> subsequent instructions.

I'm not sure what you mean by 'alignment' here -- would the assembler
actually insert no-ops to ensure that the instruction were aligned
somehow? I can't find any mention of this additional meaning of the '.w'
qualifier and find that unlikely as it would increase code size?

It sounds like gas and llvm have a different interpretation of the '.w'
qualifier for this instruction. The ARMv7-M Architecture Reference
Manual (ARM DDI 0403E.d (ID070218), says the '.W' means:

.W      Meaning wide, specifies that the assembler must select a 32-bit
        encoding for the instruction. If this is not possible, an
        assembler error is produced.

LDRB offers three encodings, called T1, T2 and T3 in the specs. T1 and
T2 have equivalent flexibility: loading indirect from a register with an
optional offset (with T1 being 16 bit and T2 being 32 bit). Selecting
the 32-bit T2 form extends the offset from 5 to 12 bits, so I imagine
that an environment that couldn't replace instructions at link time
might use the T2 form to make room for larger relocation values in case
the offset weren't known at assembly time?

However, only the 32-bit T3 encoding offers the post-indexed mode
required by the code here, and for that, there is no need to clarify
which to select using the .N or .W qualifiers. And, when reading the
description of the three encodings, only T2 includes the '.W' qualifier
in its syntax, although T1 doesn't include '.N', which kinda indicates
that the qualifiers should always be accepted, even if they aren't
necessary.

Hrm. For this instruction, perhaps the intent in the specification is to
use the .W to force a T2 encoding *over a T3 encoding*. The T3 encoding
also supports register-indexed mode, but with only an 8-bit offset, so
using .W might indicate that even if the offset *could* fit in the 8-bit
T3 form, that the assembler should instead select the T2 encoding. I
dunno.

> LLVM's assembler needs fixing if it doesn't accept '.w'.

Yes, it seems like that would be a good idea; having the '.w' in this
case is harmless as only the T3 encoding can possibly work. There is
already a discussion about this in the llvm world; I don't know if or
when that will result in a fix being applied.

I dug through the gas source and couldn't find any place where the '.w'
qualifier would affect the output in this case though, so removing it
should be harmless for gas.

I'm just responding to a bug report from a user trying to use
clang to build the library, and for that, fixing the source code to work
with both gas and clang in ways that don't appear to affect the output
of gas at all seemed like a reasonable option to me.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] arm/strlen-thumb2-Os.S: Correct assembly syntax for ldrb instruction

[Richard Earnshaw]

On 15/05/2020 16:19, Keith Packard wrote:
> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
> 
>> IIRC the .w was deliberate to keep the alignment right for some
>> subsequent instructions.
> 
> I'm not sure what you mean by 'alignment' here -- would the assembler
> actually insert no-ops to ensure that the instruction were aligned
> somehow? I can't find any mention of this additional meaning of the '.w'
> qualifier and find that unlikely as it would increase code size?
> 
> It sounds like gas and llvm have a different interpretation of the '.w'
> qualifier for this instruction. The ARMv7-M Architecture Reference
> Manual (ARM DDI 0403E.d (ID070218), says the '.W' means:
> 
> .W      Meaning wide, specifies that the assembler must select a 32-bit
>         encoding for the instruction. If this is not possible, an
>         assembler error is produced.
> 
> LDRB offers three encodings, called T1, T2 and T3 in the specs. T1 and
> T2 have equivalent flexibility: loading indirect from a register with an
> optional offset (with T1 being 16 bit and T2 being 32 bit). Selecting
> the 32-bit T2 form extends the offset from 5 to 12 bits, so I imagine
> that an environment that couldn't replace instructions at link time
> might use the T2 form to make room for larger relocation values in case
> the offset weren't known at assembly time?
> 
> However, only the 32-bit T3 encoding offers the post-indexed mode
> required by the code here, and for that, there is no need to clarify
> which to select using the .N or .W qualifiers. And, when reading the
> description of the three encodings, only T2 includes the '.W' qualifier
> in its syntax, although T1 doesn't include '.N', which kinda indicates
> that the qualifiers should always be accepted, even if they aren't
> necessary.
> 
> Hrm. For this instruction, perhaps the intent in the specification is to
> use the .W to force a T2 encoding *over a T3 encoding*. The T3 encoding
> also supports register-indexed mode, but with only an 8-bit offset, so
> using .W might indicate that even if the offset *could* fit in the 8-bit
> T3 form, that the assembler should instead select the T2 encoding. I
> dunno.
> 
>> LLVM's assembler needs fixing if it doesn't accept '.w'.
> 
> Yes, it seems like that would be a good idea; having the '.w' in this
> case is harmless as only the T3 encoding can possibly work. There is
> already a discussion about this in the llvm world; I don't know if or
> when that will result in a fix being applied.
> 
> I dug through the gas source and couldn't find any place where the '.w'
> qualifier would affect the output in this case though, so removing it
> should be harmless for gas.
> 
> I'm just responding to a bug report from a user trying to use
> clang to build the library, and for that, fixing the source code to work
> with both gas and clang in ways that don't appear to affect the output
> of gas at all seemed like a reasonable option to me.
> 

The assembler syntax for ldrb in the the copy of the Arm ARM that I have
to hand has:

LDRB<c><q>  <Rt>, [<Rn> {, #+/-<imm>}]  Offset: index==TRUE, wback==FALSE
LDRB<c><q>  <Rt>, [<Rn>, #+/-<imm>]!  Pre-indexed: index==TRUE, wback==TRUE
LDRB<c><q>  <Rt>, [<Rn>], #+/-<imm>  Post-indexed: index==FALSE, wback==TRUE

And <q> is the .N .W qualifier.

So clearly .W should be acceptable, even if it is the only permitted
from for this case.

R.