Re: [Mingw-w64-public] [PATCH] crt: Fix v*scanf functions
On Wednesday 26 January 2022 14:45:58 LIU Hao wrote: > 在 1/26/22 04:05, Pali Rohár 写道: > > > > Yes, this is truth. However amd64 asm code already reads more pointers > > from stack (when number of them is smaller then number of arguments > > needed to pass via registers when doing function call), so I think that > > this function can be documented that returns "minimal number of > > arguments". > > > > So it is really an issue? > > > > Or should be function extended to skip all characters between [ and ]? > > For the sake of simplicity, I think it should be acceptable to return an > upper limit, so I suggest we rename these functions to > `__ms_scanf_max_arg_count_internal` etc. This also eliminates the need to > check for `%*`; we only need to count `%` characters and assume there can be > no more arguments than them. Ok, I done it! I sent a new v2 patch version where I tried to address all review comments. ___ 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] crt: Fix v*scanf functions
在 1/26/22 04:05, Pali Rohár 写道: Yes, this is truth. However amd64 asm code already reads more pointers from stack (when number of them is smaller then number of arguments needed to pass via registers when doing function call), so I think that this function can be documented that returns "minimal number of arguments". So it is really an issue? Or should be function extended to skip all characters between [ and ]? For the sake of simplicity, I think it should be acceptable to return an upper limit, so I suggest we rename these functions to `__ms_scanf_max_arg_count_internal` etc. This also eliminates the need to check for `%*`; we only need to count `%` characters and assume there can be no more arguments than them. -- Best regards, LIU Hao OpenPGP_signature Description: OpenPGP digital signature ___ 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] crt: Fix v*scanf functions
On Wednesday 26 January 2022 00:23:49 LIU Hao wrote:
> 在 2022-01-16 22:44, Pali Rohár 写道:
> > diff --git a/mingw-w64-crt/stdio/scanf2-argcount-template.c
> > b/mingw-w64-crt/stdio/scanf2-argcount-template.c
> > new file mode 100644
> > index ..c07e11797c39
> > --- /dev/null
> > +++ b/mingw-w64-crt/stdio/scanf2-argcount-template.c
> > @@ -0,0 +1,22 @@
> > +/**
> > + * 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
> > +
> > +size_t FUNC(const TYPE *format);
> > +size_t FUNC(const TYPE *format)
> > +{
> > + size_t count = 0;
> > + for (; *format; format++) {
> > +if (*format != (TYPE)'%')
> > + continue;
> > +format++;
> > +if (*format == (TYPE)'%' || *format == (TYPE)'*')
> > + continue;
> > +count++;
> > + }
> > + return count;
> > +}
> >
>
> I am afraid there are two issues about this function:
>
> One is that it may read pass the end of the format string if it contains a
> stray `%` at the end (which however is undefined behavior) but I don't think
> it is a good idea to not handle it properly.
Ok. This can be fixed pretty easily.
> The other is that `%` does not necessarily start an argument specifier. For
> example `"abc%[01%]"` contains only one specifier for one argument, and this
> function produces a wrong result.
Yes, this is truth. However amd64 asm code already reads more pointers
from stack (when number of them is smaller then number of arguments
needed to pass via registers when doing function call), so I think that
this function can be documented that returns "minimal number of
arguments".
So it is really an issue?
Or should be function extended to skip all characters between [ and ]?
___
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] crt: Fix v*scanf functions
在 2022-01-16 22:44, Pali Rohár 写道:
diff --git a/mingw-w64-crt/stdio/scanf2-argcount-template.c
b/mingw-w64-crt/stdio/scanf2-argcount-template.c
new file mode 100644
index ..c07e11797c39
--- /dev/null
+++ b/mingw-w64-crt/stdio/scanf2-argcount-template.c
@@ -0,0 +1,22 @@
+/**
+ * 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
+
+size_t FUNC(const TYPE *format);
+size_t FUNC(const TYPE *format)
+{
+ size_t count = 0;
+ for (; *format; format++) {
+if (*format != (TYPE)'%')
+ continue;
+format++;
+if (*format == (TYPE)'%' || *format == (TYPE)'*')
+ continue;
+count++;
+ }
+ return count;
+}
I am afraid there are two issues about this function:
One is that it may read pass the end of the format string if it contains a stray `%` at the end
(which however is undefined behavior) but I don't think it is a good idea to not handle it properly.
The other is that `%` does not necessarily start an argument specifier. For example `"abc%[01%]"`
contains only one specifier for one argument, and this function produces a wrong result.
--
Best regards,
LIU Hao
OpenPGP_signature
Description: OpenPGP digital signature
___
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] crt: Fix v*scanf functions
On Monday 24 January 2022 13:41:27 Martin Storsjö wrote:
> On Sun, 16 Jan 2022, Pali Rohár wrote:
>
> > Currently v*scanf functions are broken and crash when are called with more
> > than 30 arguments in va_list. This is because va_list v*scanf functions are
> > redirected to variadic *scanf functions and this redirect implemented in
> > scanf.S file has fixed limit for 30 arguments.
> >
> > Number of arguments for msvcrt *scanf function can be determined from
> > format string by counting number of '%' characters which are not followed
> > by another '%' or '*'. Every scanf parameter is pointer and therefore has
> > fixed size which means that required stack size can be exactly calculated.
> >
> > Fix this scanf.S redirect implementation by dynamically allocating stack
> > for exact number of pointer parameters.
> >
> > ---
> >
> > I have tested this patch for i686 and x86_64. Both ARM (arm32 and aarch64)
> > changes are untested, so please test it if vsscanf() on these platforms
> > still works.
> >
> > With this patch following code works fine without any crashing.
> > Compile for msvcrt with -std=c89 or -D__USE_MINGW_ANSI_STDIO=0
> >
> > #include
> > #include
> >
> > int call_vsscanf(const char *str, const char *format, ...)
> > {
> >int ret;
> >va_list ap;
> >va_start(ap, format);
> >ret = vsscanf(str, format, ap);
> >va_end(ap);
> >return ret;
> > }
> >
> > int main()
> > {
> >char b[53];
> >call_vsscanf(
> > "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ",
> > "%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c"
> > "%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c",
> >
> > [0],[1],[2],[3],[4],[5],[6],[7],[8],[9],[10],[11],
> > [12],[13],[14],[15],[16],[17],[18],[19],[20],[21],
> > [22],[23],[24],[25],[26],[27],[28],[29],[30],[31],
> > [32],[33],[34],[35],[36],[37],[38],[39],[40],[41],
> > [42],[43],[44],[45],[46],[47],[48],[49],[50],[51]
> >);
> >printf("b=%s\n", b);
> >return 0;
> > }
>
> For the testcase, it might be good to test with an odd number of arguments.
Hello! That is a good point!
> I didn't read the x86 assembly changes in very close detail, but assuming it
> mirrors the arm assembly change attempts, it probably looks reasonable.
>
> I see that the code now tries to avoid loading the first couple arguments
> (that are moved to register arguments) if the number of parameters is lower
> than that. I think that it might be overkill (the arg pointer probably can't
> be null anyway, and there should be enough space on the stack to load a
> couple extra arguments), but I don't mind keeping it if you feel strongly
> about it.
Well, if you think that it is OK to always copy first pointers from
stack to registers then I'm fine with it-
> But it seems like the it doesn't try to align the stack as it's supposed to
> (it's losing a comment that looks like this):
>
> -/* The max number of pointers we support. Must be an even number
> - to keep the 64bit stack 16byte aligned. Must not be less than 4. */
That is a good point. x86_64 asm code should be fixed for 16byte
aligning.
> To make the code work for arm, I had to make some adjustments - see the
> attached patch. (Some adjustments, like doing 'subs' before 'str', is to
> reduce the latency between the 'str' and 'ldr', and likewise between 'subs'
> and the conditional branch - I guess I could skip that change too, to keep
> this diff simpler.)
>
> // Martin
I'm OK with this change.
___
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] crt: Fix v*scanf functions
On Sun, 16 Jan 2022, Pali Rohár wrote:
Currently v*scanf functions are broken and crash when are called with more
than 30 arguments in va_list. This is because va_list v*scanf functions are
redirected to variadic *scanf functions and this redirect implemented in
scanf.S file has fixed limit for 30 arguments.
Number of arguments for msvcrt *scanf function can be determined from
format string by counting number of '%' characters which are not followed
by another '%' or '*'. Every scanf parameter is pointer and therefore has
fixed size which means that required stack size can be exactly calculated.
Fix this scanf.S redirect implementation by dynamically allocating stack
for exact number of pointer parameters.
---
I have tested this patch for i686 and x86_64. Both ARM (arm32 and aarch64)
changes are untested, so please test it if vsscanf() on these platforms
still works.
With this patch following code works fine without any crashing.
Compile for msvcrt with -std=c89 or -D__USE_MINGW_ANSI_STDIO=0
#include
#include
int call_vsscanf(const char *str, const char *format, ...)
{
int ret;
va_list ap;
va_start(ap, format);
ret = vsscanf(str, format, ap);
va_end(ap);
return ret;
}
int main()
{
char b[53];
call_vsscanf(
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ",
"%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c"
"%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c",
[0],[1],[2],[3],[4],[5],[6],[7],[8],[9],[10],[11],
[12],[13],[14],[15],[16],[17],[18],[19],[20],[21],
[22],[23],[24],[25],[26],[27],[28],[29],[30],[31],
[32],[33],[34],[35],[36],[37],[38],[39],[40],[41],
[42],[43],[44],[45],[46],[47],[48],[49],[50],[51]
);
printf("b=%s\n", b);
return 0;
}
For the testcase, it might be good to test with an odd number of
arguments.
I didn't read the x86 assembly changes in very close detail, but assuming
it mirrors the arm assembly change attempts, it probably looks reasonable.
I see that the code now tries to avoid loading the first couple arguments
(that are moved to register arguments) if the number of parameters is
lower than that. I think that it might be overkill (the arg pointer
probably can't be null anyway, and there should be enough space on the
stack to load a couple extra arguments), but I don't mind keeping it if
you feel strongly about it.
But it seems like the it doesn't try to align the stack as it's supposed
to (it's losing a comment that looks like this):
-/* The max number of pointers we support. Must be an even number
- to keep the 64bit stack 16byte aligned. Must not be less than 4. */
To make the code work for arm, I had to make some adjustments - see the
attached patch. (Some adjustments, like doing 'subs' before 'str', is to
reduce the latency between the 'str' and 'ldr', and likewise between
'subs' and the conditional branch - I guess I could skip that change too,
to keep this diff simpler.)
// Martin
From 172df7002c76d7d490b6bf6d6ea6cc985369fecc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Mon, 24 Jan 2022 13:21:39 +0200
Subject: [PATCH] fixup: Fix stdio/scanf.S for arm/aarch64
---
mingw-w64-crt/stdio/scanf.S | 64 +
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/mingw-w64-crt/stdio/scanf.S b/mingw-w64-crt/stdio/scanf.S
index ae8090e4c..d9584b206 100644
--- a/mingw-w64-crt/stdio/scanf.S
+++ b/mingw-w64-crt/stdio/scanf.S
@@ -169,22 +169,27 @@ __argtos:
__argtos:
push{r4-r8, lr}
-ldr r12, [sp, #0]
+ldr r12, [sp, #24]
cmp r3, #0
-ldrne r5, [r2], #4
-subsne r3, r3, #1
-ldrne r6, [r2], #4
-subsne r3, r3, #1
-moveq r8, #0
-beq 2b
+beq 2f
+subsr3, r3, #1
+ldr r5, [r2], #4
+beq 2f
+subsr3, r3, #1
+ldr r6, [r2], #4
+beq 2f
+/* Round the number of entries to an even number, to maintain
+ * 8 byte stack alignment. */
mov r8, r3
-sub sp, sp, r8
+add r8, r8, #1
+bic r8, r8, #1
+sub sp, sp, r8, lsl #2
mov r4, sp
1: ldr r7, [r2], #4
-str r7, [r4], #4
subsr3, r3, #1
+str r7, [r4], #4
bne 1b
2:
@@ -192,7 +197,7 @@ __argtos:
mov r3, r6
blx r12
-add sp, sp, r8
+add sp, sp, r8, lsl #2
pop {r4-r8, pc}
#elif defined (__aarch64__)
@@ -208,38 +213,43 @@ __argtos:
mov x11, x3
mov x12, x4
-cmp r11, #0
-b.eq2b
+cmp x11, #0
+b.eq2f
-ldr x2, [x10], #8
subsx11, x11, #1
-b.eq2b
+ldr x2, [x10], #8
+b.eq2f
-ldr x3, [x10], #8
subsx11, x11, #1
-b.eq2b
+ldr x3, [x10], #8
+b.eq2f
-ldr x4, [x10], #8
subsx11, x11, #1
-b.eq2b
+ldr x4, [x10], #8
+b.eq2f
-
[Mingw-w64-public] [PATCH] crt: Fix v*scanf functions
Currently v*scanf functions are broken and crash when are called with more
than 30 arguments in va_list. This is because va_list v*scanf functions are
redirected to variadic *scanf functions and this redirect implemented in
scanf.S file has fixed limit for 30 arguments.
Number of arguments for msvcrt *scanf function can be determined from
format string by counting number of '%' characters which are not followed
by another '%' or '*'. Every scanf parameter is pointer and therefore has
fixed size which means that required stack size can be exactly calculated.
Fix this scanf.S redirect implementation by dynamically allocating stack
for exact number of pointer parameters.
---
I have tested this patch for i686 and x86_64. Both ARM (arm32 and aarch64)
changes are untested, so please test it if vsscanf() on these platforms
still works.
With this patch following code works fine without any crashing.
Compile for msvcrt with -std=c89 or -D__USE_MINGW_ANSI_STDIO=0
#include
#include
int call_vsscanf(const char *str, const char *format, ...)
{
int ret;
va_list ap;
va_start(ap, format);
ret = vsscanf(str, format, ap);
va_end(ap);
return ret;
}
int main()
{
char b[53];
call_vsscanf(
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ",
"%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c"
"%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c",
[0],[1],[2],[3],[4],[5],[6],[7],[8],[9],[10],[11],
[12],[13],[14],[15],[16],[17],[18],[19],[20],[21],
[22],[23],[24],[25],[26],[27],[28],[29],[30],[31],
[32],[33],[34],[35],[36],[37],[38],[39],[40],[41],
[42],[43],[44],[45],[46],[47],[48],[49],[50],[51]
);
printf("b=%s\n", b);
return 0;
}
---
mingw-w64-crt/Makefile.am | 2 +
mingw-w64-crt/stdio/scanf.S | 166 ++
mingw-w64-crt/stdio/scanf2-argcount-char.c| 9 +
.../stdio/scanf2-argcount-template.c | 22 +++
mingw-w64-crt/stdio/scanf2-argcount-wchar.c | 9 +
mingw-w64-crt/stdio/vfscanf.c | 6 +-
mingw-w64-crt/stdio/vfwscanf.c| 6 +-
mingw-w64-crt/stdio/vsscanf.c | 6 +-
mingw-w64-crt/stdio/vswscanf.c| 6 +-
9 files changed, 154 insertions(+), 78 deletions(-)
create mode 100644 mingw-w64-crt/stdio/scanf2-argcount-char.c
create mode 100644 mingw-w64-crt/stdio/scanf2-argcount-template.c
create mode 100644 mingw-w64-crt/stdio/scanf2-argcount-wchar.c
diff --git a/mingw-w64-crt/Makefile.am b/mingw-w64-crt/Makefile.am
index 5fe84f8bafe4..6cae3b4a52d2 100644
--- a/mingw-w64-crt/Makefile.am
+++ b/mingw-w64-crt/Makefile.am
@@ -504,6 +504,7 @@ src_libmingwex=\
misc/wmemset.c misc/ftw.c misc/ftw64.c
misc/mingw-access.c \
\
stdio/mingw_pformat.h\
+ stdio/scanf2-argcount-char.c stdio/scanf2-argcount-wchar.c \
stdio/vfscanf2.S stdio/vfwscanf2.S stdio/vscanf2.S
stdio/vsscanf2.S stdio/vswscanf2.S \
stdio/vwscanf2.S stdio/strtok_r.c stdio/scanf.S \
stdio/_Exit.cstdio/_findfirst64i32.c stdio/_findnext64i32.c
stdio/_fstat.c \
@@ -2244,6 +2245,7 @@ EXTRA_DIST += revstamp.h \
profile/gcrt0.c \
profile/COPYING \
profile/CYGWIN_LICENSE \
+ stdio/scanf2-argcount-template.c \
stdio/scanf2-template.S
DISTCHECK_CONFIGURE_FLAGS = --host=$(host_triplet) $(withsys)
diff --git a/mingw-w64-crt/stdio/scanf.S b/mingw-w64-crt/stdio/scanf.S
index 1e0bed9452ac..ae8090e4cf9c 100644
--- a/mingw-w64-crt/stdio/scanf.S
+++ b/mingw-w64-crt/stdio/scanf.S
@@ -9,17 +9,14 @@
The goal of this routine is to turn a call to v*scanf into a call to
s*scanf. This is needed because mingw-w64 uses msvcr100.dll, which doesn't
support the v*scanf functions instead of msvcr120.dll which does.
- Unfortunately, there is no defined way to know exactly how big a va_list
- is, so we use a hard-coded buffer.
-
- I suppose a sufficiently-motivated person could try to parse the format
- to figure out how many tokens there are... */
+*/
/* The function prototype here is (essentially):
- int __ms_vsscanf_internal (void *s,
+ int __ms_v*scanf_internal (void *s,
void *format,
void *arg,
+size_t count,
void *func);
I say 'essentially' because passing a function pointer as void in ISO
@@ -37,19 +34,6 @@
*/
.def __argtos;.scl2;.type32;.endef
-/* The max number of pointers we support. Must be an even number
- to keep the 64bit stack 16byte aligned. Must not be less than 4. */
-.equ entries, 30
-
-/* 64bit pointers are 8 bytes. */
-.equ sizeof, 8
-
-/* Size of our buffer. */
-.equ iBytes, entries * sizeof
-
-/* Stack space for first 2 args to s*scanf. */
-.equ
