unexpected codegen for inline asm

[nickdesaulniers]

Bugzilla Link	42762
Resolution	FIXED
Resolved on	Jun 23, 2020 14:17
Version	trunk
OS	Linux
Blocks	#4440 #44654
CC	@arndb,@chleroy,@hfinkel,@nathanchance,@rnav,@nemanjai,@tstellar
Fixed by commit(s)	aede24e a8eb6a5

Extended Description

The powerpc (32b) Linux kernel started panicing at runtime recently when built with LLVM due to a change in the kernel sources. We narrowed it down to commit, and a single call site of a static inline function containing extended inline assembly with constraints.

I think this concise test case distills this issue: https://godbolt.org/z/E4f1Us

Basically, we had:

// from arch/powerpc/include/asm/cache.h
// pre-commit 6c5875843b87 ("powerpc: slightly improve cache helpers")
void dcbz_old(void* addr)
{
asm volatile ("dcbz 0, %0" : : "r"(addr) : "memory");
}

then moved to:

// from arch/powerpc/include/asm/cache.h
// post-commit 6c5875843b87 ("powerpc: slightly improve cache helpers")
void dcbz_current(void* addr)
{
asm volatile ("dcbz %y0" :: "Z"((unsigned char)addr) : "memory");
}

It seems that GCC generates the same code for both cases, and LLVM matches GCC for the first case. In the second case, the codegen is wildly different, which seems like what's leading to our panic at runtime.

I'm not super familiar with the "Z" constraint and "%y" output format, but they might be related?

[nickdesaulniers]

assigned to @hfinkel

[arndb]

The kernel patch was meant to tell the compiler that it could use a more efficient addressing mode using two registers (base+offset) instead of one register to pass the 'addr' pointer.

The code that clang produces however loads a byte from the original address, puts it on the stack, and then passes a copy to the stack into the data-cache-zero instruction, which then overwrites the stack but not the memory that we want to have cleared.

[arndb]

Passing a pointer to an empty struct instead of a character makes it do the right thing, no idea if that something we want to rely on though:

asm volatile ("dcbz %y0" :: "Z"(*(struct{} *)addr) : "memory");

I also tried pointers to other types: if we pass an eight byte struct, clang will still make a copy, but if it's longer then it passes the original pointer. The actual size of the argument is machine specific, cache lines can be 32 to 128 bytes long on ppc, if I remember correctly.

[chleroy]

mpc8xx family have 16 bytes cache lines.

[rnav]

This (support for "Z" Constraint) also affects use of SDT markers in applications. As an example, trying to build the sample "heapsort" example [1] using clang throws errors such as:

heapsort.cxx:136:10: error: invalid operand in inline asm: '990: nop.pushsection .note.stapsdt,"?","note".balign 4.4byte 992f-991f,994f-993f,3991: .asciz "stapsdt"992: .balign 4993: .8byte 990b.8byte _.stapsdt.base.8byte 0.asciz "provider".asciz "heap_place".asciz "${0:n}@${1:I}$1 ${2:n}@${3:I}$3"994: .balign 4.popsection'

The problematic part there is:
.asciz "${0:n}@${1:I}$1 ${2:n}@${3:I}$3"

... and comes up since the default asm constraint used on powerpc by SDT markers is "nZr" [2]

It would be good to get support for "Z" constraint with clang, since there is interest in using it in the Linux kernel as well.

---

[1] https://sourceware.org/systemtap/wiki/AddingUserSpaceProbingToApps
[2] https://sourceware.org/git/?p=systemtap.git;a=commit;h=50f2ef5be3c24b3fe9eb4c9abc386d8dbc3b953a

[nemanjai]

Proposed fix in https://reviews.llvm.org/D77542

Please note that this won't give you optimal codegen. Namely, we will always use zero as the index and compute the address into the base register. However, it will at least generate correct code.

We can revisit optimizing codegen in the future. In the meantime, you could change something like:
void dcbz_w_offset(void* addr, int offset)
{
asm volatile ("dcbz %y0" :: "Z"((unsigned char)(addr + offset)) : "memory");
}

into:
void dcbz_w_offset(void* addr, int offset)
{
asm volatile ("dcbz %1, %0" :: "b"((unsigned char*)addr), "r"(offset) : "memory");
}

[nemanjai]

I would really like this merged into 10.1

[tstellar]

Hal,

What do you think about backporting this?

https://reviews.llvm.org/rGaede24ecaa08db806fb173faf2de9cff95df8cee

[hfinkel]

Hal,

What do you think about backporting this?

https://reviews.llvm.org/rGaede24ecaa08db806fb173faf2de9cff95df8cee

LGTM.

[tstellar]

Merged: a8eb6a5

[tstellar]

mentioned in issue #44654