[PATCH] support: Add TEST_COMPARE macro

[PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

This is currently bundled with the MPK changes, but it is useful 
separately, so I'm submitting it here as well.

I tried to expand the comments regarding the use of de-facto 65-bit 
integers, as Adhemerval suggested before.

Thanks,
Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Siddhesh Poyarekar]

On Thursday 23 November 2017 03:49 PM, Florian Weimer wrote:
> This is currently bundled with the MPK changes, but it is useful
> separately, so I'm submitting it here as well.
> 
> I tried to expand the comments regarding the use of de-facto 65-bit
> integers, as Adhemerval suggested before.

ENOPATCH

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

On 11/23/2017 11:52 AM, Siddhesh Poyarekar wrote:
> On Thursday 23 November 2017 03:49 PM, Florian Weimer wrote:
>> This is currently bundled with the MPK changes, but it is useful
>> separately, so I'm submitting it here as well.
>>
>> I tried to expand the comments regarding the use of de-facto 65-bit
>> integers, as Adhemerval suggested before.
> 
> ENOPATCH

Uh-oh.  Next try.

Thanks,
Florian

[-- Attachment #2: test_compare.patch --]
[-- Type: text/x-patch, Size: 7747 bytes --]

Subject: [PATCH] support: Add TEST_COMPARE macro
To: libc-alpha@sourceware.org

2017-11-23  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..eb02d81077 100644
--- a/support/check.h
+++ b/support/check.h
@@ -86,6 +86,40 @@ 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)                                       \
+  ({                                                                    \
+    __typeof__ (left) __left_value = (left);                            \
+    __typeof__ (right) __right_value = (right);                         \
+    /* 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");                 \
+    /* Compare the value and the sign, to avoid false equality.  */     \
+    /* (A signed value could be converted to an unsigned type.)  */     \
+    if (__left_value != __right_value                                   \
+        || ((__left_value > 0) != (__right_value > 0)))                 \
+      support_test_compare_failure                                      \
+        (__FILE__, __LINE__,                                            \
+         #left, __left_value, __left_value > 0,                         \
+         #right, __right_value, __right_value > 0);                     \
+  })
+
+/* Internal implementation of TEST_COMPARE.  LEFT_POSITIVE and
+   RIGHT_POSITIVE 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.  */
+void support_test_compare_failure (const char *file, int line,
+                                   const char *left_expr,
+                                   long long left_value,
+                                   int left_positive,
+                                   const char *right_expr,
+                                   long long right_value,
+                                   int right_positive);
+
 /* 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..007f0154f0
--- /dev/null
+++ b/support/support_test_compare_failure.c
@@ -0,0 +1,46 @@
+/* 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 positive)
+{
+  printf ("  %s: ", which);
+  if (positive)
+    printf ("%llu", (unsigned long long) value);
+  else
+    printf ("%lld", value);
+  printf (" (0x%llx); from: %s\n", (unsigned long long) value, expr);
+}
+
+void
+support_test_compare_failure (const char *file, int line,
+                              const char *left_expr,
+                              long long left_value,
+                              int left_positive,
+                              const char *right_expr,
+                              long long right_value,
+                              int right_positive)
+{
+  support_record_failure ();
+  printf ("%s:%d: numeric comparison failure\n", file, line);
+  report (" left", left_expr, left_value, left_positive);
+  report ("right", right_expr, right_value, right_positive);
+}
diff --git a/support/tst-test_compare.c b/support/tst-test_compare.c
new file mode 100644
index 0000000000..fc1d6a7095
--- /dev/null
+++ b/support/tst-test_compare.c
@@ -0,0 +1,61 @@
+/* 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)
+{
+  unsigned long long left = -1;
+  long long right = -1;
+  /* This should fail.  */
+  TEST_COMPARE (left, right);   /* Line 29.  */
+}
+
+static int
+do_test (void)
+{
+  /* This should succeed.  */
+  TEST_COMPARE (1, 1);
+  TEST_COMPARE (0ULL, 0LL);
+
+  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:29: numeric comparison failure\n"
+             "   left: 18446744073709551615 (0xffffffffffffffff); from: left\n"
+             "  right: -1 (0xffffffffffffffff); from: right\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>

Re: [PATCH] support: Add TEST_COMPARE macro

[Andreas Schwab]

On Nov 23 2017, Florian Weimer <fweimer@redhat.com> wrote:

> +/* Compare the two integers LEFT and RIGHT and report failure if they
> +   are different.  */
> +#define TEST_COMPARE(left, right)                                       \
> +  ({                                                                    \
> +    __typeof__ (left) __left_value = (left);                            \
> +    __typeof__ (right) __right_value = (right);                         \
> +    /* 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");                 \
> +    /* Compare the value and the sign, to avoid false equality.  */     \
> +    /* (A signed value could be converted to an unsigned type.)  */     \

Why do you need that?  Any signed value converted to unsigned is still
unique.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/23/2017 12:09 PM, Andreas Schwab wrote:
> On Nov 23 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> +/* Compare the two integers LEFT and RIGHT and report failure if they
>> +   are different.  */
>> +#define TEST_COMPARE(left, right)                                       \
>> +  ({                                                                    \
>> +    __typeof__ (left) __left_value = (left);                            \
>> +    __typeof__ (right) __right_value = (right);                         \
>> +    /* 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");                 \
>> +    /* Compare the value and the sign, to avoid false equality.  */     \
>> +    /* (A signed value could be converted to an unsigned type.)  */     \
> 
> Why do you need that?  Any signed value converted to unsigned is still
> unique.

Not sure I understand.

#include <stdio.h>

int
main (void)
{
   long long left = -1;
   unsigned long long right = -1;
   printf ("%d\n", left == right);
}

This prints

1

for me, although mathematically, the values are different.

Thanks,
Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Andreas Schwab]

On Nov 23 2017, Florian Weimer <fweimer@redhat.com> wrote:

> for me, although mathematically, the values are different.

Are they?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/23/2017 12:49 PM, Andreas Schwab wrote:
> On Nov 23 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> for me, although mathematically, the values are different.
> 
> Are they?

Taken by themselves, they most certainly are.  It's just that C requires 
conversion to unsigned long long before making the comparison, so the 
difference is obscured.

I think this matters for writing portable tests, where some expression 
might be signed on one architecture, and unsigned on another.

Thanks,
Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Andreas Schwab]

On Nov 23 2017, Florian Weimer <fweimer@redhat.com> wrote:

> On 11/23/2017 12:49 PM, Andreas Schwab wrote:
>> On Nov 23 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> for me, although mathematically, the values are different.
>>
>> Are they?
>
> Taken by themselves, they most certainly are.

Both are -1.

> I think this matters for writing portable tests, where some expression
> might be signed on one architecture, and unsigned on another.

Comparing two different domains is always going to be surprising.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

It'd be cleaner and simpler to test for negative than positive integers. "bool 
negative = n < 0;" extracts N's sign bit if N is signed and requires no machine 
instructions if N is unsigned; "bool positive = n > 0;" is trickier.

I don't know what the context for this new macro is, and like Andreas I'm a bit 
puzzled as to its intended use.

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/24/2017 09:46 AM, Paul Eggert wrote:
> It'd be cleaner and simpler to test for negative than positive integers. 
> "bool negative = n < 0;" extracts N's sign bit if N is signed and 
> requires no machine instructions if N is unsigned; "bool positive = n > 
> 0;" is trickier.

I expect that GCC will eventually warn about such tautological 
comparisons.  The current approach avoids such warnings.

> I don't know what the context for this new macro is, and like Andreas 
> I'm a bit puzzled as to its intended use.

The purpose is to check if two values are the same, and print them (and 
record a test failure) if they are not.

Thanks,
Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

Florian Weimer wrote:
> I expect that GCC will eventually warn about such tautological comparisons.  The 
> current approach avoids such warnings.

I've found such warnings to be more trouble than they're worth, in cases like 
these. (Why would we want GCC to warn that it's doing an optimization? We *like* 
optimizations! :-) And efforts to suppress such warnings, such as using "x > 0" 
instead of "x < 0", typically cause the compiler to generate worse code.

If performance is not important here then I suppose it's OK. However, it's a bad 
habit and I don't want the habit to leak into code where performance matters.

>> I don't know what the context for this new macro is, and like Andreas I'm a 
>> bit puzzled as to its intended use.
> 
> The purpose is to check if two values are the same, and print them (and record a 
> test failure) if they are not.

Why would we want to compare (say) an uintptr_t value with a pid_t value? That 
sounds like a typo, and I'd rather see a compile-time diagnostic for the typo. 
It should be easy to arrange for such a diagnostic, and then we won't have to 
worry about run-time sign checking or pacifying GCC about comparisons.

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/24/2017 07:31 PM, Paul Eggert wrote:
> Florian Weimer wrote:
>> I expect that GCC will eventually warn about such tautological 
>> comparisons.  The current approach avoids such warnings.
> 
> I've found such warnings to be more trouble than they're worth, in cases 
> like these. (Why would we want GCC to warn that it's doing an 
> optimization? We *like* optimizations! :-) And efforts to suppress such 
> warnings, such as using "x > 0" instead of "x < 0", typically cause the 
> compiler to generate worse code.
> 
> If performance is not important here then I suppose it's OK. However, 
> it's a bad habit and I don't want the habit to leak into code where 
> performance matters.

It's for writing tests.  Performance does not matter.  And even for the 
tests, the additional which cannot be optimized away is on the failure path.

>>> I don't know what the context for this new macro is, and like Andreas 
>>> I'm a bit puzzled as to its intended use.
>>
>> The purpose is to check if two values are the same, and print them 
>> (and record a test failure) if they are not.
> 
> Why would we want to compare (say) an uintptr_t value with a pid_t 
> value? That sounds like a typo, and I'd rather see a compile-time 
> diagnostic for the typo. It should be easy to arrange for such a 
> diagnostic, and then we won't have to worry about run-time sign checking 
> or pacifying GCC about comparisons.

The POSIX interfaces are not strongly typed.  Types like pid_t should 
have been structs, but compilers and ABIs simply weren't ready for that.

More importantly for us right now is that comparisons between actual and 
expected values often have differing types.  Concrete types vary between 
architectures, and not just their sizes (e.g., 32-bit architectures use 
longs where 64-bit architectures use int).  Considering that the macro 
tries very hard to avoid bit-altering conversions, the worst you can get 
is a spurious test failure, so I don't see why this is a problem.

(I could perhaps add an assert that the argument types are not floating 
point types.  But we use floating point values so rarely that this 
didn't occur to me earlier.)

Thanks,
Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Florian Weimer wrote:
> comparisons between actual and 
> expected values often have differing types.  Concrete types vary between 
> architectures, and not just their sizes (e.g., 32-bit architectures use longs 
> where 64-bit architectures use int).

Yes, but I'm still not seeing the motivation for doing this via a run-time test 
rather than a compile-time test. Can you show an example or two of problems that 
the signedness part of this macro will fix?

Also, why can't we rely on -Wsign-compare to warn about these problems?

As for what I have in mind for a compile-time test, please see the attached 
code. This also checks for floating-point types, and it avoids a false alarm in 
some cases when a narrower-than-int unsigned type is promoted (that's what the 
"+ (a)" and "+ (b)" are for). Finally, it suppresses -Wtype-limits since those 
GCC warnings are counterproductive in this kind of code.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test-compare.c --]
[-- Type: text/x-csrc; name="test-compare.c", Size: 2256 bytes --]

#pragma GCC diagnostic ignored "-Wtype-limits"

#define TYPE_IS_INTEGER(t) ((t) 1.5 == 1)
#define TYPE_IS_SIGNED(t) ((t) -1 < (t) 0)
#define TYPE_FITS_IN_LONG_LONG(t) \
  (sizeof (t) <= sizeof (long long) && TYPE_IS_INTEGER (t))

#define TEST_COMPARE(a, b)						\
  ({                                                                    \
    typedef __typeof__ (a) __ta;					\
    typedef __typeof__ (b) __tb;					\
    __ta __va = (a);							\
    __tb __vb = (b);							\
									\
    /* Prevent accidental use with types that do not fit in long long.  */ \
    _Static_assert (TYPE_FITS_IN_LONG_LONG (__ta),			\
                    "left value fits into long long");                  \
    _Static_assert (TYPE_FITS_IN_LONG_LONG (__tb),			\
                    "right value fits into long long");                 \
									\
    /* Prevent accidental use with types that might not be compared
       numerically.  The comparison is safe if the signedness of A and
       B agree or if the unsigned value is narrower than the promoted
       signed value.  */						\
    _Static_assert							\
     ((TYPE_IS_SIGNED (__ta) == TYPE_IS_SIGNED (__tb)			\
       || (TYPE_IS_SIGNED (__ta) && sizeof (__tb) < sizeof + (a))	\
       || (TYPE_IS_SIGNED (__tb) && sizeof (__ta) < sizeof + (b))),	\
      "TEST_COMPARE args might not compare numerically");		\
									\
    /* Report an error at run-time if A does not equal B.  */		\
    if (__va != __vb)							\
      support_test_compare_failure                                      \
        (__FILE__, __LINE__,                                            \
         #a, __va, __va < 0,						\
         #b, __vb, __vb < 0);						\
  })

void support_test_compare_failure (const char *file, int line,
                                   const char *left_expr,
                                   long long left_value,
                                   _Bool left_negative,
                                   const char *right_expr,
                                   long long right_value,
                                   _Bool right_negative);
int main (void)
{
  TEST_COMPARE (0, 1);
  TEST_COMPARE (0u, 1);
  TEST_COMPARE ((unsigned short) 0, 1);
}

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/25/2017 01:05 AM, Paul Eggert wrote:
> #pragma GCC diagnostic ignored "-Wtype-limits"

I don't think we want to use this throughout our test suite.

Thanks,
Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

On 11/27/2017 02:46 AM, Florian Weimer wrote:
>> #pragma GCC diagnostic ignored "-Wtype-limits"
>
> I don't think we want to use this throughout our test suite. 

Sure, but we can add it just for TEST_COMPARE, since -Wtype-limits is a 
false alarm and that is causing a hassle for this implementation.

Come to think of it, perhaps it would be easier to add -Wsign-compare 
only for TEST_COMPARE, and remove all the other gorp. What we're trying 
to do here, is to have the effect of -Wsign-compare. Surely it's better 
to do this check at compile-time with the compiler's help, rather than 
try to dodge the compiler and do the check at run-time.

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/27/2017 07:00 PM, Paul Eggert wrote:
> Come to think of it, perhaps it would be easier to add -Wsign-compare 
> only for TEST_COMPARE, and remove all the other gorp. What we're trying 
> to do here, is to have the effect of -Wsign-compare. Surely it's better 
> to do this check at compile-time with the compiler's help, rather than 
> try to dodge the compiler and do the check at run-time.

I'm not certain about that at all—the *types* could well differ in 
signedness between architectures (as it is common for char).  This is 
the main reason why I only want to test the values.

Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

On 11/27/2017 10:06 AM, Florian Weimer wrote:
> I'm not certain about that at all—the *types* could well differ in 
> signedness between architectures (as it is common for char). 

char should not be a problem: on all glibc platforms, it's widened to 
int before comparison, so its signedness is not relevant to the comparison.

Can you give an example of where compile-time checking would go awry?

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

On 11/27/2017 07:33 PM, Paul Eggert wrote:
> On 11/27/2017 10:06 AM, Florian Weimer wrote:
>> I'm not certain about that at all—the *types* could well differ in 
>> signedness between architectures (as it is common for char). 
> 
> char should not be a problem: on all glibc platforms, it's widened to 
> int before comparison, so its signedness is not relevant to the comparison.

I don't understand.

It is perfectly reasonable to warn for

    ch == EOF

(with ch of type char) because that is always a bug, and for us (who 
strive to write portable code) even if char is signed.

It is not reasonable at all to warn for:

    ch == '\n'
    ch == 0
    ch == '\xff'

Surely the type should feed into the warning, but it can't be the sole 
arbitrator.

Florian

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

On 11/27/2017 10:43 AM, Florian Weimer wrote:
> I don't understand.
>
> It is perfectly reasonable to warn for
>
>    ch == EOF
>
> (with ch of type char) 

Sure, but that's a different topic. I was writing about the topic at 
hand, which is that C integer comparison sometimes disagrees with 
mathematical comparison. When ch is of type char, (ch == EOF) always 
returns the mathematically-correct answer on glibc platforms. None of 
the proposed TEST_COMPARE patches would catch the char-vs-EOF problem, 
because they're not designed to catch it: they're designed to catch the 
C-vs-mathematical comparison problem.

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

On 11/27/2017 07:53 PM, Paul Eggert wrote:
> On 11/27/2017 10:43 AM, Florian Weimer wrote:
>> I don't understand.
>>
>> It is perfectly reasonable to warn for
>>
>>    ch == EOF
>>
>> (with ch of type char) 
> 
> Sure, but that's a different topic. I was writing about the topic at 
> hand, which is that C integer comparison sometimes disagrees with 
> mathematical comparison. When ch is of type char, (ch == EOF) always 
> returns the mathematically-correct answer on glibc platforms. None of 
> the proposed TEST_COMPARE patches would catch the char-vs-EOF problem, 
> because they're not designed to catch it: they're designed to catch the 
> C-vs-mathematical comparison problem.

Yeah, we got side-tracked.

I incorporated your suggestion about rejecting sign-altering promotions 
into the attached patch.  It also prints hexadecimal values with the 
appropriate (type-dependent) width.

Thanks,
Florian

[-- Attachment #2: test_compare.patch --]
[-- Type: text/x-patch, Size: 10050 bytes --]

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.

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..57adf84579 100644
--- a/support/check.h
+++ b/support/check.h
@@ -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));
diff --git a/support/support_test_compare_failure.c b/support/support_test_compare_failure.c
new file mode 100644
index 0000000000..ab55b9f51a
--- /dev/null
+++ b/support/support_test_compare_failure.c
@@ -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);
+}
diff --git a/support/tst-test_compare.c b/support/tst-test_compare.c
new file mode 100644
index 0000000000..2356e800dc
--- /dev/null
+++ b/support/tst-test_compare.c
@@ -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>

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

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.

Re: [PATCH] support: Add TEST_COMPARE macro

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 5786 bytes --]

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

[-- Attachment #2: test_compare.patch --]
[-- Type: text/x-patch, Size: 11119 bytes --]

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>

Re: [PATCH] support: Add TEST_COMPARE macro

[Paul Eggert]

On 12/04/2017 08:35 AM, Florian Weimer wrote:
> +      __left_converts_to_right = __left_is_unsigned                     \
> +        && sizeof (__left_value) < sizeof (__right_value),              \

Thanks, this implementation looks good. Two minor style issues. First, 
please indent this decl according to the usual glibc style, where 
multiline expressions are parenthesized (or perhaps you can put the " = 
" on the next line). Second, I suggest changing the name from 
"__left_converts_to_right" to "__unsigned_left_converts_to_wider", as 
this more-accurately describes the expression (as there are cases where 
the left's value is converted to the right's type but 
__left_converts_to_right is false). Similarly for 
__right_converts_to_left of course.

Re: [PATCH] support: Add TEST_COMPARE macro

[Joseph Myers]

On Fri, 24 Nov 2017, Paul Eggert wrote:

> Also, why can't we rely on -Wsign-compare to warn about these problems?

I'd like to be able to build glibc with -Wsign-compare (or indeed 
-Wextra), but I think we're a long way from a clean -Wsign-compare build 
at present on even one platform.

-- 
Joseph S. Myers
joseph@codesourcery.com