Re: [PATCH][Binutils][AArch64] Allow non-fatal errors to be emitted and for disassembly notes be placed [Patch (2/3)]

[Tamar Christina <Tamar dot Christina at arm dot com>]

Hi Nick,

> Hi Tamar,
> 
> > This patch adds a new platform option "notes" that can be used to indicate if
> > disassembly notes should be placed in the disassembly as comments.
> 
> > Ok for master?
> 
> Almost.  Please could you:
> 
>   * Add an entry in the binutils/NEWS file about this new feature.

Done, See attached patch.

> 
>   * Add a test case to check that the new feature works.  (Either to
>     the gas or binutils testsuites, whichever you prefer).

I have the testcase in the 3rd patch of the series because this one doesn't have any
consumers of the new functionality yet so I can't test it from the build binaries yet.

https://sourceware.org/ml/binutils/2018-05/msg00132.html the sysreg-diagnostic test.

> 
>   * Fix a minor spelling typo:
> 
> +  /* The way errors are collected in the back-end if a bit non-intuitive.  But
> +     essentially, because each operand template is tried recursively you may
> 
>    "...back-end if..." should be "...back-end is..."

Done.

Ok for master? (New changelog below)

Kind Regards,
Tamar

binutils/
2018-05-15  Tamar Christina  <tamar.christina@arm.com>

	PR binutils/21446
	* doc/binutils.texi (-M): Document AArch64 options.
	* NEWS: Document notes and warnings.

gas/
2018-05-15  Tamar Christina  <tamar.christina@arm.com>

	PR binutils/21446
	* config/tc-aarch64.c (print_operands): Indicate no notes.
	(output_operand_error_record): Support non-fatal errors.
	(output_operand_error_report, warn_unpredictable_ldst, md_assemble):
	Likewise.

include/
2018-05-15  Tamar Christina  <tamar.christina@arm.com>

	PR binutils/21446
	* opcode/aarch64.h (aarch64_operand_error): Add non_fatal.
	(aarch64_print_operand): Support notes.

opcodes/
2018-05-15  Tamar Christina  <tamar.christina@arm.com>

	PR binutils/21446
	* aarch64-dis.c (no_notes: New.
	(parse_aarch64_dis_option): Support notes.
	(aarch64_decode_insn, print_operands): Likewise.
	(print_aarch64_disassembler_options): Document notes.
	* aarch64-opc.c (aarch64_print_operand): Support notes.

> 
> Cheers
>   Nick
> 

-- 

diff --git a/binutils/NEWS b/binutils/NEWS
index da841de5568754ee538e19306bda5dc68a310a51..d90d889c59c8aa27013870c3156ac4e1b11fd085 100644
--- a/binutils/NEWS
+++ b/binutils/NEWS
@@ -2,6 +2,14 @@
 
 * Add support for disassembling netronome Flow Processor (NFP) firmware files.
 
+* The AArch64 port now supports showing disassembly notes which are emitted
+  when inconsistencies are found with the instruction that may result in the
+  instruction being invalid.  These can be turned on with the option -M notes
+  to objdump.
+
+* The AArch64 port now emits warnings when a combination of an instruction and
+  a named register could be invalid.
+
 Changes in 2.30:
 
 * Add --debug-dump=links option to readelf and --dwarf=links option to objdump
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 3a32cc40771a3138be4c3ae6295895aeddd99a29..e4d32b66aaa8246eacdf6926fe83cfb67edbc097 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -2342,6 +2342,11 @@ using the switch @option{--disassembler-options=force-thumb}.  This can be
 useful when attempting to disassemble thumb code produced by other
 compilers.
 
+For AArch64 targets this switch can be used to set whether instructions are
+disassembled as the most general instruction using the @option{-M no-aliases}
+option or whether instruction notes should be generated as comments in the
+disasssembly using @option{-M notes}.
+
 For the x86, some of the options duplicate functions of the @option{-m}
 switch, but allow finer grained control.  Multiple selections from the
 following may be specified as a comma separated string.
diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 750025ac11341c9b6f6e4f191596ddef3e152a13..b007fb33b5cece4f1590c877751f78b046881835 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -4533,7 +4533,8 @@ print_operands (char *buf, const aarch64_opcode *opcode,
 	break;
 
       /* Generate the operand string in STR.  */
-      aarch64_print_operand (str, sizeof (str), 0, opcode, opnds, i, NULL, NULL);
+      aarch64_print_operand (str, sizeof (str), 0, opcode, opnds, i, NULL, NULL,
+			     NULL);
 
       /* Delimiter.  */
       if (str[0] != '\0')
@@ -4579,12 +4580,14 @@ output_operand_error_record (const operand_error_record *record, char *str)
   enum aarch64_opnd opd_code = (idx >= 0 ? opcode->operands[idx]
 				: AARCH64_OPND_NIL);
 
+  typedef void (*handler_t)(const char *format, ...);
+  handler_t handler = detail->non_fatal ? as_warn : as_bad;
+
   switch (detail->kind)
     {
     case AARCH64_OPDE_NIL:
       gas_assert (0);
       break;
-
     case AARCH64_OPDE_SYNTAX_ERROR:
     case AARCH64_OPDE_RECOVERABLE:
     case AARCH64_OPDE_FATAL_SYNTAX_ERROR:
@@ -4594,21 +4597,21 @@ output_operand_error_record (const operand_error_record *record, char *str)
       if (detail->error != NULL)
 	{
 	  if (idx < 0)
-	    as_bad (_("%s -- `%s'"), detail->error, str);
+	    handler (_("%s -- `%s'"), detail->error, str);
 	  else
-	    as_bad (_("%s at operand %d -- `%s'"),
-		    detail->error, idx + 1, str);
+	    handler (_("%s at operand %d -- `%s'"),
+		     detail->error, idx + 1, str);
 	}
       else
 	{
 	  gas_assert (idx >= 0);
-	  as_bad (_("operand %d must be %s -- `%s'"), idx + 1,
-		aarch64_get_operand_desc (opd_code), str);
+	  handler (_("operand %d must be %s -- `%s'"), idx + 1,
+		   aarch64_get_operand_desc (opd_code), str);
 	}
       break;
 
     case AARCH64_OPDE_INVALID_VARIANT:
-      as_bad (_("operand mismatch -- `%s'"), str);
+      handler (_("operand mismatch -- `%s'"), str);
       if (verbose_error_p)
 	{
 	  /* We will try to correct the erroneous instruction and also provide
@@ -4705,36 +4708,36 @@ output_operand_error_record (const operand_error_record *record, char *str)
       break;
 
     case AARCH64_OPDE_UNTIED_OPERAND:
-      as_bad (_("operand %d must be the same register as operand 1 -- `%s'"),
-	      detail->index + 1, str);
+      handler (_("operand %d must be the same register as operand 1 -- `%s'"),
+	       detail->index + 1, str);
       break;
 
     case AARCH64_OPDE_OUT_OF_RANGE:
       if (detail->data[0] != detail->data[1])
-	as_bad (_("%s out of range %d to %d at operand %d -- `%s'"),
-		detail->error ? detail->error : _("immediate value"),
-		detail->data[0], detail->data[1], idx + 1, str);
+	handler (_("%s out of range %d to %d at operand %d -- `%s'"),
+		 detail->error ? detail->error : _("immediate value"),
+		 detail->data[0], detail->data[1], idx + 1, str);
       else
-	as_bad (_("%s must be %d at operand %d -- `%s'"),
-		detail->error ? detail->error : _("immediate value"),
-		detail->data[0], idx + 1, str);
+	handler (_("%s must be %d at operand %d -- `%s'"),
+		 detail->error ? detail->error : _("immediate value"),
+		 detail->data[0], idx + 1, str);
       break;
 
     case AARCH64_OPDE_REG_LIST:
       if (detail->data[0] == 1)
-	as_bad (_("invalid number of registers in the list; "
-		  "only 1 register is expected at operand %d -- `%s'"),
-		idx + 1, str);
+	handler (_("invalid number of registers in the list; "
+		   "only 1 register is expected at operand %d -- `%s'"),
+		 idx + 1, str);
       else
-	as_bad (_("invalid number of registers in the list; "
-		  "%d registers are expected at operand %d -- `%s'"),
-	      detail->data[0], idx + 1, str);
+	handler (_("invalid number of registers in the list; "
+		   "%d registers are expected at operand %d -- `%s'"),
+	       detail->data[0], idx + 1, str);
       break;
 
     case AARCH64_OPDE_UNALIGNED:
-      as_bad (_("immediate value must be a multiple of "
-		"%d at operand %d -- `%s'"),
-	      detail->data[0], idx + 1, str);
+      handler (_("immediate value must be a multiple of "
+		 "%d at operand %d -- `%s'"),
+	       detail->data[0], idx + 1, str);
       break;
 
     default:
@@ -4748,10 +4751,15 @@ output_operand_error_record (const operand_error_record *record, char *str)
    When this function is called, the operand error information had
    been collected for an assembly line and there will be multiple
    errors in the case of multiple instruction templates; output the
-   error message that most closely describes the problem.  */
+   error message that most closely describes the problem.
+
+   The errors to be printed can be filtered on printing all errors
+   or only non-fatal errors.  This distinction has to be made because
+   the error buffer may already be filled with fatal errors we don't want to
+   print due to the different instruction templates.  */
 
 static void
-output_operand_error_report (char *str)
+output_operand_error_report (char *str, bfd_boolean non_fatal_only)
 {
   int largest_error_pos;
   const char *msg = NULL;
@@ -4769,9 +4777,14 @@ output_operand_error_report (char *str)
   /* Only one error.  */
   if (head == operand_error_report.tail)
     {
-      DEBUG_TRACE ("single opcode entry with error kind: %s",
-		   operand_mismatch_kind_names[head->detail.kind]);
-      output_operand_error_record (head, str);
+      /* If the only error is a non-fatal one and we don't want to print it,
+	 just exit.  */
+      if (!non_fatal_only || head->detail.non_fatal)
+	{
+	  DEBUG_TRACE ("single opcode entry with error kind: %s",
+		       operand_mismatch_kind_names[head->detail.kind]);
+	  output_operand_error_record (head, str);
+	}
       return;
     }
 
@@ -4791,7 +4804,10 @@ output_operand_error_report (char *str)
   largest_error_pos = -2; /* Index can be -1 which means unknown index.  */
   for (curr = head; curr != NULL; curr = curr->next)
     {
-      if (curr->detail.kind != kind)
+      /* If we don't want to print non-fatal errors then don't consider them
+	 at all.  */
+      if (curr->detail.kind != kind
+	  || (non_fatal_only && !head->detail.non_fatal))
 	continue;
       /* If there are multiple errors, pick up the one with the highest
 	 mismatching operand index.  In the case of multiple errors with
@@ -4807,6 +4823,17 @@ output_operand_error_report (char *str)
 	}
     }
 
+  /* The way errors are collected in the back-end is a bit non-intuitive.  But
+     essentially, because each operand template is tried recursively you may
+     always have errors collected from the previous tried OPND.  These are
+     usually skipped if there is one successful match.  However now with the
+     non-fatal errors we have to ignore those previously collected hard errors
+     when we're only interested in printing the non-fatal ones.  This condition
+     prevents us from printing errors that are not appropriate, since we did
+     match a condition, but it also has warnings that it wants to print.  */
+  if (non_fatal_only && !record)
+    return;
+
   gas_assert (largest_error_pos != -2 && record != NULL);
   DEBUG_TRACE ("Pick up error kind %s to report",
 	       operand_mismatch_kind_names[record->detail.kind]);
@@ -6352,18 +6379,18 @@ parse_operands (char *str, const aarch64_opcode *opcode)
 	  goto regoff_addr;
 
 	case AARCH64_OPND_SYSREG:
-	 {
-	   uint32_t sysreg_flags;
-	   if ((val = parse_sys_reg (&str, aarch64_sys_regs_hsh, 1, 0,
-				     &sysreg_flags)) == PARSE_FAIL)
-	     {
-	       set_syntax_error (_("unknown or missing system register name"));
-	       goto failure;
-	     }
-	   inst.base.operands[i].sysreg.value = val;
-	   inst.base.operands[i].sysreg.flags = sysreg_flags;
-	   break;
-	 }
+	  {
+	    uint32_t sysreg_flags;
+	    if ((val = parse_sys_reg (&str, aarch64_sys_regs_hsh, 1, 0,
+				      &sysreg_flags)) == PARSE_FAIL)
+	      {
+		set_syntax_error (_("unknown or missing system register name"));
+		goto failure;
+	      }
+	    inst.base.operands[i].sysreg.value = val;
+	    inst.base.operands[i].sysreg.flags = sysreg_flags;
+	    break;
+	  }
 
 	case AARCH64_OPND_PSTATEFIELD:
 	  if ((val = parse_sys_reg (&str, aarch64_pstatefield_hsh, 0, 1, NULL))
@@ -6689,15 +6716,15 @@ do_encode (const aarch64_opcode *opcode, aarch64_inst *instr,
 	   aarch64_insn *code)
 {
   aarch64_operand_error error_info;
+  memset (&error_info, '\0', sizeof (error_info));
   error_info.kind = AARCH64_OPDE_NIL;
-  if (aarch64_opcode_encode (opcode, instr, code, NULL, &error_info))
+  if (aarch64_opcode_encode (opcode, instr, code, NULL, &error_info)
+      && !error_info.non_fatal)
     return TRUE;
-  else
-    {
-      gas_assert (error_info.kind != AARCH64_OPDE_NIL);
-      record_operand_error_info (opcode, &error_info);
-      return FALSE;
-    }
+
+  gas_assert (error_info.kind != AARCH64_OPDE_NIL);
+  record_operand_error_info (opcode, &error_info);
+  return error_info.non_fatal;
 }
 
 #ifdef DEBUG_AARCH64
@@ -6834,6 +6861,9 @@ md_assemble (char *str)
 	      memcpy (copy, &inst.base, sizeof (struct aarch64_inst));
 	      output_inst (copy);
 	    }
+
+	  /* Issue non-fatal messages if any.  */
+	  output_operand_error_report (str, TRUE);
 	  return;
 	}
 
@@ -6847,7 +6877,7 @@ md_assemble (char *str)
   while (template != NULL);
 
   /* Issue the error messages if any.  */
-  output_operand_error_report (str);
+  output_operand_error_report (str, FALSE);
 }
 
 /* Various frobbings of labels and their addresses.  */
diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
index 7bc88c572314fbd0d757102771f1a98fd76de2ec..1c0013e70629a0154a77337f7c20fe9de845ebc8 100644
--- a/include/opcode/aarch64.h
+++ b/include/opcode/aarch64.h
@@ -1101,6 +1101,7 @@ struct aarch64_operand_error
   int index;
   const char *error;
   int data[3];	/* Some data for extra information.  */
+  bfd_boolean non_fatal;
 };
 
 typedef struct aarch64_operand_error aarch64_operand_error;
@@ -1125,7 +1126,8 @@ aarch64_get_opcode (enum aarch64_op);
 /* Generate the string representation of an operand.  */
 extern void
 aarch64_print_operand (char *, size_t, bfd_vma, const aarch64_opcode *,
-		       const aarch64_opnd_info *, int, int *, bfd_vma *);
+		       const aarch64_opnd_info *, int, int *, bfd_vma *,
+		       char **);
 
 /* Miscellaneous interface.  */
 
diff --git a/opcodes/aarch64-dis.c b/opcodes/aarch64-dis.c
index 5994b2b655138885b053bf92b94ab2f52d04cf4e..ae53e49201a2aad5285ca9c44a01eafb8c950380 100644
--- a/opcodes/aarch64-dis.c
+++ b/opcodes/aarch64-dis.c
@@ -46,7 +46,8 @@ static bfd_vma last_mapping_addr = 0;
 
 /* Other options */
 static int no_aliases = 0;	/* If set disassemble as most general inst.  */
-
+static int no_notes = 1;	/* If set do not print disassemble notes in the
+				  output as comments.  */
 
 static void
 set_default_aarch64_dis_options (struct disassemble_info *info ATTRIBUTE_UNUSED)
@@ -69,6 +70,18 @@ parse_aarch64_dis_option (const char *option, unsigned int len ATTRIBUTE_UNUSED)
       return;
     }
 
+  if (CONST_STRNEQ (option, "no-notes"))
+    {
+      no_notes = 1;
+      return;
+    }
+
+  if (CONST_STRNEQ (option, "notes"))
+    {
+      no_notes = 0;
+      return;
+    }
+
 #ifdef DEBUG_AARCH64
   if (CONST_STRNEQ (option, "debug_dump"))
     {
@@ -2950,6 +2963,7 @@ print_operands (bfd_vma pc, const aarch64_opcode *opcode,
 		const aarch64_opnd_info *opnds, struct disassemble_info *info)
 {
   int i, pcrel_p, num_printed;
+  char *notes = NULL;
   for (i = 0, num_printed = 0; i < AARCH64_MAX_OPND_NUM; ++i)
     {
       char str[128];
@@ -2964,7 +2978,7 @@ print_operands (bfd_vma pc, const aarch64_opcode *opcode,
 
       /* Generate the operand string in STR.  */
       aarch64_print_operand (str, sizeof (str), pc, opcode, opnds, i, &pcrel_p,
-			     &info->target);
+			     &info->target, &notes);
 
       /* Print the delimiter (taking account of omitted operand(s)).  */
       if (str[0] != '\0')
@@ -2977,6 +2991,9 @@ print_operands (bfd_vma pc, const aarch64_opcode *opcode,
       else
 	(*info->fprintf_func) (info->stream, "%s", str);
     }
+
+    if (notes && !no_notes)
+      (*info->fprintf_func) (info->stream, "\t; note: %s", notes);
 }
 
 /* Set NAME to a copy of INST's mnemonic with the "." suffix removed.  */
@@ -3337,6 +3354,12 @@ with the -M switch (multiple options should be separated by commas):\n"));
   fprintf (stream, _("\n\
   aliases            Do print instruction aliases.\n"));
 
+  fprintf (stream, _("\n\
+  no-notes         Don't print instruction notes.\n"));
+
+  fprintf (stream, _("\n\
+  notes            Do print instruction notes.\n"));
+
 #ifdef DEBUG_AARCH64
   fprintf (stream, _("\n\
   debug_dump         Temp switch for debug trace.\n"));
diff --git a/opcodes/aarch64-opc.c b/opcodes/aarch64-opc.c
index c688b7c7745b8d7f9b6f74ee74ec3e9887a26364..9d7f941184594131feec5abca8d7ffbec0b1f793 100644
--- a/opcodes/aarch64-opc.c
+++ b/opcodes/aarch64-opc.c
@@ -3033,7 +3033,7 @@ void
 aarch64_print_operand (char *buf, size_t size, bfd_vma pc,
 		       const aarch64_opcode *opcode,
 		       const aarch64_opnd_info *opnds, int idx, int *pcrel_p,
-		       bfd_vma *address)
+		       bfd_vma *address, char** notes ATTRIBUTE_UNUSED)
 {
   unsigned int i, num_conds;
   const char *name = NULL;