| Message ID | 20230328-nolibc-printf-test-v1-3-d7290ec893dd@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: add testcases for vfprintf | expand |
On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote: > vfprintf() is complex and so far did not have proper tests. This is an excellent idea, I totally agree, and I wouldn't be surprised if there were still bugs there. > + switch (test + __LINE__ + 1) { > + CASE_TEST(empty); EXPECT_VFPRINTF(0, "", ""); break; > + CASE_TEST(simple); EXPECT_VFPRINTF(3, "foo", "foo"); break; > + CASE_TEST(string); EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break; > + CASE_TEST(number); EXPECT_VFPRINTF(4, "1234", "%d", 1234); break; > + CASE_TEST(negnumber); EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break; > + CASE_TEST(unsigned); EXPECT_VFPRINTF(5, "12345", "%u", 12345); break; > + CASE_TEST(char); EXPECT_VFPRINTF(1, "c", "%c", 'c'); break; > + CASE_TEST(hex); EXPECT_VFPRINTF(1, "f", "%x", 0xf); break; > + CASE_TEST(pointer); EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break; I don't see a reason why not to move them to the stdlib category, since these tests are there to validate that the libc-provided functions do work. Maybe you intended to further extend it ? In this case maybe we could move that to an "stdio" category then but I'd rather avoid having one category per function or it will quickly become annoying to select groups of tests. So let's just prefix these test names with "printf_" and either merge them with "stdlib" or name the category "stdio", as you prefer. Thank you! Willy
On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote: > + fd = memfd_create("vfprintf", 0); > + if (fd == -1) { > + pad_spc(llen, 64, "[FAIL]\n"); > + return 1; > + } Also for this you'll need to include <sys/mman.h> in the part where nolibc is not detected so that it continues to work with regular libcs (at least glibc so that we have one reference to compare against). Thanks, Willy
On 2023-04-02 09:51:10+0200, Willy Tarreau wrote: > On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote: > > vfprintf() is complex and so far did not have proper tests. > > This is an excellent idea, I totally agree, and I wouldn't be surprised > if there were still bugs there. The first issue I experienced was that printf("%*s", 1, "foo") would segfault because it ignored the '*' and just tried to interpret the number "1" as string. When looking for the supported features of the printf implementation there were no examples. And before I try to add code to handle this case better I really want some testcases. > > + switch (test + __LINE__ + 1) { > > + CASE_TEST(empty); EXPECT_VFPRINTF(0, "", ""); break; > > + CASE_TEST(simple); EXPECT_VFPRINTF(3, "foo", "foo"); break; > > + CASE_TEST(string); EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break; > > + CASE_TEST(number); EXPECT_VFPRINTF(4, "1234", "%d", 1234); break; > > + CASE_TEST(negnumber); EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break; > > + CASE_TEST(unsigned); EXPECT_VFPRINTF(5, "12345", "%u", 12345); break; > > + CASE_TEST(char); EXPECT_VFPRINTF(1, "c", "%c", 'c'); break; > > + CASE_TEST(hex); EXPECT_VFPRINTF(1, "f", "%x", 0xf); break; > > + CASE_TEST(pointer); EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break; > > I don't see a reason why not to move them to the stdlib category, since > these tests are there to validate that the libc-provided functions do > work. Maybe you intended to further extend it ? In this case maybe we > could move that to an "stdio" category then but I'd rather avoid having > one category per function or it will quickly become annoying to select > groups of tests. So let's just prefix these test names with "printf_" > and either merge them with "stdlib" or name the category "stdio", as > you prefer. The idea was that printf is its own very special beast that alone is more complex than many other things combined. When working on it, it would be useful to only run the relevant tests without having to manually count testcase numbers. I don't expect other single functions getting their own category. If you still prefer to put it somewhere else I can do that, too.
On 2023-04-02 10:00:05+0200, Willy Tarreau wrote: > On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote: > > + fd = memfd_create("vfprintf", 0); > > + if (fd == -1) { > > + pad_spc(llen, 64, "[FAIL]\n"); > > + return 1; > > + } > > Also for this you'll need to include <sys/mman.h> in the part where nolibc > is not detected so that it continues to work with regular libcs (at least > glibc so that we have one reference to compare against). Ack.
On Sun, Apr 02, 2023 at 12:18:29PM +0000, Thomas Weißschuh wrote: > On 2023-04-02 09:51:10+0200, Willy Tarreau wrote: > > On Tue, Mar 28, 2023 at 09:01:31PM +0000, Thomas Weißschuh wrote: > > > vfprintf() is complex and so far did not have proper tests. > > > > This is an excellent idea, I totally agree, and I wouldn't be surprised > > if there were still bugs there. > > The first issue I experienced was that > > printf("%*s", 1, "foo") would segfault because it ignored the '*' and > just tried to interpret the number "1" as string. Yes indeed, much like many older printf() implementations as well BTW, that's a common issue when you try to write portable code ;-) > When looking for the supported features of the printf implementation > there were no examples. Indeed! > And before I try to add code to handle this case better I really want > some testcases. > > > > + switch (test + __LINE__ + 1) { > > > + CASE_TEST(empty); EXPECT_VFPRINTF(0, "", ""); break; > > > + CASE_TEST(simple); EXPECT_VFPRINTF(3, "foo", "foo"); break; > > > + CASE_TEST(string); EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break; > > > + CASE_TEST(number); EXPECT_VFPRINTF(4, "1234", "%d", 1234); break; > > > + CASE_TEST(negnumber); EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break; > > > + CASE_TEST(unsigned); EXPECT_VFPRINTF(5, "12345", "%u", 12345); break; > > > + CASE_TEST(char); EXPECT_VFPRINTF(1, "c", "%c", 'c'); break; > > > + CASE_TEST(hex); EXPECT_VFPRINTF(1, "f", "%x", 0xf); break; > > > + CASE_TEST(pointer); EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break; > > > > I don't see a reason why not to move them to the stdlib category, since > > these tests are there to validate that the libc-provided functions do > > work. Maybe you intended to further extend it ? In this case maybe we > > could move that to an "stdio" category then but I'd rather avoid having > > one category per function or it will quickly become annoying to select > > groups of tests. So let's just prefix these test names with "printf_" > > and either merge them with "stdlib" or name the category "stdio", as > > you prefer. > > The idea was that printf is its own very special beast that alone is > more complex than many other things combined. > When working on it, it would be useful to only run the relevant tests > without having to manually count testcase numbers. > > I don't expect other single functions getting their own category. > > If you still prefer to put it somewhere else I can do that, too. OK, I can understand, it makes sense to some extents. And I agree that if we'd ever extend printf it would be needed to extend these tests. Then let's leave it that way. Thanks, Willy
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 47013b78972e..cc60c0f7363d 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -667,6 +667,82 @@ int run_stdlib(int min, int max) return ret; } +#define EXPECT_VFPRINTF(c, expected, fmt, ...) \ + ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) + +static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...) +{ + int ret, fd, w, r; + char buf[100]; + va_list args; + + fd = memfd_create("vfprintf", 0); + if (fd == -1) { + pad_spc(llen, 64, "[FAIL]\n"); + return 1; + } + + va_start(args, fmt); + w = vfprintf(&(FILE) { fd }, fmt, args); + va_end(args); + + if (w != c) { + llen += printf(" written(%d) != %d", w, (int) c); + pad_spc(llen, 64, "[FAIL]\n"); + return 1; + } + + lseek(fd, 0, SEEK_SET); + + r = read(fd, buf, sizeof(buf) - 1); + buf[r] = '\0'; + + close(fd); + + if (r != w) { + llen += printf(" written(%d) != read(%d)", w, r); + pad_spc(llen, 64, "[FAIL]\n"); + return 1; + } + + llen += printf(" \"%s\" = \"%s\"", expected, buf); + ret = strncmp(expected, buf, c); + + pad_spc(llen, 64, ret ? "[FAIL]\n" : " [OK]\n"); + return ret; +} + +static int run_vfprintf(int min, int max) +{ + int test; + int tmp; + int ret = 0; + void *p1, *p2; + + for (test = min; test >= 0 && test <= max; test++) { + int llen = 0; // line length + + /* avoid leaving empty lines below, this will insert holes into + * test numbers. + */ + switch (test + __LINE__ + 1) { + CASE_TEST(empty); EXPECT_VFPRINTF(0, "", ""); break; + CASE_TEST(simple); EXPECT_VFPRINTF(3, "foo", "foo"); break; + CASE_TEST(string); EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break; + CASE_TEST(number); EXPECT_VFPRINTF(4, "1234", "%d", 1234); break; + CASE_TEST(negnumber); EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break; + CASE_TEST(unsigned); EXPECT_VFPRINTF(5, "12345", "%u", 12345); break; + CASE_TEST(char); EXPECT_VFPRINTF(1, "c", "%c", 'c'); break; + CASE_TEST(hex); EXPECT_VFPRINTF(1, "f", "%x", 0xf); break; + CASE_TEST(pointer); EXPECT_VFPRINTF(3, "0x0", "%p", NULL); break; + case __LINE__: + return ret; /* must be last */ + /* note: do not set any defaults so as to permit holes above */ + } + } + return ret; +} + static int smash_stack(void) { char buf[100]; @@ -774,6 +850,7 @@ static const struct test test_names[] = { /* add new tests here */ { .name = "syscall", .func = run_syscall }, { .name = "stdlib", .func = run_stdlib }, + { .name = "vfprintf", .func = run_vfprintf }, { .name = "protection", .func = run_protection }, { 0 } };
vfprintf() is complex and so far did not have proper tests. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- tools/testing/selftests/nolibc/nolibc-test.c | 77 ++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+)