On 12/01/2017 08:05 PM, Paul Eggert wrote:
> 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.
I was worried about confusing diagnostics output. But after thinking
about it, it will not matter.
>
>> + _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.
I made the change in the attached patch.
>> + /* 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.
I moved the integer promotions to the start of the macro. As a result,
it now works with bit fields, too. I adjusted the comments and variable
names further down, too. Please have a look and tell me if this
addresses your concerns.
(I still think that a pure run-time check is conceptually simpler,
covers more cases, and is a reasonable compromise for a test case.)
> 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.
This should work now.
Thanks,
Florian
Subject: [PATCH] support: Add TEST_COMPARE macro
To: libc-alpha@sourceware.org
2017-12-04 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,65 @@ 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) \
+ ({ \
+ /* + applies the integer promotions, for bitfield support. */ \
+ 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) 1.0 == (__left_type) 1.5, \
+ "left value has floating-point type"); \
+ _Static_assert ((__right_type) 1.0 == (__right_type) 1.5, \
+ "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 integer conversions does not alter the sign. */ \
+ enum \
+ { \
+ __left_is_unsigned = (__left_type) -1 > 0, \
+ __right_is_unsigned = (__right_type) -1 > 0, \
+ __left_converts_to_right = __left_is_unsigned \
+ && sizeof (__left_value) < sizeof (__right_value), \
+ __right_converts_to_left = __right_is_unsigned \
+ && sizeof (__right_value) < sizeof (__left_value) \
+ }; \
+ _Static_assert (__left_is_unsigned == __right_is_unsigned \
+ || __left_converts_to_right \
+ || __right_converts_to_left, \
+ "integer conversions 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,55 @@
+/* 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 ();
+ if (left_size != right_size)
+ printf ("%s:%d: numeric comparison failure (widths %d and %d)\n",
+ file, line, left_size * 8, right_size * 8);
+ else
+ 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,98 @@
+/* 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. */
+}
+
+struct bitfield
+{
+ int i2 : 2;
+ int i3 : 3;
+ unsigned int u2 : 2;
+ unsigned int u3 : 3;
+ int i31 : 31;
+ unsigned int u31 : 31 ;
+ long long int i63 : 63;
+ unsigned long long int u63 : 63;
+};
+
+static int
+do_test (void)
+{
+ /* This should succeed. */
+ TEST_COMPARE (1, 1);
+ TEST_COMPARE (2LL, 2U);
+ {
+ char i8 = 3;
+ unsigned short u16 = 3;
+ TEST_COMPARE (i8, u16);
+ }
+
+ struct bitfield bitfield = { 0 };
+ TEST_COMPARE (bitfield.i2, bitfield.i3);
+ TEST_COMPARE (bitfield.u2, bitfield.u3);
+ TEST_COMPARE (bitfield.u2, bitfield.i3);
+ TEST_COMPARE (bitfield.u3, bitfield.i3);
+ TEST_COMPARE (bitfield.i2, bitfield.u3);
+ TEST_COMPARE (bitfield.i3, bitfield.u2);
+ TEST_COMPARE (bitfield.i63, bitfield.i63);
+ TEST_COMPARE (bitfield.u63, bitfield.u63);
+ TEST_COMPARE (bitfield.i31, bitfield.i63);
+ TEST_COMPARE (bitfield.i63, bitfield.i31);
+
+ 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"
+ " (widths 64 and 32)\n"
+ " left: 3 (0x3); from: 3L\n"
+ " right: -3 (0xfffffffd); 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>