On Thursday 25 April 2024 23:41:24 Martin Storsjö wrote: > On Thu, 25 Apr 2024, Pali Rohár wrote: > > > On Wednesday 24 April 2024 23:41:24 Martin Storsjö wrote: > > > On Mon, 22 Apr 2024, Pali Rohár wrote: > > > > > > > diff --git a/mingw-w64-crt/lib-common/ucrtbase.def.in > > > > b/mingw-w64-crt/lib-common/ucrtbase.def.in > > > > index 2d0552f277ef..c8fb1e33b1d7 100644 > > > > --- a/mingw-w64-crt/lib-common/ucrtbase.def.in > > > > +++ b/mingw-w64-crt/lib-common/ucrtbase.def.in > > > > @@ -327,12 +327,10 @@ _filelength > > > > _filelengthi64 > > > > _fileno > > > > _findclose > > > > -_findfirst == _findfirst64 > > > > > > Note how we unconditionally had these macros pointing at _findfirst64 in > > > ucrt, regardless of architecture. > > > > I must admit that this is an interesting issue. > > > > I looked at the mingw-w64-headers/crt/io.h file now and it is not set > > unconditionally as you wrote. > > I didn't say that the headers unconditionally uses it this way. I just said > that the exist, most probably buggy, def file line right above what I > quoted, unconditionally does "_findfirst == _findfirst64" for all > architectures. > > Yes it's probably wrong - but we shouldn't be changing it in a refactoring > patch, that's all I tried to say here. > > > Anyway, this def file defines symbol alias "_findfirst" for linking. And > > every c/c++ source file which includes headers files would result in > > object file which will use either "_findfirst32" or "_findfirst64i32". > > Not "_findfirst" symbol. So in my opinion, this change should be safe. > > It may very well be safe in practice, but I don't want such a subtle, > unannounced change buried in a patch that refactors many hundreds of lines, > where the rest mostly is a pure refactoring.
Yes, of course, I mean to put all such changes in separate patch. > > I think that meaning of the symbols should be same independently of the > > used CRT library. And for me this looks like a bug that UCRT was > > resolving _findfirst to different ABI function than msvcrt, at link > > time. > > I disagree here. UCRT has a different ABI than msvcrt, overall, with time_t > defaulting to 64 bit on UCRT on 32 bit platforms, contrary to msvcrt. ABI for ucrt and msvcrt is different, but what I see is that symbols which are in both libraries use same times. ucrt DLL libraries do not have _findfirst symbol. They have only symbols with suffixes (32, 64, 32i64 and 64i32). And those symbols with these suffixes are present in new msvcrt versions and have same meaning. As using "_findfirst" symbol directly (without any suffix) is not so common, and it is not available in UCRT at all, I would propose to define it to the same time_t bit length as in msvcrt. It would simplify symbol definitions as it would have always just one meaning (for 32-bit). And also it would allow to create object file which calls "_findfirst" symbol and then link it with any mingw-w64 CRT import library. I think it is better to have stable symbol meaning than to have symbol alias matching what the headers default to. Stable symbol meaning can be an invariant in this case. But symbol matching with header file is not stable as application can change it by -D_USE_32BIT_TIME_T. What do you think? Does it make sense? > I think the default symbol here should match what the headers default to, > when used in a UCRT configuration. > > > I guess that it was rare to hit this issue as it required to either not > > include mingw-w64 header files and declare functions manually. Or write > > application in other language which does not use mingw-w64 C header > > files (e.g. in assembly or something similar) but uses mingw-w64 CRT > > libraries for linking. > > Yes, this is not something that would be encountered often in practice, I > agree with that. > > > Please check what I wrote above. If you agree that this is a bug, I can > > prepare a separate fix for the UCRT (and then rebase 10/10 on top of > > it). > > I agree that the current state probably is a bug. > > As the headers default to 64 bit time_t, then _findfirst should probably > resolve to _findfirst64i32, for both 32 and 64 bit. Similarly for the other > symbols. > > That's most probably my original intent in how I set these up - that we > point at the "64 bit version", but I missed that it really is > _findfirst64i32 and not _findfirst64. > > I'd appreciate a patch that fixes these aliases, consistently for > ucrtbase.def.in and the api-ms-win-crt-*.def.in, and then we can rebase > patch 10/10 on top of it, where I'd expect no changes in the preprocessed > output of the ucrt def files. Yes, I agree. > > Ou, and another files for which I forget api-ms-win-crt-*.def.in. And > > this is because they do not include that common .def.in file. > > > > I can look at them, but I would prefer to do it after all existing stuff > > is resolved as basically api-ms-win-crt-*.def.in does not use any common > > aliases. > > > > In my opinion, it would be better to change *crt*.def.in and > > api-ms-win-crt-*.def.in files to use common alias (would need refactor). > > Yes, that would indeed require a bit of refactoring. I'm not requesting you > do it, but if you feel you want to take it on, feel free. > > Just make sure that the aliases in ucrtbase.def.in (or transitively from > msvcrt-common.def.in) match api-ms-win-crt-*.def.in. Ok. _______________________________________________ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
