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)

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

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

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

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.

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

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

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.