Re: [Qemu-devel] [PATCH 3/3] tests: Add unit tests for mulu64 and muls64

[Blue Swirl]

On Tue, Jan 29, 2013 at 8:52 PM, Richard Henderson <address@hidden> wrote:
> On 01/29/2013 12:06 PM, Blue Swirl wrote:
>>>
>>> +static const Test test_u_data[] = {
>>> +    { 1, 1, 0, 1 },
>>> +    { 10000, 10000, 0, 100000000 },
>>> +    { -1ull, 2, 1, -2ull },
>>
>>
>> Shouldn't '1' be '-1'? How can this test pass?
>
>
> This is 0xffff_ffff_ffff_ffff * 2 = 0x1_ffff_ffff_ffff_fffe
>
> with the knowledge that -1 = 0xf...f and -2 = 0xf...e.

OK, I somehow used signed multiplication rules.

>
>
>>> +    { -1ull, -1ull, -2ull, 1 },
>>
>>
>> This looks buggy too.
>
>
> See above.
>
>
>>> +    { -10, -10, 0, 100 },
>>> +    { 10000, 10000, 0, 100000000 },
>>> +    { -1, 2, -1, -2 },
>>> +    { 0x1122334455667788ll, 0x1122334455667788ull,
>>
>>
>> Spurious 'll', also below.
>
>
> Why spurious?

I'd use ULL/ull everywhere for hex constants.

>
>
>>> +static void test_u(void)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(test_u_data); ++i) {
>>> +        uint64_t rl, rh;
>>> +        mulu64(&rl, &rh, test_u_data[i].a, test_u_data[i].b);
>>> +        g_assert_cmpuint(rl, ==, test_u_data[i].rl);
>>
>>
>> This could explain why the test passes: g_assert_cmpuint() uses
>> unsigned ints so there is truncation from uint64_t.
>
>
> Does it?  It sure doesn't look like it:
>
>> #define g_assert_cmpuint(n1, cmp, n2)   do { guint64 __n1 = (n1), __n2 =
>> (n2); \
>>                                              if (__n1 cmp __n2) ; else \
>>                                                g_assertion_message_cmpnum
>> (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
>>                                                  #n1 " " #cmp " " #n2,
>> __n1, #cmp, __n2, 'i'); } while (0)
>
>
> I see guint64 in there, thus no truncation.

The manual only talks about unsigned integers, maybe they really mean guint64:
http://developer.gnome.org/glib/2.34/glib-Testing.html#g-assert-cmpuint

>
>
>>> +static void test_s(void)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(test_u_data); ++i) {
>>
>>
>> test_s_data
>
>
> Good catch.  I wasn't running all of the test_s data points.
>
>
> r~