bugmake - Bugs: bug #27809, several win64 fixes

 
 

bug #27809: several win64 fixes

Submitter:  Ozkan Sezer <sezero>
Submitted:  Mon 26 Oct 2009 09:07:56 AM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  eliz Open/Closed:  Closed
Component Version:  4.0 Operating System:  MS Windows
Fixed Release:  3.82 Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 09 Jul 2010 01:55:06 PM UTC, comment #14: 

In its current state the CVS doesn't build for windows targets,
because the protoype for w32_kill isn't updated to match job.c.
I think Eli is aware of it but I am posting this so that things
do not get forgotten.  And while I am here, here are some small
suggestions, see the attached new patch file w64-20100709.diff:

  • job.c (pid2str): Do not limit %Id to gcc >= 4 because

gcc-3.4.5 from mingw.org understands %Id just fine, and the
_GNUC_ > 3 condition also eliminates the possibility of MSVC
compiler being in use.
(exec_command): Cast hWaitPID to pid_t which is what pid2str()
accepts, instead of DWORD_PTR.

  • main.c (main, ADD_SIG): Pacify compiler.


  • make.h: Move malloc.h include for WINDOWS32 to an earlier

place among the alloca ifdefs.
(w32_kill): Update prototype to match what is in job.c.

  • w32/subproc/sub_proc.c: For MSVC, include stddef.h instead

of stdint.h for intptr_t.

  • getopt.c (_getopt_initialize): Pacify compiler (unused argc

and argv) when not building for glibc.
(_getopt_internal): Add braces around if to avoid ambigous else.

[Note: the malloc.h include changes suggested for make.h won't
be necessary if the alloca ifdefs there goes back to the state
of make-3.81.]


(file #20931)

Ozkan Sezer <sezero>
Fri 09 Jul 2010 11:17:16 AM UTC, comment #13: 

Fixed in CVS, usind mots of the last patch.

Eli Zaretskii <eliz>
Group Member
Mon 05 Jul 2010 08:19:10 PM UTC, comment #12: 


> It is needed earlier, otherwise line #38 of make.h provides a
> prototype for alloca because there is no alloca.h and AIX is
> not defined.


OK then the other #include <malloc.h> should probably be removed.  Thanks!

Paul D. Smith <psmith>
Group administrator
Mon 05 Jul 2010 07:58:04 PM UTC, comment #11: 

On Mon, Jul 5, 2010 at 9:42 PM, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: "Paul D. Smith" <INVALID.NOREPLY@gnu.org>
>> Date: Mon, 05 Jul 2010 18:32:15 +0000
>>
>> I've applied most of the second patch.  The first patch is
>> mostly in the w32 area so maybe Eli is a better person to
>> review it?
>
> I will try to do that over the next few days.
>


A note on this: The second patch contained changes to main.c
which are windows specific, used to fix the following win64
warnings:

main.c: In function 'handle_runtime_exceptions':
main.c:685: warning: cast from pointer to integer of different size
main.c:693: warning: cast from pointer to integer of different size
main.c:701: warning: format '%lx' expects type 'long unsigned int', but argument 3 has type 'ULONG_PTR'
main.c:701: warning: format '%lx' expects type 'long unsigned int', but argument 3 has type 'ULONG_PTR'
main.c:997: warning: suggest braces around empty body in an 'else' statement
main.c:998: warning: suggest braces around empty body in an 'else' statement
main.c:1402: warning: initialization discards qualifiers from pointer target type

To make the reviewers' lives easier, I am attaching a combined
all-in-one patch, w64-all-20100705.diff.  This obsoletes all
the other patches attached here. In addition, it also covers the
tiny patch sugggested in bug #27825 too, so bug #27825 can be
closed.

(file #20902)

Ozkan Sezer <sezero>
Mon 05 Jul 2010 07:07:05 PM UTC, comment #10: 


> I've applied most of the second patch.  The first patch is


Thanks.

> I did have one question about the first patch: you have a
> change to make.h which adds an include of malloc.h, but
> later in make.h that header is already included if WINDOWS32
> is set.  Is it needed earlier, or...?
>


It is needed earlier, otherwise line #38 of make.h provides a
prototype for alloca because there is no alloca.h and AIX is
not defined. As a result, gcc will complain like:

In file included from ../make-cvs/ar.c:19:
../make-cvs/make.h:39: warning: conflicting types for built-in function 'alloca'

Ozkan Sezer <sezero>
Mon 05 Jul 2010 06:32:15 PM UTC, comment #9: 

I've applied most of the second patch.  The first patch is mostly in the w32 area so maybe Eli is a better person to review it?

I did have one question about the first patch: you have a change to make.h which adds an include of malloc.h, but later in make.h that header is already included if WINDOWS32 is set.  Is it needed earlier, or...?

Paul D. Smith <psmith>
Group administrator
Mon 05 Jul 2010 11:43:37 AM UTC, comment #8: 

On Mon, Jul 5, 2010 at 1:35 PM, Edward Welbourne <eddy@opera.com> wrote:
[snip]

>> Finally, it seems that some of these changes are meant to
>> avoid variable names conflicting with function names (open,
>> etc.)  Is this really a warning that some compilers give?
>
> Even gcc has a flag for it: -Wshadow; I conjecture that the
> submitter is using more aggressive warning flags than you ...
>> I believe -Wshadow is not part of what's normally enabled by
>> use of -Wall -Wextra, which I tend to prefer (trusting the
>> judgement of the gcc team).
>
> Submitter: what warning flags are you passing to gcc ?
>


The standart warning flags already provided by maintMakefile
which includes -Wshadow

[snip]

--
Ozkan

Ozkan Sezer <sezero>
Sun 04 Jul 2010 06:49:11 PM UTC, comment #7: 

Paul, thank you very much for reviewing the patch,

> First you mention compiler warning fixes for UNIX, but
> none of the changes you mention fix any warnings I see
> on my UNIX tests (different platforms but using gcc -Wall
> on all of them).  Which ones do you see?


Hmm, my mistake. Truly sorry. (I have been using this patch
for several months now and it seems like I have forgotten
its history. The warning fixes are for windows. I apologize,
really.

> Second, I'm not thrilled about adding a lot MORE ifdefs
> just to allow for debugging statements.  I think we need to
> find a better way to handle this part.  I'll think about it
> and check


Yes, the ifdefs I added are ugly, I know, it was just the quick
solution. Without them I get many "cast from pointer to integer
of different size" warnings and the like, mostly because of
pid_t type being 64 bits wide in win64.

> my ISO C 1989 standard (I can't remember whether it supports
> %p) when I get back to work on Tuesday.
>


IIRC, it should be in C89 but please verify.

> Third, I really don't understand why the casts in ar.c and
> arscan.c are necessary.  Those members are already of type
> char* (through array decay) and it's perfectly reasonable to
> pass a char* to a function requiring a const char* without
> any cast required.


See lines 284 and 286 in arscan.c. The Mode member of the
_IMAGE_ARCHIVE_MEMBER_HEADER is BYTE, ie. uunsigned char, so
we have the folowing warnings:

../make-cvs/arscan.c: In function 'ar_scan':
../make-cvs/arscan.c:633: warning: pointer targets in passing argument 1 of 'sscanf' differ in signedness
/opt/W64_161668/bin/../lib/gcc/x86_64-w64-mingw32/4.4.5/../../../../x86_64-w64-mingw32/include/stdio.h:246: note: expected 'const char ' but argument is of type 'BYTE '
(... and same warnings at lines 634, 644, 645, 646, 815)

> I really don't like to use casts unless absolutely necessary
> (I've worked to REDUCE the total amount of casting in GNU
> make source) so I'd definitely prefer to avoid these.
>
> Finally, it seems that some of these changes are meant to
> avoid variable names conflicting with function names (open,
> etc.)  Is this really a warning that some compilers give?
> I've never seen that warning, and it sure seems to me like
> it would be almost impossible to write a portable C program
> that didn't hit that warning, since every C runtime defines
> a horde of functions and there is no way to know what they
> all are.  I don't like it.


Here are the shadow warnings I get:

../make-cvs/ar.c: In function 'glob_pattern_p':
../make-cvs/ar.c:215: warning: declaration of 'open' shadows a global declaration
/opt/W64_161668/bin/../lib/gcc/x86_64-w64-mingw32/4.4.5/../../../../x86_64-w64-mingw32/include/io.h:340: warning: shadowed declaration is here

../make-cvs/job.c: In function 'create_batch_file':
../make-cvs/job.c:249: warning: declaration of 'error' shadows a global declaration
../make-cvs/make.h:373: warning: shadowed declaration is here

../make-cvs/signame.c: In function 'strsignal':
../make-cvs/signame.c:232: warning: declaration of 'signal' shadows a global declaration
/opt/W64_161668/bin/../lib/gcc/x86_64-w64-mingw32/4.4.5/../../../../x86_64-w64-mingw32/include/signal.h:59: warning: shadowed declaration is here

If you have any further questions, I am willing to answer to
the best of my ability.

Regards.

Ozkan Sezer <sezero>
Sun 04 Jul 2010 06:12:15 PM UTC, comment #6: 

Thanks Ozkan.  A few comments about the parts of the patch that are not Windows-specific.

First you mention compiler warning fixes for UNIX, but none of the changes you mention fix any warnings I see on my UNIX tests (different platforms but using gcc -Wall on all of them).  Which ones do you see?

Second, I'm not thrilled about adding a lot MORE ifdefs just to allow for debugging statements.  I think we need to find a better way to handle this part.  I'll think about it and check my ISO C 1989 standard (I can't remember whether it supports %p) when I get back to work on Tuesday.

Third, I really don't understand why the casts in ar.c and arscan.c are necessary.  Those members are already of type char* (through array decay) and it's perfectly reasonable to pass a char* to a function requiring a const char* without any cast required.  I really don't like to use casts unless absolutely necessary (I've worked to REDUCE the total amount of casting in GNU make source) so I'd definitely prefer to avoid these.

Finally, it seems that some of these changes are meant to avoid variable names conflicting with function names (open, etc.)  Is this really a warning that some compilers give?  I've never seen that warning, and it sure seems to me like it would be almost impossible to write a portable C program that didn't hit that warning, since every C runtime defines a horde of functions and there is no way to know what they all are.  I don't like it.

Paul D. Smith <psmith>
Group administrator
Sun 04 Jul 2010 04:06:22 PM UTC, comment #5: 

Here is a follow-up patch (make-cvs-20100701-w64-2.patch) which
fixes a lot of compiler warnings, not only for mingw* but for
unix, too.

(file #20896)

Ozkan Sezer <sezero>
Mon 26 Oct 2009 10:44:16 PM UTC, comment #4: 

[ 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)

Ozkan Sezer <sezero>
Mon 26 Oct 2009 08:27:27 PM UTC, comment #3: 


>> 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

Ozkan Sezer <sezero>
Mon 26 Oct 2009 07:20:26 PM UTC, comment #2: 


> 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.

Ozkan Sezer <sezero>
Mon 26 Oct 2009 06:50:14 PM UTC, comment #1: 

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.



Eli Zaretskii <eliz>
Group Member
Mon 26 Oct 2009 09:07:56 AM UTC, original submission:  

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.

Ozkan Sezer <sezero>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #20931:  w64-20100709.diff added by sezero (5KiB - application/octet-stream - suggested changes against cvs-20100709)
file #20902:  w64-all-20100705.diff added by sezero (9KiB - application/octet-stream - combined all-in-one win64 patch (2010-07-05.))
file #20896:  make-cvs-20100701-w64-2.patch added by sezero (13KiB - application/octet-stream - follow-up patch fixing a lot of compiler warnings)
file #18947:  mingw-buildlogs-diff.txt added by sezero (15KiB - text/plain - diffs of builds for mingw.org without and with the w64 patch)
file #18942:  build.log added by sezero (25KiB - application/octet-stream - win64 (mingw-w64) patch, and the build log)
file #18941:  make-w64.diff added by sezero (5KiB - application/octet-stream - win64 (mingw-w64) patch, and the build log)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by eliz (Posted a comment)
  • -email is unavailable- added by sezero (Submitted the item)
  • -email is unavailable- added by sezero
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-07-28 psmith Fixed Release4.0 3.82
    2010-07-09 sezero Attached File- Added w64-20100709.diff, #20931
    2010-07-09 psmith StatusNone Fixed
    2010-07-09 psmith Assigned toNone eliz
    2010-07-09 eliz Open/ClosedOpen Closed
    2010-07-09 eliz Fixed ReleaseNone 4.0
    2010-07-05 sezero Attached File- Added w64-all-20100705.diff, #20902
    2010-07-04 sezero Attached File- Added make-cvs-20100701-w64-2.patch, #20896
    2009-10-26 sezero Attached File- Added mingw-buildlogs-diff.txt, #18947
    2009-10-26 sezero Carbon-Copy- Added sezero
        Attached File- Added build.log, #18942
        Attached File- Added make-w64.diff, #18941

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code