svn commit: r183818 - in stable/7/sys: . i386/include

Kostik Belousov kostikbel at gmail.com
Wed Oct 15 11:28:18 UTC 2008


On Tue, Oct 14, 2008 at 02:07:27PM +0200, Christoph Mallon wrote:
> Hi Konstantin
> 
> Konstantin Belousov wrote:
> >Author: kib
> >Date: Mon Oct 13 12:45:18 2008
> >New Revision: 183818
> >URL: http://svn.freebsd.org/changeset/base/183818
> >
> >Log:
> >  MFC r180756 (by luoqi):
> >  Unbreak cc -pg support on i386 by changing mcount() to always preserve 
> >  %ecx.
> >  
> >  Approved by:	re (kensmith)
> >
> >Modified:
> >  stable/7/sys/   (props changed)
> >  stable/7/sys/i386/include/profile.h
> >
> >Modified: stable/7/sys/i386/include/profile.h
> >==============================================================================
> >--- stable/7/sys/i386/include/profile.h	Mon Oct 13 12:28:33 2008 
> >(r183817)
> >+++ stable/7/sys/i386/include/profile.h	Mon Oct 13 12:45:18 2008 
> >(r183818)
> >@@ -115,7 +115,15 @@ void user(void);
> > void									\
> > mcount()								\
> > {									\
> >-	uintfptr_t selfpc, frompc;					\
> >+	uintfptr_t selfpc, frompc, ecx;					\
> >+	/*								\
> >+	 * In gcc 4.2, ecx might be used in the caller as the arg	\
> >+	 * pointer if the stack realignment option is set (-mstackrealign) \
> >+	 * or if the caller has the force_align_arg_pointer attribute	\
> >+	 * (stack realignment is ALWAYS on for main).  Preserve ecx	\
> >+	 * here.							\
> >+	 */								\
> >+	__asm("" : "=c" (ecx));						\
> > 	/*								\
> > 	 * Find the return address for mcount,				\
> > 	 * and the return address for mcount's caller.			\
> >@@ -132,6 +140,7 @@ mcount()						 \
> > 	__asm("movl (%%ebp),%0" : "=r" (frompc));			\
> > 	frompc = ((uintfptr_t *)frompc)[1];				\
> > 	_mcount(frompc, selfpc);					\
> >+	__asm("" : : "c" (ecx));					\
> > }
> > #else /* !__GNUCLIKE_ASM */
> > #define	MCOUNT
> 
> This fix is conceptually broken and an accident waiting to happen. There 
> is no way to prevent the compiler from shuffling instructions and 
> clobbering %ecx. Here is a simple example, which demonstrates this problem:
> 
> unsigned f(unsigned a)
> {
>         unsigned ecx;
>         asm("nop" : "=c" (ecx));
>         a = 1 << a;
>         asm("nop" : : "c" (ecx));
>         return a;
> }
> 
> GCC compiles this to:
> f:
> #APP
>         nop
>         nop
> #NO_APP
>         movl    4(%esp), %ecx
>         movl    $1, %eax
>         sall    %cl, %eax
>         ret
> 
> As you can see, %ecx gets destroyed (GCC does not emit the #APP marker 
> for empty asm statements, so I added "nop" for clarity. Even then GCC 
> merged the two #APP blocks!). In mcount() the compiler could choose to 
> place selfpc or frompc into %ecx and change the order of statements, 
> which would destroy the contents of %ecx. In fact, if -mstackrealign is 
> used, the stack realignment in mcount() destroys %ecx before any of the 
> inline assembler statements is executed for sure. The only safe way is 
> to implement mcount() using a global asm statement:
> 
> #define _MCOUNT_DECL static __attribute((cdecl,noinline)) void _mcount
> 
> #define MCOUNT                                         \
> asm(                                                   \
> 	".globl mcount\n\t"                            \
> 	".type	mcount, @function\n"                   \
> 	"mcount:\n\t"                                  \
> 	"pushl %ecx\n\t"                               \
> 	"pushl 4(%esp)\n\t" // my return address       \
> 	"pushl 4(%ebp)\n\t" // caller's return address \
> 	"call  _mcount\n\t"                            \
> 	"addl  $8, %esp\n\t"                           \
> 	"pop   %ecx\n\t"                               \
> 	"ret\n\t"                                      \
> 	".size   mcount, .-mcount");
> 
> Considering the whole issue, I think this is a bug/misfeature of GCC. It 
> could easily restore %ecx after calling mcount(), which it does for any 
> normal (i.e. non-pg-induced) function call().
I was worried too about suspiciously looking direct asm manipulations of
the registers that could interfere with optimizer, when I have seen the
patch committed to the HEAD. On the other hand, it magically works for
the present version of the gcc and used compiler flags.

We cannot have any other fix for 7.1, I suspect. Feel free to submit
more accurate patch. And, as was stated in the original commit message,
saving of the whole register file may be right thing to do.
> 
> 
> On a related note, I have submitted PR i386/127387 with patch 
> (http://www.freebsd.org/cgi/query-pr.cgi?pr=i386/127387) about a similar 
> problem in _start() in crt1.c for x86.
> 
> Regards
> 	Christoph
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-stable-7/attachments/20081015/de625d00/attachment.pgp


More information about the svn-src-stable-7 mailing list