Re: [Mingw-w64-public] [PATCH 7/7] crt: Move find, stat and time aliases to def-include/msvcrt-common.def.in
On Tuesday 30 April 2024 12:01:53 Martin Storsjö wrote: > On Sat, 27 Apr 2024, Pali Rohár wrote: > > > Add 4 new macros FIXED_SIZE_SYMBOLS, NO_I64_FIXED_SIZE, > > NO_FIXED_SIZE_64_ALIAS and NO_TIME_ALIAS to distinguish > > between different crt versions. > > > > This change adds new symbol aliases which were missing. > > There is no symbol change or removal. > > > > For reference here is list of changes between individual outputs from: > > > > cpp -x c $FILE -Wp,-w -undef -P -Imingw-w64-crt/def-include -D$PLAT | sed > > -E 's/\s*;.*//' | LC_ALL=C sort -u > > > > diff --git a/mingw-w64-crt/def-include/msvcrt-common.def.in > > b/mingw-w64-crt/def-include/msvcrt-common.def.in > > index 3e2c674d3699..26aa13e6b661 100644 > > --- a/mingw-w64-crt/def-include/msvcrt-common.def.in > > +++ b/mingw-w64-crt/def-include/msvcrt-common.def.in > > @@ -119,6 +119,9 @@ ADD_UNDERSCORE(ungetch) > > ADD_UNDERSCORE(unlink) > > #ifndef UCRTBASE > > ADD_UNDERSCORE(utime) > > +#else > > +F32(utime == _utime32) > > +F64(utime == _utime64) > > #endif > > ADD_UNDERSCORE(wcsdup) > > ADD_UNDERSCORE(wcsicmp) > > @@ -197,6 +200,160 @@ _strtoimax_l == _strtoi64_l > > _strtoumax_l == _strtoui64_l > > #endif > > > > +; This is list of find symbol aliases, every CRT library has either find > > symbols with SIZE suffix or without them > > +#ifdef FIXED_SIZE_SYMBOLS > > +F32(_findfirst32 == _findfirst) > > +F64(_findfirst64i32 == _findfirst) > > +#ifndef NO_I64_FIXED_SIZE > > +F32(_findfirst32i64 == _findfirsti64) > > +#ifndef NO_FIXED_SIZE_64_ALIAS > > +F64(_findfirst64 == _findfirsti64) > > I think this patch looks reasonable; the diff of the def files looks good. I > didn't try to follow this macro soup in detail, but I guess it's reasonable > as long as the output is correct. > > Overall, I'm ok with this patch set now, thanks! (I just had a few minor > nits about the commit messages.) I think that you and Liu Hao can write better commit message. So feel free to modify them. I think it is easier to do when applying changes to git, than resending whole patch series with just modification of commit messages. > I'll leave the naming discussion regarding > "alias"/redirect to Liu Hao. (Also, note the name "alias" exists in multiple > commit messages, and also in macros in this patch.) In this patch series I used the word "alias" to describe procedure which adds a _new_ import symbol which points to another _old_ import symbol name (that is already present in the def file). For me this is aliasing as _new_ alias to _old_, and both _new_ and _old_ names are present in import library (both names can be used by application). But if you have different opinion how word "alias" should be used and if it does not match what is being done in this patch series, then fill free to choose other word or change commit messages. I'm fine with whatever you prefer. > I'll be away for the rest of the week, so unless someone else wants to apply > it once the naming discussion has been settled, I can probably get to it > sometime next week. > > (Also, if someone else wants to apply it, note that there are changes to > Makefile.am that need regenerating Makefile.in after each change.) > > // Martin ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 3/7] crt: msvcr120 and UCRT: Fix X64 _(w)findfirst and _(w)findnext symbol aliases
On Tuesday 30 April 2024 11:57:02 Martin Storsjö wrote: > On Sat, 27 Apr 2024, Pali Rohár wrote: > > > These symbols on X64 should resolve to _findfirst64i32/_findnext64i32 > > functions, like in other CRT libraries and header files. > > It could be worth mentioning in the commit message, that the 32 bit versions > of these aliases or redirects, are intentionally left as they were, even if > we at this point know its wrong, as we are about to change it in a later > patch soon. (Leaving things untouched is one thing, but here we're adding > new F32() wrappers, even if they are kept pointing at the wrong function.) > > // Martin Yes, Adding sentence like that should really help. ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 6/7] crt: UCRT: Change I386 time functions without suffix to use 32-bit time_t
On Tuesday 30 April 2024 11:54:48 Martin Storsjö wrote: > On Sat, 27 Apr 2024, Pali Rohár wrote: > > > CRT header files ensures that time symbols without 32/64 suffixes are not > > emitted. And linker always sees time symbols with explicit 32 or 64 suffix > > name. > > > > When CRT header files are not included then 32-bit MSVC compiler + linker > > treats symbols without "64" suffix name as functions which use 32-bit > > time_t, even for UCRT builds. > > > > Do some in mingw-w64, change I386 time symbol aliases which do not have > > "64" in symbol name, to point to symbols which use 32-bit time_t type. > > Typo in this sentence, I presume you mean "Do the same" or something like > that? Yes. > Another nitpick for the commit messages in this whole series; the commit > messages talk about I386 and X64, while it actually is generic for 32 and 64 > bit, as we do support armv7 and aarch64 too. So in order to avoid confusion, > especially as there actually are lots of cases that are i386 specific too, > but not these, it would be good to refer more generically to this as 32/64 > bit in these commit messages. I see, that is truth. Anyway, feel free to modify commit messages. I have no problem with it. And I would be happy if somebody else can improve them. ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 7/7] crt: Move find, stat and time aliases to def-include/msvcrt-common.def.in
On Sat, 27 Apr 2024, Pali Rohár wrote: Add 4 new macros FIXED_SIZE_SYMBOLS, NO_I64_FIXED_SIZE, NO_FIXED_SIZE_64_ALIAS and NO_TIME_ALIAS to distinguish between different crt versions. This change adds new symbol aliases which were missing. There is no symbol change or removal. For reference here is list of changes between individual outputs from: cpp -x c $FILE -Wp,-w -undef -P -Imingw-w64-crt/def-include -D$PLAT | sed -E 's/\s*;.*//' | LC_ALL=C sort -u diff --git a/mingw-w64-crt/def-include/msvcrt-common.def.in b/mingw-w64-crt/def-include/msvcrt-common.def.in index 3e2c674d3699..26aa13e6b661 100644 --- a/mingw-w64-crt/def-include/msvcrt-common.def.in +++ b/mingw-w64-crt/def-include/msvcrt-common.def.in @@ -119,6 +119,9 @@ ADD_UNDERSCORE(ungetch) ADD_UNDERSCORE(unlink) #ifndef UCRTBASE ADD_UNDERSCORE(utime) +#else +F32(utime == _utime32) +F64(utime == _utime64) #endif ADD_UNDERSCORE(wcsdup) ADD_UNDERSCORE(wcsicmp) @@ -197,6 +200,160 @@ _strtoimax_l == _strtoi64_l _strtoumax_l == _strtoui64_l #endif +; This is list of find symbol aliases, every CRT library has either find symbols with SIZE suffix or without them +#ifdef FIXED_SIZE_SYMBOLS +F32(_findfirst32 == _findfirst) +F64(_findfirst64i32 == _findfirst) +#ifndef NO_I64_FIXED_SIZE +F32(_findfirst32i64 == _findfirsti64) +#ifndef NO_FIXED_SIZE_64_ALIAS +F64(_findfirst64 == _findfirsti64) I think this patch looks reasonable; the diff of the def files looks good. I didn't try to follow this macro soup in detail, but I guess it's reasonable as long as the output is correct. Overall, I'm ok with this patch set now, thanks! (I just had a few minor nits about the commit messages.) I'll leave the naming discussion regarding "alias"/redirect to Liu Hao. (Also, note the name "alias" exists in multiple commit messages, and also in macros in this patch.) I'll be away for the rest of the week, so unless someone else wants to apply it once the naming discussion has been settled, I can probably get to it sometime next week. (Also, if someone else wants to apply it, note that there are changes to Makefile.am that need regenerating Makefile.in after each change.) // Martin ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 3/7] crt: msvcr120 and UCRT: Fix X64 _(w)findfirst and _(w)findnext symbol aliases
On Sat, 27 Apr 2024, Pali Rohár wrote: These symbols on X64 should resolve to _findfirst64i32/_findnext64i32 functions, like in other CRT libraries and header files. It could be worth mentioning in the commit message, that the 32 bit versions of these aliases or redirects, are intentionally left as they were, even if we at this point know its wrong, as we are about to change it in a later patch soon. (Leaving things untouched is one thing, but here we're adding new F32() wrappers, even if they are kept pointing at the wrong function.) // Martin ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 6/7] crt: UCRT: Change I386 time functions without suffix to use 32-bit time_t
On Sat, 27 Apr 2024, Pali Rohár wrote: CRT header files ensures that time symbols without 32/64 suffixes are not emitted. And linker always sees time symbols with explicit 32 or 64 suffix name. When CRT header files are not included then 32-bit MSVC compiler + linker treats symbols without "64" suffix name as functions which use 32-bit time_t, even for UCRT builds. Do some in mingw-w64, change I386 time symbol aliases which do not have "64" in symbol name, to point to symbols which use 32-bit time_t type. Typo in this sentence, I presume you mean "Do the same" or something like that? Another nitpick for the commit messages in this whole series; the commit messages talk about I386 and X64, while it actually is generic for 32 and 64 bit, as we do support armv7 and aarch64 too. So in order to avoid confusion, especially as there actually are lots of cases that are i386 specific too, but not these, it would be good to refer more generically to this as 32/64 bit in these commit messages. // Martin ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
