On Saturday 30 November 2024 00:46:09 Martin Storsjö wrote: > On Fri, 29 Nov 2024, Martin Storsjö wrote: > > > On Tue, 19 Nov 2024, Pali Rohár wrote: > > > > > C99 vscanf-family functions are natively available since msvcr120. > > > > > > This change allows to use native UCRT and msvcr120 C99 vscanf-family > > > functions. > > > > I think this commit message would require a much longer explanation > > about the status quo of these functions - I didn't quite remember what > > the situation was, and I felt surprised to read that we didn't use the > > native vscanf family of functions in the UCRT. > > > > From a brief digging around, I see that for e.g. vsscanf, we have: > > - stdio/vsscanf2.S, providing vsscanf, redirecting to __ms_vsscanf > > - stdio/vsscanf.c, providing __ms_vsscanf, calling __argtos > > - stdio/scanf.S, providing __argtos, which converts from a va_list back > > to parameters on the stack, and calls sscanf() > > > > And this, because older msvcr*.dll doesn't provide v*scanf functions at > > all. The stdio/scanf.S file has got quite good comments explaining parts > > of it. But some parts of the comments there feel inaccurate (like "This > > is needed because mingw-w64 uses msvcr100.dll" - that's not really the > > default anywhere), so a patch to fix up inaccuracies in the comments > > there would be nice.
That is good catch. That comment does not make sense. mingw-w64 was using msvcrt.dll before UCRT. Not the msvcr100.dll. What about updating comment to something like this? "This is needed for pre-msvcr120 CRT libraries, which do not support the v*scanf functions." > > And now we move this to the older msvcr* import libraries, because we > > actually do have proper v*scanf in msvcr120 and UCRT. > > > > So this seems reasonable, but the commit message needs an explanation > > that explanins this, as it's not at all very clear. > > > > (The surprise about this situation was what converted my reaction on > > this patchset from "seems straightforward and probably good", to "wait, > > what? I need to look into that"; having more such explanations in the > > commit messages makes it easier to reason about the patch.) > > > > Can you try to write that up? Otherwise I can try to have a look at > > writing it tomorrow. > > I rewrote this commit message into this bit: > > crt: Move vscanf-family functions from libmingwex.a to individual CRT > import libraries > > msvcr110 and earlier did not have any v*scanf functions. Because of > that, the mingw-w64-crt contained wrappers that implement the > v*scanf functions based on the non-variadic *scanf functions, by > reading the va_list object and re-passing them as regular parameters > to the *scanf functions. > > The sequence involved for vsscanf is: > - stdio/vsscanf2.S, providing vsscanf, calling __ms_vsscanf > - stdio/vsscanf.c, providing __ms_vsscanf, calling __argtos > - stdio/scanf.S, providing __argtos, which converts from a > va_list back to regular parameters, and calls sscanf() > > These functions were provided in libmingwex, making them available > and used with all CRT versions. > > However, since msvcr120 (and in UCRT), the proper v*scanf functions > are available. Therefore, move this set of wrappers into the > individual CRT import libraries, allowing using the proper v*scanf > functions on msvcr120 and UCRT. > > > Does that sound ok? > > // Martin Thank you very much for looking at this. Your commit message is much better than my very short description. So for sure, it sounds good. _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
