Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer <fweimer at redhat dot com>]

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.

diff --git a/support/Makefile b/support/Makefile
index 384fecb65a..bb81825fc2 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -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)
diff --git a/support/check.h b/support/check.h
index bdcd12952a..f7a7bbdab2 100644
--- a/support/check.h
+++ b/support/check.h
@@ -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));
diff --git a/support/support_test_compare_failure.c b/support/support_test_compare_failure.c
new file mode 100644
index 0000000000..894145b56d
--- /dev/null
+++ b/support/support_test_compare_failure.c
@@ -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);
+}
diff --git a/support/tst-test_compare.c b/support/tst-test_compare.c
new file mode 100644
index 0000000000..e4dcfd462a
--- /dev/null
+++ b/support/tst-test_compare.c
@@ -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>