[PATCH] Apparent thinko in closures.c causing GCC bootstrap failure on Cygwin

[Dave Korn-6]

    Hello lists,

  Some minor fallout resulting after the libffi merge.  The code in closures.c
is currently structured like this:

> #if !defined(X86_WIN32) && !defined(X86_WIN64)
> /* Use these for mmap and munmap within dlmalloc.c.  */
> static void *dlmmap(void *, size_t, int, int, int, off_t);
> static int dlmunmap(void *, size_t);
> #endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
>
> #define mmap dlmmap
> #define munmap dlmunmap
>
> #include "dlmalloc.c"
>
> #undef mmap
> #undef munmap
>
> #if !defined(X86_WIN32) && !defined(X86_WIN64)
>
>  [ definitions of dlmmap, dlmunmap ]
>
> #endif

  It currently causes bootstrap failure on cygwin like this:

libtool: link: /gnu/gcc/obj-patched3/./gcc/gcj
-B/gnu/gcc/obj-patched3/i686-pc-cygwin/libjava/ -B/gnu/gcc/obj-patched3/./gcc/
-B/opt/gcc-tools/i686-pc-cygwin/bin/ -B/opt/gcc-tools/i686-pc-cygwin/lib/
-isystem /opt/gcc-tools/i686-pc-cygwin/include -isystem
/opt/gcc-tools/i686-pc-cygwin/sys-include -ffloat-store -fomit-frame-pointer
-Usun -g -O2 -o .libs/jv-convert.exe --main=gnu.gcj.convert.Convert
-shared-libgcc  -L/gnu/gcc/obj-patched3/i686-pc-cygwin/libjava/.libs
-L/gnu/gcc/obj-patched3/i686-pc-cygwin/libjava ./.libs/libgcj.a -ldl
-L/opt/gcc-tools/lib/gcc/i686-pc-cygwin/4.5.0
/opt/gcc-tools/bin/ld: Dwarf Error: mangled line number section.
./.libs/libgcj.a(closures.o):closures.c:(.text+0x8af): undefined reference to
`_dlmunmap'
./.libs/libgcj.a(closures.o):closures.c:(.text+0xe59): undefined reference to
`_dlmmap'
./.libs/libgcj.a(closures.o):closures.c:(.text+0x1173): undefined reference to
`_dlmmap'
./.libs/libgcj.a(closures.o):closures.c:(.text+0x1c82): undefined reference to
`_dlmunmap'
./.libs/libgcj.a(closures.o):closures.c:(.text+0x1d5f): undefined reference to
`_dlmunmap'
collect2: ld returned 1 exit status
make[3]: *** [jv-convert.exe] Error 1

  Ignoring the dwarf error, which is caused by an unrelated problem, it looks
to me like the intent of the above code is to provide - only on non-windows
platforms - these two replacement functions and override dlmalloc's use of
mmap/munmap with them.  But in that case, the #defines should be inside the
#if guards as well, otherwise we're unconditionally doing the replacement but
only conditionally supplying the replacement functions!

  I'm testing the attached on Cygwin.  I'm not set up for win64 testing but I
don't suppose it offers dl* functions either; I'll see if I can find one of
the w64 guys to comment.

libffi/ChangeLog:

        * closures.c (mmap, munmap):  Don't define replacement macros pointing
        to dl* versions on windows platforms.

  Assuming it passes bootstrap, OK?

    cheers,
      DaveK

Index: libffi/src/closures.c
===================================================================
--- libffi/src/closures.c (revision 149030)
+++ libffi/src/closures.c (working copy)
@@ -189,18 +189,18 @@
 /* Use these for mmap and munmap within dlmalloc.c.  */
 static void *dlmmap(void *, size_t, int, int, int, off_t);
 static int dlmunmap(void *, size_t);
-#endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
 
 #define mmap dlmmap
 #define munmap dlmunmap
+#endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
 
 #include "dlmalloc.c"
 
+#if !defined(X86_WIN32) && !defined(X86_WIN64)
+
 #undef mmap
 #undef munmap
 
-#if !defined(X86_WIN32) && !defined(X86_WIN64)
-
 /* A mutex used to synchronize access to *exec* variables in this file.  */
 static pthread_mutex_t open_temp_exec_file_mutex = PTHREAD_MUTEX_INITIALIZER;
 

[Timothy Wall-4]

On Jun 30, 2009, at 8:17 AM, Dave Korn wrote:

>
>    Hello lists,
>
>  Some minor fallout resulting after the libffi merge.  The code in  
> closures.c
> is currently structured like this:
>
>> #if !defined(X86_WIN32) && !defined(X86_WIN64)
>> /* Use these for mmap and munmap within dlmalloc.c.  */
>> static void *dlmmap(void *, size_t, int, int, int, off_t);
>> static int dlmunmap(void *, size_t);
>> #endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
>>
>> #define mmap dlmmap
>> #define munmap dlmunmap
>>
>> #include "dlmalloc.c"
>>
>> #undef mmap
>> #undef munmap
>>
>> #if !defined(X86_WIN32) && !defined(X86_WIN64)
>>
>> [ definitions of dlmmap, dlmunmap ]
>>
>> #endif
>
>  It currently causes bootstrap failure on cygwin like this:
>
> libtool: link: /gnu/gcc/obj-patched3/./gcc/gcj
> -B/gnu/gcc/obj-patched3/i686-pc-cygwin/libjava/ -B/gnu/gcc/obj-
> patched3/./gcc/
> -B/opt/gcc-tools/i686-pc-cygwin/bin/ -B/opt/gcc-tools/i686-pc-cygwin/
> lib/
> -isystem /opt/gcc-tools/i686-pc-cygwin/include -isystem
> /opt/gcc-tools/i686-pc-cygwin/sys-include -ffloat-store -fomit-frame-
> pointer
> -Usun -g -O2 -o .libs/jv-convert.exe --main=gnu.gcj.convert.Convert
> -shared-libgcc  -L/gnu/gcc/obj-patched3/i686-pc-cygwin/libjava/.libs
> -L/gnu/gcc/obj-patched3/i686-pc-cygwin/libjava ./.libs/libgcj.a -ldl
> -L/opt/gcc-tools/lib/gcc/i686-pc-cygwin/4.5.0
> /opt/gcc-tools/bin/ld: Dwarf Error: mangled line number section.
> ./.libs/libgcj.a(closures.o):closures.c:(.text+0x8af): undefined  
> reference to
> `_dlmunmap'
> ./.libs/libgcj.a(closures.o):closures.c:(.text+0xe59): undefined  
> reference to
> `_dlmmap'
> ./.libs/libgcj.a(closures.o):closures.c:(.text+0x1173): undefined  
> reference to
> `_dlmmap'
> ./.libs/libgcj.a(closures.o):closures.c:(.text+0x1c82): undefined  
> reference to
> `_dlmunmap'
> ./.libs/libgcj.a(closures.o):closures.c:(.text+0x1d5f): undefined  
> reference to
> `_dlmunmap'
> collect2: ld returned 1 exit status
> make[3]: *** [jv-convert.exe] Error 1
>
>  Ignoring the dwarf error, which is caused by an unrelated problem,  
> it looks
> to me like the intent of the above code is to provide - only on non-
> windows
> platforms - these two replacement functions and override dlmalloc's  
> use of
> mmap/munmap with them.  But in that case, the #defines should be  
> inside the
> #if guards as well, otherwise we're unconditionally doing the  
> replacement but
> only conditionally supplying the replacement functions!

When compiling for windows, there shouldn't be any mmap/munmap  
references at all.  While cygwin may supply some version of these,  
dlmalloc defines w32mmap/w32unmap which directly calls the appropriate  
routines for windows when WIN32 is defined (which I believe is always  
the case for the mingw compiler).

Apparently the cygwin build is defining macros differently than -mno-
cygwin (which is what I use for w32/w64), resulting in extant  
references to mmap/munmap.

Have you run the tests on a system with DEP enabled (System control  
panel->Performance Options->Data Execution Prevention)?  That's likely  
the only place they'd fail if mmap isn't set up correctly.  Prior to  
the w64 patches the dlmalloc stuff never enabled the w32 mmap  
equivalents, so it was always using vanilla malloc for closures.

>
>  I'm testing the attached on Cygwin.  I'm not set up for win64  
> testing but I
> don't suppose it offers dl* functions either; I'll see if I can find  
> one of
> the w64 guys to comment.
>
> libffi/ChangeLog:
>
> * closures.c (mmap, munmap):  Don't define replacement macros  
> pointing
> to dl* versions on windows platforms.
>
>  Assuming it passes bootstrap, OK?
>
>    cheers,
>      DaveK
> Index: libffi/src/closures.c
> ===================================================================
> --- libffi/src/closures.c (revision 149030)
> +++ libffi/src/closures.c (working copy)
> @@ -189,18 +189,18 @@
> /* Use these for mmap and munmap within dlmalloc.c.  */
> static void *dlmmap(void *, size_t, int, int, int, off_t);
> static int dlmunmap(void *, size_t);
> -#endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
>
> #define mmap dlmmap
> #define munmap dlmunmap
> +#endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
>
> #include "dlmalloc.c"
>
> +#if !defined(X86_WIN32) && !defined(X86_WIN64)
> +
> #undef mmap
> #undef munmap
>
> -#if !defined(X86_WIN32) && !defined(X86_WIN64)
> -
> /* A mutex used to synchronize access to *exec* variables in this  
> file.  */
> static pthread_mutex_t open_temp_exec_file_mutex =  
> PTHREAD_MUTEX_INITIALIZER;
>

[Andrew Haley]

Timothy Wall wrote:
>
> On Jun 30, 2009, at 8:17 AM, Dave Korn wrote:
>
>>
>>  Some minor fallout resulting after the libffi merge.  The code in
>> closures.c
>> is currently structured like this:

I don't think this is anything to do with the merge, it's Timothy's
Windows changes.

> When compiling for windows, there shouldn't be any mmap/munmap
> references at all.  While cygwin may supply some version of these,
> dlmalloc defines w32mmap/w32unmap which directly calls the appropriate
> routines for windows when WIN32 is defined (which I believe is always
> the case for the mingw compiler).
>
> Apparently the cygwin build is defining macros differently than
> -mno-cygwin (which is what I use for w32/w64), resulting in extant
> references to mmap/munmap.
>
> Have you run the tests on a system with DEP enabled (System control
> panel->Performance Options->Data Execution Prevention)?  That's likely
> the only place they'd fail if mmap isn't set up correctly.  Prior to the
> w64 patches the dlmalloc stuff never enabled the w32 mmap equivalents,
> so it was always using vanilla malloc for closures.

So can we get back to the status quo ante for Cygwin?

Andrew.

[Dave Korn-6]

Andrew Haley wrote:
> Timothy Wall wrote:
>> On Jun 30, 2009, at 8:17 AM, Dave Korn wrote:
>>
>>>  Some minor fallout resulting after the libffi merge.  The code in
>>> closures.c
>>> is currently structured like this:
>
> I don't think this is anything to do with the merge, it's Timothy's
> Windows changes.

  Yes, you are correct, it's more recent than the merge.

    cheers,
      DaveK

[Dave Korn-6]

Timothy Wall wrote:

> When compiling for windows, there shouldn't be any mmap/munmap
> references at all.  While cygwin may supply some version of these,
> dlmalloc defines w32mmap/w32unmap which directly calls the appropriate
> routines for windows when WIN32 is defined (which I believe is always
> the case for the mingw compiler).
>
> Apparently the cygwin build is defining macros differently than
> -mno-cygwin (which is what I use for w32/w64), resulting in extant
> references to mmap/munmap.

  Yes, -mno-cygwin is *very* different from -mcygwin!

  When you use "-mno-cygwin" you are basically using a cross compiler
targeting MinGW.  MinGW is built upon the MSVC runtime and only supports the
functionality offered by the windows C library.  Cygwin on the other hand is
an entire POSIX-compatibility layer.  So when you are using Cygwin you are
programming for a very different target environment, and the compiler defines
appropriate macros to help.

> Have you run the tests on a system with DEP enabled (System control
> panel->Performance Options->Data Execution Prevention)?  That's likely
> the only place they'd fail if mmap isn't set up correctly.  Prior to the
> w64 patches the dlmalloc stuff never enabled the w32 mmap equivalents,
> so it was always using vanilla malloc for closures.

  I haven't tried that yet.  We have code in libgcc to make stack space
executable (so that trampolines don't get broken by DEP), but nothing
equivalent for heap space.  ISTM we might want the Cygwin DLL to render mmap'd
space executable by default, I don't know; might have to suggest that to the
Cygwin list.

  I'll take a look at the dlmalloc w32 functions and see whether it makes more
sense to enable them on cygwin, or to use cygwin's POSIX mmap/munmap, or
perhaps even to just enable the dlmmap/dlmunmap implementations.  More later;
consider the patch submission temporarily on hold.

    cheers,
      DaveK

[Timothy Wall-4]

On Jun 30, 2009, at 10:47 AM, Dave Korn wrote:

> Timothy Wall wrote:
>
>> When compiling for windows, there shouldn't be any mmap/munmap
>> references at all.  While cygwin may supply some version of these,
>> dlmalloc defines w32mmap/w32unmap which directly calls the  
>> appropriate
>> routines for windows when WIN32 is defined (which I believe is always
>> the case for the mingw compiler).
>>
>> Apparently the cygwin build is defining macros differently than
>> -mno-cygwin (which is what I use for w32/w64), resulting in extant
>> references to mmap/munmap.
>
>  Yes, -mno-cygwin is *very* different from -mcygwin!

yeah, yeah, I know that in general.  I was referring to a very narrow  
scope...

>
>  When you use "-mno-cygwin" you are basically using a cross compiler
> targeting MinGW.  MinGW is built upon the MSVC runtime and only  
> supports the
> functionality offered by the windows C library.  Cygwin on the other  
> hand is
> an entire POSIX-compatibility layer.  So when you are using Cygwin  
> you are
> programming for a very different target environment, and the  
> compiler defines
> appropriate macros to help.
>
>> Have you run the tests on a system with DEP enabled (System control
>> panel->Performance Options->Data Execution Prevention)?  That's  
>> likely
>> the only place they'd fail if mmap isn't set up correctly.  Prior  
>> to the
>> w64 patches the dlmalloc stuff never enabled the w32 mmap  
>> equivalents,
>> so it was always using vanilla malloc for closures.
>
>  I haven't tried that yet.  We have code in libgcc to make stack space
> executable (so that trampolines don't get broken by DEP), but nothing
> equivalent for heap space.  ISTM we might want the Cygwin DLL to  
> render mmap'd
> space executable by default, I don't know; might have to suggest  
> that to the
> Cygwin list.

mmap has explicit flags to ask for read/write/execute, which  
presumably cygwin's DLL appropriately maps to the VirtualMalloc  
equivalents somewhere down the line.

>
>  I'll take a look at the dlmalloc w32 functions and see whether it  
> makes more
> sense to enable them on cygwin, or to use cygwin's POSIX mmap/
> munmap, or
> perhaps even to just enable the dlmmap/dlmunmap implementations.  
> More later;
> consider the patch submission temporarily on hold.

While following the *nix variants and wrapping the mmap/munmap calls  
with dlmmap/dlunmap (to add the exec flag) seems like the consistent  
thing to do for cygwin, in this case cygwin is only going to insert  
some translation code between the ultimate calls to VirtualAlloc/
VirtualFree.

[Dave Korn-6]

Timothy Wall wrote:

> While following the *nix variants and wrapping the mmap/munmap calls
> with dlmmap/dlunmap (to add the exec flag) seems like the consistent
> thing to do for cygwin, in this case cygwin is only going to insert some
> translation code between the ultimate calls to VirtualAlloc/VirtualFree.

  Actually, it may well need to do some behind-the-scenes bookkeeping in order
to support fork semantics (which don't come naturally to windows!).  I'll
generate a patch that enables the dlmmap/dlmunmap functions on Cygwin;
sticking to the standard POSIX APIs is the best thing to do.  Patch withdrawn;
respin to follow.  Thanks for the advice.

    cheers,
      DaveK