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

[Jiong Wang]

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