[z80] Treat undocumented instruction sli properly.

Message ID 6214862.0BXtLZDtFP@phurua
State New
Headers show
Series
  • [z80] Treat undocumented instruction sli properly.
Related show

Commit Message

Arnold Metselaar Aug. 16, 2018, 11:16 a.m.
Hello,

Just after I sent in the patch to retire myself as maintainer for z80, I 
spotted an error in the code that may lead to as-z80 silently accepting 
instructions that should be rejected.

The undocumented instruction in question is sll, also known as sli. It should 
be treated as unportable as it behaves differently on R800, but it is treated 
as if it were a documented instruction on z80.
The patch below fixes this. Can the new maintainer review and push this change?

Kind regards, 
Arnold Metselaar

---

gas/Changelog
2018-08-16 Arnold Metselaar <arnold.metsel@gmail.com>
	* config/tc-z80.c: Correct treatment of undocumented instruction sli/sll. 
	(emit_mr): Add argument unportable.
	(emit_bit): Adapt call to emit_mr.
	(emit_mr_z80): New function.
	(emit_mr_unportable): New function.
	(instab[]): Replace emit_mr with emit_mr_z80 or emit_mr_unportable as 
appropriate.

Comments

Nick Clifton Aug. 21, 2018, 2:52 p.m. | #1
Hi Arnold,

> gas/Changelog

> 2018-08-16 Arnold Metselaar <arnold.metsel@gmail.com>

> 	* config/tc-z80.c: Correct treatment of undocumented instruction sli/sll. 

> 	(emit_mr): Add argument unportable.

> 	(emit_bit): Adapt call to emit_mr.

> 	(emit_mr_z80): New function.

> 	(emit_mr_unportable): New function.

> 	(instab[]): Replace emit_mr with emit_mr_z80 or emit_mr_unportable as 

> appropriate.

 
Approved and applied.  Thanks for making one final patch! :-)

Cheers
  Nick
Gunther Nikl Aug. 21, 2018, 8:19 p.m. | #2
Arnold Metselaar <arnold.metselaar@planet.nl> wrote:
> gas/Changelog

> 2018-08-16 Arnold Metselaar <arnold.metsel@gmail.com>

> 	* config/tc-z80.c: Correct treatment of undocumented

> instruction sli/sll. (emit_mr): Add argument unportable.

> 	(emit_bit): Adapt call to emit_mr.

> 	(emit_mr_z80): New function.

> 	(emit_mr_unportable): New function.



The ChangeLog entry does not agree with the actual modification.
In the config/tc-z80.c the function name is "emit_mr_unport".

Regards,
Gunther

> 	(instab[]): Replace emit_mr with emit_mr_z80 or

> emit_mr_unportable as appropriate.

> 

> 

> diff --git a/gas/config/tc-z80.c b/gas/config/tc-z80.c 

> index 5a4fd38fce..ede7cbe1db 100644 

> --- a/gas/config/tc-z80.c 

> +++ b/gas/config/tc-z80.c 

> @@ -850,7 +850,7 @@ emit_m (char prefix, char opcode, const char

> *args) combinations (ix+d),r and (iy+d),r (if unportable instructions 

>    are allowed).  */ 

> static const char * 

> -emit_mr (char prefix, char opcode, const char *args) 

> +emit_mr (char prefix, char opcode, const char *args, char

> unportable) { 

>   expressionS arg_m, arg_r; 

>   const char *p; 

> @@ -873,10 +873,12 @@ emit_mr (char prefix, char opcode, const char

> *args) ill_op (); 

>              break; 

>            } 

> -         check_mach (INS_UNPORT); 

> +          unportable = 1; 

>        } 

>       /* Fall through.  */ 

>     case O_register: 

> +      if (unportable) 

> +          check_mach (INS_UNPORT); 

>       emit_mx (prefix, opcode, 0, & arg_m); 

>       break; 

>     default: 

> @@ -885,6 +887,18 @@ emit_mr (char prefix, char opcode, const char

> *args) return p; 

> } 

>  

> +static const char * 

> +emit_mr_z80 (char prefix, char opcode, const char *args) 

> +{ 

> +    return emit_mr(prefix, opcode, args, 0); 

> +} 

> + 

> +static const char * 

> +emit_mr_unport (char prefix, char opcode, const char *args) 

> +{ 

> +    return emit_mr(prefix, opcode, args, 1); 

> +} 

> + 

> static void 

> emit_sx (char prefix, char opcode, expressionS * arg_p) 

> { 

> @@ -1203,7 +1217,7 @@ emit_bit (char prefix, char opcode, const char

> * args) p = emit_m (prefix, opcode + (bn << 3), p); 

>       else 

>        /* Set, res : resulting byte can be copied to register.  */ 

> -       p = emit_mr (prefix, opcode + (bn << 3), p); 

> +        p = emit_mr (prefix, opcode + (bn << 3), p, 0); 

>     } 

>   else 

>     ill_op (); 

> @@ -1888,25 +1902,25 @@ static table_t instab[] = 

>   { "ret",  0xC9, 0xC0, emit_retcc }, 

>   { "reti", 0xED, 0x4D, emit_insn }, 

>   { "retn", 0xED, 0x45, emit_insn }, 

> -  { "rl",   0xCB, 0x10, emit_mr }, 

> +  { "rl",   0xCB, 0x10, emit_mr_z80 }, 

>   { "rla",  0x00, 0x17, emit_insn }, 

> -  { "rlc",  0xCB, 0x00, emit_mr }, 

> +  { "rlc",  0xCB, 0x00, emit_mr_z80 }, 

>   { "rlca", 0x00, 0x07, emit_insn }, 

>   { "rld",  0xED, 0x6F, emit_insn }, 

> -  { "rr",   0xCB, 0x18, emit_mr }, 

> +  { "rr",   0xCB, 0x18, emit_mr_z80 }, 

>   { "rra",  0x00, 0x1F, emit_insn }, 

> -  { "rrc",  0xCB, 0x08, emit_mr }, 

> +  { "rrc",  0xCB, 0x08, emit_mr_z80 }, 

>   { "rrca", 0x00, 0x0F, emit_insn }, 

>   { "rrd",  0xED, 0x67, emit_insn }, 

>   { "rst",  0x00, 0xC7, emit_rst}, 

>   { "sbc",  0x98, 0x42, emit_adc }, 

>   { "scf",  0x00, 0x37, emit_insn }, 

>   { "set",  0xCB, 0xC0, emit_bit }, 

> -  { "sla",  0xCB, 0x20, emit_mr }, 

> -  { "sli",  0xCB, 0x30, emit_mr }, 

> -  { "sll",  0xCB, 0x30, emit_mr }, 

> -  { "sra",  0xCB, 0x28, emit_mr }, 

> -  { "srl",  0xCB, 0x38, emit_mr }, 

> +  { "sla",  0xCB, 0x20, emit_mr_z80 }, 

> +  { "sli",  0xCB, 0x30, emit_mr_unport }, 

> +  { "sll",  0xCB, 0x30, emit_mr_unport }, 

> +  { "sra",  0xCB, 0x28, emit_mr_z80 }, 

> +  { "srl",  0xCB, 0x38, emit_mr_z80 }, 

>   { "sub",  0x00, 0x90, emit_s }, 

>   { "xor",  0x00, 0xA8, emit_s }, 

> } ;
Nick Clifton Aug. 22, 2018, 9 a.m. | #3
Hi Gunther,

>> 	(emit_mr_unportable): New function.

> 

> The ChangeLog entry does not agree with the actual modification.

> In the config/tc-z80.c the function name is "emit_mr_unport".

 Thanks for pointing this out.  I have corrected the changelog entry.

Cheers
  Nick

Patch

diff --git a/gas/config/tc-z80.c b/gas/config/tc-z80.c 
index 5a4fd38fce..ede7cbe1db 100644 
--- a/gas/config/tc-z80.c 
+++ b/gas/config/tc-z80.c 
@@ -850,7 +850,7 @@  emit_m (char prefix, char opcode, const char *args) 
   combinations (ix+d),r and (iy+d),r (if unportable instructions 
   are allowed).  */ 
static const char * 
-emit_mr (char prefix, char opcode, const char *args) 
+emit_mr (char prefix, char opcode, const char *args, char unportable) 
{ 
  expressionS arg_m, arg_r; 
  const char *p; 
@@ -873,10 +873,12 @@  emit_mr (char prefix, char opcode, const char *args) 
             ill_op (); 
             break; 
           } 
-         check_mach (INS_UNPORT); 
+          unportable = 1; 
       } 
      /* Fall through.  */ 
    case O_register: 
+      if (unportable) 
+          check_mach (INS_UNPORT); 
      emit_mx (prefix, opcode, 0, & arg_m); 
      break; 
    default: 
@@ -885,6 +887,18 @@  emit_mr (char prefix, char opcode, const char *args) 
  return p; 
} 
 
+static const char * 
+emit_mr_z80 (char prefix, char opcode, const char *args) 
+{ 
+    return emit_mr(prefix, opcode, args, 0); 
+} 
+ 
+static const char * 
+emit_mr_unport (char prefix, char opcode, const char *args) 
+{ 
+    return emit_mr(prefix, opcode, args, 1); 
+} 
+ 
static void 
emit_sx (char prefix, char opcode, expressionS * arg_p) 
{ 
@@ -1203,7 +1217,7 @@  emit_bit (char prefix, char opcode, const char * args) 
       p = emit_m (prefix, opcode + (bn << 3), p); 
      else 
       /* Set, res : resulting byte can be copied to register.  */ 
-       p = emit_mr (prefix, opcode + (bn << 3), p); 
+        p = emit_mr (prefix, opcode + (bn << 3), p, 0); 
    } 
  else 
    ill_op (); 
@@ -1888,25 +1902,25 @@  static table_t instab[] = 
  { "ret",  0xC9, 0xC0, emit_retcc }, 
  { "reti", 0xED, 0x4D, emit_insn }, 
  { "retn", 0xED, 0x45, emit_insn }, 
-  { "rl",   0xCB, 0x10, emit_mr }, 
+  { "rl",   0xCB, 0x10, emit_mr_z80 }, 
  { "rla",  0x00, 0x17, emit_insn }, 
-  { "rlc",  0xCB, 0x00, emit_mr }, 
+  { "rlc",  0xCB, 0x00, emit_mr_z80 }, 
  { "rlca", 0x00, 0x07, emit_insn }, 
  { "rld",  0xED, 0x6F, emit_insn }, 
-  { "rr",   0xCB, 0x18, emit_mr }, 
+  { "rr",   0xCB, 0x18, emit_mr_z80 }, 
  { "rra",  0x00, 0x1F, emit_insn }, 
-  { "rrc",  0xCB, 0x08, emit_mr }, 
+  { "rrc",  0xCB, 0x08, emit_mr_z80 }, 
  { "rrca", 0x00, 0x0F, emit_insn }, 
  { "rrd",  0xED, 0x67, emit_insn }, 
  { "rst",  0x00, 0xC7, emit_rst}, 
  { "sbc",  0x98, 0x42, emit_adc }, 
  { "scf",  0x00, 0x37, emit_insn }, 
  { "set",  0xCB, 0xC0, emit_bit }, 
-  { "sla",  0xCB, 0x20, emit_mr }, 
-  { "sli",  0xCB, 0x30, emit_mr }, 
-  { "sll",  0xCB, 0x30, emit_mr }, 
-  { "sra",  0xCB, 0x28, emit_mr }, 
-  { "srl",  0xCB, 0x38, emit_mr }, 
+  { "sla",  0xCB, 0x20, emit_mr_z80 }, 
+  { "sli",  0xCB, 0x30, emit_mr_unport }, 
+  { "sll",  0xCB, 0x30, emit_mr_unport }, 
+  { "sra",  0xCB, 0x28, emit_mr_z80 }, 
+  { "srl",  0xCB, 0x38, emit_mr_z80 }, 
  { "sub",  0x00, 0x90, emit_s }, 
  { "xor",  0x00, 0xA8, emit_s }, 
} ;