Patch for pformat.c in winsup/mingw CVS

Hi,

The latest CVS winsup/mingw runtime caused a crash, for me, in strlen() when
testing previously 'good' code built with it in the toolchain.
I found this patch fixed the problem for me, so it may be of help to
others...

--- ./mingw/mingwex/stdio/save_pformat.c        2008-07-29
00:24:20.000000000 +0100
+++ ./mingw/mingwex/stdio/pformat.c     2008-07-31 19:30:29.000000000 +0100
 <at>  <at>  -358,7 +358,7  <at>  <at> 
    * This is implemented as a trivial call to `__pformat_putchars()',
    * passing the length of the input string as the character count.
    */
-  __pformat_putchars( s, strlen( s ), stream );
+  __pformat_putchars( s, s ? strlen( s ) : 0, stream );
 }

 static
 <at>  <at>  -436,7 +436,7  <at>  <at> 
    * This is implemented as a trivial call to `__pformat_wputchars()',
    * passing the length of the input string as the character count.
    */
-  __pformat_wputchars( s, wcslen( s ), stream );
+  __pformat_wputchars( s, s ? wcslen( s ) : 0, stream );
 }

 static __inline__

Thanks

Re: Patch for pformat.c in winsup/mingw CVS

On Fri, Aug 01, 2008 at 02:53:53PM +0100, Colin Harrison wrote:
>The latest CVS winsup/mingw runtime caused a crash, for me, in strlen()
>when testing previously 'good' code built with it in the toolchain.  I
>found this patch fixed the problem for me, so it may be of help to
>others...

You should send mingw changes to the MinGW project following the
guidelines set forth here:

http://www.mingw.org/MinGWiki/index.php/SubmitPatches

cgf

[PATCH] fix profiling

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.10&content-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.

Brian

2008-08-04  Brian Dessent  <brian <at> dessent.net>

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

Index: config/i386/profile.h
===================================================================

Re: [PATCH] fix profiling

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?rev=1.10&content-type=text/x-cvsweb-markup>.

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.

Brian

RE: [PATCH] fix profiling

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.10&content-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

On Mon, Aug 04, 2008 at 10:11:04PM -0700, Brian Dessent wrote:
>2008-08-04  Brian Dessent
>
>	* config/i386/profile.h (mcount): Mark asms volatile.

Go ahead and check this in and I'll roll a new release.

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

cgf

Re: [PATCH] fix profiling

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