On Tuesday 12 November 2024 23:13:28 Martin Storsjö wrote:
> On Tue, 12 Nov 2024, Pali Rohár wrote:
>
> > > See https://github.com/mstorsjo/llvm-mingw/commits/c95-test for the added
> > > tests. At the end, I have two "WIP: test: Waive ..." commits that makes
> > > the
> > > tests pass, covering up the issues that I found.
> > >
> > > These tests are in the form of one large file, test/crt-test.c, which can
> > > be
> > > compiled with MSVC (and with unix compilers as well, to crosscheck CRT
> > > behaviours there).
> >
> > I have just two comments. Otherwise everything looks good.
> >
> > You wrote this code:
> >
> > #ifndef __GLIBC__
> > // On overflow, glibc doesn't seem to null terminate the array.
> > TEST_INT(wbuf2[ARRAYLEN(wbuf2)-1], '\0');
> > #endif
> >
> > This is known glibc issue and was fixed just year ago in version 2.37:
> > https://sourceware.org/bugzilla/show_bug.cgi?id=27857
> >
> > You could extend that check with __GLIBC__ and __GLIBC_MINOR__
> > preprocessor macros to run that test on 2.37+ glibc versions too.
>
> Oh, thanks for the reference! Yeah this felt like a big omission.
>
> I'll see if I add such a check, or if I just keep it simple.
>
> > And I spotted that for vsnwprintf test you have comment:
> > // On overflow, snwprintf null terminates.
> >
> > There is missing leading "v" in comment for function name.
>
> Thanks, will fix.
>
> > > The issues I ran into were the following:
> > >
> > > - In C++ mode, we have an inline overload of wcstok() with 2 parameters.
> > > However, this doesn't work when compiling in -O0 mode; in this mode, the
> > > compiler predefines __NO_INLINE__, which makes our headers set
> > > __CRT__NO_INLINE and skip inline functions. This maybe is reasonable for
> > > performance/code size cases, but here, it affects what code is valid and
> > > can
> > > be built or not. So I wonder if we should skip the check for
> > > __CRT__NO_INLINE for the 2 parameter C++ overload of wcstok?
> >
> > I think it makes sense. The code is C++-only with extern "C++" so we
> > should drop "!defined(__CRT__NO_INLINE)" check and maybe also changing
> > __CRT_INLINE directly to "inline". So it always use C++ inline semantics
> > to prevent linking problems.
>
> Thanks, this sounds reasonable to me.
Ok, I will include it into next batch.
> > > - The commit message explanation of _swprintf_c seems weird. The commit
> > > message says:
> > >
> > > _(v)swprintf_c - clears the first wide character in buffer on error
> > > (error means that there is no space for nul-term or
> > > conversion error occurred)
> > >
> > > However I don't see that behaviour (clearing the _first_ char in the
> > > buffer
> > > on error sounds odd - then the whole buffer would be an empty string?).
> > > Can
> > > you recheck this explanation? In any case, this function seemed fringe
> > > enough that I didn't add a test for it.
> >
> > Ok. I will recheck it. IIRC I tried to decode header files, and I'm not
> > sure if I still have test case for this function, as this function
> > something not very widely used.
>
> Indeed, it seems very rare.
Yes, it is rare and I have included it into commit message just because
of completeness and to explain why cannot use it as alias for the
compliant swprintf.
> > Anyway this comment should apply for msvcr80-msvcr120. Not for msvcrt
> > and neither for ucrt.
>
> Ah, that would probably explain it. I think I tested the behaviour of it
> with MSVC with UCRT possibly, or just the mingw behaviour with msvcrt/ucrt.
>
> In any case, the function is such a rare one that I chose to skip testing of
> it.
I wrote results in previous email. UCRT has really different behavior
than msvcr*.
> > > - The functions snwprintf and vswnprintf can't be found, when linking
> > > against msvcrt, with -D__USE_MINGW_ANSI_STDIO=0. There's a patch "crt: Fix
> > > non-ISO (v)snwprintf() functions when __USE_MINGW_ANSI_STDIO=0" in this
> > > patchset, but despite this, I seem to be running into this issue.
> >
> > I see. I forgot to add __MINGW_ASM_CALL into header file for this case.
> > And also I forgot to include aliases for import symbols (in case
> > somebody does not use header file with __MINGW_ASM_CALL). So missing of
> > both these two parts caused linking error.
> >
> > In attachment I'm sending fixup for this issue.
>
> Thanks, this does seem to work fine for these tests!
>
> > > - I noted that MSVC (v)swprintf returns the required buffer length, if
> > > called with a NULL buffer and 0 buffer size, so I added a test for it.
> > > But I
> > > didn't get this behaviour on either Glibc, Musl or Darwin, so I guess it's
> > > not necessary to test this case and care about it?
> >
> > IMHO, this MSVC behavior is a bug and not compliant with C95+
> > specifications.
> >
> > N1570 ISO/IEC 9899:201x (C11) specification for swprintf in 7.29.2.3 says:
> >
> > "The swprintf function returns the number of wide characters written in
> > the array, not counting the terminating null wide character, or a
> > negative value if an encoding error occurred or if n or more wide
> > characters were requested to be written."
> >
> > My understanding is that when swprintf is called with N=0 then more wide
> > characters are requested to be written (because at least nul-term wide
> > has to be written) and so negative value has to be returned.
>
> Ah, right.
>
> > Maybe we should rather test that swprintf returns negative value in this
> > case and skip this test case for MSVC?
>
> I think I'll just skip this test altogether, as we get the wrong result with
> UCRT in mingw as well, and with msvcrt when not using
> __USE_MINGW_ANSI_STDIO.
That is bad if we are getting wrong/different results.
I looked at it and it is a real bug which you found. It is in the file:
mingw-w64-crt/stdio/vswprintf.c
int __cdecl __ms_vswprintf(wchar_t *__restrict__ ws, size_t n, const wchar_t
*__restrict__ format, va_list arg)
{
int retval;
retval = _vsnwprintf(ws, n-1, format, arg);
When n=0 then _vsnwprintf is called with MAX_SIZE_T value.
Now I checked that _vsnwprintf() in msvcr* returns -1 when n=0.
I'm going to apply following change to fix n-1 underflow:
diff --git a/mingw-w64-crt/stdio/vswprintf.c b/mingw-w64-crt/stdio/vswprintf.c
index f7bdc3cdc897..7edc52050711 100644
--- a/mingw-w64-crt/stdio/vswprintf.c
+++ b/mingw-w64-crt/stdio/vswprintf.c
@@ -11,6 +11,9 @@ int __cdecl __ms_vswprintf(wchar_t *__restrict__ ws, size_t
n, const wchar_t *__
{
int retval;
+ if (n == 0)
+ return -1;
+
retval = _vsnwprintf(ws, n-1, format, arg);
/* _vsnwprintf() does not fill trailing nul wide char if there is not
place for it */
It is fine?
I think that the test for this n=0 case is really useful as it already
found a bug in my new code.
> With all these fixes in place, I think this patchset starts to look good.
> Can you post a rebased version (there's a bit of conflicts with the recent
> refactoring in Makefile.am) and with all the assorted fixes squashed in the
> right places? I can then run it through all my tests and see if we can hope
> to land this soon.
>
>
> // Martin
Yes, I will send a new rebased version with all fixups applied.
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
