This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] support: Increase usability of TEST_COMPARE
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Mon, 8 Jan 2018 08:44:32 -0800
- Subject: Re: [PATCH] support: Increase usability of TEST_COMPARE
- Authentication-results: sourceware.org; auth=none
- References: <20180108123111.2CD4B4018908A@oldenburg.str.redhat.com>
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);
>
--
Cheers,
Carlos.