On 12/01/2017 07:53 AM, Florian Weimer wrote:
> +/* Compare the two integers LEFT and RIGHT and report failure if they
> + are different. */
> +#define TEST_COMPARE(left, right) \
> + ({ \
> + typedef __typeof__ (left) __left_type; \
> + typedef __typeof__ (right) __right_type; \
> + __left_type __left_value = (left); \
> + __right_type __right_value = (right); \
> + /* Prevent use with floating-point and boolean types. */ \
Why prevent booleans? They don't have problems when compared as integers.
> + _Static_assert ((__left_type) 0.1 == (__left_type) 0, \
> + "left value has floating-point type"); \
I suggest changing this to something like "(__left_type) 1.5 ==
(__left_type) 1", so that boolean types are allowed, and so that the
expression matches the string.
> + /* Make sure that type promotion does not alter the sign. */ \
> + enum \
> + { \
> + __left_is_unsigned = (__left_type) -1 > 0, \
> + __right_is_unsigned = (__right_type) -1 > 0, \
> + __left_promotes_to_right = __left_is_unsigned \
> + && sizeof (__left_value) < sizeof (__right_value), \
> + __right_promotes_to_left = __right_is_unsigned \
> + && sizeof (__right_value) < sizeof (__left_value) \
> + }; \
> + _Static_assert (__left_is_unsigned == __right_is_unsigned \
> + || __left_promotes_to_right \
> + || __right_promotes_to_left, \
> + "integer promotion may alter sign of operands"); \
This confuses integer promotion (which always converts to int or to
unsigned) with the usual integer conversions (which can convert to a
type wider than int or unsigned int). Integer promotion does not cause
any problems that TEST_COMPARE is trying to detect; on the contrary,
promotion helps to prevent such problems.
For example, if __left_value is signed char and __right_value is
unsigned short and unsigned short is narrower than int, the static
assertion will fail even though the comparison is safe because both
operands are promoted to int before comparison.
Please see the proposal in
<https://sourceware.org/ml/libc-alpha/2017-11/msg00903.html>, which
should do the right thing for such cases.
Subject: [PATCH] support: Add TEST_COMPARE macro
To: libc-alpha@sourceware.org
2017-12-01 Florian Weimer <fweimer@redhat.com>
* support/check.h (TEST_COMPARE): Define.
(support_test_compare_failure): Declare.
* support/Makefile (libsupport-routines): Add
support_test_compare_failure.
(tests): Add tst-test_compare.
* support /support_test_compare_failure.c: New file.
* support/tst-test_compare.c: Likewise.
@@ -54,6 +54,7 @@ libsupport-routines = \
support_record_failure \
support_run_diff \
support_shared_allocate \
+ support_test_compare_failure \
support_write_file_string \
support_test_main \
support_test_verify_impl \
@@ -143,6 +144,7 @@ tests = \
tst-support_capture_subprocess \
tst-support_format_dns_packet \
tst-support_record_failure \
+ tst-test_compare \
tst-xreadlink \
ifeq ($(run-built-tests),yes)
@@ -86,6 +86,64 @@ void support_test_verify_exit_impl (int status, const char *file, int line,
does not support reporting failures from a DSO. */
void support_record_failure (void);
+/* Compare the two integers LEFT and RIGHT and report failure if they
+ are different. */
+#define TEST_COMPARE(left, right) \
+ ({ \
+ typedef __typeof__ (left) __left_type; \
+ typedef __typeof__ (right) __right_type; \
+ __left_type __left_value = (left); \
+ __right_type __right_value = (right); \
+ /* Prevent use with floating-point and boolean types. */ \
+ _Static_assert ((__left_type) 0.1 == (__left_type) 0, \
+ "left value has floating-point type"); \
+ _Static_assert ((__right_type) 0.1 == (__right_type) 0, \
+ "right value has floating-point type"); \
+ /* Prevent accidental use with larger-than-long long types. */ \
+ _Static_assert (sizeof (__left_value) <= sizeof (long long), \
+ "left value fits into long long"); \
+ _Static_assert (sizeof (__right_value) <= sizeof (long long), \
+ "right value fits into long long"); \
+ /* Make sure that type promotion does not alter the sign. */ \
+ enum \
+ { \
+ __left_is_unsigned = (__left_type) -1 > 0, \
+ __right_is_unsigned = (__right_type) -1 > 0, \
+ __left_promotes_to_right = __left_is_unsigned \
+ && sizeof (__left_value) < sizeof (__right_value), \
+ __right_promotes_to_left = __right_is_unsigned \
+ && sizeof (__right_value) < sizeof (__left_value) \
+ }; \
+ _Static_assert (__left_is_unsigned == __right_is_unsigned \
+ || __left_promotes_to_right \
+ || __right_promotes_to_left, \
+ "integer promotion may alter sign of operands"); \
+ /* Compare the value. */ \
+ if (__left_value != __right_value) \
+ /* Pass the sign for printing the correct value. */ \
+ support_test_compare_failure \
+ (__FILE__, __LINE__, \
+ #left, __left_value, __left_value < 0, sizeof (__left_type), \
+ #right, __right_value, __right_value < 0, sizeof (__right_type)); \
+ })
+
+/* Internal implementation of TEST_COMPARE. LEFT_NEGATIVE and
+ RIGHT_NEGATIVE are used to store the sign separately, so that both
+ unsigned long long and long long arguments fit into LEFT_VALUE and
+ RIGHT_VALUE, and the function can still print the original value.
+ LEFT_SIZE and RIGHT_SIZE specify the size of the argument in bytes,
+ for hexadecimal formatting. */
+void support_test_compare_failure (const char *file, int line,
+ const char *left_expr,
+ long long left_value,
+ int left_negative,
+ int left_size,
+ const char *right_expr,
+ long long right_value,
+ int right_negative,
+ int right_size);
+
+
/* Internal function called by the test driver. */
int support_report_failure (int status)
__attribute__ ((weak, warn_unused_result));
new file mode 100644
@@ -0,0 +1,51 @@
+/* Reporting a numeric comparison failure.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <support/check.h>
+
+static void
+report (const char *which, const char *expr, long long value, int negative,
+ int size)
+{
+ printf (" %s: ", which);
+ if (negative)
+ printf ("%lld", value);
+ else
+ printf ("%llu", (unsigned long long) value);
+ unsigned long long mask
+ = (~0ULL) >> (8 * (sizeof (unsigned long long) - size));
+ printf (" (0x%llx); from: %s\n", (unsigned long long) value & mask, expr);
+}
+
+void
+support_test_compare_failure (const char *file, int line,
+ const char *left_expr,
+ long long left_value,
+ int left_negative,
+ int left_size,
+ const char *right_expr,
+ long long right_value,
+ int right_negative,
+ int right_size)
+{
+ support_record_failure ();
+ printf ("%s:%d: numeric comparison failure\n", file, line);
+ report (" left", left_expr, left_value, left_negative, left_size);
+ report ("right", right_expr, right_value, right_negative, right_size);
+}
new file mode 100644
@@ -0,0 +1,68 @@
+/* Basic test for the TEST_COMPARE macro.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <string.h>
+#include <support/check.h>
+#include <support/capture_subprocess.h>
+
+static void
+subprocess (void *closure)
+{
+ char ch = 1;
+ /* These tests should fail. */
+ TEST_COMPARE (ch, -1); /* Line 28. */
+ TEST_COMPARE (2LL, -2LL); /* Line 29. */
+ TEST_COMPARE (3L, (short) -3); /* Line 30. */
+}
+
+static int
+do_test (void)
+{
+ /* This should succeed. */
+ TEST_COMPARE (1, 1);
+ TEST_COMPARE (2LL, 2U);
+
+ struct support_capture_subprocess proc = support_capture_subprocess
+ (&subprocess, NULL);
+
+ /* Discard the reported error. */
+ support_record_failure_reset ();
+
+ puts ("info: *** subprocess output starts ***");
+ fputs (proc.out.buffer, stdout);
+ puts ("info: *** subprocess output ends ***");
+
+ TEST_VERIFY
+ (strcmp (proc.out.buffer,
+ "tst-test_compare.c:28: numeric comparison failure\n"
+ " left: 1 (0x1); from: ch\n"
+ " right: -1 (0xffffffff); from: -1\n"
+ "tst-test_compare.c:29: numeric comparison failure\n"
+ " left: 2 (0x2); from: 2LL\n"
+ " right: -2 (0xfffffffffffffffe); from: -2LL\n"
+ "tst-test_compare.c:30: numeric comparison failure\n"
+ " left: 3 (0x3); from: 3L\n"
+ " right: -3 (0xfffd); from: (short) -3\n") == 0);
+
+ /* Check that there is no output on standard error. */
+ support_capture_subprocess_check (&proc, "TEST_COMPARE", 0, sc_allow_stdout);
+
+ return 0;
+}
+
+#include <support/test-driver.c>