[Verifier] Reject va_start in non-variadic function #88809
Conversation
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-backend-aarch64 Author: Jon Chesterfield (JonChesterfield) ChangesA va_start intrinsic lowers to something derived from the variadic parameter to the function. If there is no such parameter, it can't lower meaningfully. Clang sema rejects the same with Moves the existing lint warning into a verifier error. Updates the one lit test that had a va_start in a non-variadic function. Full diff: https://github.com/llvm/llvm-project/pull/88809.diff 5 Files Affected:
diff --git a/llvm/lib/Analysis/Lint.cpp b/llvm/lib/Analysis/Lint.cpp
index 0694c2995dfcce..1ab856ac8830a9 100644
--- a/llvm/lib/Analysis/Lint.cpp
+++ b/llvm/lib/Analysis/Lint.cpp
@@ -350,10 +350,7 @@ void Lint::visitCallBase(CallBase &I) {
}
case Intrinsic::vastart:
- Check(I.getParent()->getParent()->isVarArg(),
- "Undefined behavior: va_start called in a non-varargs function",
- &I);
-
+ // vastart in non-varargs function is rejected by the verifier
visitMemoryReference(I, MemoryLocation::getForArgument(&I, 0, TLI),
std::nullopt, nullptr, MemRef::Read | MemRef::Write);
break;
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 33f358440a312d..8720bfc2093b5b 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -5769,6 +5769,11 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
break;
}
+ case Intrinsic::vastart: {
+ Check(Call.getFunction()->isVarArg(),
+ "va_start called in a non-varargs function");
+ break;
+ }
case Intrinsic::vector_reduce_and:
case Intrinsic::vector_reduce_or:
case Intrinsic::vector_reduce_xor:
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/vastart.ll b/llvm/test/CodeGen/AArch64/GlobalISel/vastart.ll
index bd576d0f70e9c1..8c6e01d934c2d8 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/vastart.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/vastart.ll
@@ -3,7 +3,7 @@
declare void @llvm.va_start(ptr)
-define void @test_va_start(ptr %list) {
+define void @test_va_start(ptr %list, ...) {
; CHECK-LABEL: name: test_va_start
; CHECK: [[LIST:%[0-9]+]]:_(p0) = COPY $x0
; CHECK-IOS: G_VASTART [[LIST]](p0) :: (store (s64) into %ir.list, align 1)
diff --git a/llvm/test/Other/lint.ll b/llvm/test/Other/lint.ll
index 6b31b31a78c98a..6fd2d40cd2f298 100644
--- a/llvm/test/Other/lint.ll
+++ b/llvm/test/Other/lint.ll
@@ -124,13 +124,6 @@ define void @0() nounwind {
ret void
}
-; CHECK: va_start called in a non-varargs function
-declare void @llvm.va_start(ptr)
-define void @not_vararg(ptr %p) nounwind {
- call void @llvm.va_start(ptr %p)
- ret void
-}
-
; CHECK: Undefined behavior: Branch to non-blockaddress
define void @use_indbr() {
indirectbr ptr @foo, [label %block]
diff --git a/llvm/test/Verifier/variadic.ll b/llvm/test/Verifier/variadic.ll
new file mode 100644
index 00000000000000..55e4a4da0a9203
--- /dev/null
+++ b/llvm/test/Verifier/variadic.ll
@@ -0,0 +1,8 @@
+; RUN: not opt -S -passes=verify 2>&1 < %s | FileCheck %s
+
+; CHECK: va_start called in a non-varargs function
+declare void @llvm.va_start(ptr)
+define void @not_vararg(ptr %p) nounwind {
+ call void @llvm.va_start(ptr %p)
+ ret void
+}
|
|
Do we want AutoUpgrade for this, or should we assume this doesn't show up in any existing bitcode files anyone cares about? Otherwise looks fine. |
|
The best case interpretation for the construct is UB - we could do something like replace it with undef if it occurs, but as the construct is nonsense and clang rejects it in sema anyway, I don't think this arises in reality. I'd like it to be an IR invariant so I don't have to handle it in varargs lowering. |
There was a problem hiding this comment.
With autoupgrade, we'd replace it with undef or something like that, so you'd maintain the invariant in memory, so you maintain the invariant either way. The real question is whether such invalid constructs have somehow leaked into existing IR files despite clang not generating them.
That said, thinking about it a bit more, it seems pretty unlikely; most IR using va_list is going to originate from C/Rust, and they can't ever generate bad IR like this as far as I know. And I think LLVM transformations have been pretty consistent about not constructing invalid code through inlining etc.
So LGTM
This reverts commit 61717c1. Failed a MLIR test
|
This change broke MLIR tests (PR buildkite failed as well), they are probably needeed to be updated |
This reverts commit f4960da. Includes a fix for the MLIR test case.
|
Please make sure to verify this works in C23 mode as well. e.g., I suspect it will Just Work™ but an explicit test for that scenario would be useful IMO. |
|
@AaronBallman I don't understand what you're asking here. C23 didn't change anything applicable to this as far as I'm aware. If it did, the problem would be in clang sema, not in the IR verifier. Prepending the stdarg include and specifying c23 https://godbolt.org/z/Eqr3vTa9K gets the expected errors: Did you want which is fine, it's a va_start in a variadic function. |
Ugh, sorry for the confusion with my example, that was... egregiously confusing. :-D I meant: and the only goal for the request was to make sure that the IR verifier still considers |
)" This reverts commit f4960da. Includes a fix for the MLIR test case.
)" This reverts commit f4960da. Includes a fix for the MLIR test case. Change-Id: I0dbfcd5f9d95813047626b2b885313c8d52ba29f
)" This reverts commit f4960da. Includes a fix for the MLIR test case. Change-Id: I0dbfcd5f9d95813047626b2b885313c8d52ba29f
…)" This reverts commit 61717c1. Failed a MLIR test
A va_start intrinsic lowers to something derived from the variadic parameter to the function. If there is no such parameter, it can't lower meaningfully. Clang sema rejects the same with
error: 'va_start' used in function with fixed args.Moves the existing lint warning into a verifier error. Updates the one lit test that had a va_start in a non-variadic function.