Follow-up Comment #2, bug #65771 (group make):
[comment #1 comment #1:]
> Thanks. Some comments to the proposed patch:
>
> @@ -576,8 +578,8 @@ char *ttyname (int);
>
> /* Define {u,}intmax_t if not defined in <stdint.h> or <inttypes.h>. */
> #if !HAVE_STDINT_H && !HAVE_INTTYPES_H
> -#define intmax_t long long
> -#define uintmax_t unsigned long long
> +#define intmax_t __int64
> +#define uintmax_t unsigned __int64
> #endif
>
> IMO, these changes should not be unconditional, but conditioned by _MSC_VER.
It makes little sense to use __int64 in a GCC build.
Agree.
> --- a/src/dir.c
> +++ b/src/dir.c
> @@ -1106,7 +1106,7 @@ print_dir_data_base (void)
> #if MK_OS_W32
> printf (_("# %s (key %s, mtime %s): could not be opened.\n"),
> dir->name, dir->contents->path_key,
> - make_ulltoa ((unsigned long long)dir->contents->mtime,
buf));
> + make_ulltoa ((unsigned _quad_t)dir->contents->mtime,
buf));
>
> Sane here. And I don't understand why you use the _quad_t type here; at the
very least you should use unsigned __int64, no? Similar comments to other
uses of _quad_t.
>
> The definition of _quad_t that you suggest in makeint.h is perhaps okay, but
then (a) please don't use lower-case symbold that begins with an underscore,
as that is generally reserved, and (b) why have both intmax_t/uintmax_t and
_quad_t?
file "dir.c" is cross platform, so I was thinking to minimize OS specific
code, especially multi line "#ifdef"s. But there is a better way to rewrite it
on me doing a 2nd look.
Other FOSS projects uses term "QUAD"/"quad" or VENDORPREFIX_64_T etc in a PP
define or typedef, for a portable, MS Win32-okay 64b int type. "int64_t" I
think is too new for gnumake's compat standards, all OSes (correct if wrong).
Gnumake vs Win32 vs maybe other OSes in 2020s already had compat and
portable-problems with "int64_t" when T look in git histoy, which caused "long
long" to get added, etc, etc. I didnt want to introduce >=C99 fallbacks, for
old CCs, and the fallbacks have unprefixed C99/ANSI tokens/identifiers. I just
invented "_quad_t", I assumed the community would suggest a better label
anyways when I post the patch for public review.
I considered, some mistake, either by gnumake devs, or end user patches, IDK
what, accidentally the C99 fallback code is compiled on a C99 or 2020s CC, or
future CCs, and a gmake 2024 era code snapshot, the gmake build somehow breaks
in 2024 or a 2024 gmake code tarball with a future 2030 CC in 2030. So I
picked "_quad_t" label so no conflict even if a mistake happen and fallback is
built on a C99/modern CC. I couldn't think of a better label in the patch 1st
revision.
"#if _MSC_VER < VC 2010 \n #define int64_t __int64" works, but egh, Ive seen
other FOSS projects typically create a VENDOR_64_T, not "typedef signed
__int64 int64_t;"
On a second thinking, there is a better fix. That "long long" cast was added
in
https://git.savannah.gnu.org/cgit/make.git/commit/?id=6f7fb050b4af7335de259f570b75bf6c217eb1cd
and it already is in a "#ifdef WIN32". So for any "retro Win32 computing"
builds, I declare VC6 support is lowest denominator needed. And VC 6 has 64b
ints native. You get Win95/NT 3.51 IDK NT 4 compat with VC6, my personal biz
req for private code is W2K. gmake already, so for 100% of Win32 32b, all CCs
versions, retro and modern already use __int64. Mingw GCC obv must support
__int64 for compat.
current Win32 gmake, has "uintmax_t"->unsigned __int64" hard coded. So that
should be a (uintmax_t) cast, not (unsigned long long), and not (VENDOR_64T).
A zero/sign extend guard, is always good to put in, even if not need. Future
code reasons/future user reasons. Current Win32 gmake doesn't need that cast,
but obv some Win32 CC was printing console warning, probably on 32b sign
extend vs zero extend to 64b hazard. (uintmax_t) cast solves that.
current gmake is
FILE_TIMESTAMP->uintmax_t->Windows unsigned __int64
So a cast is just for CC silence the warning, and a tiny bit future dev
error.
Some accident, some end user changes, gmake C code, for his personal built, at
some future point, and gmake FILE_TIMESTAMP type is signed 32b, and uintmax_t
type is 64b (user mistake), on a 64b OS, atleast do the cast, for zero extend.
I will rework it to (uintmax_t).
> --- a/src/job.c
> +++ b/src/job.c
> @@ -22,6 +22,7 @@ this program. If not, see
<https://www.gnu.org/licenses/>. */
> /* Default shell to use. */
> #if MK_OS_W32
> # include <windows.h>
> +# include <winsock2.h> /* for WSABASEERR */
>
> I don't understand the need for this. The following MS documentation page:
>
>
https://learn.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2
>
> says that these errors are also declared in winerror.h, which windows.h
includes. So why did you need an explicit inclusion on winsock2.h?
"WSA" is a prefix for win sockets API. Old VCs/old MS Win SDKs/old headers
don't auto #include "winsock.h". I didnt research if old 2000s era Mingw has
same header problem. Maybe it does, maybe not. Its irrel. Use socket api? then
you need socket header. If VC/SDK 2022 "winerror.h" auto includes
"winsock2.h", that is 2020s new by MS. forgot if VC 6 or VC 2003 or both need
"winsock2.h"
If brand new/modern Visual C/Windows SDK, automatically
"windows.h"->#include->"winsock.h", it is an accident for gnumake only and
something specific to Win32 gmake, or MS was being "helpful" with 2020s VC/SDK
for public at large, for the public making 2020s era greenfield new apps. It
is 2024, assuming "99.999%" of devs write TCPIP apps. Not crazy. If MS
intentionally for VC 2022/SDK 2022, auto includes always "winsock.h", its
irrel. Anyways old Win CCs need #include winsock.h. And the #include harmless
for 2020s CCs.
> +#if (defined(_MSC_VER) && (_MSC_VER < 1900))
> +//must use shim on all < VC 2015 s
> +//
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170
> +#define snprintf _snprintf_c99
>
> First, please don't use C++-style "//" comments.
> And second, why was this needed? (I've read the page which you quote, and I
still don't think I understand the need, please explain.)
Will fix comments.
gmake adds use of POSIX snprintf(), added 2022 in
https://git.savannah.gnu.org/cgit/make.git/commit/?id=97e51c0285109ebb6bf8be3d1e21cb0bd5058e5a
"snprintf" per MS docs
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170
is Windows 10 runtime OS required, or a claimed crash/corruption/overflow
risk. Even tho "VC 2015-2022 CRT" is officially approved to run on Win7. I'd
rather not research, what the MS doc says and the runtime observed behavior on
Win 7, about the snprintf() retval warning in the MS docs. I think the MS docs
are indirectly documenting a long-standing bug, VC 2015->VC 2022 ???
patchlevel.
Or there is some headache, being described there, of some permutation of
symbol "snprintf()" and MS VC CC 2015->20XX against Win 7/8 clean ISO install
(core OS DLLs) or Win 7/8 no-auto-updates, vs something in gmake.exe's build
time "EXE Manifest file", or build time gmake.exe's "signed certificate" or
gmake.exe's non-existent UWP/MS App Store "flags", vs Windows 7? 8? 10?, vs
Windows App Compat engine, or MS has a bug-compat problem, and can't fix, the
"snprintf()" broken behaviour, because 3rd party published code now depends on
the bug for correct execution. And then, for VC 2015-2022, for ISO C correct
retval, something involving, VC 2022 patchlevel*gmake.exe build time PE DLL
XML Manifest*OS weekly auto updates*runtime Major OS ver*your .exe's embedded
self-signed unapproved "MS App Store" certificate. Its some level of
headache.
In any case, "snprintf" symbol/func is 2020s era Visual C (>=2015). So usage
"snprintf" is a gmake old VC broken build. 1990s/2000s/2010s Visual C only has
"_snprintf", with unique non portable behaviour. I guess older "_snprintf"
could be directly dropped into Win32 gmake for old CCs. But I'm thinking its
just smarter to defensive code, around the maybe-bug "VC 202X" on "Win 7/8",
and fix build break old CC, at same time, same fix. And defend future, gmake
code, against, new gmake usage, of "snprintf()", that could assume retval is
ISO C/POSIX behaviour, all OSes, when retval is NOT ISO/POSIX correct for
Win32.
I don't see in CURRENT gmake code, any exploitable %s user-string-inputs, to
snprintf() that would trigger a buffer overflow/no-NUL-char. But I was
thinking about unknown future gmake code changes add NEW snprintf() calls, and
depend on much more of snprintf() features than now. Defensive code.
> -# include <stddef.h> /* for intptr_t */
> +# include <stddef.h> /* for INT_PTR */
> Is INT_PTR indeed defined in stddef.h? I very much doubt that.
VC 6 headers yes, VC 6 has it in stddef.h, VC 2022 has INT_PTR in basetsd.h.
Comment will prob go away in rev #2 of this patch. with other clean up in the
patch.
>
> osync_parse_mutex (const char *mutex)
> {
> char *endp;
> - unsigned long long i;
> + UINT_PTR i;
>
> errno = 0;
> +#if defined _WIN64
> i = strtoull (mutex, &endp, 16);
> +#else
> + i = strtoul (mutex, &endp, 16);
> +#endif
>
> Why this change? It adds an ugly #ifdef for no good reason. Why not handle
the mutex as unsigned long long (a 64-bit type) in both 32-bit and 64-bit
builds?
Win32 Mutex is a Kernel Handle, and a Win32 Kernel handles C type, is always
and forever equal to a "void *" (32b or 64b), all CCs, all vers, VC+GCC+all.
"strtoul" part has to be 32b vs 64b specific. Very remote truncation hazard
x64 OS.
https://net-snmp-coders.narkive.com/395WzMVL/windows-msvc-2008-64-bit-and-platform-sdk
Even if "strtoull()" is used on 32b windows, ull() has old VC build break
compat problems, and best to not use strtoull() on 32b Win32 gmake.exe, all
CCs old or modern.
I agree my new code, and current code, casts/var decls can be cleaned up in
osync_parse_mutex(). Type "HANDLE i_or_h = (HANDLE) foo();" is cleaner.
"unsigned int i" vs HANDLE i" decls (32b OS), ONLY, has meaning for C step
debugging, so a dev step debugging osync_parse_mutex() can, visually compare
"1234" vs int 1234 in a watch tool, but osync_parse_mutex()/gmake hex encode
the HANDLE, so HANDLE/void * better type for that C auto.
Why strtoull() on 64b?
https://learn.microsoft.com/en-us/windows/win32/winprog64/interprocess-communication?redirectedfrom=MSDN
says AMD x64 Kernels and x64 user procs, ALWAYS get 32b truncate safe, kernel
HANDLEs. But I see issues.
Lets assume x64 "gmake -j8", with x64 child gmake.exe. Or extremely bizzare (I
argue user error, someone will argue no), x64 "gmake.exe -j8", starts 32b
"gmake.exe" child procs. And passes "8 byte" kernel handles (mutex obj), in a
ASCII string, through %ENV% to those 32b gmake.exe's. This should be fine
accord to MS.
MS says in 2024/now, "truncate safe" are kernel handles. No bug, no error. So
2024/currently x64 gmake.exe could use safely "strtoul()" in x64, for the
mutex (kernel handle), then either store 32b machine val on x64 from
strtoul(), to a C 32b int global in a x64 gmake process (4 bytes of RAM saved
total, in exchange for very unusual C code, I disagree intentionally
truncating). Then LATER still x64 gmake.exe, will always cast, the mutex
kernel handle, in that strange 32b C static var, back to native x64 machine
type "HANDLE" aka void *, to pass to Win API/Win kernel fn calls anyways.
Just use strtoull() on 64b.
#2 Defensive code, what if MS in future in "Windows 2030" or "Windows 2035" or
Windows ARM/RISC-V/not invented yet CPUs, drops 32b x86 WoW
emulation/subsystem, and "x64 gmake.exe" with compiled with VC 2005 or VC
2022, suddenly get full length 64b kernel handles, high 32 bit suddenly have
1s, so using strtoul() (32b) on x64 to de/serialize kernel handles to ASCII
strings, while okay in 2024 on Win 10 x64 gmake.exe, seems questionable for
future-compat. Best to use ull() on 64b, ul() on 32b. Its natural. "#if\n
#else\n ul() vs ull()", protects against 2024 x64 gmake.exe suddenly
"breaking" 20 years from now.
If 32b gmake.exe -j4 child process, parses a 64b int in a string, from %ENV%,
its 100% error case. Let strtoul() 32b do bounds check for high 32b in 64b,
and ret EAX 32b 0/-1 to 32b gmake, vs 32b gmake checking for, EDX==0 && EAX >
0 && EAX < 0xFFFFFFFF, EDX register being MS x86 32b ABI for retval 64b in C,
with EDX holds high 32b split. For a 32b proc, a faux-HANDLE, stored in a 64b
var, with any bits on in upper 32 bits, is always error. A 32b proc on 64b OS
can NEVER pass that 64b faux-HANDLE to WoW 32b WinApi kernel functions no
matter what. So bizzare case, 64b gmake.exe -j8 starts child 32b gmake.exe'es,
in 2024, 32b childs will exec uneventfully, and Windows 2035 with no WOW
32on64 layer, irrel. User can't start their now "legacy" in 2035 bizzare
Makefile build process anyway since 32b .exe s can't start.
#3
It would be extra end-user work/difficulty, on what-if "Windows 2035", with
2024 "x64 gmake.exe", that truncates 64b->32b in x64 proc, to have to
use"Windows 2035" App Compat flags/wizard/framework, to force 32b kernel
handles, to be delivered to a "legacy" in 2024 x64 gmake.exe. Just defense
code to ull() on x64 (cur 2024 gmake.exe). The change is only for 32b
gmake.exe builds for old CC compat. Don't use truncating ul(), even if "safe"
on x64 builds, to save 5 lines of text, in .c file.
> +/*VC 6 doesn't have strtoi64, VC 7/2003 does */
> +#if defined(_MSC_VER) && (_MSC_VER < 1300)
> +/* from ReactOS /sdk/lib/crt/string/strtoi64.c
> + https://git.reactos.org/?p=reactos.git;a=blob;f=COPYING3;hb=HEAD
> +*/
> +/* Based on Wine Staging 1.9.9 - dlls/msvcrt/string.c */
>
> If this code is indeed from Wine, I'm not sure we can accept it without them
assigning the copyright to us. That's Paul's decision, obviously, but AFAIK
those are the rules of the GNU project regarding non-trivial contributions of
code.
>
>
I have other less perfect ideas for strtoi64() VC6 other than a direct
re-implementation shim (above code).
While its nice for 2024 gmake built with VC6 to have the strtoi64() shim. A
shim covers all retro VCs, on all retro Win OSes. And keep's gmake.exe's on
VC's static link MS Libc design for user friendlyness and a "portable"
gmake.exe for copy/pasting/distributing between PCs. (and Win32 gmake with VC,
the current gmake win32 build script, right now doesn't support a MS dynamic
link external CRT build option, my test shows, its NOT "Replace All" button
"/MT" -> "/MD" @ build_w32.bat in an IDE, gmake has build fail+IIRC from last
week, no console output hang-ed with /MD).
Back compat design issues if a VC 6+gmake+external/alternative MS provided
strtoi64() is used. Its possible, but strange to static link MS VC CRT lib,
then for 1 func call, load in addr space, DLL MS CRT (msvcr1XX.dll or
msvcrt.dll), and still keep gmake.exe as static link LIBC, not just convert
default gmake.exe VC build to more common dynamic link to the MS CRT DLL in
C:/windows/system32 like gmake.exe with Mingw GCC has done from day 1. But
from day 1, Mingw uses version-less universal msvcrt.dll, which solves the big
drama about VC 20XX Redistributable installer/dep drama. Just kooky,
intentional keeping, 2 CRTs in 1 proc, by design. Its okay IMO if its
no-binary vs unusual but works back compat fix in C app. If a VC 6 build has
to runtime search for a MS OS supplied DLL for strtoi64(), some more design
problems.
My WinXP 32b msvcrt.dll ("not public" but "public", all GCCs use it) has
strtoi64() exported.
My Win2k 32b msvcrt.dll does NOT have a strtoi64() NOT GOOD (2000s Mingws
probably keep a copy in libgcc_s_dw2-1.dll or libmingwex.dll etc, or secret
static link it in, or mingw (2024? or 2010s?) .exe won't start on win2k, its a
user problem, not mingw dev problem.
msvcr70.dll 32b has strtoi64() export, 99% chance a Win2k user by now has VC
2002/VC 2003 redistributable. Statistically only way to be missing msvcr70.dll
on Win2K by now is VMs with "clean installs" of Win2k. VC6 only gmake, would
be having a dep for runtime >=XP, or install MS VC 2002 runtime, <= 2K/98.
Reasonable to me. For a ancient VC, on ancient OS. Not old*ancient, but
ancient*ancient.
Call path for current gmake code to strtoi64()
"$(word n,text)"->parse_numeric->strtoi64()
"$(wordlist s,e,text)"->parse_numeric->strtoi64()
So user_input > 32b || user_input > size_t on 32b proc, any OS, with gmake,
will fatal error with message, or OOM crash instant.
"future" gmake code, I can guess, someone uses parse_numeric() for a new
feature gnumake only feature "read 10 bytes starting at 5.1 GB, of 5.5 GB disk
file, into make recipie " in a Makefile var/macro/rule/directive,
or gnumake adds some new date/time Makefile math calc features. Then it can be
argued, 32b has legit reason, to parse a 64b from user input, then using the
64b int in 32b proc for something that is NOT always a 4GB malloc() call. I'd
rather get strtoi64() to exec on VC 6, vs hypthetic special case unique
behavior on gmake VC 6 only, vs gmake >=2002 legacy up.
Alt 1:
macro back parse_numeric() to 32b strtoi() all 32b OSes, linux also, b/c gmake
will SEGV anyways with a 5GB makefile or 5GB malloc(), and 32b constant -1
from strtol() errors branches/OOM SEGV, even faster than a truncated 64b
value, since truncated 64b could silently keep gmake.exe "going" for a few
lines of C, then error branch/fatal exit, or invisible failure by gmake.exe
for rest of proc.
Alt 2:
macro back parse_numeric() to 32b strtoi() VC 6 32b gmake only, all other CCs,
all other versions, aka VC >= 2003 (2002??), no chagges. the user's bad
Makefile syntax would maybe cause flip OOM error msg vs other fatal message,
vs SEGV behaviour, of ancient VC6 gmake vs ancient VC 2003->modern,
errors/SEGVs/out of memory at all, is a different topic
Alt 3:
VC 6 gmake only, with VC 6 static link CRT build (cur default), very clunky,
at ever gmake proc start (egh, perf issue), or on runtime hitting the rarely
used gmake "$(word)", at makefile "$(word)" parse hit time, calls
LoadLibrary('msvcrt.dll') (XP and up) or LoadLibrary('ntdll.dll') (egh, Win
95/98) and fetch-es strtoi64() fn ptr, with maybe a final fall back to
searching msvcr70.dll for strtoi64() on Win2k/ME/98 older, which is probably
even less used vs WinXP in 2024. VC 2003 I think, publically supports,
building obinaries for Win95/NT 4, so I assume VC 2003 LIBC is installable on
NT 4/Win 95 up. And gmake+VC 2003+vanilla static link VC CRT, is uneventful
and unchanged.
And realistically, clean installs, from a CD, of NT 4/98/2000, in 2024, and a
dev/user, wanting 2024 gmake VC6, to run on a clean install, of NT 4/98/2000,
no VC 2002/2003 redistributable MS CRT/LIBC. Nope. Just Nope. Any legit user,
with an air gapped, legacy Win 98/2000, box, wanting to rebuild any
1990s/2000s era private code, with 2024 gnumake, and they probably run some
legacy air gapped biz apps, either in mid 2000s, or by 2024, some user
manually installed on airgapped legacy PC, or some other app auto installed
with itself, installed the VC 2002 or 2003 runtime on that air gapped legacy
dev machine. Its only VC 2005 that started the decade of SxS DLL Manifest
version hell for MS CRT/LIBC. 2002/2003 CRTs redist installers I assume are
zero drama universally. I personally always use VC 2003+Win2k to maintain my
legacy systems in 2024. Me fixing gmake for VC 6 is more academic reasons or
research of VC x86 machine code gen and changes over the years to VC's code
gen. Plus VC 6 support, covers 100% of any retro/legacy Win32 hobby needs, of
the general public. Very tiny "portable code" benefit, since VC 6 compat
shakes out code issues, trial by fire, with VC6's bugs, flaws, and limits, for
future, brand new hobby Compilers/hobby OSes.
Alt 3 is less clunky if I add to gmake, build option for VC 6->VC 2022, not
default!!, build gmake with VC Ver Specific, DLL based MS CRT (MS
default/99.999% Win32 use DLL CRT and don't static link the MS VC CRT).
Then, VC6, do NOT static link LIBC, do DLL CRT build. VC 6 only, This way a VC
6 gmake, on XP and up, at runtime, already has "msvcrt.dll" in address space,
and GetProcAddress("strtoi64") is cheap. And no "but 2 LIBCs 1 proc"
complaints. Note, its 2024, modern code, running on legacy, obv compat on
legacy, and prevent bugs, prevent diff behavior legacy vs modern comes way
before perf complaints on legacy. 2 MS libcs, 1 proc, is a perf complaint. I
call it tiny. I'd rather have a slow binary vs a tool's release notes saying
"ver prior to X.Y, not supported EOM".
Alt 4:
have gmake wrap VC 6's static linked MS LIBC 's "atoi64()", I see a atoi64()
in my VC 6's "stdlib.h", having gmake on VC 6, do a shim, with a char count
and search loop, after calling atoi64(), for next (char *), NotEq ASCII + and
- and 0-9, is basic simple. VC6 also have sscanf(), but both need a shim, with
loop to search string buffer, for end of integer. So just make shim of VC 6's
native atoi64()+shim with char search loop
Alt 5:
write for gmake with VC 6, a no ReactOS code, strtoi64() implementation,
decimal ints only, no UTF8 support, no non-latin numbers (even works any OS
??), covers only gmake needs/parse_numeric() needs, code is too basic to have
any IP/license issues
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/bugs/?65771>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
