[PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast
Sui Jingfeng
15330273260 at 189.cn
Mon May 22 12:53:36 UTC 2023
Hi,
On 2023/5/22 20:13, Jani Nikula wrote:
> On Mon, 22 May 2023, Sui Jingfeng <15330273260 at 189.cn> wrote:
>> Hi,
>>
>> On 2023/5/22 19:29, Jani Nikula wrote:
>>> On Thu, 18 May 2023, Sui Jingfeng <15330273260 at 189.cn> wrote:
>>>> On 2023/5/17 18:59, David Laight wrote:
>>>>> From: 15330273260 at 189.cn
>>>>>> Sent: 16 May 2023 18:30
>>>>>>
>>>>>> From: Sui Jingfeng <suijingfeng at loongson.cn>
>>>>>>
>>>>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type,
>>>>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type.
>>>>> Nope, u16 gets promoted to 'signed int' and the result of the
>>>>> multiply is also signed.
>>>> I believe that signed or unsigned is dependent on the declaration.
>>>>
>>>> I am talk about the math, while you are talking about compiler.
>>>>
>>>> I admit that u16 gets promoted to 'signed int' is true, but this is
>>>> irrelevant,
>>>>
>>>> the point is how to understand the returned value.
>>>>
>>>>
>>>> How does the compiler generate the code is one thing, how do we
>>>> interpret the result is another
>>>>
>>>> How does the compiler generate the code is NOT determined by us, while
>>>> how do we interpret the result is determined by us.
>>>>
>>>>
>>>> I believe that using a u32 type to interpret the result(u16 * u16) is
>>>> always true, it is true in the perspective of *math*.
>>>>
>>>> Integer promotions is the details of C program language. If the result
>>>> of the multiply is signed, then there are risks that
>>>>
>>>> the result is negative, what's the benefit to present this risk to the
>>>> programmer?
>>>>
>>>> What's the benefit to tell me(and others) that u16 * u16 yield a signed
>>>> value? and can be negative?
>>>>
>>>> Using int type as the return type bring concerns to the programmer and
>>>> the user of the function,
>>>>
>>>> even though this is not impossible in practice.
>>> In general, do not use unsigned types in arithmethic to avoid negative
>>> values, because most people will be tripped over by integer promotion
>>> rules, and you'll get negative values anyway.
>>>
>>> I'll bet most people will be surprised to see what this prints:
>>>
>>> #include <stdio.h>
>>> #include <stdint.h>
>>>
>>> int main(void)
>>> {
>>> uint16_t x = 0xffff;
>>> uint16_t y = 0xffff;
>>> uint64_t z = x * y;
>>>
>>> printf("0x%016lx\n", z);
>>> printf("%ld\n", z);
>> Here, please replace the "%ld\n" with the "%lu\n", then you will see the
>> difference.
>>
>> you are casting the variable 'z' to signed value, "%d" is for printing
>> signed value, and "%u" is for printing unsigned value.
>>
>>
>> Your simple code explained exactly why you are still in confusion,
> Am I?
>
> Take a look at the values, and explain the math.
I meant the value itself is represent with 2's compliment,
when you print a value with '%ld', then you will get the signed version,
when you print a value with '%lu', then you will get the unsigned version.
The result of a u16*u16 couldn't be negative in math.
But when you using a '%ld' or '%d' to print a unsigned value, then is wrong.
This is also the case which you shouldn't using a int type to store the result of u16*u16.
because when I seen a int type, I will choose '%d' to print it,
when I seen a unsigned int type, I will choose '%u' to print it.
when using a int type as the return type, this could lead people to using '%d' to print
such a value. Then, it generate the confusion as this little test program shows.
>
> BR,
> Jani.
>
>> that is u16 * u16 can yield a negative value if you use the int as the
>> return type. Because it overflowed.
>>
>>> printf("%d\n", x * y);
>>> }
>>>
>>> And it's not that different from what you have below. Your patch doesn't
>>> change anything, and doesn't make it any less confusing.
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>>> Using a u32 is enough to store the result, but considering that the
>>>>>> result will be casted to u64 soon after. We use a u64 type directly.
>>>>>> So there no need to cast it to signed type and cast back then.
>>>>> ....
>>>>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>>>>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>>>>> ...
>>>>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
>>>>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock);
>>>>> The (u64) cast is there to extend the value to 64bits, not
>>>>> because the original type is signed.
>>>> Sorry about my expression, I think my sentence did not mention anything
>>>> about 'because the original type is signed'.
>>>>
>>>> In the contrary, my patch eliminated the concerns to the reviewer. It
>>>> say that the results of the multiply can't be negative.
>>>>
>>>> My intent is to tell the compiler we want a unsigned return type, but
>>>> GCC emit 'imul' instruction for the multiply......
>>>>
>>>> I'm using u64 as the return type, because div_u64() function accept a
>>>> u64 type value as its first argument.
>>>>
>>>>> The compiler will detect that the old code is a 32x32 multiply
>>>>> where a 64bit result is needed, that may not be true for the
>>>>> changed code (it would need to track back as far as the u16s).
>>>> I don't believe my code could be wrong.
>>>>
>>>> when you use the word 'may', you are saying that it could be wrong after
>>>> apply my patch.
>>>>
>>>> Then you have to find at least one test example to prove you point, in
>>>> which case my codes generate wrong results.
>>>>
>>>> Again I don't believe you could find one.
>>>>
>>>>> It is not uncommon to force a 64bit result from a multiply
>>>>> by making the constant 64bit. As in:
>>>>> div_u64(frame_size * 1000000ULL, dotclock);
>>>> In fact, After apply this patch, the ASM code generated is same with before.
>>>>
>>>> This may because the GCC is smart enough to generate optimized code in
>>>> either case,
>>>>
>>>> I think It could be different with a different optimization-level.
>>>>
>>>> I have tested this patch on three different architecture, I can not
>>>> find error still.
>>>>
>>>> Below is the assembly extract on x86-64: because GCC generate the same
>>>> code in either case,
>>>>
>>>> so I pasted only one copy here.
>>>>
>>>>
>>>> 0000000000000530 <drm_calc_timestamping_constants>:
>>>> 530: f3 0f 1e fa endbr64
>>>> 534: e8 00 00 00 00 callq 539
>>>> <drm_calc_timestamping_constants+0x9>
>>>> 539: 55 push %rbp
>>>> 53a: 48 89 e5 mov %rsp,%rbp
>>>> 53d: 41 57 push %r15
>>>> 53f: 41 56 push %r14
>>>> 541: 41 55 push %r13
>>>> 543: 41 54 push %r12
>>>> 545: 53 push %rbx
>>>> 546: 48 83 ec 18 sub $0x18,%rsp
>>>> 54a: 4c 8b 3f mov (%rdi),%r15
>>>> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax
>>>> 554: 85 c0 test %eax,%eax
>>>> 556: 0f 84 ec 00 00 00 je 648
>>>> <drm_calc_timestamping_constants+0x118>
>>>> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d
>>>> 563: 49 89 fc mov %rdi,%r12
>>>> 566: 44 39 c0 cmp %r8d,%eax
>>>> 569: 0f 86 40 01 00 00 jbe 6af
>>>> <drm_calc_timestamping_constants+0x17f>
>>>> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d
>>>> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx
>>>> 57a: 48 89 f3 mov %rsi,%rbx
>>>> 57d: 45 85 f6 test %r14d,%r14d
>>>> 580: 0f 8e d5 00 00 00 jle 65b
>>>> <drm_calc_timestamping_constants+0x12b>
>>>> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax
>>>> 58a: 49 63 f6 movslq %r14d,%rsi
>>>> 58d: 31 d2 xor %edx,%edx
>>>> 58f: 48 89 c7 mov %rax,%rdi
>>>> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax
>>>> 599: 48 f7 f6 div %rsi
>>>> 59c: 31 d2 xor %edx,%edx
>>>> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp)
>>>> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax
>>>> 5a6: 0f af c7 imul %edi,%eax
>>>> 5a9: 48 98 cltq
>>>> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax
>>>> 5b2: 48 f7 f6 div %rsi
>>>> 5b5: 41 89 c5 mov %eax,%r13d
>>>> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx)
>>>> 5bc: 74 0a je 5c8
>>>> <drm_calc_timestamping_constants+0x98>
>>>> 5be: 41 c1 ed 1f shr $0x1f,%r13d
>>>> 5c2: 41 01 c5 add %eax,%r13d
>>>> 5c5: 41 d1 fd sar %r13d
>>>> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax
>>>> 5cc: 48 89 de mov %rbx,%rsi
>>>> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi
>>>> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax
>>>> 5d6: 48 c1 e7 04 shl $0x4,%rdi
>>>> 5da: 48 01 cf add %rcx,%rdi
>>>> 5dd: 89 47 78 mov %eax,0x78(%rdi)
>>>> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi
>>>> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi)
>>>> 5e8: e8 00 00 00 00 callq 5ed
>>>> <drm_calc_timestamping_constants+0xbd>
>>>> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx
>>>> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax
>>>> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d
>>>> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d
>>>> 5ff: 4d 85 ff test %r15,%r15
>>>> 602: 0f 84 87 00 00 00 je 68f
>>>> <drm_calc_timestamping_constants+0x15f>
>>>> 608: 49 8b 77 08 mov 0x8(%r15),%rsi
>>>> 60c: 52 push %rdx
>>>> 60d: 31 ff xor %edi,%edi
>>>> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>>>> 616: 50 push %rax
>>>> 617: 31 d2 xor %edx,%edx
>>>> 619: e8 00 00 00 00 callq 61e
>>>> <drm_calc_timestamping_constants+0xee>
>>>> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d
>>>> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15
>>>> 627: 5f pop %rdi
>>>> 628: 41 59 pop %r9
>>>> 62a: 8b 45 d0 mov -0x30(%rbp),%eax
>>>> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>>>> 634: 4c 89 fe mov %r15,%rsi
>>>> 637: 45 89 f1 mov %r14d,%r9d
>>>> 63a: 31 d2 xor %edx,%edx
>>>> 63c: 31 ff xor %edi,%edi
>>>> 63e: 50 push %rax
>>>> 63f: 41 55 push %r13
>>>> 641: e8 00 00 00 00 callq 646
>>>> <drm_calc_timestamping_constants+0x116>
>>>> 646: 59 pop %rcx
>>>> 647: 5e pop %rsi
>>>> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp
>>>> 64c: 5b pop %rbx
>>>> 64d: 41 5c pop %r12
>>>> 64f: 41 5d pop %r13
>>>> 651: 41 5e pop %r14
>>>> 653: 41 5f pop %r15
>>>> 655: 5d pop %rbp
>>>> 656: e9 00 00 00 00 jmpq 65b
>>>> <drm_calc_timestamping_constants+0x12b>
>>>> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx
>>>> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi
>>>> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp)
>>>> 668: 45 31 ed xor %r13d,%r13d
>>>> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi
>>>> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp)
>>>> 676: e8 00 00 00 00 callq 67b
>>>> <drm_calc_timestamping_constants+0x14b>
>>>> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp)
>>>> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d
>>>> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx
>>>> 68a: e9 39 ff ff ff jmpq 5c8
>>>> <drm_calc_timestamping_constants+0x98>
>>>> 68f: 52 push %rdx
>>>> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>>>> 697: 31 d2 xor %edx,%edx
>>>> 699: 31 f6 xor %esi,%esi
>>>> 69b: 50 push %rax
>>>> 69c: 31 ff xor %edi,%edi
>>>> 69e: e8 00 00 00 00 callq 6a3
>>>> <drm_calc_timestamping_constants+0x173>
>>>> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d
>>>> 6a8: 58 pop %rax
>>>> 6a9: 5a pop %rdx
>>>> 6aa: e9 7b ff ff ff jmpq 62a
>>>> <drm_calc_timestamping_constants+0xfa>
>>>> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi
>>>> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12
>>>> 6b7: 4d 85 e4 test %r12,%r12
>>>> 6ba: 74 25 je 6e1
>>>> <drm_calc_timestamping_constants+0x1b1>
>>>> 6bc: e8 00 00 00 00 callq 6c1
>>>> <drm_calc_timestamping_constants+0x191>
>>>> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>>>> 6c8: 4c 89 e2 mov %r12,%rdx
>>>> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
>>>> 6d2: 48 89 c6 mov %rax,%rsi
>>>> 6d5: e8 00 00 00 00 callq 6da
>>>> <drm_calc_timestamping_constants+0x1aa>
>>>> 6da: 0f 0b ud2
>>>> 6dc: e9 67 ff ff ff jmpq 648
>>>> <drm_calc_timestamping_constants+0x118>
>>>> 6e1: 4c 8b 27 mov (%rdi),%r12
>>>> 6e4: eb d6 jmp 6bc
>>>> <drm_calc_timestamping_constants+0x18c>
>>>> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
>>>> 6ed: 00 00 00
>>>> 6f0: 90 nop
>>>> 6f1: 90 nop
>>>> 6f2: 90 nop
>>>> 6f3: 90 nop
>>>> 6f4: 90 nop
>>>> 6f5: 90 nop
>>>> 6f6: 90 nop
>>>> 6f7: 90 nop
>>>> 6f8: 90 nop
>>>> 6f9: 90 nop
>>>> 6fa: 90 nop
>>>> 6fb: 90 nop
>>>> 6fc: 90 nop
>>>> 6fd: 90 nop
>>>> 6fe: 90 nop
>>>> 6ff: 90 nop
>>>>
>>>>
>>>>> David
>>>>>
>>>>> -
>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>>>>> Registration No: 1397386 (Wales)
>>>>>
More information about the dri-devel
mailing list