[PATCH] fix bad interaction between -pg and stack realignment on x86

[PATCH] fix bad interaction between -pg and stack realignment on x86

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]

Hello,

On x86 targets, the "force_align_arg_pointer" implementation bits
potentially badly interact with code generated for profiling purposes.

The former's idea is to use one "free" register as the argument pointer
value, set on function entry before the stack pointer is aligned.

Typically, for

   int main (int argc, char * argv [])
   {
     int x = argc;

     return 0;
   }

we get code like (gcc -S -fverbose-asm)

   .globl main
	   .type   main, @function
   main:
	   leal    4(%esp), %ecx   #,               save arg pointer
	   andl    $-16, %esp      #,               align sp

	   pushl   -4(%ecx)        #
	   pushl   %ebp    #                        ...
	   movl    %esp, %ebp      #,               misc details
	   pushl   %ecx    #                        ...
	   subl    $16, %esp       #,

	   movl    (%ecx), %eax    # argc, argc     use arg pointer
	   movl    %eax, -8(%ebp)  # argc, x
	   ...


With -pg, a call to mcount is inserted between the "save" and the
"use", just before the use in the specific case above.

There is nothing to guarantee that mcount won't clobber ecx in the
absolute (no ABI mandate), and there is a clear problem when this happens.

It turns out not to happen on linux and windows, hence not to be
extremely visible, because mcount explicitly preserves the normally
call clobbered regs.

We have seen the problem on at least freebsd and solaris, still, and I
think this is what http://gcc.gnu.org/ml/gcc/2006-12/msg00319.html is
about with DJGPP.

The code below exposes the problem on i386-solaris, where it aborts
when compiled with -pg:

   #define EXPECTED_ARG 0x12345678

   void __attribute__ ((force_align_arg_pointer)) foo (int arg)
   {
     if (arg != EXPECTED_ARG)
       abort ();
   }

   int main (int argc, char *argv[])
   {
     foo (EXPECTED_ARG);
     return 0;
   }

The attached patch is a suggestion to address this by having
x86_function_profiler save/restore the "force_align_arg_pointer"
register as needed, either because the mcount we call might clobber
it, or if it conflicts with PROFILE_COUNT_REGNUM and we are setting
the latter as an mcount argument.

It splits the function into 32 and 64 bits variants for readability
purposes, and turns PROFILE_COUNT_REGISTER into PROFILE_COUNT_REGNUM
to make comparisons against force_align_arg_pointer easier.

Bootstrapped and regression tested for c,c++,objc,ada on
i386-pc-solaris2.10 + for all,ada on x86_64-redhat-linux.

Thanks in advance,

Olivier

	2007-02-26  Olivier Hainque  <hainque@adacore.com>

	gcc/

	* config/i386/i386.h (PROFILE_COUNT_REGISTER): Turn into ...
	(PROFILE_COUNT_REGNUM): New macro.
	(MCOUNT_PRESERVES_ALL_REGS): New macro, tells whether mcount
	preserves all regs.  Default to 1 here, to be redefined by subtarget
	configurations as needed.  
	* config/i386/i386.c (X86_SET_PROFILE_COUNTERS): New local macro,
	always defined.  Conveys whether NO_PROFILE_COUNTERS is defined.
	(x86_function_profiler): Split into ...
	(x86_64_function_profiler): New function, and
	(x86_32_function_profiler): New function.  Save/restore the
	force_align_arg_pointer register when needed.
	* config/i386/sol2.h (MCOUNT_PRESERVES_ALL_REGS): Redefine to 0.

	testsuite/

	* gcc.target/i386/force_align_mcount.c: New case.















[-- Attachment #2: force_align_mcount.dif --]
[-- Type: text/plain, Size: 7028 bytes --]

*** ./gcc/config/i386/i386.h.ori	Fri Feb 23 11:17:35 2007
--- ./gcc/config/i386/i386.h	Mon Feb 26 15:33:42 2007
*************** typedef struct ix86_args {
*** 1559,1566 ****
  #define FUNCTION_PROFILER(FILE, LABELNO) x86_function_profiler (FILE, LABELNO)
  
  #define MCOUNT_NAME "_mcount"
  
! #define PROFILE_COUNT_REGISTER "edx"
  
  /* EXIT_IGNORE_STACK should be nonzero if, when returning from a function,
     the stack pointer does not matter.  The value is tested only in
--- 1559,1571 ----
  #define FUNCTION_PROFILER(FILE, LABELNO) x86_function_profiler (FILE, LABELNO)
  
  #define MCOUNT_NAME "_mcount"
+ #define PROFILE_COUNT_REGNUM 1  /* edx */
  
! /* Whether mcount preserves all regs - to be redefined by subtarget
!    configurations as needed.  This is used by the function_profiler
!    implementations to decide if they should explicitly save/restore
!    call-clobbered registers possibly live at the point of the call.  */
! #define MCOUNT_PRESERVES_ALL_REGS 1
  
  /* EXIT_IGNORE_STACK should be nonzero if, when returning from a function,
     the stack pointer does not matter.  The value is tested only in
*** ./gcc/config/i386/i386.c.ori	Fri Feb 23 11:17:35 2007
--- ./gcc/config/i386/i386.c	Fri Feb 23 11:18:01 2007
*************** x86_field_alignment (tree field, int com
*** 19567,19608 ****
    return computed;
  }
  
! /* Output assembler code to FILE to increment profiler label # LABELNO
!    for profiling a function entry.  */
! void
! x86_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
  {
!   if (TARGET_64BIT)
!     if (flag_pic)
!       {
! #ifndef NO_PROFILE_COUNTERS
  	fprintf (file, "\tleaq\t%sP%d@(%%rip),%%r11\n", LPREFIX, labelno);
! #endif
! 	fprintf (file, "\tcall\t*%s@GOTPCREL(%%rip)\n", MCOUNT_NAME);
!       }
!     else
!       {
! #ifndef NO_PROFILE_COUNTERS
  	fprintf (file, "\tmovq\t$%sP%d,%%r11\n", LPREFIX, labelno);
! #endif
! 	fprintf (file, "\tcall\t%s\n", MCOUNT_NAME);
!       }
!   else if (flag_pic)
      {
! #ifndef NO_PROFILE_COUNTERS
!       fprintf (file, "\tleal\t%sP%d@GOTOFF(%%ebx),%%%s\n",
! 	       LPREFIX, labelno, PROFILE_COUNT_REGISTER);
! #endif
        fprintf (file, "\tcall\t*%s@GOT(%%ebx)\n", MCOUNT_NAME);
      }
    else
      {
! #ifndef NO_PROFILE_COUNTERS
!       fprintf (file, "\tmovl\t$%sP%d,%%%s\n", LPREFIX, labelno,
! 	       PROFILE_COUNT_REGISTER);
! #endif
        fprintf (file, "\tcall\t%s\n", MCOUNT_NAME);
      }
  }
  
  /* We don't have exact information about the insn sizes, but we may assume
--- 19567,19665 ----
    return computed;
  }
  
! /* Variants of x86_function_profiler for the 32/64bit ABIs.  Output assembler
!    code to FILE to increment profiler label # LABELNO for profiling a function
!    entry.  */
! 
! /* We'll need to check several times if NO_PROFILE_COUNTERS is #defined and
!    using preprocessor directives everywhere really impairs readability.  */
! #ifdef NO_PROFILE_COUNTERS
! #define X86_SET_PROFILE_COUNTERS 0
! #else
! #define X86_SET_PROFILE_COUNTERS 1
! #endif
! 
! /* 64bit ABI implementation.  */
! static void
! x86_64_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
  {
!   if (flag_pic)
!     {
!       if (X86_SET_PROFILE_COUNTERS)
  	fprintf (file, "\tleaq\t%sP%d@(%%rip),%%r11\n", LPREFIX, labelno);
!       fprintf (file, "\tcall\t*%s@GOTPCREL(%%rip)\n", MCOUNT_NAME);
!     }
!   else
!     {
!       if (X86_SET_PROFILE_COUNTERS)
  	fprintf (file, "\tmovq\t$%sP%d,%%r11\n", LPREFIX, labelno);
!       fprintf (file, "\tcall\t%s\n", MCOUNT_NAME);
!     }
! }
! 
! /* 32bit ABI implementation.  */
! static void
! x86_32_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
! {
!   /* We must be careful not to clobber the internal arg pointer used for stack
!      realignment purposes, if any.  This could happen in two different ways
!      from the code we emit below:  calling a careless 'mcount', which doesn't
!      save/restore the ABI caller-saved registers, or moving a value into the
!      PROFILE_COUNT register.  */
! 
!   /* Register to be preserved, set to the proper number if there is a forced
!      internal arg pointer for the current function and we need to push/pop it
!      around the profiling code per se.  */
!   unsigned int reg_to_preserve = INVALID_REGNUM;
! 
!   if (cfun->machine->force_align_arg_pointer)
!     reg_to_preserve = REGNO (cfun->machine->force_align_arg_pointer);
!   
!   /* No action is actually needed if mcount is safe and we're not going
!      to clobber ourselves.  */
!   if (reg_to_preserve != INVALID_REGNUM
!       && MCOUNT_PRESERVES_ALL_REGS
!       && (! X86_SET_PROFILE_COUNTERS
! 	  || reg_to_preserve != PROFILE_COUNT_REGNUM))
!     reg_to_preserve = INVALID_REGNUM;
! 
!   /* Push reg we need to preserve, if any.  */
!   if (reg_to_preserve != INVALID_REGNUM)
!     ASM_OUTPUT_REG_PUSH (file, reg_to_preserve);
! 
!   /* Emit the profiling code per se.  */
!   if (flag_pic)
      {
!       if (X86_SET_PROFILE_COUNTERS)
! 	fprintf (file, "\tleal\t%sP%d@GOTOFF(%%ebx),%%e%s\n",
! 		 LPREFIX, labelno, reg_names [PROFILE_COUNT_REGNUM]);
! 
        fprintf (file, "\tcall\t*%s@GOT(%%ebx)\n", MCOUNT_NAME);
      }
    else
      {
!       if (X86_SET_PROFILE_COUNTERS)
! 	fprintf (file, "\tmovl\t$%sP%d,%%e%s\n",
! 		 LPREFIX, labelno, reg_names [PROFILE_COUNT_REGNUM]);
! 
        fprintf (file, "\tcall\t%s\n", MCOUNT_NAME);
      }
+ 
+   /* Pop reg we need to preserve, if any.  */
+   if (reg_to_preserve != INVALID_REGNUM)
+     ASM_OUTPUT_REG_POP (file, reg_to_preserve);
+ }
+ 
+ /* Output assembler code to FILE to increment profiler label # LABELNO for
+    profiling a function entry.  FUNCTION_PROFILER gate to either the 64bit or
+    the 32bit helper above.  */
+ void
+ x86_function_profiler (FILE *file, int labelno)
+ {
+   if (TARGET_64BIT)
+     x86_64_function_profiler (file, labelno);
+   else
+     x86_32_function_profiler (file, labelno);
  }
  
  /* We don't have exact information about the insn sizes, but we may assume
*** ./gcc/config/i386/sol2.h.ori	Fri Feb 23 11:17:35 2007
--- ./gcc/config/i386/sol2.h	Fri Feb 23 11:18:01 2007
*************** Boston, MA 02110-1301, USA.  */
*** 111,113 ****
--- 111,118 ----
  /* We do not need NT_VERSION notes.  */
  #undef X86_FILE_START_VERSION_DIRECTIVE
  #define X86_FILE_START_VERSION_DIRECTIVE false
+ 
+ /* mcount may clobber caller-saved registers, so ...  */
+ #undef  MCOUNT_PRESERVES_ALL_REGS
+ #define MCOUNT_PRESERVES_ALL_REGS 0
+ 
*** ./gcc/testsuite/gcc.target/i386/force_align_mcount.c.ori	Mon Feb 26 14:39:58 2007
--- ./gcc/testsuite/gcc.target/i386/force_align_mcount.c	Mon Feb 26 14:43:25 2007
***************
*** 0 ****
--- 1,19 ----
+ /* { dg-do run { target i?86-*-* } } */
+ /* { dg-options "-pg" } */
+ 
+ extern void abort (void);
+ 
+ #define EXPECTED_ARG 0x12345678
+ 
+ void __attribute__ ((force_align_arg_pointer)) foo (int arg)
+ {
+   if (arg != EXPECTED_ARG)
+     abort ();
+ }
+ 
+ int main (int argc, char *argv[])
+ {
+   foo (EXPECTED_ARG);
+   return 0;
+ }
+ 

Re: [PATCH] fix bad interaction between -pg and stack realignment on x86

[Richard Henderson]

On Wed, Feb 28, 2007 at 11:53:08AM +0100, Olivier Hainque wrote:
> 	gcc/
> 
> 	* config/i386/i386.h (PROFILE_COUNT_REGISTER): Turn into ...
> 	(PROFILE_COUNT_REGNUM): New macro.
> 	(MCOUNT_PRESERVES_ALL_REGS): New macro, tells whether mcount
> 	preserves all regs.  Default to 1 here, to be redefined by subtarget
> 	configurations as needed.  
> 	* config/i386/i386.c (X86_SET_PROFILE_COUNTERS): New local macro,
> 	always defined.  Conveys whether NO_PROFILE_COUNTERS is defined.
> 	(x86_function_profiler): Split into ...
> 	(x86_64_function_profiler): New function, and
> 	(x86_32_function_profiler): New function.  Save/restore the
> 	force_align_arg_pointer register when needed.
> 	* config/i386/sol2.h (MCOUNT_PRESERVES_ALL_REGS): Redefine to 0.
> 
> 	testsuite/
> 
> 	* gcc.target/i386/force_align_mcount.c: New case.

Ok.

You might also save/restore the argument regisers, if any.  E.g.

/* { dg-do run { target i?86-*-* } } */
/* { dg-options "-pg" } */

extern void abort (void);

void __attribute__ ((regparm(3))) foo (int a, int b, int c)
{
  if (a != 1 || b != 2 || c != 3)
    abort ();
}

int main (int argc, char *argv[])
{
  foo (1, 2, 3);
  return 0;
}

Re: [PATCH] fix bad interaction between -pg and stack realignment on x86

[Olivier Hainque]

Richard Henderson wrote:
> Ok.

> You might also save/restore the argument regisers, if any.  E.g. [...]

 Oh, indeed. I have pieces for another enhancement (expanding the set of
 possible force_align_arg_pointer candidates) which should help for this
 purpose. I'll put these pieces together with this specific fix and
 followup, before submitting the other enhancement over.

 Thanks for your prompt feedback.

 Olivier