From: Michael Ellerman <mpe@ellerman•id.au>
To: Segher Boessenkool <segher@kernel•crashing.org>
Cc: nathan@kernel•org, linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH] powerpc/bug: Cast to unsigned long before passing to inline asm
Date: Wed, 01 Sep 2021 17:17:26 +1000 [thread overview]
Message-ID: <87tuj43gu1.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20210831213432.GF1583@gate.crashing.org>
Segher Boessenkool <segher@kernel•crashing.org> writes:
> On Tue, Aug 31, 2021 at 11:27:20PM +1000, Michael Ellerman wrote:
>> Nathan filed an LLVM bug [2], in which Eli Friedman explained that "if
>> you pass a value of a type that's narrower than a register to an inline
>> asm, the high bits are undefined". In this case we are passing a bool
>> to the inline asm, which is a single bit value, and so the compiler
>> thinks it can leave the high bits of r30 unmasked, because only the
>> value of bit 0 matters.
>>
>> Because the inline asm compares the full width of the register (32 or
>> 64-bit) we need to ensure the value passed to the inline asm has all
>> bits defined. So fix it by casting to long.
>>
>> We also already cast to long for the similar case in BUG_ENTRY(), which
>> it turns out was added to fix a similar bug in 2005 in commit
>> 32818c2eb6b8 ("[PATCH] ppc64: Fix issue with gcc 4.0 compiled kernels").
>
> That points to <https://gcc.gnu.org/PR23422>, which shows the correct
> explanation.
That's a pity because I don't understand that explanation ^_^
Why does sign extension matter when we're comparing against zero?
> The code as it was did **not** pass a bool. It perhaps passed an int
> (so many macros in play, it is hard to tell for sure, but it is int or
> long int, perhaps unsigned (which does not change anything here).
I don't understand that. It definitely is passing a bool at the source
level. Are you saying it's getting promoted somehow?
It expands to:
asm goto(
"1: "
"tdnei"
"
" " % 4,
0 " "\n " ".section __ex_table,\"a\";"
" "
".balign 4;"
" "
".long (1b) - . ;"
" "
".long (%l[__label_warn_on]) - . ;"
" "
".previous"
" "
".section __bug_table,\"aw\"\n"
"2:\t.4byte 1b - 2b, %0 - 2b\n"
"\t.short %1, %2\n"
".org 2b+%3\n"
".previous\n"
:
: "i"("lib/klist.c"), "i"(62),
"i"((1 << 0) | ((9) << 8)),
"i"(sizeof(struct bug_entry)),
"r"(knode_dead(knode))
^^^^^^^^^^^^^^^^^^^^^
:
: __label_warn_on);
And knode_dead() returns bool:
static bool knode_dead(struct klist_node *knode)
{
return (unsigned long)knode->n_klist & KNODE_DEAD;
}
So in my book that means the type there is bool. But I'm not a compiler
guy so I guessing I'm missing something.
> But td wants a 64-bit register, not a 32-bit one (which are the only two
> possibilities for the "r" constraint on PowerPC). The cast to "long" is
> fine for powerpc64, but it has nothing to do with "narrower than a
> register".
If it's 32-bit vs 64-bit, and the clang explanation is correct, then
we'd expect the low 32-bits of the value passed to the asm to have the
correct value, ie. have been masked with KNODE_DEAD.
> If this is not the correct explanation for LLVM, it needs to solve some
> other bug.
OK. I just need to get this fixed in the kernel, so I'll do a new
version with a changelog that is ~= "shrug not sure what's going on" and
merge that. Then we can argue about what is really going on later :)
cheeers
next prev parent reply other threads:[~2021-09-01 7:18 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-31 13:27 [PATCH] powerpc/bug: Cast to unsigned long before passing to inline asm Michael Ellerman
2021-08-31 19:46 ` Nathan Chancellor
2021-08-31 21:34 ` Segher Boessenkool
2021-09-01 7:17 ` Michael Ellerman [this message]
2021-09-01 14:44 ` Segher Boessenkool
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=87tuj43gu1.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=nathan@kernel$(echo .)org \
--cc=segher@kernel$(echo .)crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox