[AArch64] Fix diagnostic error message for structural load/store by element.

Message ID AM0PR08MB36506C4D6C7CFB2FBB9C1E99EAE20@AM0PR08MB3650.eurprd08.prod.outlook.com
State New
Headers show
Series
  • [AArch64] Fix diagnostic error message for structural load/store by element.
Related show

Commit Message

Barnaby Wilks June 26, 2019, 2:20 p.m.
Hello,

This patch fixes a bug where an incorrect structural load/store by element instruction
would generate the wrong error message.

For example, when provided with the (incorrect) instruction

st4 {v0.16b-v3.16b}[4],[x0]

currently assembler provides the following error message
"Error: comma expected between operands at operand 2 -- `st4 {v0.16b-v3.16b}[4],[x0]'".

This was due to the assembler consuming the {v0.16b-v3.16b} as the first operand leaving
[4],[x0] as what it believed to be the second operand.

The actual error is that the first operand should be of element type and not
vector type (as provided). The new diagnostic for this error is
"Error: expected element type rather than vector type at operand 1 -- `st4 {v0.16b-v3.16b}[4],[x0]'.

Added testcases to check for the correct diagnostic message as well as checking that
variations of the structural load/store by element instruction also generate the error
when they have the same problem. 

Cross compiled and regtested aarch64-none-elf and aarch64-none-linux-gnu and no issues found.

I don't have write access, so if it's OK then could someone commit on my behalf? 

Thanks,
Barney

gas/ChangeLog:

2019-06-26  Barnaby Wilks  <barnaby.wilks@arm.com>

	* config/tc-aarch64.c (parse_operands): Add error check.
	* testsuite/gas/aarch64/diagnostic.l: New test.
	* testsuite/gas/aarch64/diagnostic.s: New test.
	* testsuite/gas/aarch64/illegal.l: New tests.
	* testsuite/gas/aarch64/illegal.s: New tests.

Comments

Tamar Christina July 1, 2019, 7:20 a.m. | #1
Hi Barnaby,

Your patch looks OK to be, however I am not a maintainer so you still need maintainer approval for this.

Thanks for the fix,
Tamar

> -----Original Message-----

> From: binutils-owner@sourceware.org <binutils-owner@sourceware.org> On 

> Behalf Of Barnaby Wilks

> Sent: Wednesday, June 26, 2019 15:20

> To: binutils@sourceware.org

> Cc: nd <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; 

> Marcus Shawcroft <Marcus.Shawcroft@arm.com>

> Subject: [PATCH][binutils][AArch64] Fix diagnostic error message for 

> structural load/store by element.

> 

> Hello,

> 

> This patch fixes a bug where an incorrect structural load/store by 

> element instruction would generate the wrong error message.

> 

> For example, when provided with the (incorrect) instruction

> 

> st4 {v0.16b-v3.16b}[4],[x0]

> 

> currently assembler provides the following error message

> "Error: comma expected between operands at operand 2 -- `st4 {v0.16b- 

> v3.16b}[4],[x0]'".

> 

> This was due to the assembler consuming the {v0.16b-v3.16b} as the 

> first operand leaving [4],[x0] as what it believed to be the second operand.

> 

> The actual error is that the first operand should be of element type 

> and not vector type (as provided). The new diagnostic for this error 

> is

> "Error: expected element type rather than vector type at operand 1 -- 

> `st4 {v0.16b-v3.16b}[4],[x0]'.

> 

> Added testcases to check for the correct diagnostic message as well as 

> checking that variations of the structural load/store by element 

> instruction also generate the error when they have the same problem.

> 

> Cross compiled and regtested aarch64-none-elf and 

> aarch64-none-linux-gnu and no issues found.

> 

> I don't have write access, so if it's OK then could someone commit on 

> my behalf?

> 

> Thanks,

> Barney

> 

> gas/ChangeLog:

> 

> 2019-06-26  Barnaby Wilks  <barnaby.wilks@arm.com>

> 

> 	* config/tc-aarch64.c (parse_operands): Add error check.

> 	* testsuite/gas/aarch64/diagnostic.l: New test.

> 	* testsuite/gas/aarch64/diagnostic.s: New test.

> 	* testsuite/gas/aarch64/illegal.l: New tests.

> 	* testsuite/gas/aarch64/illegal.s: New tests.
Nick Clifton July 2, 2019, 1:12 p.m. | #2
Hi Barnaby,

> 2019-06-26  Barnaby Wilks  <barnaby.wilks@arm.com>

> 

> 	* config/tc-aarch64.c (parse_operands): Add error check.

> 	* testsuite/gas/aarch64/diagnostic.l: New test.

> 	* testsuite/gas/aarch64/diagnostic.s: New test.

> 	* testsuite/gas/aarch64/illegal.l: New tests.

> 	* testsuite/gas/aarch64/illegal.s: New tests.


Approved and applied.

Cheers
  Nick

Patch

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 000a81c64f8e95eef920fe447358f99bb68b84f9..b71b34a9f8beec7fe4dc44f0ab36c8c2bf3c7b67 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -5733,11 +5733,20 @@  parse_operands (char *str, const aarch64_opcode *opcode)
 	      val = parse_vector_reg_list (&str, reg_type, &vectype);
 	      if (val == PARSE_FAIL)
 		goto failure;
+
 	      if (! reg_list_valid_p (val, /* accept_alternate */ 0))
 		{
 		  set_fatal_syntax_error (_("invalid register list"));
 		  goto failure;
 		}
+
+	      if (vectype.width != 0 && *str != ',')
+		{
+		  set_fatal_syntax_error (_("expected element type "
+					    "rather than vector type"));
+		  goto failure;
+		}
+
 	      info->reglist.first_regno = (val >> 2) & 0x1f;
 	      info->reglist.num_regs = (val & 0x3) + 1;
 	    }
diff --git a/gas/testsuite/gas/aarch64/diagnostic.l b/gas/testsuite/gas/aarch64/diagnostic.l
index 6aae306a915d48ad8215d9abb00d7016132702f2..b5f304af70b35da0e488fe363eab9d040d5b95d5 100644
--- a/gas/testsuite/gas/aarch64/diagnostic.l
+++ b/gas/testsuite/gas/aarch64/diagnostic.l
@@ -182,3 +182,4 @@ 
 [^:]*:311: Warning: unpredictable: identical transfer and status registers --`stlxr w26,x26,\[x3\]'
 [^:]*:312: Warning: unpredictable: identical transfer and status registers --`ldxp x26,x26,\[x5\]'
 [^:]*:313: Warning: unpredictable: identical transfer and status registers --`ldxp x26,x1,\[x26\]'
+[^:]*:314: Error: expected element type rather than vector type at operand 1 -- `st4 {v0\.16b-v3\.16b}\[4\],\[x0\]'
diff --git a/gas/testsuite/gas/aarch64/diagnostic.s b/gas/testsuite/gas/aarch64/diagnostic.s
index c18c48e11694d96177bbe0b7690400fa580d3cf8..21cbc53d8972bdf542a8c2088729af1591973cf1 100644
--- a/gas/testsuite/gas/aarch64/diagnostic.s
+++ b/gas/testsuite/gas/aarch64/diagnostic.s
@@ -311,3 +311,4 @@ 
 	stlxr	w26, x26, [x3]
 	ldxp	x26, x26, [x5]
 	ldxp	x26, x1, [x26]
+	st4	{v0.16b-v3.16b}[4], [x0]
diff --git a/gas/testsuite/gas/aarch64/illegal.l b/gas/testsuite/gas/aarch64/illegal.l
index 0c90110580b569e30f28d9314abbfc694c6537ff..75ef830a8f1e2e35574fb9c96d9c9c3a8683bb17 100644
--- a/gas/testsuite/gas/aarch64/illegal.l
+++ b/gas/testsuite/gas/aarch64/illegal.l
@@ -575,4 +575,8 @@ 
 [^:]*:585: Error: .*`fcmgt v0\.2d,v0\.2d,#-0\.0'
 [^:]*:589: Error: .*`fmov s9,x0'
 [^:]*:590: Error: .*`fmov d7,w1'
-[^:]*:592: Error: .*
+[^:]*:592: Error: .*`st1 {v0\.16b}\[0\],\[x0\]'
+[^:]*:593: Error: .*`st2 {v0\.16b-v1\.16b}\[1\],\[x0\]'
+[^:]*:594: Error: .*`st3 {v0\.16b-v2\.16b}\[2\],\[x0\]'
+[^:]*:595: Error: .*`st4 {v0\.8b-v3\.8b}\[4\],\[x0\]'
+[^:]*:597: Error: .*
diff --git a/gas/testsuite/gas/aarch64/illegal.s b/gas/testsuite/gas/aarch64/illegal.s
index 7efe69ea0b785a56df1afac43a83b2dc5c72f612..ef03cc95aae71fb1b5d620ec2f06ba78724d9727 100644
--- a/gas/testsuite/gas/aarch64/illegal.s
+++ b/gas/testsuite/gas/aarch64/illegal.s
@@ -589,4 +589,9 @@  one_label:
 	fmov 	s9, x0
 	fmov	d7, w1
 
+	st1 {v0.16b}[0],[x0]
+	st2 {v0.16b-v1.16b}[1],[x0]
+	st3 {v0.16b-v2.16b}[2],[x0]
+	st4 {v0.8b-v3.8b}[4],[x0]
+
 	// End (for errors during literal pool generation)