Closed nathanchance closed 2 years ago
reduced ir (via llvm-reduce
):
; llc x.ll
define void @vmcs_set_bits() {
entry:
%0 = callbr i64 asm "", "={rsp},X,{rsp},~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@vmcs_set_bits, %do_exception), i64 0)
to label %asm.fallthrough [label %do_exception]
asm.fallthrough: ; preds = %entry
br label %do_exception
do_exception: ; preds = %asm.fallthrough, %entry
ret void
}
; Function Attrs: nounwind readonly
declare i64 @llvm.read_register.i64(metadata) #0
; Function Attrs: nounwind
declare void @llvm.write_register.i64(metadata, i64) #1
attributes #0 = { nounwind readonly }
attributes #1 = { nounwind }
ah, the change that regressed this was strictly clang, so the emitted IR is irrelevant. The difference in emitted IR between clang-11 and 12 looks like:
--- clang-11.ll 2021-11-30 16:05:16.268337463 -0800
+++ clang-12.ll 2021-11-30 16:05:25.844400186 -0800
@@ -1,6 +1,6 @@
define dso_local void @vmcs_set_bits() #0 !dbg !8 {
%1 = call i64 @llvm.read_register.i64(metadata !3), !dbg !12
- %2 = callbr i64 asm "", "={rsp},X,0,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@vmcs_set_bits, %4), i64 %1) #4
+ %2 = callbr i64 asm "", "={rsp},X,{rsp},~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@vmcs_set_bits, %4), i64 %1) #4
to label %3 [label %4], !dbg !12, !srcloc !13
3:
the statement if (Info.allowsRegister() && (GCCReg.empty() || Info.earlyClobber()))
in EmitAsmStmt()
in clang/lib/CodeGen/CGStmt.cpp is what regressed this.
hmmm...I wonder whether it's clang producing one less constraint than necessary, or if SelectionDAGBuilder::visitInlineAsm
is choking on valid IR?
23c2a5ce33f0f's change to SelectionDAGBuilder::visitInlineAsm
regarding NumMatchingOps
is suspicious.
clang may no longer generate tied inputs (those numbered constraints) for "+r"
output constraints, but I think the terminating condition is perhaps incorrect for when SelectionDAGBuilder::visitInlineAsm
should try to parse an argument as a BlockAddress
.
I need to spend more time understanding how llvm represents the parameters to an inline asm call
; the ordering of the parameters and the constraint string is confusing+unreadable.
https://reviews.llvm.org/D114895
I should remove the fixes tag; we may need a kernel side fix for this, too.
That patch resolves the crash under the original configuration and does not regress allmodconfig
. I can test other configurations as well.
I appreciate the testing, as always and thanks for formalizing the report here, too.
I can test other configurations as well.
There's been feedback on the bug; might as well wait for further changes to the patch at this point.
I'll test the next revision when it is ready then!
The latest version of D114895 still fixes the original report/configuration and I do not see any new regressions across any of the configurations that I test.
I tested diff 391716 of D114895 on top of https://github.com/llvm/llvm-project/commit/2f16b87b4b1d3fee07aff71a9bf79c3a8d514e7a against Peter's x86/wip.extable
branch @ 2e09d82a746a288b97c268537d2af8917a346b4c, where I saw the original error fixed and no new errors.
``` i386 defconfig successful in 0:03:30 i386 defconfig qemu boot successful i386 defconfig + CONFIG_LTO_CLANG_THIN=y successful in 0:03:52 i386 defconfig + CONFIG_LTO_CLANG_THIN=y qemu boot successful i386 allmodconfig successful in 0:28:46 i386 allnoconfig successful in 0:00:29 i386 tinyconfig successful in 0:00:26 i386 debian config + CONFIG_SYSTEM_TRUSTED_KEYS=n + CONFIG_NETFILTER_SYNPROXY=n (https://github.com/ClangBuiltLinux/linux/issues/1442) successful in 0:16:44 i686 fedora config + CONFIG_BPF_PRELOAD=n (https://github.com/ClangBuiltLinux/linux/issues/1433) + CONFIG_NETFILTER_SYNPROXY=n (https://github.com/ClangBuiltLinux/linux/issues/1442) successful in 0:19:11 i386 opensuse config successful in 0:01:20 x86_64 defconfig successful in 0:03:38 x86_64 qemu boot successful x86_64 defconfig + CONFIG_LTO_CLANG_THIN=y successful in 0:04:02 x86_64 defconfig + CONFIG_LTO_CLANG_THIN=y qemu boot successful x86_64 allmodconfig + CONFIG_WERROR=n successful in 0:35:32 x86_64 allmodconfig at -O3 + CONFIG_WERROR=n successful in 0:35:15 x86_64 allmodconfig + CONFIG_GCOV_KERNEL=n + CONFIG_KASAN=n + CONFIG_WERROR=n + CONFIG_LTO_CLANG_THIN=y successful in 0:32:44 x86_64 alpine config successful in 0:15:29 x86_64 alpine config qemu boot successful x86_64 archlinux config + CONFIG_BPF_PRELOAD=n (https://github.com/ClangBuiltLinux/linux/issues/1433) successful in 0:24:18 x86_64 archlinux config + CONFIG_BPF_PRELOAD=n (https://github.com/ClangBuiltLinux/linux/issues/1433) qemu boot successful x86_64 debian config successful in 0:19:37 x86_64 debian config qemu boot successful x86_64 fedora config + CONFIG_BPF_PRELOAD=n (https://github.com/ClangBuiltLinux/linux/issues/1433) successful in 0:22:14 x86_64 fedora config + CONFIG_BPF_PRELOAD=n (https://github.com/ClangBuiltLinux/linux/issues/1433) qemu boot successful x86_64 opensuse config successful in 0:21:30 x86_64 opensuse config qemu boot successful ```
Link might die but: x86/vmx: Provide asm-goto-output vmread
I've split the patch up into a series:
(There's still some discussion ongoing in https://reviews.llvm.org/D114895, but I'm likely to abandon that approach)
It looks like Peter has a version of the patch that does not break with current clang
versions so the kernel side will be taken care of when the LLVM fixes land: https://lore.kernel.org/r/YbcbbGW2GcMx6KpD@hirez.programming.kicks-ass.net/
Been discussing this a lot with @jyknight . Based on https://reviews.llvm.org/D115409#3190589, I think I'm going to go with:
The kernel test robot reported an issue with a proposed change that uses asm goto with outputs.
cvise
spits out:This appears to be a regression between LLVM 11 and LLVM 12.
LLVM 11:
LLVM 12:
I intend to report this upstream but it seems like Bugzilla is being migrated to GitHub this week so I will wait until after that is done.