common: Conditionally support printf %l and %i modifiers

The libfp library prints some values with PRIx32 or PRId32 format
specifiers which, in their compilation environment, output "%lx" and
"%ld".

Unfortunately, support for printing any '%l' format in EC
code was deprecated in issuetracker.google.com/issues/172210614
after changing it from it being treated as a hard-coded 64-bit length.
There was concern that new code using %l with 32-bit values would be
cherry-picked to older branches without the updated printf. In these
cases, the older code would interpret that %l as 64-bit argument,
causing it to over-ingest arguments and potentially behave in an
undefined manner.

Printing 32-bit values with "%l" or "%i" is safe as long as we can
guarantee no legacy code will attempt to print using "%l" with a
64-bit value. The logic here is protected by a config flag that is
only enabled for FPMCU and FPMCU doesn't use long running release
branches.

A printf test is also added to ensure that only dartmonkey and
bloonchipper boards have long32 enabled.

BRANCH=none
BUG=b:234781655
BUG=b:234143158
TEST=./test/run_device_tests.py -b dartmonkey -t printf
TEST=./test/run_device_tests.py -b bloonchipper -t printf
TEST=make runhosttests

Signed-off-by: Bobby Casey <bobbycasey@google.com>
Change-Id: If432f507a31cc12a4c5c4bdcd07c6141407bd70d
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3707743
Reviewed-by: Andrea Grandi <agrandi@google.com>
Reviewed-by: Tom Hughes <tomhughes@chromium.org>

10 files changed, 140 insertions, 32 deletions

diff --git a/baseboard/nucleo-f412zg/base-board.h b/baseboard/nucleo-f412zg/base-board.h
index b104621ec7..bd5710e247 100644
--- a/baseboard/nucleo-f412zg/base-board.h
+++ b/baseboard/nucleo-f412zg/base-board.h
@@ -164,7 +164,7 @@
 #define CONFIG_HOST_COMMAND_STATUS
 #define CONFIG_MKBP_EVENT
 #define CONFIG_MKBP_USE_GPIO
-#define CONFIG_PRINTF_LEGACY_LI_FORMAT
+#define CONFIG_PRINTF_LONG_IS_32BITS
 #define CONFIG_RNG
 #define CONFIG_SHA256
 #define CONFIG_SHA256_UNROLLED
diff --git a/baseboard/nucleo-h743zi/base-board.h b/baseboard/nucleo-h743zi/base-board.h
index 10f780505d..f96ba41bcc 100644
--- a/baseboard/nucleo-h743zi/base-board.h
+++ b/baseboard/nucleo-h743zi/base-board.h
@@ -92,7 +92,7 @@
 #define CONFIG_LOW_POWER_IDLE
 #define CONFIG_MKBP_EVENT
 #define CONFIG_MKBP_USE_GPIO
-#define CONFIG_PRINTF_LEGACY_LI_FORMAT
+#define CONFIG_PRINTF_LONG_IS_32BITS
 #define CONFIG_RNG
 #define CONFIG_RWSIG_TYPE_RWSIG
 #define CONFIG_SHA256
diff --git a/board/hatch_fp/board.h b/board/hatch_fp/board.h
index 2702152576..a4c4e2923b 100644
--- a/board/hatch_fp/board.h
+++ b/board/hatch_fp/board.h
@@ -230,7 +230,7 @@
 #define CONFIG_HOST_COMMAND_STATUS
 #define CONFIG_MKBP_EVENT
 #define CONFIG_MKBP_USE_GPIO
-#define CONFIG_PRINTF_LEGACY_LI_FORMAT
+#define CONFIG_PRINTF_LONG_IS_32BITS
 #define CONFIG_RNG
 #define CONFIG_SHA256
 #define CONFIG_SHA256_UNROLLED
diff --git a/board/nocturne_fp/board.h b/board/nocturne_fp/board.h
index 5b12d05321..2d0f5586cf 100644
--- a/board/nocturne_fp/board.h
+++ b/board/nocturne_fp/board.h
@@ -106,7 +106,7 @@
 #undef CONFIG_LID_SWITCH
 #define CONFIG_MKBP_EVENT
 #define CONFIG_MKBP_USE_GPIO
-#define CONFIG_PRINTF_LEGACY_LI_FORMAT
+#define CONFIG_PRINTF_LONG_IS_32BITS
 #define CONFIG_SHA256
 #define CONFIG_SHA256_UNROLLED
 #define CONFIG_SPI
diff --git a/builtin/stdint.h b/builtin/stdint.h
index 87993d8e2d..f47c9e6422 100644
--- a/builtin/stdint.h
+++ b/builtin/stdint.h
@@ -67,6 +67,9 @@ typedef int64_t int_fast64_t;
 #ifndef INT32_MAX
 #define INT32_MAX (2147483647U)
 #endif
+#ifndef INT32_MIN
+#define INT32_MIN (-2147483648)
+#endif
 
 #ifndef UINT64_C
 #define UINT64_C(c) c##ULL
@@ -81,5 +84,8 @@ typedef int64_t int_fast64_t;
 #ifndef INT64_MAX
 #define INT64_MAX INT64_C(9223372036854775807)
 #endif
+#ifndef INT64_MIN
+#define INT64_MIN (INT64_C(-9223372036854775807) - 1)
+#endif
 
 #endif /* __CROS_EC_STDINT_H__ */
diff --git a/common/printf.c b/common/printf.c
index 9fbac19e20..5102dd8f20 100644
--- a/common/printf.c
+++ b/common/printf.c
@@ -224,7 +224,9 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 
 			/*
 			 * Handle length:
-			 * %l - DEPRECATED (see below)
+			 * %l - supports 64-bit longs, 32-bit longs are
+			 *      supported with a config flag, see comment
+			 *      below for more details
 			 * %ll - long long
 			 * %z - size_t
 			 */
@@ -239,18 +241,23 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 				}
 
 				/*
-				 * %l on 32-bit systems is deliberately
-				 * deprecated. It was originally used as
-				 * shorthand for 64-bit values. When
+				 * The CONFIG_PRINTF_LONG_IS_32BITS flag is
+				 * required to enable the %l flag on systems
+				 * where it would signify a 32-bit value.
+				 * Otherwise, %l on 32-bit systems is
+				 * deliberately deprecated. %l was originally
+				 * used as shorthand for 64-bit values. When
 				 * compile-time printf format checking was
 				 * enabled, it had to be cleaned up to be
 				 * sizeof(long), which is 32 bits on today's
 				 * ECs. This presents a mismatch which can be
 				 * dangerous if a new-style printf call is
 				 * cherry-picked into an old firmware branch.
-				 * See crbug.com/984041 for more context.
+				 * For more context, see
+				 * https://issuetracker.google.com/issues/172210614
 				 */
-				if (!(flags & PF_64BIT)) {
+				if (!IS_ENABLED(CONFIG_PRINTF_LONG_IS_32BITS)
+				    && !(flags & PF_64BIT)) {
 					format = error_str;
 					continue;
 				}
@@ -336,12 +343,9 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 			}
 
 			switch (c) {
-#ifdef CONFIG_PRINTF_LEGACY_LI_FORMAT
+#ifdef CONFIG_PRINTF_LONG_IS_32BITS
 			case 'i':
-				/* force 32-bit for compatibility */
-				flags &= ~PF_64BIT;
-				/* fall-through */
-#endif /* CONFIG_PRINTF_LEGACY_LI_FORMAT */
+#endif /* CONFIG_PRINTF_LONG_IS_32BITS */
 			case 'd':
 				if (flags & PF_64BIT) {
 					if ((int64_t)v < 0) {
diff --git a/include/config.h b/include/config.h
index bcf070f82f..70dc284a8f 100644
--- a/include/config.h
+++ b/include/config.h
@@ -3639,10 +3639,12 @@
 #undef CONFIG_POWER_TRACK_HOST_SLEEP_STATE
 
 /*
- * Implement the '%li' printf format as a *32-bit* integer format,
- * as it might be expected by non-EC code.
+ * Allow the use of the "long" printf length modifier ('l') to be in 32-bit
+ * systems along with any supported conversion specifiers. Note that this also
+ * reenables support for the 'i' printf format. This config will only take
+ * effect if sizeof(long) == sizeof(uint32_t).
  */
-#undef CONFIG_PRINTF_LEGACY_LI_FORMAT
+#undef CONFIG_PRINTF_LONG_IS_32BITS
 
 /*
  * On x86 systems, define this option if the CPU_PROCHOT signal is active low.
diff --git a/include/printf.h b/include/printf.h
index 8eed1618f8..333e622b7b 100644
--- a/include/printf.h
+++ b/include/printf.h
@@ -51,7 +51,7 @@
  *   - 'c' - character
  *   - 's' - null-terminated ASCII string
  *   - 'd' - signed integer
- *   - 'i' - signed integer if CONFIG_PRINTF_LEGACY_LI_FORMAT is set (ignore l)
+ *   - 'i' - signed integer (if CONFIG_PRINTF_LONG_IS_32BITS is enabled)
  *   - 'u' - unsigned integer
  *   - 'x' - unsigned integer, print as lower-case hexadecimal
  *   - 'X' - unsigned integer, print as upper-case hexadecimal
diff --git a/test/printf.c b/test/printf.c
index 83d03c210f..388e0c7e80 100644
--- a/test/printf.c
+++ b/test/printf.c
@@ -171,23 +171,118 @@ test_static int test_vsnprintf_int(void)
 	T(expect_success("5e", "%x", 0X5E));
 	T(expect_success("5E", "%X", 0X5E));
 
+	return EC_SUCCESS;
+}
+
+test_static int test_printf_long32_enabled(void)
+{
+	bool use_l32 = IS_ENABLED(CONFIG_PRINTF_LONG_IS_32BITS);
+
+	if (IS_ENABLED(BOARD_BLOONCHIPPER) || IS_ENABLED(BOARD_DARTMONKEY))
+		TEST_ASSERT(use_l32);
+	else
+		TEST_ASSERT(!use_l32);
+	return EC_SUCCESS;
+}
+
+test_static int test_vsnprintf_32bit_long_supported(void)
+{
+	long long_min = INT32_MIN;
+	long long_max = INT32_MAX;
+	unsigned long ulong_max = UINT32_MAX;
+	char const *long_min_str = "-2147483648";
+	char const *long_max_str = "2147483647";
+	char const *ulong_max_str = "4294967295";
+	char const *long_min_hexstr = "80000000";
+	char const *long_max_hexstr = "7fffffff";
+	char const *ulong_max_hexstr = "ffffffff";
+
+	T(expect_success(long_min_str, "%ld", long_min));
+	T(expect_success(long_min_hexstr, "%lx", long_min));
+	T(expect_success(long_max_str, "%ld", long_max));
+	T(expect_success(long_max_hexstr, "%lx", long_max));
+	T(expect_success(ulong_max_str, "%lu", ulong_max));
+	T(expect_success(ulong_max_hexstr, "%lx", ulong_max));
+	T(expect_success(long_max_str, "%ld", long_max));
+
+	T(expect_success(" +123", "%+*ld", 5, 123));
+	T(expect_success("00000123", "%08lu", 123));
+	T(expect_success("131415", "%d%lu%d", 13, 14L, 15));
+
 	/*
-	 * %l is deprecated on 32-bit systems (see crbug.com/984041), but is
-	 * is still functional on 64-bit systems.
+	 * %i and %li are only supported via the CONFIG_PRINTF_LONG_IS_32BITS
+	 * configuration (see https://issuetracker.google.com/issues/172210614).
 	 */
-	if (sizeof(long) == sizeof(uint32_t)) {
-		T(expect_success(err_str, "%lx", 0x7b));
-		T(expect_success(err_str, "%08lu", 0x7b));
-		T(expect_success("13ERROR", "%d%lu", 13, 14));
-	} else {
-		T(expect_success("7b", "%lx", 0x7b));
-		T(expect_success("00000123", "%08lu", 123));
-		T(expect_success("131415", "%d%lu%d", 13, 14L, 15));
-	}
+	T(expect_success("123", "%i", 123));
+	T(expect_success("123", "%li", 123));
+
+	return EC_SUCCESS;
+}
+
+test_static int test_vsnprintf_64bit_long_supported(void)
+{
+	/* These lines are only executed when sizeof(long) is 64-bits but are
+	 * still compiled by systems with 32-bit longs, so the casts are needed
+	 * to avoid compilation errors.
+	 */
+	long long_min = (long)INT64_MIN;
+	long long_max = (long)INT64_MAX;
+	unsigned long ulong_max = (unsigned long)UINT64_MAX;
+	char const *long_min_str = "-9223372036854775808";
+	char const *long_max_str = "9223372036854775807";
+	char const *ulong_max_str = "18446744073709551615";
+	char const *long_min_hexstr = "8000000000000000";
+	char const *long_max_hexstr = "7fffffffffffffff";
+	char const *ulong_max_hexstr = "ffffffffffffffff";
+
+	T(expect_success(long_min_str, "%ld", long_min));
+	T(expect_success(long_min_hexstr, "%lx", long_min));
+	T(expect_success(long_max_str, "%ld", long_max));
+	T(expect_success(long_max_hexstr, "%lx", long_max));
+	T(expect_success(ulong_max_str, "%lu", ulong_max));
+	T(expect_success(ulong_max_hexstr, "%lx", ulong_max));
+	T(expect_success(long_max_str, "%ld", long_max));
+
+	T(expect_success(" +123", "%+*ld", 5, 123));
+	T(expect_success("00000123", "%08lu", 123));
+	T(expect_success("131415", "%d%lu%d", 13, 14L, 15));
+
+	T(expect_success(err_str, "%i", 123));
+	T(expect_success(err_str, "%li", 123));
 
 	return EC_SUCCESS;
 }
 
+test_static int test_vsnprintf_long_not_supported(void)
+{
+	T(expect_success(err_str, "%ld", 0x7b));
+	T(expect_success(err_str, "%li", 0x7b));
+	T(expect_success(err_str, "%lu", 0x7b));
+	T(expect_success(err_str, "%lx", 0x7b));
+	T(expect_success(err_str, "%08lu", 123));
+	T(expect_success("13ERROR", "%d%lu%d", 13, 14L, 15));
+
+	T(expect_success(err_str, "%i", 123));
+	T(expect_success(err_str, "%li", 123));
+
+	return EC_SUCCESS;
+}
+
+test_static int test_vsnprintf_long(void)
+{
+	/*
+	 * %l is functional on 64-bit systems but is not supported on 32-bit
+	 * systems (see https://issuetracker.google.com/issues/172210614) unless
+	 * explicitly enabled via configuration.
+	 */
+	if (IS_ENABLED(CONFIG_PRINTF_LONG_IS_32BITS))
+		return test_vsnprintf_32bit_long_supported();
+	else if (sizeof(long) == sizeof(uint64_t))
+		return test_vsnprintf_64bit_long_supported();
+	else
+		return test_vsnprintf_long_not_supported();
+}
+
 test_static int test_vsnprintf_pointers(void)
 {
 	void *ptr = (void *)0x55005E00;
@@ -288,12 +383,13 @@ void run_test(int argc, char **argv)
 
 	RUN_TEST(test_vsnprintf_args);
 	RUN_TEST(test_vsnprintf_int);
+	RUN_TEST(test_printf_long32_enabled);
+	RUN_TEST(test_vsnprintf_long);
 	RUN_TEST(test_vsnprintf_pointers);
 	RUN_TEST(test_vsnprintf_chars);
 	RUN_TEST(test_vsnprintf_strings);
 	RUN_TEST(test_vsnprintf_timestamps);
 	RUN_TEST(test_vsnprintf_hexdump);
 	RUN_TEST(test_vsnprintf_combined);
-
 	test_print_result();
 }
diff --git a/util/config_allowed.txt b/util/config_allowed.txt
index 58317b8384..0e352e7602 100644
--- a/util/config_allowed.txt
+++ b/util/config_allowed.txt
@@ -716,7 +716,7 @@ CONFIG_POWER_SIGNAL_INTERRUPT_STORM_DETECT_THRESHOLD
 CONFIG_POWER_SIGNAL_RUNTIME_CONFIG
 CONFIG_POWER_TRACK_HOST_SLEEP_STATE
 CONFIG_PRESERVE_LOGS
-CONFIG_PRINTF_LEGACY_LI_FORMAT
+CONFIG_PRINTF_LONG_IS_32BITS
 CONFIG_PRINT_IN_INT
 CONFIG_PROGRAM_MEMORY_BASE
 CONFIG_PROGRAM_MEMORY_BASE_LOAD