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 at the end of this mail)

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 at the end of this mail)
----------------------------------------------------------
#***********************************************************************
#       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:
=============================================
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:
======================================================
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.


Complete listing of zasm.s
==========================
#***********************************************************************
#       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
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         add    $0x10000,%eax
         loop   bugloop
         shr    $0x11,%eax

         mov    0x8(%ebp),%edx         # ival
         mov    %eax,(%edx)

         leave
         ret

Complete listing of bugloop.for.
================================
c***********************************************************************
c      program bugloop.for
c***********************************************************************
      integer*1 ich(80)
      nchmax=80
c
c----------------------------- creation of the assembly file (zasm.s)
c
      write(*,1000)
 1000 format($,'  The jump will be 5*n+2 bytes; input n: ')
      read *,n 
c
      open(unit=2,file='zasm.s',status='unknown',access='sequential',
     1 form='formatted')

      write(2,1100) '#************************************************',
     1 '***********************'
 1100 format(a,a)
      write(2,1100) '#       subroutine zasm(ival)'
      write(2,1100) '#       ival is the returned value'
      write(2,1100) '#************************************************',
     1 '***********************'
      write(2,1100) '.text'
      write(2,1100) '.global zasm_'
      write(2,1100) 'zasm_:'
      write(2,1100)
      write(2,1100) '         push   %ebp'
      write(2,1100) '         mov    %esp,%ebp'
      write(2,1100)
      write(2,1100) '         xor    %eax,%eax'
      write(2,1100) '         mov    $0x02,%ecx'
      write(2,1100)
      write(2,1100) 'bugloop:'
c
      do 10 i=1,n
   10 write(2,1100) '         add    $0x10000,%eax'
c
      write(2,1100) '         loop   bugloop'
      write(2,1100) '         shr    $0x11,%eax'
      write(2,1100)
      write(2,1100) '         mov    0x8(%ebp),%edx         # ival'
      write(2,1100) '         mov    %eax,(%edx)'
      write(2,1100)
      write(2,1100) '         leave'
      write(2,1100) '         ret'
c
      close(unit=2)
c
c----------------------------- as of the file (zasm.o)
c
      call system('as zasm.s -o zasm.o\0')
c
c----------------------------- objdump of the file (zasm.d)
c
      call system('objdump -d zasm.o > zasm.d\0')
c
c----------------------------- extraction of the loop line of the dump file
c
      open(unit=3,file='zasm.d',status='old',access='direct',
     1 form='unformatted',recl=1)
      line=0
      nbread=0
c
   20 call readline(3,line,ich,nch,nchmax,nbread,ierr)
      if(ierr.eq.1) go to 22
      if(nch.lt.32) go to 20
      if(ich(29).ne.ichar('l')) go to 20
      if(ich(30).ne.ichar('o')) go to 20
      if(ich(31).ne.ichar('o')) go to 20
      if(ich(32).eq.ichar('p')) go to 25
c
   22 stop '  loop line not found in the disassembled file'
c
   25 close(unit=3)
      print *,'   disassembled loop line'
      print *,' '
      write(*,1010) (ich(i),i=1,nch)
 1010 format(80a1)
      print *,' '
c
c----------------------------- compilation
c
      call system('g77 -o bugloop2 bugloop2.for zasm.o >& /dev/null\0')
c
c----------------------------- execution
c
      call system('bugloop2\0')
c
      end
c
c***********************************************************************
      subroutine readline(iunit,line,ich,nch,nchmax,nbread,ierr)
c      read a line of the logical unit "iunit" file into the array "ich"
c      "line" is the number of the line and "nch" its number of characters
c      "nbread" is the total number of read bytes of the file
c      "ierr" is returned 1 in case of unexpected end of file
c
c      carriage return are dropped
c
c      the file is to be opened 'direct', 'unformatted', recl=1
c      "line" and "nbread" must be set to 0
c      "nchmax" is the size of the array ich
c
      integer*1 icar,ich(1)
c
      ierr=0
      nch=0
      line=line+1
c
   10 nbread=nbread+1
      read(iunit,rec=nbread,err=20) icar
c
      if(icar.eq.13) go to 10
      if(icar.eq.10) return
c
      nch=nch+1
      if(nch.lt.nchmax) go to 12
      write(*,1200) nchmax,line,iunit
 1200 format('  too many chars (max=',i3,') at line',i6,' iunit=',i3)
      stop
c
   12 ich(nch)=icar
      go to 10
c
   20 ierr=1
      return
      end


Complete listing of bugloop2.for.
================================
c***********************************************************************
c      program bugloop2.for
c***********************************************************************
c
      print *,'  executing'
      print *,' '
c
      call zasm(ival)
c
      write(*,1000) ival
 1000 format('  The returned value (should be n) is: ',i4)
c
      end


Yours sincerely.