On Wed, Sep 01, 2021 at 05:17:26PM +1000, Michael Ellerman wrote: > Segher Boessenkool <seg...@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 "td" insn wants a 64-bit quantity. You have to provide one, the compiler will not do an extend itself, it does not try to understand the asm template in any way. > > 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: <snip> > 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. I was confused by all the macros and stuff. And "bool" in the kernel means "_Bool" now (so it is a character type, with GCC). > > 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 :) One thing you should probably do is never pass expressions as asm operands that are "r". Instead, make a temporary var and assign to that, so it will have the type you want, without being able to forget to add a cast :-) Segher