Hi,

this is a regression present on all active branches: if you compile the 
attached Ada testcase with -O2 -gnatp -fno-omit-frame-pointer for 32-bit 
Windows, you'll see that the compiler swaps a load based on the stack pointer 
with a store based on the frame pointer, thus clobbering a saved argument:

        pushl   %ebp
        movl    %esp, %ebp
        pushl   %esi
        pushl   %ebx
        pushl   %eax              <- %eax save
        movl    $4108, %eax
        call    ___chkstk_ms
        leal    8(%ebp), %esi
        subl    %eax, %esp
        movl    8(%ebp), %ebx
        movl    %edx, -20(%ebp)
        movl    %esi, -12(%ebp)    <- fp-based store
        movl    (%esp,%eax), %eax  <- sp-based load
        ... wrong value in eax...

The load and the store are swapped because there are not based on the same 
register and the offset between them is seen as variable.  The proposed fix is 
to add a memory blockage, like in other frame-related constructs.

Tested on x86/Windows and x86-64/Windows, OK for all active branches?


2019-02-06  Eric Botcazou  <ebotca...@adacore.com>

        * config/i386/i386.c (ix86_expand_prologue): Generate a memory blockage
        after restoring registers saved to allocate the frame on Windows.


2019-02-06  Eric Botcazou  <ebotca...@adacore.com>

        * gnat.dg/opt76.adb: New test.

-- 
Eric Botcazou
Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 268508)
+++ config/i386/i386.c	(working copy)
@@ -13579,8 +13579,9 @@ ix86_expand_prologue (void)
 	}
       m->fs.sp_offset += allocate;
 
-      /* Use stack_pointer_rtx for relative addressing so that code
-	 works for realigned stack, too.  */
+      /* Use stack_pointer_rtx for relative addressing so that code works for
+	 realigned stack.  But this means that we need a blockage to prevent
+	 stores based on the frame pointer from being scheduled before.  */
       if (r10_live && eax_live)
         {
 	  t = gen_rtx_PLUS (Pmode, stack_pointer_rtx, eax);
@@ -13589,6 +13590,7 @@ ix86_expand_prologue (void)
 	  t = plus_constant (Pmode, t, UNITS_PER_WORD);
 	  emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
 			  gen_frame_mem (word_mode, t));
+	  emit_insn (gen_memory_blockage ());
 	}
       else if (eax_live || r10_live)
 	{
@@ -13596,6 +13598,7 @@ ix86_expand_prologue (void)
 	  emit_move_insn (gen_rtx_REG (word_mode,
 				       (eax_live ? AX_REG : R10_REG)),
 			  gen_frame_mem (word_mode, t));
+	  emit_insn (gen_memory_blockage ());
 	}
     }
   gcc_assert (m->fs.sp_offset == frame.stack_pointer_offset);
-- { dg-do run }
-- { dg-options "-O2 -gnatp -fno-omit-frame-pointer" }

procedure Opt76 is

   type Integer_Access is access Integer;
   type Registry_Array is array (Natural range <>) of Integer_Access;

   procedure Nested (Input, Parser : Integer; A, B : Boolean) is

      Index : Registry_Array (1 .. 1024);
      Not_B : constant Boolean := not B;

      procedure Inner (Input : Integer) is
      begin
         if Input /= 1 then
            raise Program_Error;
         end if;

         if Parser = 128 and then A and then Not_B then
            Inner (Input);
            Index (Index'First) := null;
         end if;
      end;

   begin
      Inner (Input);
   end;

   Input : Integer := 1 with Volatile;
   Parser : Integer := 2 with Volatile;
      
begin
   Nested (Input, Parser, False, True);
   Nested (Input, Parser, True, False);
end;

Reply via email to