From 0f76d84a97c7046f328fc4ad0b92ceb944f42599 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Sat, 12 Dec 2020 12:56:26 -0600 Subject: [PATCH 1/8] tests: cbprintf: sanitize language Replace SANITYCHECK flag with TWISTER. Signed-off-by: Peter Bigot --- tests/unit/cbprintf/main.c | 24 +++++++++++------------ tests/unit/cbprintf/testcase.yaml | 32 +++++++++++++++---------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/tests/unit/cbprintf/main.c b/tests/unit/cbprintf/main.c index 1e4f0b6c76847..c2feb48568baa 100644 --- a/tests/unit/cbprintf/main.c +++ b/tests/unit/cbprintf/main.c @@ -17,10 +17,10 @@ #include /* Unit testing doesn't use Kconfig, so if we're not building from - * sanitycheck force selection of all features. If we are use flags - * to determine which features are desired. Yes, this is a mess. + * twister force selection of all features. If we are use flags to + * determine which features are desired. Yes, this is a mess. */ -#ifndef VIA_SANITYCHECK +#ifndef VIA_TWISTER /* Set this to truthy to use libc's snprintf as external validation. * This should be used with all options enabled. */ @@ -58,34 +58,34 @@ #define CONFIG_CBPRINTF_REDUCED_INTEGRAL 1 #endif -#else /* VIA_SANITYCHECK */ -#if (VIA_SANITYCHECK & 0x01) != 0 +#else /* VIA_TWISTER */ +#if (VIA_TWISTER & 0x01) != 0 #define CONFIG_CBPRINTF_FULL_INTEGRAL 1 #else #define CONFIG_CBPRINTF_REDUCED_INTEGRAL 1 #endif -#if (VIA_SANITYCHECK & 0x02) != 0 +#if (VIA_TWISTER & 0x02) != 0 #define CONFIG_CBPRINTF_FP_SUPPORT 1 #endif -#if (VIA_SANITYCHECK & 0x04) != 0 +#if (VIA_TWISTER & 0x04) != 0 #define CONFIG_CBPRINTF_FP_A_SUPPORT 1 #endif -#if (VIA_SANITYCHECK & 0x08) != 0 +#if (VIA_TWISTER & 0x08) != 0 #define CONFIG_CBPRINTF_N_SPECIFIER 1 #endif -#if (VIA_SANITYCHECK & 0x40) != 0 +#if (VIA_TWISTER & 0x40) != 0 #define CONFIG_CBPRINTF_FP_ALWAYS_A 1 #endif -#if (VIA_SANITYCHECK & 0x80) != 0 +#if (VIA_TWISTER & 0x80) != 0 #define CONFIG_CBPRINTF_NANO 1 #else /* 0x80 */ #define CONFIG_CBPRINTF_COMPLETE 1 #endif /* 0x80 */ -#if (VIA_SANITYCHECK & 0x100) != 0 +#if (VIA_TWISTER & 0x100) != 0 #define CONFIG_CBPRINTF_LIBC_SUBSTS 1 #endif -#endif /* VIA_SANITYCHECK */ +#endif /* VIA_TWISTER */ #include "../../../lib/os/cbprintf.c" diff --git a/tests/unit/cbprintf/testcase.yaml b/tests/unit/cbprintf/testcase.yaml index a7cfe65a15941..01d9054babc1f 100644 --- a/tests/unit/cbprintf/testcase.yaml +++ b/tests/unit/cbprintf/testcase.yaml @@ -4,49 +4,49 @@ common: tests: utilities.prf.m32v00: # REDUCED - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x00 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x00 utilities.prf.m32v01: # FULL - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x01 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x01 utilities.prf.m32v02: # REDUCED + FP - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x02 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x02 utilities.prf.m32v03: # FULL + FP - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x03 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x03 utilities.prf.m32v07: # FULL + FP + FP_A - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x07 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x07 utilities.prf.m32v08: # %n - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x08 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x08 utilities.prf.m32v80: # NANO - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x80 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x80 utilities.prf.m32v101: # FULL + LIBC - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x101 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x101 utilities.prf.m32v181: # NANO + FULL + LIBC - extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x181 + extra_args: M64_MODE=0 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x181 utilities.prf.m64v00: # m64 - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x00 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x00 utilities.prf.m64v01: # m64 FULL - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x01 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x01 utilities.prf.m64v03: # m64 FULL & FP - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x03 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x03 utilities.prf.m64v17: # m64 FULL & FP & FP_A - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x07 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x07 utilities.prf.m64v80: # NANO - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x80 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x80 utilities.prf.m64v101: # FULL + LIBC - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x101 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x101 utilities.prf.m64v181: # NANO + FULL + LIBC - extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_SANITYCHECK=0x181 + extra_args: M64_MODE=1 EXTRA_CPPFLAGS=-DVIA_TWISTER=0x181 From 22ad5b4bc291ea87b5aeeb6b7e4a38a307cda994 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Sat, 12 Dec 2020 12:44:59 -0600 Subject: [PATCH 2/8] tests: cbprintf: make skip messages consistent TC_PRINTF doesn't append a newline, so the skip messages sometimes ran into the test result message. Signed-off-by: Peter Bigot --- tests/unit/cbprintf/main.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/unit/cbprintf/main.c b/tests/unit/cbprintf/main.c index c2feb48568baa..e53315e2a23c6 100644 --- a/tests/unit/cbprintf/main.c +++ b/tests/unit/cbprintf/main.c @@ -292,7 +292,7 @@ static void test_c(void) PRF_CHECK("a", rc); if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("short test for nano"); + TC_PRINT("short test for nano\n"); return; } @@ -337,7 +337,7 @@ static void test_s(void) } if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("short test for nano"); + TC_PRINT("short test for nano\n"); return; } @@ -414,7 +414,7 @@ static void test_d_length(void) } if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("short test for nano"); + TC_PRINT("short test for nano\n"); return; } @@ -450,7 +450,7 @@ static void test_d_flags(void) int rc; if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -509,7 +509,7 @@ static void test_x_length(void) } if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("short test for nano"); + TC_PRINT("short test for nano\n"); return; } @@ -569,7 +569,7 @@ static void test_x_flags(void) int rc; if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -602,7 +602,7 @@ static void test_o(void) int rc; if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -615,7 +615,7 @@ static void test_o(void) static void test_fp_value(void) { if (!IS_ENABLED(CONFIG_CBPRINTF_FP_SUPPORT)) { - TC_PRINT("skipping unsupported feature"); + TC_PRINT("skipping unsupported feature\n"); return; } @@ -808,7 +808,7 @@ static void test_fp_value(void) static void test_fp_length(void) { if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -849,7 +849,7 @@ static void test_fp_length(void) static void test_fp_flags(void) { if (!IS_ENABLED(CONFIG_CBPRINTF_FP_SUPPORT)) { - TC_PRINT("skipping unsupported feature"); + TC_PRINT("skipping unsupported feature\n"); return; } @@ -882,7 +882,7 @@ static void test_fp_flags(void) static void test_star_width(void) { if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -898,7 +898,7 @@ static void test_star_width(void) static void test_star_precision(void) { if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -927,11 +927,11 @@ static void test_star_precision(void) static void test_n(void) { if (!IS_ENABLED(CONFIG_CBPRINTF_N_SPECIFIER)) { - TC_PRINT("skipping unsupported feature"); + TC_PRINT("skipping unsupported feature\n"); return; } if (IS_ENABLED(CONFIG_CBPRINTF_NANO)) { - TC_PRINT("skipped test for nano"); + TC_PRINT("skipped test for nano\n"); return; } @@ -1008,7 +1008,7 @@ static void test_arglen(void) static void test_p(void) { if (IS_ENABLED(USE_LIBC)) { - TC_PRINT("skipping on libc"); + TC_PRINT("skipping on libc\n"); return; } From 99747ed11e34a97de609464acda838ce1aa9635a Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Wed, 6 Jan 2021 11:50:23 -0600 Subject: [PATCH 3/8] tests: cbprintf: avoid checkpatch diagnostic checkpatch wants parameters to IS_ENABLED() to be Kconfig constants, i.e. ones that start with CONFIG_. Avoid the whinage. Signed-off-by: Peter Bigot --- tests/unit/cbprintf/main.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/unit/cbprintf/main.c b/tests/unit/cbprintf/main.c index e53315e2a23c6..e130115127b54 100644 --- a/tests/unit/cbprintf/main.c +++ b/tests/unit/cbprintf/main.c @@ -87,6 +87,15 @@ #endif /* VIA_TWISTER */ +/* Can't use IS_ENABLED on symbols that don't start with CONFIG_ + * without checkpatch complaints, so do something else. + */ +#if USE_LIBC +#define ENABLED_USE_LIBC true +#else +#define ENABLED_USE_LIBC false +#endif + #include "../../../lib/os/cbprintf.c" #if defined(CONFIG_CBPRINTF_COMPLETE) @@ -297,7 +306,7 @@ static void test_c(void) } rc = TEST_PRF("%lc", (wint_t)'a'); - if (IS_ENABLED(USE_LIBC)) { + if (ENABLED_USE_LIBC) { PRF_CHECK("a", rc); } else { PRF_CHECK("%lc", rc); @@ -345,7 +354,7 @@ static void test_s(void) PRF_CHECK("/123/12//", rc); rc = TEST_PRF("%ls", ws); - if (IS_ENABLED(USE_LIBC)) { + if (ENABLED_USE_LIBC) { PRF_CHECK("abc", rc); } else { PRF_CHECK("%ls", rc); @@ -361,7 +370,7 @@ static void test_v_c(void) rc = rawprf("%c", 'a'); zassert_equal(rc, 1, NULL); zassert_equal(buf[0], 'a', NULL); - if (!IS_ENABLED(USE_LIBC)) { + if (!ENABLED_USE_LIBC) { zassert_equal(buf[1], 'b', "wth %x", buf[1]); } } @@ -830,7 +839,7 @@ static void test_fp_length(void) } rc = TEST_PRF("/%Lg/", (long double)dv); - if (IS_ENABLED(USE_LIBC)) { + if (ENABLED_USE_LIBC) { PRF_CHECK("/1.2345/", rc); } else { PRF_CHECK("/%Lg/", rc); @@ -1007,7 +1016,7 @@ static void test_arglen(void) static void test_p(void) { - if (IS_ENABLED(USE_LIBC)) { + if (ENABLED_USE_LIBC) { TC_PRINT("skipping on libc\n"); return; } @@ -1124,7 +1133,7 @@ void test_main(void) } TC_PRINT("Opts: " COND_CODE_1(M64_MODE, ("m64"), ("m32")) "\n"); - if (IS_ENABLED(USE_LIBC)) { + if (ENABLED_USE_LIBC) { TC_PRINT(" LIBC"); } if (IS_ENABLED(CONFIG_CBPRINTF_COMPLETE)) { From 6b9cd06f4108401e4469a0ec1a3f8ab99db8b33c Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Sat, 12 Dec 2020 13:09:44 -0600 Subject: [PATCH 4/8] lib: cbprintf: improve coverage Providing a literal width or precision that exceeds the non-negative range of int does not appear to be rejected by the standard, but it does produce a build diagnostic so we can't test it. Switch to an equivalent form that doesn't affect line coverage. Signed-off-by: Peter Bigot --- lib/os/cbprintf_complete.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/os/cbprintf_complete.c b/lib/os/cbprintf_complete.c index fb431f57c4258..4201080fe031c 100644 --- a/lib/os/cbprintf_complete.c +++ b/lib/os/cbprintf_complete.c @@ -360,10 +360,8 @@ static inline const char *extract_width(struct conversion *conv, if (sp != wp) { conv->width_present = true; conv->width_value = width; - if (width != conv->width_value) { - /* Lost width data */ - conv->unsupported = true; - } + conv->unsupported |= ((conv->width_value < 0) + || (width != (size_t)conv->width_value)); } return sp; @@ -396,10 +394,8 @@ static inline const char *extract_prec(struct conversion *conv, size_t prec = extract_decimal(&sp); conv->prec_value = prec; - if (prec != conv->prec_value) { - /* Lost precision data */ - conv->unsupported = true; - } + conv->unsupported |= ((conv->prec_value < 0) + || (prec != (size_t)conv->prec_value)); return sp; } From 1406bf2ca84ea441b3e324452831090787da3ed4 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Sat, 12 Dec 2020 11:03:50 -0600 Subject: [PATCH 5/8] lib: cbprintf: remove cbprintf_arglen This function was designed to support the logging infrastructure's need to copy values from va_list structures. It did not meet that need, since some values need to be changed based on additional data that is only available when the complete format specification is examined. Remove the function as unnecessary. Signed-off-by: Peter Bigot --- include/sys/cbprintf.h | 20 -------- lib/os/cbprintf_complete.c | 95 -------------------------------------- lib/os/cbprintf_nano.c | 19 -------- tests/unit/cbprintf/main.c | 31 ------------- 4 files changed, 165 deletions(-) diff --git a/include/sys/cbprintf.h b/include/sys/cbprintf.h index 674a557ec2bde..e32c88fc7f89e 100644 --- a/include/sys/cbprintf.h +++ b/include/sys/cbprintf.h @@ -69,26 +69,6 @@ typedef int (*cbprintf_cb)(/* int c, void *ctx */); __printf_like(3, 4) int cbprintf(cbprintf_cb out, void *ctx, const char *format, ...); -/** @brief Calculate the number of words required for arguments to a cbprintf - * format specification. - * - * This can be used in cases where the arguments must be copied off the stack - * into separate storage for processing the conversion in another context. - * - * @note The length returned does not count bytes. It counts native words - * defined as the size of an `int`. - * - * @note If `CONFIG_CBPRINTF_NANO` is selected the count will be incorrect if - * any passed arguments require more than one `int`. - * - * @param format a standard ISO C format string with characters and conversion - * specifications. - * - * @return the number of `int` elements required to provide all arguments - * required for the conversion. - */ -size_t cbprintf_arglen(const char *format); - /** @brief varargs-aware *printf-like output through a callback. * * This is essentially vsprintf() except the output is generated diff --git a/lib/os/cbprintf_complete.c b/lib/os/cbprintf_complete.c index 4201080fe031c..4161b1d6164d6 100644 --- a/lib/os/cbprintf_complete.c +++ b/lib/os/cbprintf_complete.c @@ -616,84 +616,6 @@ static inline const char *extract_conversion(struct conversion *conv, return sp; } - -/* Get the number of int-sized objects required to provide the arguments for - * the conversion. - * - * This has a role in the logging subsystem where the arguments must - * be captured for formatting in another thread. - * - * If the conversion specifier is invalid the calculated length may - * not match what was actually passed as arguments. - */ -static size_t conversion_arglen(const struct conversion *conv) -{ - enum specifier_cat_enum specifier_cat - = (enum specifier_cat_enum)conv->specifier_cat; - enum length_mod_enum length_mod - = (enum length_mod_enum)conv->length_mod; - size_t words = 0; - - /* If the conversion is invalid behavior is undefined. What - * this does is try to consume the argument anyway, in hopes - * that subsequent valid arguments will format correctly. - */ - - /* Percent has no arguments */ - if (conv->specifier == '%') { - return words; - } - - if (conv->width_star) { - words += sizeof(int) / sizeof(int); - } - - if (conv->prec_star) { - words += sizeof(int) / sizeof(int); - } - - if ((specifier_cat == SPECIFIER_SINT) - || (specifier_cat == SPECIFIER_UINT)) { - /* The size of integral values is the same regardless - * of signedness. - */ - switch (length_mod) { - case LENGTH_NONE: - case LENGTH_HH: - case LENGTH_H: - words += sizeof(int) / sizeof(int); - break; - case LENGTH_L: - words += sizeof(long) / sizeof(int); - break; - case LENGTH_LL: - words += sizeof(long long) / sizeof(int); - break; - case LENGTH_J: - words += sizeof(intmax_t) / sizeof(int); - break; - case LENGTH_Z: - words += sizeof(size_t) / sizeof(int); - break; - case LENGTH_T: - words += sizeof(ptrdiff_t) / sizeof(int); - break; - default: - break; - } - } else if (specifier_cat == SPECIFIER_FP) { - if (length_mod == LENGTH_UPPER_L) { - words += sizeof(long double) / sizeof(int); - } else { - words += sizeof(double) / sizeof(int); - } - } else if (specifier_cat == SPECIFIER_PTR) { - words += sizeof(void *) / sizeof(int); - } - - return words; -} - #ifdef CONFIG_64BIT static void _ldiv5(uint64_t *v) @@ -1823,20 +1745,3 @@ int cbvprintf(cbprintf_cb out, void *ctx, const char *fp, va_list ap) #undef OUTS #undef OUTC } - -size_t cbprintf_arglen(const char *format) -{ - size_t rv = 0; - struct conversion conv; - - while (*format) { - if (*format == '%') { - format = extract_conversion(&conv, format); - rv += conversion_arglen(&conv); - } else { - ++format; - } - } - - return rv; -} diff --git a/lib/os/cbprintf_nano.c b/lib/os/cbprintf_nano.c index 76d3da13a7cbb..e26d4277d9af4 100644 --- a/lib/os/cbprintf_nano.c +++ b/lib/os/cbprintf_nano.c @@ -294,22 +294,3 @@ int cbvprintf(cbprintf_cb out, void *ctx, const char *fmt, va_list ap) return count; } - -size_t cbprintf_arglen(const char *format) -{ - size_t rv = 0; - bool last_pct = false; - - while (*format != 0) { - if (*format == '%') { - last_pct = !last_pct; - } else if (last_pct) { - ++rv; - last_pct = false; - } else { - } - ++format; - } - - return rv; -} diff --git a/tests/unit/cbprintf/main.c b/tests/unit/cbprintf/main.c index e130115127b54..3c562206d2e33 100644 --- a/tests/unit/cbprintf/main.c +++ b/tests/unit/cbprintf/main.c @@ -984,36 +984,6 @@ static void test_n(void) #define EXPECTED_1ARG(_t) (IS_ENABLED(CONFIG_CBPRINTF_NANO) \ ? 1U : (sizeof(_t) / sizeof(int))) -static void test_arglen(void) -{ - zassert_equal(cbprintf_arglen("/%hhd/"), 1U, NULL); - zassert_equal(cbprintf_arglen("/%hd/"), 1U, NULL); - zassert_equal(cbprintf_arglen("/%d/"), 1U, NULL); - zassert_equal(cbprintf_arglen("/%ld/"), - EXPECTED_1ARG(long), NULL); - zassert_equal(cbprintf_arglen("/%lld/"), - EXPECTED_1ARG(long long), NULL); - zassert_equal(cbprintf_arglen("/%jd/"), - EXPECTED_1ARG(intmax_t), NULL); - zassert_equal(cbprintf_arglen("/%zd/"), - EXPECTED_1ARG(size_t), NULL); - zassert_equal(cbprintf_arglen("/%td/"), - EXPECTED_1ARG(ptrdiff_t), NULL); - zassert_equal(cbprintf_arglen("/%f/"), - EXPECTED_1ARG(double), NULL); - zassert_equal(cbprintf_arglen("/%Lf/"), - EXPECTED_1ARG(long double), NULL); - zassert_equal(cbprintf_arglen("/%p/"), - EXPECTED_1ARG(void *), NULL); - - zassert_equal(cbprintf_arglen("/%%/"), 0U, NULL); - zassert_equal(cbprintf_arglen("/%*d%/"), 2U, NULL); - zassert_equal(cbprintf_arglen("/%.*d%/"), 2U, NULL); - zassert_equal(cbprintf_arglen("/%*.*d%/"), - IS_ENABLED(CONFIG_CBPRINTF_NANO) ? 2U : 3U, - NULL); -} - static void test_p(void) { if (ENABLED_USE_LIBC) { @@ -1182,7 +1152,6 @@ void test_main(void) ztest_unit_test(test_star_precision), ztest_unit_test(test_n), ztest_unit_test(test_p), - ztest_unit_test(test_arglen), ztest_unit_test(test_libc_substs), ztest_unit_test(test_nop) ); From ce04ae5d6f037b4dfc3f264f6e7c444a2e8b7393 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Sat, 12 Dec 2020 09:45:06 -0600 Subject: [PATCH 6/8] lib: cbprintf: document length modifiers It may not be clear that the length modifiers reference native C types with specific ranks. Document the core type for each modifier. Signed-off-by: Peter Bigot --- lib/os/cbprintf_complete.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/os/cbprintf_complete.c b/lib/os/cbprintf_complete.c index 4161b1d6164d6..e0e02a6f00686 100644 --- a/lib/os/cbprintf_complete.c +++ b/lib/os/cbprintf_complete.c @@ -54,15 +54,15 @@ typedef uint32_t uint_value_type; /* The allowed types of length modifier. */ enum length_mod_enum { - LENGTH_NONE, - LENGTH_HH, - LENGTH_H, - LENGTH_L, - LENGTH_LL, - LENGTH_J, - LENGTH_Z, - LENGTH_T, - LENGTH_UPPER_L, + LENGTH_NONE, /* int */ + LENGTH_HH, /* char */ + LENGTH_H, /* short */ + LENGTH_L, /* long */ + LENGTH_LL, /* long long */ + LENGTH_J, /* intmax */ + LENGTH_Z, /* size_t */ + LENGTH_T, /* ptrdiff_t */ + LENGTH_UPPER_L, /* long double */ }; /* Categories of conversion specifiers. */ From 82fe5d2b3fc408d0ae4d85ec16fb672be80f79b1 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Wed, 6 Jan 2021 10:59:49 -0600 Subject: [PATCH 7/8] lib: os: cbprintf: correctly handle signed vs unsigned char Whether char is signed or unsigned is toolchain and target specific. Rather than assume it's signed (which is true for x86, but not for ARM), do the right thing based on whether the minimum representable value is less than zero. Signed-off-by: Peter Bigot --- lib/os/cbprintf_complete.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/os/cbprintf_complete.c b/lib/os/cbprintf_complete.c index e0e02a6f00686..a19db423df9ac 100644 --- a/lib/os/cbprintf_complete.c +++ b/lib/os/cbprintf_complete.c @@ -79,12 +79,22 @@ enum specifier_cat_enum { SPECIFIER_FP, }; +#define CHAR_IS_SIGNED (CHAR_MIN != 0) +#if CHAR_IS_SIGNED +#define CASE_SINT_CHAR case 'c': +#define CASE_UINT_CHAR +#else +#define CASE_SINT_CHAR +#define CASE_UINT_CHAR case 'c': +#endif + /* Case label to identify conversions for signed integral values. The * corresponding argument_value tag is sint and category is * SPECIFIER_SINT. */ #define SINT_CONV_CASES \ 'd': \ + CASE_SINT_CHAR \ case 'i' /* Case label to identify conversions for signed integral arguments. @@ -92,8 +102,8 @@ enum specifier_cat_enum { * SPECIFIER_UINT. */ #define UINT_CONV_CASES \ - 'c': \ - case 'o': \ + 'o': \ + CASE_UINT_CHAR \ case 'u': \ case 'x': \ case 'X' @@ -1515,7 +1525,7 @@ int cbvprintf(cbprintf_cb out, void *ctx, const char *fp, va_list ap) } case 'c': bps = buf; - buf[0] = value->uint; + buf[0] = CHAR_IS_SIGNED ? value->sint : value->uint; bpe = buf + 1; break; case 'd': From 1da2455ec4c845ecf3868042eb6fc787844bfe61 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Wed, 6 Jan 2021 11:24:39 -0600 Subject: [PATCH 8/8] lib: os: cbprintf: correct arg extraction of wide characters The l length modifier can apply to the c format specifier; in that case the expected value is of type wint_t. Minimal libc doesn't define wint_t, and it is complex to do so correctly (must add , and use a lot of conditional tricks). wint_t can differ from wchar_t in rank when wchar_t undergoes default integral promotion, which it does on xtensa (wchar_t is unsigned short). So we can use wchar_t as an approximation, except in va_arg where we need to use a wider type: int covers this case. Note that we still don't format wide characters, but we do want to consume the correct amount of data for a default-promoted extended character. Signed-off-by: Peter Bigot --- lib/os/cbprintf_complete.c | 49 +++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/os/cbprintf_complete.c b/lib/os/cbprintf_complete.c index a19db423df9ac..c30cd52a82881 100644 --- a/lib/os/cbprintf_complete.c +++ b/lib/os/cbprintf_complete.c @@ -88,6 +88,37 @@ enum specifier_cat_enum { #define CASE_UINT_CHAR case 'c': #endif +/* We need two pieces of information about wchar_t: + * * WCHAR_IS_SIGNED: whether it's signed or unsigned; + * * WINT_TYPE: the type to use when extracting it from va_args + * + * The former can be determined from the value of WCHAR_MIN if it's defined. + * It's not for minimal libc, so treat it as whatever char is. + * + * The latter should be wint_t, but minimal libc doesn't provide it. We can + * substitute wchar_t as long as that type does not undergo default integral + * promotion as an argument. But it does for at least one toolchain (xtensa), + * and where it does we need to use the promoted type in va_arg() to avoid + * build errors, otherwise we can use the base type. We can tell that + * integral promotion occurs if WCHAR_MAX is strictly less than INT_MAX. + */ +#ifndef WCHAR_MIN +#define WCHAR_IS_SIGNED CHAR_IS_SIGNED +#if WCHAR_IS_SIGNED +#define WINT_TYPE int +#else /* wchar signed */ +#define WINT_TYPE unsigned int +#endif /* wchar signed */ +#else /* WCHAR_MIN defined */ +#define WCHAR_IS_SIGNED ((WCHAR_MIN - 0) != 0) +#if WCHAR_MAX < INT_MAX +/* Signed or unsigned, it'll be int */ +#define WINT_TYPE int +#else /* wchar rank vs int */ +#define WINT_TYPE wchar_t +#endif /* wchar rank vs int */ +#endif /* WCHAR_MIN defined */ + /* Case label to identify conversions for signed integral values. The * corresponding argument_value tag is sint and category is * SPECIFIER_SINT. @@ -499,7 +530,7 @@ static inline const char *extract_specifier(struct conversion *conv, } /* For c LENGTH_NONE and LENGTH_L would be ok, - * but we don't support wide characters. + * but we don't support formatting wide characters. */ if (conv->specifier == 'c') { unsupported = (conv->length_mod != LENGTH_NONE); @@ -1420,7 +1451,13 @@ int cbvprintf(cbprintf_cb out, void *ctx, const char *fp, va_list ap) value->sint = va_arg(ap, int); break; case LENGTH_L: - value->sint = va_arg(ap, long); + if (WCHAR_IS_SIGNED + && (conv->specifier == 'c')) { + value->sint = (wchar_t)va_arg(ap, + WINT_TYPE); + } else { + value->sint = va_arg(ap, long); + } break; case LENGTH_LL: value->sint = @@ -1457,7 +1494,13 @@ int cbvprintf(cbprintf_cb out, void *ctx, const char *fp, va_list ap) value->uint = va_arg(ap, unsigned int); break; case LENGTH_L: - value->uint = va_arg(ap, unsigned long); + if ((!WCHAR_IS_SIGNED) + && (conv->specifier == 'c')) { + value->uint = (wchar_t)va_arg(ap, + WINT_TYPE); + } else { + value->uint = va_arg(ap, unsigned long); + } break; case LENGTH_LL: value->uint =