support: Increase usability of TEST_COMPARE

The previous implementation of the TEST_COMPARE macro would fail
to compile code like this:

  int ret = res_send (query, sizeof (query), buf, sizeof (buf));
  TEST_COMPARE (ret,
                sizeof (query)
                + 2             /* Compression reference.  */
                + 2 + 2 + 4 + 2 /* Type, class, TTL, RDATA length.  */
                + 1             /* Pascal-style string length.  */
                + strlen (expected_name));

This resulted in a failed static assertion, "integer conversions
may alter sign of operands".  A user of the TEST_COMPARE would have
to add a cast to fix this.

This patch reverts to the original proposed solution of a run-time
check, making TEST_COMPARE usable for comparisons of numbers with
types with different signedness in more contexts.

2018-01-08  Florian Weimer  <fweimer@redhat.com>

	* support/check.h (TEST_COMPARE): Allow sign mismatch at compile
	time.  Pass positive flag instead of negative flag to
	support_test_compare_failure.
	(support_test_compare_failure): Change negative parameter to
	positive.
	* support/support_test_compare_failure.c (report)
	(support_test_compare_failure): Likewise.
	* support/tst-test_compare.c (return_ssize_t, return_int): New.
	(do_test): Check int/size_t, ssize_t/size_t comparisons.

On 01/08/2018 04:31 AM, Florian Weimer wrote:
> The previous implementation of the TEST_COMPARE macro would fail
> to compile code like this:
> 
>   int ret = res_send (query, sizeof (query), buf, sizeof (buf));
>   TEST_COMPARE (ret,
>                 sizeof (query)
>                 + 2             /* Compression reference.  */
>                 + 2 + 2 + 4 + 2 /* Type, class, TTL, RDATA length.  */
>                 + 1             /* Pascal-style string length.  */
>                 + strlen (expected_name));

That's unfortunate, I expect many people want to just "add" various
sizes with the length of the string data in the same sequence.
 
> This resulted in a failed static assertion, "integer conversions
> may alter sign of operands".  A user of the TEST_COMPARE would have
> to add a cast to fix this.
> 
> This patch reverts to the original proposed solution of a run-time
> check, making TEST_COMPARE usable for comparisons of numbers with
> types with different signedness in more contexts.

I went back and reviewed the original discussions, and have extracted the
following points which seemed to need resolution. I have provided my input
here and considered why each was relevant to this patch.

* Computing "n > 0" is more costly.
  - This is not an issue because the code here is for test purposes only.
  - If we find that tests need a faster TEST_COMPARE then we can work on
    those cases to add optimizations to improve testing. This can be done
    in the future.

* Break bad habits for performance critical code.
  - Good habits make writing code easier, but at the end of the day if
    we care about performance we will carry out profile driven optimizations.
  - I don't think it is a serious issue here to use > 0, and it avoids the
    future compiler issues, along with expanding the scope and usability
    of TEST_COMPARE for test writers.

* Arrange for compile-time diagnostics of mismatched comparisons.
  - Such diagnostics are good, but do not solve the problem at hand with
    TEST_COMPARE. I think the original suggestion was good, and indeed
    seemed to cover all cases we needed.
  - The new example that doesn't work with TEST_COMPARE is a good counter-
    point to the original discussion.
  - I think that even one example where the compile-time diagnostics raise
    a false positive is enough to consider using run-time checking.

Given the above three points, and the fact that this makes TEST_COMPARE more
useful in more cases, this looks good to me.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2018-01-08  Florian Weimer  <fweimer@redhat.com>
> 
> 	* support/check.h (TEST_COMPARE): Allow sign mismatch at compile
> 	time.  Pass positive flag instead of negative flag to
> 	support_test_compare_failure.
> 	(support_test_compare_failure): Change negative parameter to
> 	positive.
> 	* support/support_test_compare_failure.c (report)
> 	(support_test_compare_failure): Likewise.
> 	* support/tst-test_compare.c (return_ssize_t, return_int): New.
> 	(do_test): Check int/size_t, ssize_t/size_t comparisons.
> 
> diff --git a/support/check.h b/support/check.h
> index de8fce0dc1..2ac25a187c 100644
> --- a/support/check.h
> +++ b/support/check.h
> @@ -95,7 +95,9 @@ void support_record_failure (void);
>      typedef __typeof__ (+ (right)) __right_type;                        \
>      __left_type __left_value = (left);                                  \
>      __right_type __right_value = (right);                               \
> -    /* Prevent use with floating-point and boolean types.  */           \
> +    int __left_is_positive = __left_value > 0;                          \
> +    int __right_is_positive = __right_value > 0;                        \

OK.

> +    /* Prevent use with floating-point 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,           \
> @@ -105,33 +107,18 @@ void support_record_failure (void);
>                      "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,                      \
> -      __unsigned_left_converts_to_wider = (__left_is_unsigned           \
> -                                           && (sizeof (__left_value)    \
> -                                               < sizeof (__right_value))), \
> -      __unsigned_right_converts_to_wider = (__right_is_unsigned         \
> -                                            && (sizeof (__right_value)  \
> -                                                < sizeof (__left_value))) \
> -    };                                                                  \
> -    _Static_assert (__left_is_unsigned == __right_is_unsigned           \
> -                    || __unsigned_left_converts_to_wider                \
> -                    || __unsigned_right_converts_to_wider,              \
> -                    "integer conversions may alter sign of operands");  \

OK.

>      /* Compare the value.  */                                           \
> -    if (__left_value != __right_value)                                  \
> +    if (__left_value != __right_value                                   \
> +        || __left_is_positive != __right_is_positive)                   \

OK. Add back check since we have removed compile-time checks.

>        /* 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)); \
> +         #left, __left_value, __left_is_positive, sizeof (__left_type), \
> +         #right, __right_value, __right_is_positive, sizeof (__right_type)); \

OK.

>    })
>  
> -/* Internal implementation of TEST_COMPARE.  LEFT_NEGATIVE and
> -   RIGHT_NEGATIVE are used to store the sign separately, so that both
> +/* Internal implementation of TEST_COMPARE.  LEFT_POSITIVE and
> +   RIGHT_POSITIVE are used to store the sign separately, so that both

OK.

>     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,
> @@ -139,11 +126,11 @@ void support_record_failure (void);
>  void support_test_compare_failure (const char *file, int line,
>                                     const char *left_expr,
>                                     long long left_value,
> -                                   int left_negative,
> +                                   int left_positive,
>                                     int left_size,
>                                     const char *right_expr,
>                                     long long right_value,
> -                                   int right_negative,
> +                                   int right_positive,
>                                     int right_size);
>  

OK.

>  
> diff --git a/support/support_test_compare_failure.c b/support/support_test_compare_failure.c
> index a789283f86..e5596fd121 100644
> --- a/support/support_test_compare_failure.c
> +++ b/support/support_test_compare_failure.c
> @@ -20,14 +20,14 @@
>  #include <support/check.h>
>  
>  static void
> -report (const char *which, const char *expr, long long value, int negative,
> +report (const char *which, const char *expr, long long value, int positive,
>          int size)
>  {
>    printf ("  %s: ", which);
> -  if (negative)
> -    printf ("%lld", value);
> -  else
> +  if (positive)
>      printf ("%llu", (unsigned long long) value);
> +  else
> +    printf ("%lld", value);

OK.

>    unsigned long long mask
>      = (~0ULL) >> (8 * (sizeof (unsigned long long) - size));
>    printf (" (0x%llx); from: %s\n", (unsigned long long) value & mask, expr);
> @@ -37,11 +37,11 @@ void
>  support_test_compare_failure (const char *file, int line,
>                                const char *left_expr,
>                                long long left_value,
> -                              int left_negative,
> +                              int left_positive,
>                                int left_size,
>                                const char *right_expr,
>                                long long right_value,
> -                              int right_negative,
> +                              int right_positive,
>                                int right_size)
>  {
>    support_record_failure ();
> @@ -50,6 +50,6 @@ support_test_compare_failure (const char *file, int line,
>              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);
> +  report (" left", left_expr, left_value, left_positive, left_size);
> +  report ("right", right_expr, right_value, right_positive, right_size);
>  }
> diff --git a/support/tst-test_compare.c b/support/tst-test_compare.c
> index 340268fadf..123ba1bc3c 100644
> --- a/support/tst-test_compare.c
> +++ b/support/tst-test_compare.c
> @@ -42,6 +42,22 @@ struct bitfield
>    unsigned long long int u63 : 63;
>  };
>  
> +/* Functions which return signed sizes are common, so test that these
> +   results can readily checked using TEST_COMPARE.  */

OK.

Any further tests we could add?

> +
> +static int
> +return_ssize_t (void)
> +{
> +  return 4;
> +}
> +
> +static int
> +return_int (void)
> +{
> +  return 4;
> +}
> +
> +
>  static int
>  do_test (void)
>  {
> @@ -53,6 +69,8 @@ do_test (void)
>      unsigned short u16 = 3;
>      TEST_COMPARE (i8, u16);
>    }
> +  TEST_COMPARE (return_ssize_t (), sizeof (char[4]));
> +  TEST_COMPARE (return_int (), sizeof (char[4]));

OK.

>  
>    struct bitfield bitfield = { 0 };
>    TEST_COMPARE (bitfield.i2, bitfield.i3);
>