From: Peter Zijlstra <peterz@infradead.org>
To: Tiezhu Yang <yangtiezhu@loongson.cn>
Cc: Alex Deucher <alexdeucher@gmail.com>,
Josh Poimboeuf <jpoimboe@kernel.org>,
Huacai Chen <chenhuacai@kernel.org>,
loongarch@lists.linux.dev, amd-gfx@lists.freedesktop.org,
linux-kernel@vger.kernel.org,
Nick Desaulniers <ndesaulniers@google.com>,
nathan@kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH v6 9/9] drm/amd/display: Mark dc_fixpt_from_fraction() noinline
Date: Fri, 20 Dec 2024 11:31:00 +0100 [thread overview]
Message-ID: <20241220103100.GB17537@noisy.programming.kicks-ass.net> (raw)
In-Reply-To: <4bace457-cc26-13a3-bc90-ad6ad12bc2ed@loongson.cn>
On Fri, Dec 20, 2024 at 01:02:18PM +0800, Tiezhu Yang wrote:
> 2. For x86
>
> I tested with LLVM 19.1.6 and the latest mainline LLVM, the test result
> is same with LoongArch.
Debian's clang-19 is 19.1.5.
> (1) objdump info with LLVM release version 19.1.6:
Please always use -r, that's ever so much more readable.
> $ clang --version | head -1
> clang version 19.1.6
>
> There is an jump instruction "jmp" at the end of dc_fixpt_recip(), it
> maybe jumps to a position and then steps to the return instruction, so
> there is no "falls through" objtool warning.
>
> 0000000000000290 <dc_fixpt_recip>:
> 290: f3 0f 1e fa endbr64
> 294: e8 00 00 00 00 call 299 <dc_fixpt_recip+0x9>
> 299: 48 85 ff test %rdi,%rdi
> 29c: 0f 84 a8 00 00 00 je 34a <dc_fixpt_recip+0xba>
> 2a2: 48 89 f9 mov %rdi,%rcx
> 2a5: 48 f7 d9 neg %rcx
> 2a8: 48 0f 48 cf cmovs %rdi,%rcx
> 2ac: 53 push %rbx
> 2ad: 48 b8 00 00 00 00 01 movabs $0x100000000,%rax
> 2b4: 00 00 00
> 2b7: 31 d2 xor %edx,%edx
> 2b9: 48 f7 f1 div %rcx
> 2bc: be e0 ff ff ff mov $0xffffffe0,%esi
> 2c1: eb 1b jmp 2de <dc_fixpt_recip+0x4e>
> 2c3: 45 88 c8 mov %r9b,%r8b
> 2c6: 4d 01 c0 add %r8,%r8
> 2c9: 4d 8d 04 80 lea (%r8,%rax,4),%r8
> 2cd: 48 29 da sub %rbx,%rdx
> 2d0: 45 88 da mov %r11b,%r10b
> 2d3: 4c 89 d0 mov %r10,%rax
> 2d6: 4c 09 c0 or %r8,%rax
> 2d9: 83 c6 02 add $0x2,%esi
> 2dc: 74 31 je 30f <dc_fixpt_recip+0x7f>
> 2de: 48 01 d2 add %rdx,%rdx
> 2e1: 45 31 c0 xor %r8d,%r8d
> 2e4: 41 ba 00 00 00 00 mov $0x0,%r10d
> 2ea: 48 39 ca cmp %rcx,%rdx
> 2ed: 41 0f 93 c1 setae %r9b
> 2f1: 72 03 jb 2f6 <dc_fixpt_recip+0x66>
> 2f3: 49 89 ca mov %rcx,%r10
> 2f6: 4c 29 d2 sub %r10,%rdx
> 2f9: 48 01 d2 add %rdx,%rdx
> 2fc: 45 31 d2 xor %r10d,%r10d
> 2ff: 48 89 cb mov %rcx,%rbx
> 302: 48 39 ca cmp %rcx,%rdx
> 305: 41 0f 93 c3 setae %r11b
> 309: 73 b8 jae 2c3 <dc_fixpt_recip+0x33>
> 30b: 31 db xor %ebx,%ebx
> 30d: eb b4 jmp 2c3 <dc_fixpt_recip+0x33>
> 30f: 48 01 d2 add %rdx,%rdx
> 312: 48 be fe ff ff ff ff movabs $0x7ffffffffffffffe,%rsi
> 319: ff ff 7f
> 31c: 4c 8d 46 01 lea 0x1(%rsi),%r8
> 320: 48 39 ca cmp %rcx,%rdx
> 323: 4c 0f 43 c6 cmovae %rsi,%r8
> 327: 4c 39 c0 cmp %r8,%rax
> 32a: 77 29 ja 355 <dc_fixpt_recip+0xc5>
> 32c: 48 39 ca cmp %rcx,%rdx
> 32f: 48 83 d8 ff sbb $0xffffffffffffffff,%rax
> 333: 48 89 c1 mov %rax,%rcx
> 336: 48 f7 d9 neg %rcx
> 339: 48 85 ff test %rdi,%rdi
> 33c: 48 0f 49 c8 cmovns %rax,%rcx
> 340: 48 89 c8 mov %rcx,%rax
> 343: 5b pop %rbx
> 344: 2e e9 00 00 00 00 cs jmp 34a <dc_fixpt_recip+0xba>
> 34a: 0f 0b ud2
> 34c: 0f 0b ud2
> 34e: 31 c9 xor %ecx,%ecx
> 350: e9 57 ff ff ff jmp 2ac <dc_fixpt_recip+0x1c>
> 355: 0f 0b ud2
> 357: eb d3 jmp 32c <dc_fixpt_recip+0x9c>
> 359: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
I had to puzzle a bit to get that double ud2 -- not all configs do that.
Also, curse the DRM Makefiles, you can't do:
make drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.s
:-(
They are the two ASSERT()s on line 217 and 54 respectively. They end up
asserting the same value twice, so that makes sense.
> $ clang --version | head -1
> clang version 20.0.0git (https://github.com/llvm/llvm-project.git
> 8daf4f16fa08b5d876e98108721dd1743a360326)
So I didn't have a recent build at hand.. so I've not validated the
below.
> There is "ud2" instruction at the end of dc_fixpt_recip(), its offset
> is "350", this "ud2" instruction is not dead end due to the offset "352"
> is in the relocation section '.rela.discard.reachable', that is to say,
> dc_fixpt_recip() doesn't end with a return instruction or an
> unconditional jump, so objtool determined that the function can fall
> through into the next function, thus there is "falls through" objtool
> warning.
>
> 0000000000000290 <dc_fixpt_recip>:
> 290: f3 0f 1e fa endbr64
> 294: e8 00 00 00 00 call 299 <dc_fixpt_recip+0x9>
> 299: 48 85 ff test %rdi,%rdi
> 29c: 0f 84 ac 00 00 00 je 34e <dc_fixpt_recip+0xbe>
> 2a2: 53 push %rbx
> 2a3: 48 89 f9 mov %rdi,%rcx
> 2a6: 48 f7 d9 neg %rcx
> 2a9: 48 0f 48 cf cmovs %rdi,%rcx
> 2ad: 48 b8 00 00 00 00 01 movabs $0x100000000,%rax
> 2b4: 00 00 00
> 2b7: 31 d2 xor %edx,%edx
> 2b9: 48 f7 f1 div %rcx
> 2bc: be e0 ff ff ff mov $0xffffffe0,%esi
> 2c1: eb 1b jmp 2de <dc_fixpt_recip+0x4e>
> 2c3: 45 88 c8 mov %r9b,%r8b
> 2c6: 4d 01 c0 add %r8,%r8
> 2c9: 4d 8d 04 80 lea (%r8,%rax,4),%r8
> 2cd: 48 29 da sub %rbx,%rdx
> 2d0: 45 88 da mov %r11b,%r10b
> 2d3: 4c 89 d0 mov %r10,%rax
> 2d6: 4c 09 c0 or %r8,%rax
> 2d9: 83 c6 02 add $0x2,%esi
> 2dc: 74 31 je 30f <dc_fixpt_recip+0x7f>
> 2de: 48 01 d2 add %rdx,%rdx
> 2e1: 45 31 c0 xor %r8d,%r8d
> 2e4: 41 ba 00 00 00 00 mov $0x0,%r10d
> 2ea: 48 39 ca cmp %rcx,%rdx
> 2ed: 41 0f 93 c1 setae %r9b
> 2f1: 72 03 jb 2f6 <dc_fixpt_recip+0x66>
> 2f3: 49 89 ca mov %rcx,%r10
> 2f6: 4c 29 d2 sub %r10,%rdx
> 2f9: 48 01 d2 add %rdx,%rdx
> 2fc: 45 31 d2 xor %r10d,%r10d
> 2ff: 48 89 cb mov %rcx,%rbx
> 302: 48 39 ca cmp %rcx,%rdx
> 305: 41 0f 93 c3 setae %r11b
> 309: 73 b8 jae 2c3 <dc_fixpt_recip+0x33>
> 30b: 31 db xor %ebx,%ebx
> 30d: eb b4 jmp 2c3 <dc_fixpt_recip+0x33>
> 30f: 48 01 d2 add %rdx,%rdx
> 312: 48 be fe ff ff ff ff movabs $0x7ffffffffffffffe,%rsi
> 319: ff ff 7f
> 31c: 4c 8d 46 01 lea 0x1(%rsi),%r8
> 320: 48 39 ca cmp %rcx,%rdx
> 323: 4c 0f 43 c6 cmovae %rsi,%r8
> 327: 4c 39 c0 cmp %r8,%rax
> 32a: 77 1e ja 34a <dc_fixpt_recip+0xba>
> 32c: 48 39 ca cmp %rcx,%rdx
> 32f: 48 83 d8 ff sbb $0xffffffffffffffff,%rax
> 333: 48 89 c1 mov %rax,%rcx
> 336: 48 f7 d9 neg %rcx
> 339: 48 85 ff test %rdi,%rdi
> 33c: 48 0f 49 c8 cmovns %rax,%rcx
> 340: 48 89 c8 mov %rcx,%rax
> 343: 5b pop %rbx
> 344: 2e e9 00 00 00 00 cs jmp 34a <dc_fixpt_recip+0xba>
> 34a: 0f 0b ud2
> 34c: eb de jmp 32c <dc_fixpt_recip+0x9c>
> 34e: 0f 0b ud2
> 350: 0f 0b ud2
> 352: 66 66 66 66 66 2e 0f data16 data16 data16 data16 cs nopw
> 0x0(%rax,%rax,1)
> 359: 1f 84 00 00 00 00 00
If you put them size-by-side, you'll see it's more or less the same
code-gen (trivial differences), but now it just stops code-gen, where
previously it would continue.
So this really is a compiler problem, this needs no annotation, it's
straight up broken.
Now, the thing is, these ASSERT()s are checking for divide-by-zero, I
suspect clang figured that out and invokes UB on us and just stops
code-gen.
Nathan, Nick, don't we have a compiler flag that forces __builtin_trap()
whenever clang pulls something like this? I think UBSAN does this, but
we really shouldn't pull in the whole of that for sanity.
next prev parent reply other threads:[~2024-12-20 14:15 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-17 1:08 [PATCH v6 0/9] Add jump table support for objtool on LoongArch Tiezhu Yang
2024-12-17 1:08 ` [PATCH v6 1/9] objtool: Handle various symbol types of rodata Tiezhu Yang
2024-12-17 1:08 ` [PATCH v6 2/9] objtool: Handle different entry size " Tiezhu Yang
2025-02-11 14:19 ` [PATCH] objtool: remove duplicate case value R_PPC64_REL32 Kexy Biscuit
2025-02-11 23:34 ` Josh Poimboeuf
2024-12-17 1:08 ` [PATCH v6 3/9] objtool: Handle PC relative relocation type Tiezhu Yang
2024-12-17 1:09 ` [PATCH v6 4/9] objtool: Handle unreachable entry of rodata Tiezhu Yang
2025-02-10 21:17 ` Josh Poimboeuf
2024-12-17 1:09 ` [PATCH v6 5/9] objtool/LoongArch: Add support for switch table Tiezhu Yang
2024-12-17 1:09 ` [PATCH v6 6/9] objtool/LoongArch: Add support for goto table Tiezhu Yang
2024-12-17 1:09 ` [PATCH v6 7/9] LoongArch: Enable jump table for objtool Tiezhu Yang
2025-03-14 20:03 ` [tip: objtool/core] " tip-bot2 for Tiezhu Yang
2024-12-17 1:09 ` [PATCH v6 8/9] LoongArch: Convert unreachable() to BUG() Tiezhu Yang
2024-12-17 1:50 ` [PATCH v6 9/9] drm/amd/display: Mark dc_fixpt_from_fraction() noinline Tiezhu Yang
2024-12-18 14:36 ` Huacai Chen
2024-12-18 19:05 ` Josh Poimboeuf
2024-12-18 19:22 ` Alex Deucher
2024-12-20 5:02 ` Tiezhu Yang
2024-12-20 10:31 ` Peter Zijlstra [this message]
2024-12-20 22:34 ` Nathan Chancellor
2024-12-21 7:40 ` Xi Ruoyao
2024-12-22 4:27 ` Tiezhu Yang
2024-12-23 21:46 ` Nathan Chancellor
2024-12-25 9:43 ` Tiezhu Yang
2025-01-04 13:58 ` [PATCH v6 0/9] Add jump table support for objtool on LoongArch Huacai Chen
2025-01-11 6:57 ` Tiezhu Yang
2025-01-15 1:34 ` Josh Poimboeuf
2025-02-10 6:07 ` Tiezhu Yang
2025-02-10 21:26 ` Josh Poimboeuf
2025-02-11 11:59 ` Huacai Chen
2025-02-11 23:30 ` Josh Poimboeuf
2025-02-12 7:22 ` Huacai Chen
2025-02-13 2:51 ` Josh Poimboeuf
2025-02-17 3:13 ` Huacai Chen
2025-02-18 17:46 ` Josh Poimboeuf
2025-02-19 9:49 ` Huacai Chen
2025-02-20 21:33 ` Josh Poimboeuf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241220103100.GB17537@noisy.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=alexdeucher@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=chenhuacai@kernel.org \
--cc=jpoimboe@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=loongarch@lists.linux.dev \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=yangtiezhu@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.