On Tuesday 12 November 2024 12:30:08 Martin Storsjö wrote:
> On Thu, 24 Oct 2024, Pali Rohár wrote:
>
> > > While debugging this, I also noted that libc++ fails to build (when using
> > > __USE_MINGW_ANSI_STDIO=1) after patch "crt: Add support for C95
> > > (v)swprintf() functions when __USE_MINGW_ANSI_STDIO=0", because there's
> > > only
> > > a 2 parameter version of swprintf, but it starts working again after patch
> > > working again after "crt: Add support for C95 (v)swprintf() functions when
> > > __USE_MINGW_ANSI_STDIO=1". Ideally, we wouldn't have this kind of
> > > temporary
> > > regression in a patch series, as it makes it harder to check for issues
> > > with
> > > it.
> >
> > I separated logical changes into more separates patches. Just because it
> > is already big and smaller changes are easier for review. If needed you
> > can squash particular changes into bigger when committing into git.
>
> Right... Perhaps it's ok to keep those separated like that, as long as the
> inconsistent period in the branch is kept short.
>
> > > With that typo fixed, the branch passes all of my CI tests I had set up
> > > before, but I'd still like to extend my own testing for these functions
> > > before giving the branch my blessing.
>
>
> Now I've finally taken the time to extend my own tests to cover these
> functions (primarily the wcstok and swprintf families, I didn't add tests
> for fwgets and those functions). I did find a couple of issues.
Thank you for doing it.
> 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.
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.
>
> 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.
> - 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. Anyway this comment should apply for
msvcr80-msvcr120. Not for msvcrt and neither for ucrt.
> - 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.
> - 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.
Maybe we should rather test that swprintf returns negative value in this
case and skip this test case for MSVC?
> Other than that, this looks quite good, and it's good to get these functions
> consistent across CRT versions. (I just recently tried to run the libcxx
> testsuite with msvcrt.dll builds, and ran into an issue with wcstok, which I
> presume gets fixed by this patchset.)
>
> // Martin
Yes, wcstok should be fixed with this patch series, and be compliant
with C and C++ standards.
diff --git a/mingw-w64-crt/Makefile.am b/mingw-w64-crt/Makefile.am
index 4959e04c7d9d..253554d60fef 100644
--- a/mingw-w64-crt/Makefile.am
+++ b/mingw-w64-crt/Makefile.am
@@ -172,9 +172,11 @@ src_msvcrt_common=\
stdio/acrt_iob_func.c \
stdio/strtof.c \
stdio/snprintf_alias.c \
+ stdio/snwprintf_alias.c \
stdio/swprintf.c \
stdio/swprintf_alias.c \
stdio/vsnprintf_alias.c \
+ stdio/vsnwprintf_alias.c \
stdio/vswprintf.c \
stdio/vswprintf_alias.c \
math/cbrt.c math/cbrtf.c \
diff --git a/mingw-w64-crt/stdio/snwprintf_alias.c b/mingw-w64-crt/stdio/snwprintf_alias.c
new file mode 100644
index 000000000000..4027699d352d
--- /dev/null
+++ b/mingw-w64-crt/stdio/snwprintf_alias.c
@@ -0,0 +1,26 @@
+/**
+ * This file has no copyright assigned and is placed in the Public Domain.
+ * This file is part of the mingw-w64 runtime package.
+ * No warranty is given; refer to the file DISCLAIMER.PD within this package.
+ */
+
+#include <stdarg.h>
+#include <stddef.h>
+
+/* Intentionally not including stdio.h, as it unconditionally defines the
+ * swprintf inline, and it can't be renamed with "#define snwprintf othername"
+ * either, as stdio.h contains "#undef snwprintf". */
+
+int __cdecl __ms_vsnwprintf(wchar_t *buffer, size_t n, const wchar_t *format, va_list arg);
+
+int __cdecl snwprintf(wchar_t *buffer, size_t n, const wchar_t *format, ...);
+int __cdecl snwprintf(wchar_t *buffer, size_t n, const wchar_t *format, ...)
+{
+ int retval;
+ va_list argptr;
+
+ va_start(argptr, format);
+ retval = __ms_vsnwprintf(buffer, n, format, argptr);
+ va_end(argptr);
+ return retval;
+}
diff --git a/mingw-w64-crt/stdio/vsnwprintf_alias.c b/mingw-w64-crt/stdio/vsnwprintf_alias.c
new file mode 100644
index 000000000000..00db0dc9e54a
--- /dev/null
+++ b/mingw-w64-crt/stdio/vsnwprintf_alias.c
@@ -0,0 +1,20 @@
+/**
+ * This file has no copyright assigned and is placed in the Public Domain.
+ * This file is part of the mingw-w64 runtime package.
+ * No warranty is given; refer to the file DISCLAIMER.PD within this package.
+ */
+
+#include <stdarg.h>
+#include <stddef.h>
+
+/* Intentionally not including stdio.h, as it unconditionally defines the
+ * vswprintf inline, and it can't be renamed with "#define vsnwprintf othername"
+ * either, as stdio.h contains "#undef vsnwprintf". */
+
+int __cdecl __ms_vsnwprintf(wchar_t *buffer, size_t n, const wchar_t *format, va_list argptr);
+
+int __cdecl vsnwprintf(wchar_t *buffer, size_t n, const wchar_t *format, va_list argptr);
+int __cdecl vsnwprintf(wchar_t *buffer, size_t n, const wchar_t *format, va_list argptr)
+{
+ return __ms_vsnwprintf(buffer, n, format, argptr);
+}
diff --git a/mingw-w64-headers/crt/stdio.h b/mingw-w64-headers/crt/stdio.h
index f33009a0f593..f93d12e40ade 100644
--- a/mingw-w64-headers/crt/stdio.h
+++ b/mingw-w64-headers/crt/stdio.h
@@ -1265,8 +1265,8 @@ int vsnwprintf (wchar_t *__stream, size_t __n, const wchar_t *__format, __builti
#if __USE_MINGW_ANSI_STDIO == 0
int __cdecl __ms_snwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, ...);
int __cdecl __ms_vsnwprintf (wchar_t * __restrict__ , size_t, const wchar_t * __restrict__ , va_list);
- int __cdecl snwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, ...);
- int __cdecl vsnwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, va_list arg);
+ int __cdecl snwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, ...) __MINGW_ASM_CALL(__ms_snwprintf);
+ int __cdecl vsnwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, va_list arg) __MINGW_ASM_CALL(__ms_vsnwprintf);
#endif
#endif /* ! __NO_ISOCEXT */
diff --git a/mingw-w64-headers/crt/wchar.h b/mingw-w64-headers/crt/wchar.h
index f141808731f7..cabe4f9ff39d 100644
--- a/mingw-w64-headers/crt/wchar.h
+++ b/mingw-w64-headers/crt/wchar.h
@@ -765,8 +765,8 @@ __MINGW_ASM_CALL(__mingw_vsnwprintf);
# undef vsnwprintf
int __cdecl __ms_snwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, ...);
int __cdecl __ms_vsnwprintf (wchar_t * __restrict__ , size_t, const wchar_t * __restrict__ , va_list);
- int __cdecl snwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, ...);
- int __cdecl vsnwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, va_list arg);
+ int __cdecl snwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, ...) __MINGW_ASM_CALL(__ms_snwprintf);
+ int __cdecl vsnwprintf (wchar_t * __restrict__ s, size_t n, const wchar_t * __restrict__ format, va_list arg) __MINGW_ASM_CALL(__ms_vsnwprintf);
#pragma pop_macro ("vsnwprintf")
#pragma pop_macro ("snwprintf")
#endif
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
