[Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[James Greenhalgh]

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

Hi,

This patch fixes an issue with strex[b/h] in thumb mode.

strex[b/h] do not support an addressing mode with writeback.
This fault was caught correctly in ARM mode but was undiagnosed
under thumb.

Instructions of the form:
STREXH r0, r1, [r2, #+0x00]!
will now produce a "BAD_ADDR_MODE" error. This matches their
behaviour under ARM.

Tested using make check without regression.

James Greenhalgh

gas/

2011-07-22  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/tc-arm.c (do_t_strexbh): New.
	(insns): Update accordingly.

gas/testsuite/

2011-07-22  James Greenhalgh  <james.greenhalgh@arm.com>

	* gas/arm/strex-bad-t.d: New testcase.
	* gas/arm/strex-bad-t.s: Likewise.
	* gas/arm/strex-bad-t.l: Likewise.
	* gas/arm/strex-t.s: Likewise.
	* gas/arm/strex-t.d: Likewise.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-ARM-Gas-strexh-strexb-warn-for-bad-addressing-.patch --]
[-- Type: text/x-patch;  name=0001-Patch-ARM-Gas-strexh-strexb-warn-for-bad-addressing-.patch, Size: 5313 bytes --]

diff --git gas/config/tc-arm.c gas/config/tc-arm.c
index 1592322..800a6ff 100644
--- gas/config/tc-arm.c
+++ gas/config/tc-arm.c
@@ -8484,6 +8484,21 @@ do_strex (void)
 }
 
 static void
+do_t_strexbh (void)
+{
+  constraint (!inst.operands[2].isreg || !inst.operands[2].preind
+	      || inst.operands[2].postind || inst.operands[2].writeback
+	      || inst.operands[2].immisreg || inst.operands[2].shifted
+	      || inst.operands[2].negative,
+	      BAD_ADDR_MODE);
+
+  constraint (inst.operands[0].reg == inst.operands[1].reg
+	      || inst.operands[0].reg == inst.operands[2].reg, BAD_OVERLAP);
+
+  do_rm_rd_rn ();
+}
+
+static void
 do_strexd (void)
 {
   constraint (inst.operands[1].reg % 2 != 0,
@@ -17499,9 +17514,9 @@ static const struct asm_opcode insns[] =
  TCE("ldrexh",	1f00f9f, e8d00f5f, 2, (RRnpc_npcsp, RRnpcb),
      rd_rn,  rd_rn),
  TCE("strexb",	1c00f90, e8c00f40, 3, (RRnpc_npcsp, RRnpc_npcsp, ADDR),
-     strex, rm_rd_rn),
+     strex, t_strexbh),
  TCE("strexh",	1e00f90, e8c00f50, 3, (RRnpc_npcsp, RRnpc_npcsp, ADDR),
-     strex, rm_rd_rn), 
+     strex, t_strexbh),
  TUF("clrex",	57ff01f, f3bf8f2f, 0, (),			      noargs, noargs),
 
 #undef  ARM_VARIANT
diff --git gas/testsuite/gas/arm/strex-bad-t.d gas/testsuite/gas/arm/strex-bad-t.d
new file mode 100644
index 0000000..f5ec4c5
--- /dev/null
+++ gas/testsuite/gas/arm/strex-bad-t.d
@@ -0,0 +1,3 @@
+# name: Bad addressing modes STREXH/STREXB. - THUMB
+# error-output: strex-bad-t.l
+
diff --git gas/testsuite/gas/arm/strex-bad-t.l gas/testsuite/gas/arm/strex-bad-t.l
new file mode 100644
index 0000000..a490096
--- /dev/null
+++ gas/testsuite/gas/arm/strex-bad-t.l
@@ -0,0 +1,24 @@
+[^:]*: Assembler messages:
+[^:]*:7: Error: r15 not allowed here -- `strexh r0,r1,#0x04'
+[^:]*:8: Error: instruction does not accept this addressing mode -- `strexh r0,r1,\[r2\],#0x04'
+[^:]*:9: Error: instruction does not accept this addressing mode -- `strexh r0,r1,\[r2,#\+0x00\]!'
+[^:]*:10: Error: instruction does not accept this addressing mode -- `strexh r0,r1,\[r2,r3\]'
+[^:]*:11: Error: registers may not be the same -- `strexh r0,r0,\[r1]'
+[^:]*:12: Error: instruction does not accept this addressing mode -- `strexh r0,r1,\[r2,#-0x04\]'
+[^:]*:13: Error: r15 not allowed here -- `strexh r0,r1,\[r15\]'
+[^:]*:14: Error: r13 not allowed here -- `strexh r0,r13,\[r1\]'
+[^:]*:15: Error: r15 not allowed here -- `strexh r0,r15,\[r1\]'
+[^:]*:16: Error: r13 not allowed here -- `strexh r13,r0,\[r1\]'
+[^:]*:17: Error: r15 not allowed here -- `strexh r15,r0,\[r1\]'
+[^:]*:21: Error: r15 not allowed here -- `strexb r0,r1,#0x04'
+[^:]*:22: Error: instruction does not accept this addressing mode -- `strexb r0,r1,\[r2\],#0x04'
+[^:]*:23: Error: instruction does not accept this addressing mode -- `strexb r0,r1,\[r2,#\+0x00\]!'
+[^:]*:24: Error: instruction does not accept this addressing mode -- `strexb r0,r1,\[r2,r3\]'
+[^:]*:25: Error: registers may not be the same -- `strexb r0,r0,\[r1]'
+[^:]*:26: Error: instruction does not accept this addressing mode -- `strexb r0,r1,\[r2,#-0x04\]'
+[^:]*:27: Error: r15 not allowed here -- `strexb r0,r1,\[r15\]'
+[^:]*:28: Error: r13 not allowed here -- `strexb r0,r13,\[r1\]'
+[^:]*:29: Error: r15 not allowed here -- `strexb r0,r15,\[r1\]'
+[^:]*:30: Error: r13 not allowed here -- `strexb r13,r0,\[r1\]'
+[^:]*:31: Error: r15 not allowed here -- `strexb r15,r0,\[r1\]'
+
diff --git gas/testsuite/gas/arm/strex-bad-t.s gas/testsuite/gas/arm/strex-bad-t.s
new file mode 100644
index 0000000..1466ca5
--- /dev/null
+++ gas/testsuite/gas/arm/strex-bad-t.s
@@ -0,0 +1,32 @@
+.syntax unified
+
+.thumb
+
+@ strexh
+
+strexh r0, r1, #0x04
+strexh r0, r1, [r2], #0x04
+strexh r0, r1, [r2, #+0x00]!
+strexh r0, r1, [r2, r3]
+strexh r0, r0, [r1]
+strexh r0, r1, [r2, #-0x04]
+strexh r0, r1, [r15]
+strexh r0, r13, [r1]
+strexh r0, r15, [r1]
+strexh r13, r0, [r1]
+strexh r15, r0, [r1]
+
+@ strexb
+
+strexb r0, r1, #0x04
+strexb r0, r1, [r2], #0x04
+strexb r0, r1, [r2, #+0x00]!
+strexb r0, r1, [r2, r3]
+strexb r0, r0, [r1]
+strexb r0, r1, [r2, #-0x04]
+strexb r0, r1, [r15]
+strexb r0, r13, [r1]
+strexb r0, r15, [r1]
+strexb r13, r0, [r1]
+strexb r15, r0, [r1]
+
diff --git gas/testsuite/gas/arm/strex-t.d gas/testsuite/gas/arm/strex-t.d
new file mode 100644
index 0000000..04b5d0b
--- /dev/null
+++ gas/testsuite/gas/arm/strex-t.d
@@ -0,0 +1,13 @@
+# name: STREXH/STREXB. - Thumb
+#objdump: -dr --prefix-address --show-raw-insn
+
+.*: +file format .*arm.*
+
+Disassembly of section \.text:
+0+00 <[^>]+> e8c2 1f50 	strexh	r0, r1, \[r2\]
+0+04 <[^>]+> e8c2 1f50 	strexh	r0, r1, \[r2\]
+0+08 <[^>]+> e8cd 1f50 	strexh	r0, r1, \[sp\]
+0+0c <[^>]+> e8c2 1f40 	strexb	r0, r1, \[r2\]
+0+10 <[^>]+> e8c2 1f40 	strexb	r0, r1, \[r2\]
+0+14 <[^>]+> e8cd 1f40 	strexb	r0, r1, \[sp\]
+
diff --git gas/testsuite/gas/arm/strex-t.s gas/testsuite/gas/arm/strex-t.s
new file mode 100644
index 0000000..d8cddfc
--- /dev/null
+++ gas/testsuite/gas/arm/strex-t.s
@@ -0,0 +1,10 @@
+.syntax unified
+.thumb
+	strexh r0, r1, [r2]
+	strexh r0, r1, [r2, #+0x00]
+	strexh r0, r1, [r13]
+
+	strexb r0, r1, [r2]
+	strexb r0, r1, [r2, #+0x00]
+	strexb r0, r1, [r13]
+

Re: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[Nick Clifton]

Hi James,

> This patch fixes an issue with strex[b/h] in thumb mode.

What issue ?  As far as I can see the current version of gas does 
correctly detect and report these bad addressing modes.  Certainly when 
I ran the test cases you supplied through an unpatched assembler they 
all worked.

Cheers
   Nick

RE: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[James Greenhalgh]

Hi Nick,

Apologies for the lack of clarity.

strexh and strexb do not have writeback modes in either arm or thumb
mode. An unpatched assembler will return an error for the following
under ARM, but not for thumb. 

	strexh r1, r2, [r3, #0]!

This was line 9 of strex-bad-t.s which `FAIL's on an unpatched assembler
for me.

Likewise,

	strexb r1, r2, [r3, #0]!

The patch adds the error for thumb mode.

The test cases are perhaps a little... over-enthusiastic in testing bad
addressing modes for strexh/strexb, but form a thorough check that
problem cases for this instruction are reported.

Thanks,
James

> -----Original Message-----
> From: Nick Clifton [mailto:nickc@redhat.com]
> Sent: 29 July 2011 16:08
> To: James Greenhalgh
> Cc: binutils@sourceware.org
> Subject: Re: [Patch ARM Gas] - strexh/strexb warn for bad addressing
> modes.
> 
> Hi James,
> 
> > This patch fixes an issue with strex[b/h] in thumb mode.
> 
> What issue ?  As far as I can see the current version of gas does
> correctly detect and report these bad addressing modes.  Certainly when
> I ran the test cases you supplied through an unpatched assembler they
> all worked.
> 
> Cheers
>    Nick
> 

Re: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[Nick Clifton]

Hi James,

> strexh and strexb do not have writeback modes in either arm or thumb
> mode. An unpatched assembler will return an error for the following
> under ARM, but not for thumb.
>
> 	strexh r1, r2, [r3, #0]!

Thanks - I understand now.  I have applied the patch.

Cheers
   Nick

RE: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[hazelnusse]

James Greenhalgh-2 wrote:
> 
> Hi Nick,
> 
> Apologies for the lack of clarity.
> 
> strexh and strexb do not have writeback modes in either arm or thumb
> mode. An unpatched assembler will return an error for the following
> under ARM, but not for thumb. 
> 
> 	strexh r1, r2, [r3, #0]!
> 
> This was line 9 of strex-bad-t.s which `FAIL's on an unpatched assembler
> for me.
> 
> Likewise,
> 
> 	strexb r1, r2, [r3, #0]!
> 
> The patch adds the error for thumb mode.
> 
> The test cases are perhaps a little... over-enthusiastic in testing bad
> addressing modes for strexh/strexb, but form a thorough check that
> problem cases for this instruction are reported.
> 
> Thanks,
> James
> 
> 

I am working on a STMicroelectronics Cortex M-3, and just updated to
binutils as 2.21.53.20110915, and when compiling the core_cm3.c and
core_cm3.h as provided by ARM, I get this error:

/tmp/ccRg7rUW.s: Assembler messages:
/tmp/ccRg7rUW.s:511: Error: registers may not be the same -- `strexb
r0,r0,[r1]'
/tmp/ccRg7rUW.s:536: Error: registers may not be the same -- `strexh
r0,r0,[r1]'

This error is arising from this code in the ARM provided CMSIS files
core_cm3.c and core_cm3.h:

/**
 * @brief  STR Exclusive (8 bit)
 *
 * @param  value  value to store
 * @param  *addr  address pointer
 * @return        successful / failed
 *
 * Exclusive STR command for 8 bit values
 */
uint32_t __STREXB(uint8_t value, uint8_t *addr)
{
  __ASM("strexb r0, r0, [r1]");
  __ASM("bx lr");
}

/**
 * @brief  STR Exclusive (16 bit)
 *
 * @param  value  value to store
 * @param  *addr  address pointer
 * @return        successful / failed
 *
 * Exclusive STR command for 16 bit values
 */
uint32_t __STREXH(uint16_t value, uint16_t *addr)
{
  __ASM("strexh r0, r0, [r1]");
  __ASM("bx lr");
}

Looking into the ARM Cortex M3 documentation about these instructions [0],
it says:
Syntax
LDREX{cond} Rt, [Rn {, #offset}]
STREX{cond} Rd, Rt, [Rn {, #offset}]
LDREXB{cond} Rt, [Rn]
STREXB{cond} Rd, Rt, [Rn]
LDREXH{cond} Rt, [Rn]
STREXH{cond} Rd, Rt, [Rn]

...

Restrictions
In these instructions:
do not use PC
do not use SP for Rd and Rt
for STREX, Rd must be different from both Rt and Rn
the value of offset must be a multiple of four in the range 0-1020.

If you read this literally, it only says STREX has the restriction that
you've added to gas, and that it doesn't apply to any of the other ones (in
particular STREXH and STREXB).  I'm not sure if this is the same across all
the ARM7/9/11 processors, but it certainly seems that this should be an
allowed instruction for Cortex M3, given that they are shipping this
instruction with their CMSIS stuff.

[0] --
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/BABFFBJB.html
-- 
View this message in context: http://old.nabble.com/-Patch-ARM-Gas----strexh-strexb-warn-for-bad-addressing-modes.-tp32114456p32502715.html
Sent from the Sourceware - binutils list mailing list archive at Nabble.com.

RE: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[James Greenhalgh]

Hi,
 
> If you read this literally, it only says STREX has the restriction that
> you've added to gas, and that it doesn't apply to any of the other ones
> (in
> particular STREXH and STREXB).  I'm not sure if this is the same across
> all
> the ARM7/9/11 processors, but it certainly seems that this should be an
> allowed instruction for Cortex M3, given that they are shipping this
> instruction with their CMSIS stuff.

In my copy of the ARMARMv7M [0] strexb, strexh and strex are all listed
as UNPREDICTABLE when rd == rt || rd == rn.

I would read the quoted passage of the User Guide Reference Material
as saying  "for Store-Exclusive Instructions, Rd must be different
from both Rt and Rn". Which better fits with the description in the
ARMARM.

There is potentially an argument for making these UNPREDICTABLE behaviours
warnings, but this would just mask the real issue, which, IMHO is
that the library is not sufficiently cautious with the registers it uses
for these instructions.

If you want further advice on the CMSIS libraries I would suggest going 
through your normal CMSIS support channels.

Regards
James Greenhalgh

[0] -
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0403c/index.h
tml

Re: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[Luke]

You are correct, the behavior is undefined and your patch does the
right thing.  I'll pass this along to the CMSIS folks and give them a
heads up.

~Luke

On Thu, Sep 22, 2011 at 5:59 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> Hi,
>
>> If you read this literally, it only says STREX has the restriction that
>> you've added to gas, and that it doesn't apply to any of the other ones
>> (in
>> particular STREXH and STREXB).  I'm not sure if this is the same across
>> all
>> the ARM7/9/11 processors, but it certainly seems that this should be an
>> allowed instruction for Cortex M3, given that they are shipping this
>> instruction with their CMSIS stuff.
>
> In my copy of the ARMARMv7M [0] strexb, strexh and strex are all listed
> as UNPREDICTABLE when rd == rt || rd == rn.
>
> I would read the quoted passage of the User Guide Reference Material
> as saying  "for Store-Exclusive Instructions, Rd must be different
> from both Rt and Rn". Which better fits with the description in the
> ARMARM.
>
> There is potentially an argument for making these UNPREDICTABLE behaviours
> warnings, but this would just mask the real issue, which, IMHO is
> that the library is not sufficiently cautious with the registers it uses
> for these instructions.
>
> If you want further advice on the CMSIS libraries I would suggest going
> through your normal CMSIS support channels.
>
> Regards
> James Greenhalgh
>
> [0] -
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0403c/index.h
> tml
>
>
>
>



-- 
"Those who would give up essential liberty to purchase a little
temporary safety deserve neither liberty nor safety."

-- Benjamin Franklin, Historical Review of Pennsylvania, 1759

Re: [Patch ARM Gas] - strexh/strexb warn for bad addressing modes.

[Nick Clifton]

Hi James,

   Well finally, about two months late, I have committed your patch. 
Please accept my apology for taking so long to get to it, and thank you 
very much for writing the patch in the first place.

Cheers
   Nick

2011-08-26  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/tc-arm.c (check_ldr_r15_aligned): New.
	(do_ldst): Warn in upredictable cases.
	(do_t_ldst): Likewise.
	(insns): Update accordingly.

gas/testsuite

2011-08-26  James Greenhalgh  <james.greenhalgh@arm.com>

	* gas/arm/ldr-bad.s: New testcase.
	* gas/arm/ldr-bad.l: Likewise.
	* gas/arm/ldr-bad.d: Likewise.
	* gas/arm/ldr.s: Likewise.
	* gas/arm/ldr.d: Likewise.
	* gas/arm/ldr-t-bad.s: Likewise.
	* gas/arm/ldr-t-bad.l: Likewise.
	* gas/arm/ldr-t-bad.d: Likewise.
	* gas/arm/ldr-t.s: Likewise.
	* gas/arm/ldr-t.d: Likewise.
	* gas/arm/sp-pc-usage-t.s: Correct.
	* gas/arm/sp-pc-usage-t.d: Update accordingly.