[PATCH] gas/aarch64: fix assertion failure caused by glitch in optional operand handling

[PATCH] gas/aarch64: fix assertion failure caused by glitch in optional operand handling

[Jiong Wang]

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

AArch64 gas support optional operand format, for example, "ret x30" could be
written as "ret".

The user could also give the register themselves, like "ret xx", while the
given
operand may be in wrong register format.

The problem is We have glitch in our current invalid operand handling code
when
optional operand involved in.

our current code logic is:

if operand_is_wrong
  goto invalid_label

invalid_label:
  if (!operand_could_be_optional)
    report_operand_error ();

  use_default_value () <-- A
  clear_errors ()

The logic is wrong because if the operand given by user is in wrong format,
and
the operand is optional, still we will reach A, and the error is propagated
to
later stage in assembler, and we eventually run into the following assertion
failure:

02:38:40: Assertion failure in output_operand_error_record at
...tc-aarch64.c line 3905.
02:38:40: Please report this bug.

*this patch modify the code logic to*

if operand_is_wrong
  goto invalid_label

invalid_label:
  if (!operand_could_be_optional)
    report_operand_error ();
   
  if (given_operand)
    report_operand_error ();

  use_default_value ()
  clear_errors ()

OK for trunk ?

Thanks.

Jiong

gas/
  * config/tc-aarch64.c (END_OF_INSN): New macro.
  (parse_operands): Handle operand given and be in wrong
  format when operand is optional.

gas/testsuite/
  * gas/aarch64/diagnostic.s: New test patterns.
  * gas/aarch64/diagnostic.l: Likewise.

[-- Attachment #2: fix-optionalv2.patch --]
[-- Type: application/octet-stream, Size: 4539 bytes --]

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 67c0871..6c476d7 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -42,6 +42,8 @@
 
 #define streq(a, b)	      (strcmp (a, b) == 0)
 
+#define END_OF_INSN '\0'
+
 static aarch64_feature_set cpu_variant;
 
 /* Variables that we set while parsing command-line options.  Once all
@@ -5301,6 +5303,41 @@ failure:
 
       if (! backtrack_pos)
 	goto parse_operands_return;
+      else
+	{
+	  /* We reach here because this operand is marked as optional.
+	     For an optional operand, user may have given an operand
+	     instead of using the default value and the given operand
+	     may itself be in wrong format. So although "backtrace_pos"
+	     be true here, it may still caused by operand format error.
+	     we need to identify this.  */
+	  char *tmp = backtrack_pos;
+	  char endchar = END_OF_INSN;
+
+	  if (i != (aarch64_num_of_operands (opcode) - 1))
+	    endchar = ',';
+	  skip_past_char (&tmp, ',');
+
+	  /* User have given optional operand, but the operand is in
+	     wrong format.  */
+	  if (*tmp != endchar)
+	    goto parse_operands_return;
+
+	  /* Try to use the default value, but before that, make sure there is no
+	     extra ',' which is wrong format.  For example, the fifth operand of
+	     'sys' is optional:
+
+	       sys #0,c0,c0,#0,  <--- wrong
+	       sys #0,c0,c0,#0   <--- correct
+
+	       The second format is correct.  */
+	  else if (comma_skipped_p && i && endchar == END_OF_INSN)
+	    {
+	      set_fatal_syntax_error
+		(_("unexpected comma before the omitted optional operand"));
+	      goto parse_operands_return;
+	    }
+	}
 
       /* Reaching here means we are dealing with an optional operand that is
 	 omitted from the assembly line.  */
@@ -5312,15 +5349,6 @@ failure:
       str = backtrack_pos;
       backtrack_pos = 0;
 
-      /* If this is the last operand that is optional and omitted, but without
-	 the presence of a comma.  */
-      if (i && comma_skipped_p && i == aarch64_num_of_operands (opcode) - 1)
-	{
-	  set_fatal_syntax_error
-	    (_("unexpected comma before the omitted optional operand"));
-	  goto parse_operands_return;
-	}
-
       /* Clear any error record after the omitted optional operand has been
 	 successfully handled.  */
       clear_error ();
diff --git a/gas/testsuite/gas/aarch64/diagnostic.l b/gas/testsuite/gas/aarch64/diagnostic.l
index 00a0d7a..d906ce3 100644
--- a/gas/testsuite/gas/aarch64/diagnostic.l
+++ b/gas/testsuite/gas/aarch64/diagnostic.l
@@ -14,7 +14,7 @@
 [^:]*:16: Error: immediate value out of range 0 to 31 at operand 2 -- `ccmp x0,-1,10,le'
 [^:]*:17: Error: extraneous register at operand 2 -- `tlbi alle3is,x0'
 [^:]*:18: Error: missing register at operand 2 -- `tlbi vaale1is'
-[^:]*:19: Error: unexpected characters following instruction at operand 1 -- `tlbi vaale1is x0'
+[^:]*:19: Error: comma expected between operands at operand 2 -- `tlbi vaale1is x0'
 [^:]*:20: Error: immediate value out of range 0 to 1 at operand 1 -- `msr spsel,3'
 [^:]*:21: Error: immediate value out of range 1 to 64 at operand 3 -- `fcvtzu x15,d31,#66'
 [^:]*:22: Error: immediate value out of range 1 to 32 at operand 3 -- `scvtf s0,w0,33'
@@ -97,3 +97,10 @@
 [^:]*:99: Error: operand 3 should be one of the standard conditions, excluding AL and NV. -- `cinc w0,w1,nv'
 [^:]*:100: Error: operand 2 should be one of the standard conditions, excluding AL and NV. -- `cset w0,al'
 [^:]*:101: Error: operand 2 should be one of the standard conditions, excluding AL and NV. -- `cset w0,nv'
+[^:]*:104: Error: operand 1 should be an integer register -- `ret lr'
+[^:]*:105: Error: operand 1 should be an integer register -- `ret kk'
+[^:]*:106: Error: immediate operand required at operand 1 -- `clrex x0'
+[^:]*:107: Error: immediate operand required at operand 1 -- `clrex w0'
+[^:]*:108: Error: constant expression required at operand 1 -- `clrex kk'
+[^:]*:109: Error: operand 5 should be an integer register -- `sys #0,c0,c0,#0,kk'
+[^:]*:110: Error: operand 5 should be an integer register -- `sys #0,c0,c0,#0,lr'
diff --git a/gas/testsuite/gas/aarch64/diagnostic.s b/gas/testsuite/gas/aarch64/diagnostic.s
index 2bb16b0..4210530 100644
--- a/gas/testsuite/gas/aarch64/diagnostic.s
+++ b/gas/testsuite/gas/aarch64/diagnostic.s
@@ -99,3 +99,12 @@
 	cinc	w0, w1, nv
 	cset	w0, al
 	cset	w0, nv
+
+	# test diagnostic info on optional operand
+	ret	lr
+	ret	kk
+	clrex	x0
+	clrex	w0
+	clrex	kk
+	sys	#0, c0, c0, #0, kk
+	sys	#0, c0, c0, #0, lr

Re: [PATCH] gas/aarch64: fix assertion failure caused by glitch in optional operand handling

[Nicholas Clifton]

Hi Jiong,

> gas/
>    * config/tc-aarch64.c (END_OF_INSN): New macro.
>    (parse_operands): Handle operand given and be in wrong
>    format when operand is optional.
>
> gas/testsuite/
>    * gas/aarch64/diagnostic.s: New test patterns.
>    * gas/aarch64/diagnostic.l: Likewise.

Approved and applied.

Cheers
   Nick