BUG: Gas (tc-i386.c). The check for loop/jecxz disp is done unsigned.

[Michel SIX]

                                           Paris, 10th october 2002

                                           Michel Six
                                           Departement de Geophysique Appliquee
                                           case 105
                                           Universite Pierre et Marie Curie
                                           4 Place Jussieu
                                           75252 Paris cedex 05
                                           France

                                           <address@hidden>

                                           to <address@hidden>


Dear colleague,

BUG: Gas (tc-i386.c). The check for loop/jecxz disp is done unsigned.
I.e, assembling a backwards loop instruction may produce a forward jump
without any warning.

I run Linux Slackware 8.1 distribution on a PC with an AMD K6 3D processor.
I use the ext2 filesystem.

Linux version 2.4.18 (address@hidden) (gcc version 2.95.3 20010315 (release))
 #4 Fri May 31 01:25:31 PDT 2002

GNU assembler 2.12.90.0.9 20020526


Description of the bug.
=======================
In order to make several assembly routines work, I had got to replace some
loop instructions by the equivalent suite of 2 instructions, "dec ecx" then a
"jnz".

So I decided to make some experiments, the conclusion of which is:

If the displacement (backwards) is greater than 256 "as" detects the error,
but says nothing if it is between 128 and 255 and writes a byte that will
send the instruction pointer in the forward direction.

Of course the same bug occurs for the jcxz instruction, that is similar.


Showing the bug (method+example).
=================================
In order to exercise the bug at leisure, I did contruct two simple routines.
They are written in gnu-fortran-77 (see listings in the mail attachments).

The main routine bugloop.for does in succession:

   - ask for a number n that gives by the simple formula 5*n+2 the relative
     jump of the loop.

   - create a file zasm.s that will exercise 2 times the loop and recompute
     the given number.

   - assemble the zasm.s into zasm.o

   - disassemble with objdump zasm.o into zasm.d

   - extract from the dump file the "loop" line and write it to the screen,
     showing then the displacement as computed by "as".

   - compile an auxiliary routine bugloop2.for and link it with zasm.o
     (that routine will call zasm and write the output to the screen)

   - run bugloop2

The formula is 5*n+2, so:
  with n <= 25 everything is ok and the output is n.
  with n >= 26 and n <= 50 no warning and bugloop2 writes nothing.
  with n >= 51 "as" refuses to assemble zasm and produces a message.
  
(Of course I did a complete backup before the 26 to 50 tries, just in case... )

Here is the shortened zasm.s for n=26 (full version in attachment)
----------------------------------------------------------
#***********************************************************************
#       subroutine zasm(ival)
#       ival is the returned value
#***********************************************************************
.text
.global zasm_
zasm_:
         push   %ebp
         mov    %esp,%ebp
         xor    %eax,%eax
         mov    $0x02,%ecx
bugloop:
         add    $0x10000,%eax  (26 times)
         ...
         loop   bugloop
         shr    $0x11,%eax
         mov    0x8(%ebp),%edx         # ival
         mov    %eax,(%edx)
         leave
         ret
----------------------------------------------------------

and here is the shortened disassembled zasm.d for n=26
----------------------------------------------------------

zasm.o:     file format elf32-i386

Disassembly of section .text:

00000000 <zasm_>:
   0:   55                      push   %ebp
   1:   89 e5                   mov    %esp,%ebp
   3:   31 c0                   xor    %eax,%eax
   5:   b9 02 00 00 00          mov    $0x2,%ecx

0000000a <bugloop>:
   a:   05 00 00 01 00          add    $0x10000,%eax    (26 times)
   ...
  8c:   e2 7c                   loop   10a <bugloop+0x100>    ! nice isnt'it
  8e:   c1 e8 11                shr    $0x11,%eax
  91:   8b 55 08                mov    0x8(%ebp),%edx
  94:   89 02                   mov    %eax,(%edx)
  96:   c9                      leave  
  97:   c3                      ret
----------------------------------------------------------
For that peculiar value of n -and only on one of my computers- at least there
was a message "Trace trap"


Diagnostic:
===========
 For n=55, the message is:
 "zasm.s:71: Error: value of fffffffffffffeeb too large for field of 1 bytes
  at 000000000000011e"

(Note: in the following all paths will be relative to a "binutils-2.12.90.0.9"
source tree)

This message comes from ./gas/write.c (fixup_segment) lines 2890-2908: i.e.
--------------------------------------------------------
            {
              valueT mask;

              mask = 0;
              mask--;           /* Set all bits to one.  */
              mask <<= size * 8 - (fixP->fx_signed ? 1 : 0);
              if ((add_number & mask) != 0 && (add_number & mask) != mask)
                {
                  char buf[50], buf2[50];
                  sprint_value (buf, fragP->fr_address + where);
                  if (add_number > 1000)
                    sprint_value (buf2, add_number);
                  else
                    sprintf (buf2, "%ld", (long) add_number);
                  as_bad_where (fixP->fx_file, fixP->fx_line,
                                _("value of %s too large for field of %d bytes 
at %s"),
                                buf2, size, buf);
                } /* Generic error checking.  */
            }
--------------------------------------------------------

A print of fixP->fx_signed showed that it was set to 0 , whence the bug for
26-50 values.
And yet a print of fixP->fx_r_type showed this to be 0x0d, i.e as it would
have been, BFD_RELOC_8_PCREL.


A solution:
===========
Searching for fx_signed in every file in the source tree, one can see that
it is set to 0 in fix_new_internal (subfunction of ./gas/write.c).

It is only set to 1, unconditionally in ./gas/config/tc-h3800.c, and conditio-
nally in ./gas/config/tc-m68k.c, which is not surprising for Motorola cpu's.
 
Looking more precisely into ./gas/config/tc-i386.c, one can see that in
the case of loop or jcxz, md_assemble() calls output_insn(), from where 
output_jump() is called.
This last routine then calls fix_new_exp(), that finally calls another write.c
subfunction -that does the real job- fix_new_internal().

fix_new_exp() returns the pointer of the fix. 

So the right place to set fx_signed is just after the call to fix_new_exp()
in output_jump(), but of course after a check that we are really dealing
with a loop/jecxz source line.

Testing again our "experiment" routines after a recompilation of the modified
gas package shows that the bug has indeed disappeared.


The diff -u file (also found in attachments):
=============================================
I hope I did put the diff in the right order. The READER writer obviously 
thinks that everybody has *his* idea of diff!. The only unambiguous way to
put what is wanted is through an example: diff file.original file.modified.
(from some examples of diffs I infer that it is the case) 
----------------------------------------------------------
--- tc-i386.cori        2002-05-24 00:10:10.000000000 +0200
+++ tc-i386.c   2002-10-08 13:50:04.000000000 +0200
@@ -2993,6 +2993,7 @@
 {
   char *p;
   int size;
+  fixS *fixP;
 
   if (i.tm.opcode_modifier & JumpByte)
     {
@@ -3043,8 +3044,11 @@
   p = frag_more (1 + size);
   *p++ = i.tm.base_opcode;
 
-  fix_new_exp (frag_now, p - frag_now->fr_literal, size,
+  fixP = fix_new_exp (frag_now, p - frag_now->fr_literal, size,
               i.op[0].disps, 1, reloc (size, 1, 1, i.reloc[0]));
+
+  if (i.tm.opcode_modifier & JumpByte)
+    fixP->fx_signed = 1;
 }
 
 static void
----------------------------------------------------------


A proposed ChangeLog item (also found in attachments):
======================================================
My english is probably not good enough!
----------------------------------------------------------
2002-mm-dd  Michel Six  <address@hidden>

        * config/tc-i386.c (output_jump): Set signed the test of disp value
        for loop/jecxz instructions.
----------------------------------------------------------


PS.
===
As I am curious, I did a recompile of gcc-2.95.3, using the modified as.
Of course I got no new warnings. It would indeed have been surprising if
the translation from RTL to intel codes would have used such special codes
as loop/jecxz.

Attached files:
===============
zasm.s   zasm.d   bugloop.for   bugloop2.for   tc-i386.c.diff  changelog.txt

Yours sincerely.

zasm.s
Description: zasm.s

zasm.d
Description: zasm.d

bugloop.for
Description: bugloop.for

bugloop2.for
Description: bugloop2.for

tc-i386.c.diff
Description: tc-i386.c.diff

changelog.txt
Description: changelog.txt