Re: [PATCH] powerpc/bug: Cast to unsigned long before passing to inline asm

On Wed, Sep 01, 2021 at 05:17:26PM +1000, Michael Ellerman wrote:

Segher Boessenkool [off-list ref] writes:

quoted

On Tue, Aug 31, 2021 at 11:27:20PM +1000, Michael Ellerman wrote:

quoted

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.

quoted

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).

quoted

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