Messages in this thread | | | Date | Mon, 22 May 2023 19:55:45 +0800 | Subject | Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast | From | Sui Jingfeng <> |
| |
Hi,
On 2023/5/22 19:29, Jani Nikula wrote: > On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >> On 2023/5/17 18:59, David Laight wrote: >>> From: 15330273260@189.cn >>>> Sent: 16 May 2023 18:30 >>>> >>>> From: Sui Jingfeng <suijingfeng@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,
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) >>>
| |