RE: [PATCH] fix profiling

[Dave Korn]

Brian Dessent wrote on 05 August 2008 06:28:

 Brian Dessent wrote:
 
 Since this code is lifted from the BSDs I did check that this change was
 made there as well, e.g.

http://www.openbsd.org/cgi-bin/cvsweb/src/sys/arch/i386/include/profile.h?r
ev=1.10content-type=text/x-cvsweb-markup.

  Adding 'volatile' to asms that didn't need it could never do any harm,
anyway, apart from the minor missed-optimisation opportunities it implies,
but correctness is more important here!
 
 Actually, I also missed that the above version uses +r instead of =r for
 the second constraint.  I guess we should make that change too.

  I think that change is wrong.  The register is written, not read; the
input value is not used.  (The fact it is used as a source operand in the
second instruction doesn't matter, because it's not the input value being
used there but the previously-written value that just clobbered the input
value).  It looks like a misunderstanding of what read-write means in terms
of gcc constraints.


cheers,
  DaveK
-- 
Can't think of a witty .sigline today

Re: [PATCH] fix profiling

[Brian Dessent]

Christopher Faylor wrote:

 Please use your best judgement about the +r/=r thing given Dave's
 comments.

I think Dave's right, because when I compile with +r I get a may be
used uninitialized warning, so I'll just leave it as it is.

Both patches are now committed.  I wonder how long this was broken...

Brian

[PATCH] fix profiling

[Brian Dessent]

Long story short: some asm()s have missing volatile modifiers.

The mcount() profiling hook is implemented with a short wrapper around
the actual mcount function.  The wrapper's purpose is to get the pc of
the caller as well as the return value of the caller's frame, and pass
those on as arguments to the actual mcount function.  Because it's a
local static function the compiler inlines all this into one function. 
The problem is these asm()s aren't marked volatile and so the compiler
freely rearranges them and interleaves them with the prologue of the
inlined function.  Thus mcount gets some bogus value for the pc and
ignores the data because it's not in the valid range of .text.

Since this code is lifted from the BSDs I did check that this change was
made there as well, e.g.
http://www.openbsd.org/cgi-bin/cvsweb/src/sys/arch/i386/include/profile.h?rev=1.10content-type=text/x-cvsweb-markup.

Unfortuantely there seems to also be some bitrot in the gprof side, as
the codepath to read BSD style gmon.out files is also broken.  I've
posted a separate patch to the binutils list.  With both these fixes,
gprof again works with Cygwin.

Brian2008-08-04  Brian Dessent  [EMAIL PROTECTED]

* config/i386/profile.h (mcount): Mark asms volatile.

Index: config/i386/profile.h
===
RCS file: /cvs/src/src/winsup/cygwin/config/i386/profile.h,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 profile.h
--- config/i386/profile.h   17 Feb 2000 19:38:31 -  1.1.1.1
+++ config/i386/profile.h   5 Aug 2008 05:02:25 -
@@ -48,11 +48,11 @@ mcount()
\
 *  \
 * selfpc = pc pushed by mcount call\
 */ \
-   __asm(movl 4(%%ebp),%0 : =r (selfpc));  \
+   __asm __volatile (movl 4(%%ebp),%0 : =r (selfpc));  \
/*  \
 * frompcindex = pc pushed by call into self.   \
 */ \
-   __asm(movl (%%ebp),%0;movl 4(%0),%0 : =r (frompcindex));\
+   __asm __volatile (movl (%%ebp),%0;movl 4(%0),%0 : =r 
(frompcindex));\
_mcount(frompcindex, selfpc);   \
 }