Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast

Sui Jingfeng <15330273260@xxxxxx> · Mon, 22 May 2023 19:55:45 +0800

Hi,

On 2023/5/22 19:29, Jani Nikula wrote:
On Thu, 18 May 2023, Sui Jingfeng <15330273260@xxxxxx> wrote:
On 2023/5/17 18:59, David Laight wrote:
From: 15330273260@xxxxxx
Sent: 16 May 2023 18:30

From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>

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,

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)