[bug #27809] several win64 fixes

[Mario Castelán Castro]

URL:
  <http://savannah.gnu.org/bugs/?27809>

                 Summary: several win64 fixes
                 Project: make
            Submitted by: sezero
            Submitted on: Mon 26 Oct 2009 09:07:56 AM GMT
                Severity: 3 - Normal
              Item Group: Bug
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
       Component Version: CVS
        Operating System: MS Windows
           Fixed Release: None
           Triage Status: None

    _______________________________________________________

Details:

Hello:

The make windows code, as it is currently in the
cvs, is fairly win32-centric and doesn't know about
win64 and that it's following the llp64 convention.
Most of the issues are related to pid_t, where job.h
actually uses pid_t while some other places use int
for it.  And there are a few other similar places
where casting pointers to long or int is bad and
intptr_t is the correct answer (mingw32 (mingw.org)
and mingw-w64 (mingw-w64.sourceforge.net) both do
provide intptr_t.)

Attached a small patch which addresses some of these
issues.  There are many other warnings from the build
process (see the attached log, recorded after applying
this patch), but most (if not all) of them are from
debug message prints and not as catastrophic.

Please review the changes and apply if they are OK.

Regards.




    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Mon 26 Oct 2009 09:07:56 AM GMT  Name: build.log  Size: 25kB   By:
sezero
win64 (mingw-w64) patch, and the build log
<http://savannah.gnu.org/bugs/download.php?file_id=18942>
-------------------------------------------------------
Date: Mon 26 Oct 2009 09:07:56 AM GMT  Name: make-w64.diff  Size: 5kB   By:
sezero
win64 (mingw-w64) patch, and the build log
<http://savannah.gnu.org/bugs/download.php?file_id=18941>

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27809>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Mario Castelán Castro]

Follow-up Comment #1, bug #27809 (project make):

Thank you for your contribution.  I have a few questions for the patches you
sent:

1. What versions of GCC and of MinGW runtime did you use to build Make?

2. Which header defines pid_t as appropriate for both 32-bit and 64-bit
Windows?  What is the definition for each one of them?

3. Why did you need casts in assignments, like this:

-    *pid_p = (int) hProcess;
+    *pid_p = (pid_t) hProcess;

Didn't it work without a cast?  (There are quite a few such casts; please see
which ones are absolutely necessary.)

4. This change:

-  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
+  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);

assumes that _open_osfhandle accepts an intptr_t type as its first argument.
But the prototype I have on my machine (in io.h) says the first argument is a
`long'.  Which version of MinGW changed that?

5. Finally, could you please see if the build_w32.bat script works for a
64-bit MinGW GCC?  If you see problems there, please report them.

Thanks.




    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27809>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Mario Castelán Castro]

Follow-up Comment #2, bug #27809 (project make):

> 1. What versions of GCC and of MinGW runtime did you use to build Make?

Gcc version: gcc-4.4.3 prerelease.

MinGW runtime, however, seems as the confusion here:  mingw.org
does not support win64.  however the mingw-w64 project, (sf.net
project page: http://sourceforge.net/projects/mingw-w64/ ,  the
svn repo:  http://mingw-w64.svn.sourceforge.net/ ) does support
both win32 and win64.  As for the version of it, I use the release
v1.0 branch for the mingw-w64 svn.

> 2. Which header defines pid_t as appropriate for both 32-bit and 64-bit
> Windows?  What is the definition for each one of them?
>

Either of process.h or sys/types.h:
Here is the mingw-w64 version from sys/types.h:
#ifndef _PID_T_
#define _PID_T_
#ifndef _WIN64
typedef int     _pid_t;
#else
__MINGW_EXTENSION typedef __int64       _pid_t;
#endif

> 3. Why did you need casts in assignments, like this:
>
> -    *pid_p = (int) hProcess;
> +    *pid_p = (pid_t) hProcess;
>

Because you are casting a handle, which is a ptr*,  to an int.
On the other hand, pid_t is wordsize dependant, int on w32 and
int64 on w64, and casting to pid_t is the correct way of doing
it.

> Didn't it work without a cast?  (There are quite a few such casts; please
see

As I said above:  the current code in the cvs is like casting a
pointter to an int on amd64 which results in pointer truncation.

> which ones are absolutely necessary.)

I tried my best to pick, but I can look again if necessary.

>
> 4. This change:
>
> -  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
> +  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);
>
> assumes that _open_osfhandle accepts an intptr_t type as its first
argument.
> But the prototype I have on my machine (in io.h) says the first argument is
a
> `long'.  Which version of MinGW changed that?

It is the same case.  Your version is for w32-only.
The mingw-w64 version is like (from io.h):
_CRTIMP int __cdecl _open_osfhandle(intptr_t _OSFileHandle,int _Flags);

> 5. Finally, could you please see if the build_w32.bat script works for a
> 64-bit MinGW GCC?  If you see problems there, please report them.

I did all my work on linux, cross-compiling for w64.  When I reboot into
windows, I can try that one.

>
> Thanks.

My pleasure.


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27809>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Mario Castelán Castro]

Follow-up Comment #3, bug #27809 (project make):

>> 5. Finally, could you please see if the build_w32.bat script works for a
>> 64-bit MinGW GCC?  If you see problems there, please report them.
>
> I did all my work on linux, cross-compiling for w64.  When I reboot into
> windows, I can try that one.
>

Still didn't boot windows, but some notes on this batch file just by visual
inspection:

-gdwarf-2 :  AFAIK, DW2 is verboten for win64 until SEH2 stuf is completed.
Only SJLJ exceptions are supported.  My cross-compiler seems to ignore that
switch, including or removing it gives the same output.

-mthreads :  This one is a noop for mingw-w64 crt.  Our _mingw.h defines _MT
by itself if not already defined.  And the libmingwthrd.a is just a stub for
mingw-w64 (see libsrc/mingwthrd_mt.c in the mingw-w64-crt.)  The switch is
useless, but harmless.

These were for the gcc switches.  As for cl.exe, I might know only after I
test it for real.

--
Ozkan


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27809>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Eli Zaretskii]

> From: Ozkan Sezer <INVALID.NOREPLY@...>
> Date: Mon, 26 Oct 2009 19:20:27 +0000
>
>
> > 3. Why did you need casts in assignments, like this:
> >
> > -    *pid_p = (int) hProcess;
> > +    *pid_p = (pid_t) hProcess;
> >
>
> Because you are casting a handle, which is a ptr*,  to an int.

But you change pid_p to point to a pid_t type, which is no longer an
int on a 64-bit host.  So why can't you get rid of the cast
altogether, like this:

   pid_p = hProcess;

?  Does this work on w64?

> > 4. This change:
> >
> > -  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
> > +  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);
> >
> > assumes that _open_osfhandle accepts an intptr_t type as its first
> argument.
> > But the prototype I have on my machine (in io.h) says the first argument is
> a
> > `long'.  Which version of MinGW changed that?
>
> It is the same case.  Your version is for w32-only.
> The mingw-w64 version is like (from io.h):
> _CRTIMP int __cdecl _open_osfhandle(intptr_t _OSFileHandle,int _Flags);

But that means I cannot simply apply your patch, because users of
MinGW will then complain about compiler warnings, right?

Thanks for the other info.


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Eli Zaretskii]

> From: Ozkan Sezer <INVALID.NOREPLY@...>
> Date: Mon, 26 Oct 2009 20:27:27 +0000
>
>
> -mthreads :  This one is a noop for mingw-w64 crt.

This is needed to properly handle Ctrl-C, since (at least on w32) the
signal handler runs in a separate thread.  So what happens in a w64
build of Make if you press Ctrl-C while Make runs?  Does it behave as
expected (read: as on Posix systems)?


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Martin Dorey]

> why can't you get rid of the cast altogether

Because there is no implicit conversion from pointer to integer.

> users of MinGW will then complain about compiler warnings, right?

Because of an implicit conversion from unsigned int (the definition of
uintptr_t on w32) to long, on a platform where both types are the same
size?  That seems unlikely.

-----Original Message-----
From: bug-make-bounces+mdorey=bluearc.com@...
[mailto:bug-make-bounces+mdorey=bluearc.com@...] On Behalf Of Eli
Zaretskii
Sent: Monday, October 26, 2009 13:47
To: sezeroz@...; psmith@...; boris@...;
bug-make@...
Subject: Re: [bug #27809] several win64 fixes

> From: Ozkan Sezer <INVALID.NOREPLY@...>
> Date: Mon, 26 Oct 2009 19:20:27 +0000
>
>
> > 3. Why did you need casts in assignments, like this:
> >
> > -    *pid_p = (int) hProcess;
> > +    *pid_p = (pid_t) hProcess;
> >
>
> Because you are casting a handle, which is a ptr*,  to an int.

But you change pid_p to point to a pid_t type, which is no longer an
int on a 64-bit host.  So why can't you get rid of the cast
altogether, like this:

   pid_p = hProcess;

?  Does this work on w64?

> > 4. This change:
> >
> > -  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
> > +  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);
> >
> > assumes that _open_osfhandle accepts an intptr_t type as its first
> argument.
> > But the prototype I have on my machine (in io.h) says the first
argument is
> a
> > `long'.  Which version of MinGW changed that?
>
> It is the same case.  Your version is for w32-only.
> The mingw-w64 version is like (from io.h):
> _CRTIMP int __cdecl _open_osfhandle(intptr_t _OSFileHandle,int
_Flags);

But that means I cannot simply apply your patch, because users of
MinGW will then complain about compiler warnings, right?

Thanks for the other info.


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Ozkan Sezer]

On Mon, Oct 26, 2009 at 10:47 PM, Eli Zaretskii <eliz@...> wrote:

>> From: Ozkan Sezer <INVALID.NOREPLY@...>
>> Date: Mon, 26 Oct 2009 19:20:27 +0000
>>
>>
>> > 3. Why did you need casts in assignments, like this:
>> >
>> > -    *pid_p = (int) hProcess;
>> > +    *pid_p = (pid_t) hProcess;
>> >
>>
>> Because you are casting a handle, which is a ptr*,  to an int.
>
> But you change pid_p to point to a pid_t type, which is no longer an
> int on a 64-bit host.  So why can't you get rid of the cast
> altogether, like this:
>
>   pid_p = hProcess;
>
> ?  Does this work on w64?
>

Well,

typedef struct _PROCESS_INFORMATION {
  HANDLE hProcess;
  HANDLE hThread;
  DWORD dwProcessId;
  DWORD dwThreadId;
} PROCESS_INFORMATION,*PPROCESS_INFORMATION,*LPPROCESS_INFORMATION;

hProcess is HANDLE here, but your pid_p is int in your
original version or pid_t in my version.  Without casting
you'll get warnings from the compiler.

>> > 4. This change:
>> >
>> > -  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
>> > +  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);
>> >
>> > assumes that _open_osfhandle accepts an intptr_t type as its first
>> argument.
>> > But the prototype I have on my machine (in io.h) says the first argument is
>> a
>> > `long'.  Which version of MinGW changed that?
>>
>> It is the same case.  Your version is for w32-only.
>> The mingw-w64 version is like (from io.h):
>> _CRTIMP int __cdecl _open_osfhandle(intptr_t _OSFileHandle,int _Flags);
>
> But that means I cannot simply apply your patch, because users of
> MinGW will then complain about compiler warnings, right?
>

How so??  MinGW does provide intptr_t, I really can't see the
problem here.

> Thanks for the other info.
>

My pleasure.

--
Ozkan


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Ozkan Sezer]

On Mon, Oct 26, 2009 at 10:49 PM, Eli Zaretskii <eliz@...> wrote:

>> From: Ozkan Sezer <INVALID.NOREPLY@...>
>> Date: Mon, 26 Oct 2009 20:27:27 +0000
>>
>>
>> -mthreads :  This one is a noop for mingw-w64 crt.
>
> This is needed to properly handle Ctrl-C, since (at least on w32) the
> signal handler runs in a separate thread.  So what happens in a w64
> build of Make if you press Ctrl-C while Make runs?  Does it behave as
> expected (read: as on Posix systems)?
>

Just tried running a makefile with it (under an msys console) and it
just exists.

--
Ozkan


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Ozkan Sezer]

On Mon, Oct 26, 2009 at 11:21 PM, Ozkan Sezer <sezeroz@...> wrote:

> On Mon, Oct 26, 2009 at 10:47 PM, Eli Zaretskii <eliz@...> wrote:
>>> From: Ozkan Sezer <INVALID.NOREPLY@...>
>>> Date: Mon, 26 Oct 2009 19:20:27 +0000
>>>
>>>
>>> > 3. Why did you need casts in assignments, like this:
>>> >
>>> > -    *pid_p = (int) hProcess;
>>> > +    *pid_p = (pid_t) hProcess;
>>> >
>>>
>>> Because you are casting a handle, which is a ptr*,  to an int.
>>
>> But you change pid_p to point to a pid_t type, which is no longer an
>> int on a 64-bit host.  So why can't you get rid of the cast
>> altogether, like this:
>>
>>   pid_p = hProcess;
>>
>> ?  Does this work on w64?
>>
>
> Well,
>
> typedef struct _PROCESS_INFORMATION {
>  HANDLE hProcess;
>  HANDLE hThread;
>  DWORD dwProcessId;
>  DWORD dwThreadId;
> } PROCESS_INFORMATION,*PPROCESS_INFORMATION,*LPPROCESS_INFORMATION;
>
> hProcess is HANDLE here, but your pid_p is int in your
> original version or pid_t in my version.  Without casting
> you'll get warnings from the compiler.
>
>>> > 4. This change:
>>> >
>>> > -  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
>>> > +  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);
>>> >
>>> > assumes that _open_osfhandle accepts an intptr_t type as its first
>>> argument.
>>> > But the prototype I have on my machine (in io.h) says the first argument is
>>> a
>>> > `long'.  Which version of MinGW changed that?
>>>
>>> It is the same case.  Your version is for w32-only.
>>> The mingw-w64 version is like (from io.h):
>>> _CRTIMP int __cdecl _open_osfhandle(intptr_t _OSFileHandle,int _Flags);
>>
>> But that means I cannot simply apply your patch, because users of
>> MinGW will then complain about compiler warnings, right?
>>
>
> How so??  MinGW does provide intptr_t, I really can't see the
> problem here.
>

...  and, for the record, I just built the make-cvs for mingw.org,
without and with my patch applied, there seems to be no new
warnings generated (and many of the alloca warnings are just
cured.)  Attaching a diff of build logs.

--
Ozkan


>> Thanks for the other info.
>>
>
> My pleasure.
>
> --
> Ozkan
>


_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make

[Mario Castelán Castro]

Follow-up Comment #4, bug #27809 (project make):

[ I forgot replying from within the bug tracker.
  sorry if this becomes a duplicate. ]

>> > 3. Why did you need casts in assignments, like this:
>> >
>> > -    *pid_p = (int) hProcess;
>> > +    *pid_p = (pid_t) hProcess;
>> >
>>
>> Because you are casting a handle, which is a ptr*,  to an int.
>
> But you change pid_p to point to a pid_t type, which is no longer an
> int on a 64-bit host.  So why can't you get rid of the cast
> altogether, like this:
>
>   pid_p = hProcess;
>
> ?  Does this work on w64?
>

Well,

typedef struct _PROCESS_INFORMATION {
 HANDLE hProcess;
 HANDLE hThread;
 DWORD dwProcessId;
 DWORD dwThreadId;
} PROCESS_INFORMATION,*PPROCESS_INFORMATION,*LPPROCESS_INFORMATION;

hProcess is HANDLE here, but your pid_p is int in your
original version or pid_t in my version.  Without casting
you'll get warnings from the compiler.

>> > 4. This change:
>> >
>> > -  pipedes[0] = _open_osfhandle((long) hChildOutRd, O_RDONLY);
>> > +  pipedes[0] = _open_osfhandle((intptr_t) hChildOutRd, O_RDONLY);
>> >
>> > assumes that _open_osfhandle accepts an intptr_t type as its first
>> argument.
>> > But the prototype I have on my machine (in io.h) says the first argument
is

>> a
>> > `long'.  Which version of MinGW changed that?
>>
>> It is the same case.  Your version is for w32-only.
>> The mingw-w64 version is like (from io.h):
>> _CRTIMP int __cdecl _open_osfhandle(intptr_t _OSFileHandle,int _Flags);
>
> But that means I cannot simply apply your patch, because users of
> MinGW will then complain about compiler warnings, right?
>

How so??  MinGW does provide intptr_t, I really can't see the
problem here.

and, for the record, I just built the make-cvs for mingw.org,
without and with my patch applied, there seems to be no new
warnings generated (and many of the alloca warnings are just
cured.)  Attaching a diff of build logs.

>>
>> -mthreads :  This one is a noop for mingw-w64 crt.
>
> This is needed to properly handle Ctrl-C, since (at least on w32) the
> signal handler runs in a separate thread.  So what happens in a w64
> build of Make if you press Ctrl-C while Make runs?  Does it behave as
> expected (read: as on Posix systems)?
>

Just tried running a makefile with it (under an msys console) and it
just exists.

> Thanks for the other info.
>

My pleasure.

--
Ozkan


(file #18947)
    _______________________________________________________

Additional Item Attachment:

File name: mingw-buildlogs-diff.txt       Size:15 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27809>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-make mailing list
Bug-make@...
http://lists.gnu.org/mailman/listinfo/bug-make