Re: [Patch][AArch64] - Error on load pair to same register

[Jiong Wang <jiong dot wang at arm dot com>]

On 12/11/14 20:07, Ryan Mansfield wrote:

> Hi,
>
> I came across a SIGILL at runtime caused by a typo specifying the same
> register in a load pair.
>
> The ARM DDI 0487A says on page C6-507
>
> if memop == MemOp_LOAD && t == t2 then
> 	Constraint c = ConstrainUnpredictable();
> 	assert c IN {Constraint_UNKNOWN, Constraint_UNDEF, Constraint_NOP};
> 	case c of
> 		when Constraint_UNKNOWN rt_unknown = TRUE; // result is UNKNOWN
> 		when Constraint_UNDEF UnallocatedEncoding();
> 		when Constraint_NOP EndOfInstruction();
>
> I doubt that any one would intentionally do this operation since the
> behaviour is unpredictable so it just seems safer to reject the code.

Hi Ryan,

  thanks for reporting  this.

  I think the encoding of same register is allowed in ISA manual, while
  the behavior is unpredictable. In principle, all allowed encoding need
  to be supported, while if its behavior is unpredictable, then we need
  to give warning instead of error which abort the assembling.

  for the code, see my comments below.

> 2014-11-12  Ryan Mansfield  <rmansfield@qnx.com>
>
>           * aarch64-opc.c (operand_general_constraint_met_p): Add constraint
>           that load pair must have different registers.

==> could you please add a testcase for this?

+
+      switch (opcode->iclass)
+	{
+	  case ldstpair_indexed:
+	  case ldstpair_off:
+	  case ldstnapair_offs:
+	    if (type == AARCH64_OPND_Rt2)
+	      {
+		assert (idx == 1 && (aarch64_get_operand_class (opnds[0].type)
+				     == AARCH64_OPND_CLASS_INT_REG));

==> the indention above is not very clear, better to be

    assert (idx == 1
            && (aarch64_get_operand_class (opnds[0].type)
                == AARCH64_OPND_CLASS_INT_REG));

+		if ((opcode->opcode & (1 << 22)) && opnds[idx].reg.regno
+		    == opnds[idx - 1].reg.regno)

==> likewise.


+		  {
+		    set_other_error (mismatch_detail, idx,
+				     _("reg pair must differ"));
+		    return 0;
+		  }
+	      }
+	    break;
+	  default:
+	    break;
+	}
+

 ===> above same register check code are duplicated for both INT and FP.
 ===> just remain the check in FP, and let INT fall through. something like

    AARCH64_OPND_CLASS_INT_REG:
    ...
    /* Fall through.  */
    
    AARCH64_OPND_CLASS_FP_REG:
      switch (opcode->iclass)
	{
	  case ldstpair_indexed:
	  case ldstpair_off:
          ...
          ...
   

Regards,
Jiong

> Regards,
>
> Ryan Mansfield