Re: [PATCH] Fix type errors in win32.
> From: Paul Smith > Cc: bug-make@gnu.org > Date: Sun, 28 Nov 2021 15:19:15 -0500 > > On Mon, 2021-10-25 at 15:32 +0300, Eli Zaretskii wrote: > > the default C runtime used by Make doesn't support %llu, at least not > > on Windows versions older than Windows 10. > > Hrm, I just added a new use of sprintf() with %lld into GNU make, to > format a long long variable. It worked for me in my trivial Windows > tests but I am using Windows 10 with MSVC 2019 so pretty new. I don't > have the facilities to test older systems. If you used MSVC 2019, you are not using the default MSVCRT.DLL runtime. > Should I be using %I64d instead on Windows, to allow support for older > systems? Yes, IMO. You can also use PRIdMAX from inttypes.h for portability.
Re: [PATCH] Fix type errors in win32.
On Mon, 2021-10-25 at 15:32 +0300, Eli Zaretskii wrote: > the default C runtime used by Make doesn't support %llu, at least not > on Windows versions older than Windows 10. Hrm, I just added a new use of sprintf() with %lld into GNU make, to format a long long variable. It worked for me in my trivial Windows tests but I am using Windows 10 with MSVC 2019 so pretty new. I don't have the facilities to test older systems. Should I be using %I64d instead on Windows, to allow support for older systems?
Re: [PATCH] Fix type errors in win32.
[Please use Reply All to reply, so that the mailing list is CC'ed.]
> From: U2FsdGVkX1
> Date: Tue, 26 Oct 2021 09:44:01 +0800
>
> >> From: U2FsdGVkX1
> >> Date: Mon, 25 Oct 2021 10:55:03 +0800
> >> Cc: U2FsdGVkX1
> >>
> >> * src/commands.c (fatal_error_signal): DWORD type should be unsigned
> >> * src/dir.c (print_dir_data_base): Fix format type mismatch
> >> * src/function.c (windows32_openpipe): Use the WINAPI GetStdHandle
> >> function instead
> >> * src/getopt.c (_getopt_internal): Improve code readability
> >
> > Thanks. A couple of comments to some of the hunks you propose:
> >
> >> --- a/src/dir.c
> >> +++ b/src/dir.c
> >> @@ -1119,7 +1119,7 @@ print_dir_data_base (void)
> >> else if (dir->contents->dirfiles.ht_vec == 0)
> >> {
> >> #ifdef WINDOWS32
> >> - printf (_("# %s (key %s, mtime %I64u): could not be
> >> opened.\n"),
> >> + printf (_("# %s (key %s, mtime %llu): could not be
> >> opened.\n"),
> >
> > This change is problematic, because the default C runtime used by Make
> > doesn't support %llu, at least not on Windows versions older than
> > Windows 10. MinGW64 links programs by default with an extension
> > library of its own, which provides ANSI-compatible replacements for
> > printf and stuff, and those do support %llu, but we don't want to make
> > the sources compile only with those tools.
> >
> > Are there any problems with using %I64u there? if so, what problems
> > did you see?
> >
> >> - tmpErr = (HANDLE)_get_osfhandle (errfd);
> >> + tmpErr = GetStdHandle (errfd);
> >
> > This one I don't understand. _get_osfhandle is the documented way of
> > obtaining the OS handle for any CRT file descriptor, so what's wrong
> > with using it here? By contrast, GetStdHandle isn't even explicitly
> > documented to serve this purpose, i.e. to obtain a handle from a CRT
> > file descriptor (since errfd comes from 'fileno'). maybe it can do
> > that, but why use undocumented functionality when a documented one is
> > available?
> >
>
> Hi, thanks for the review!
>
> > - tmpErr = (HANDLE)_get_osfhandle (errfd);
> > + tmpErr = GetStdHandle (errfd);
> Use the GetStdHandle function instead of _get_osfhandle because, in
> Win32 runtime library the _get_osfhandle is defined as
> > intptr_t _get_osfhandle(
> > int fd
> > );
>
> And the GetStdHandle function is defined as
> > // https://docs.microsoft.com/en-us/windows/console/getstdhandle
> > HANDLE WINAPI GetStdHandle(
> > _In_ DWORD nStdHandle
> > );
The fact that 2 functions have the same argument types and return
types doesn't mean they do the same.
> The tmpErr variable can call DuplicateHandle without converting to the
> HANDLE type
>
> And the following also uses the GetStdHandle instead of the
> _get_osfhandle function
> http://git.savannah.gnu.org/cgit/make.git/tree/src/function.c#n1564
We use GetStdHandle in that function when the argument is
STD_INPUT_HANDLE, which is a special fixed value -10. That is, this
is not a real file descriptor, since file descriptors are always
non-negative.
So bottom line, I still don't think we should make this particular
change.
Re: [PATCH] Fix type errors in win32.
> From: U2FsdGVkX1
> Date: Mon, 25 Oct 2021 10:55:03 +0800
> Cc: U2FsdGVkX1
>
> * src/commands.c (fatal_error_signal): DWORD type should be unsigned
> * src/dir.c (print_dir_data_base): Fix format type mismatch
> * src/function.c (windows32_openpipe): Use the WINAPI GetStdHandle function
> instead
> * src/getopt.c (_getopt_internal): Improve code readability
Thanks. A couple of comments to some of the hunks you propose:
> --- a/src/dir.c
> +++ b/src/dir.c
> @@ -1119,7 +1119,7 @@ print_dir_data_base (void)
>else if (dir->contents->dirfiles.ht_vec == 0)
> {
> #ifdef WINDOWS32
> - printf (_("# %s (key %s, mtime %I64u): could not be
> opened.\n"),
> + printf (_("# %s (key %s, mtime %llu): could not be opened.\n"),
This change is problematic, because the default C runtime used by Make
doesn't support %llu, at least not on Windows versions older than
Windows 10. MinGW64 links programs by default with an extension
library of its own, which provides ANSI-compatible replacements for
printf and stuff, and those do support %llu, but we don't want to make
the sources compile only with those tools.
Are there any problems with using %I64u there? if so, what problems
did you see?
> - tmpErr = (HANDLE)_get_osfhandle (errfd);
> + tmpErr = GetStdHandle (errfd);
This one I don't understand. _get_osfhandle is the documented way of
obtaining the OS handle for any CRT file descriptor, so what's wrong
with using it here? By contrast, GetStdHandle isn't even explicitly
documented to serve this purpose, i.e. to obtain a handle from a CRT
file descriptor (since errfd comes from 'fileno'). maybe it can do
that, but why use undocumented functionality when a documented one is
available?
[PATCH] Fix type errors in win32.
* src/commands.c (fatal_error_signal): DWORD type should be unsigned
* src/dir.c (print_dir_data_base): Fix format type mismatch
* src/function.c (windows32_openpipe): Use the WINAPI GetStdHandle function
instead
* src/getopt.c (_getopt_internal): Improve code readability
---
src/commands.c | 4 ++--
src/dir.c | 4 ++--
src/function.c | 8
src/getopt.c | 3 ++-
4 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/commands.c b/src/commands.c
index 8a483bd..cacbfc8 100644
--- a/src/commands.c
+++ b/src/commands.c
@@ -515,12 +515,12 @@ fatal_error_signal (int sig)
DWORD susp_count = SuspendThread (main_thread);
if (susp_count != 0)
-fprintf (stderr, "SuspendThread: suspend count = %ld\n", susp_count);
+fprintf (stderr, "SuspendThread: suspend count = %lu\n", susp_count);
else if (susp_count == (DWORD)-1)
{
DWORD ierr = GetLastError ();
- fprintf (stderr, "SuspendThread: error %ld: %s\n",
+ fprintf (stderr, "SuspendThread: error %lu: %s\n",
ierr, map_windows32_error_to_string (ierr));
}
}
diff --git a/src/dir.c b/src/dir.c
index edabcaf..724f750 100644
--- a/src/dir.c
+++ b/src/dir.c
@@ -1119,7 +1119,7 @@ print_dir_data_base (void)
else if (dir->contents->dirfiles.ht_vec == 0)
{
#ifdef WINDOWS32
- printf (_("# %s (key %s, mtime %I64u): could not be opened.\n"),
+ printf (_("# %s (key %s, mtime %llu): could not be opened.\n"),
dir->name, dir->contents->path_key,
(unsigned long long)dir->contents->mtime);
#else /* WINDOWS32 */
@@ -1156,7 +1156,7 @@ print_dir_data_base (void)
}
}
#ifdef WINDOWS32
- printf (_("# %s (key %s, mtime %I64u): "),
+ printf (_("# %s (key %s, mtime %llu): "),
dir->name, dir->contents->path_key,
(unsigned long long)dir->contents->mtime);
#else /* WINDOWS32 */
diff --git a/src/function.c b/src/function.c
index 5a7ad3a..a598d74 100644
--- a/src/function.c
+++ b/src/function.c
@@ -1581,11 +1581,11 @@ windows32_openpipe (int *pipedes, int errfd, pid_t
*pid_p, char **command_argv,
if (hIn == INVALID_HANDLE_VALUE)
{
ON (error, NILF,
- _("windows32_openpipe: DuplicateHandle(In) failed (e=%ld)\n"),
e);
+ _("windows32_openpipe: DuplicateHandle(In) failed (e=%lu)\n"),
e);
return -1;
}
}
- tmpErr = (HANDLE)_get_osfhandle (errfd);
+ tmpErr = GetStdHandle (errfd);
if (DuplicateHandle (GetCurrentProcess (), tmpErr,
GetCurrentProcess (), ,
0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE)
@@ -1605,14 +1605,14 @@ windows32_openpipe (int *pipedes, int errfd, pid_t
*pid_p, char **command_argv,
if (hErr == INVALID_HANDLE_VALUE)
{
ON (error, NILF,
- _("windows32_openpipe: DuplicateHandle(Err) failed (e=%ld)\n"),
e);
+ _("windows32_openpipe: DuplicateHandle(Err) failed (e=%lu)\n"),
e);
return -1;
}
}
if (! CreatePipe (, , , 0))
{
- ON (error, NILF, _("CreatePipe() failed (e=%ld)\n"), GetLastError());
+ ON (error, NILF, _("CreatePipe() failed (e=%lu)\n"), GetLastError());
return -1;
}
diff --git a/src/getopt.c b/src/getopt.c
index 35e71ef..262633f 100644
--- a/src/getopt.c
+++ b/src/getopt.c
@@ -676,7 +676,7 @@ _getopt_internal (int argc, char *const *argv, const char
*optstring,
optarg = nameend + 1;
else
{
- if (opterr)
+ if (opterr) {
if (argv[optind - 1][1] == '-')
/* --option */
fprintf (stderr,
@@ -687,6 +687,7 @@ _getopt_internal (int argc, char *const *argv, const char
*optstring,
fprintf (stderr,
_("%s: option '%c%s' doesn't allow an argument\n"),
argv[0], argv[optind - 1][0], pfound->name);
+ }
nextchar += strlen (nextchar);
--
2.25.1
