Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm][NFC] Assert VAArg type #67148

Closed
wants to merge 1 commit into from
Closed

Conversation

urnathan
Copy link
Contributor

Due to the way we have a backend arranged, we fell over a varadic struct bug, and noticed the generic machinery presumes all by-val structs are by hidden pointer. The documentation even warns:

'Note that the code generator does not yet fully support va_arg on many targets. Also, it does not currently support va_arg with aggregate types on any target.'

While this assert would not have triggered in our case, it was surprising to not see a check. So here's one. Of course it doesn't trigger :)

Assert that VAArg is not given a structure or array type, for those
absolutely won't work (as documented in LangRef.rst).
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 22, 2023

@llvm/pr-subscribers-llvm-ir

Changes

Due to the way we have a backend arranged, we fell over a varadic struct bug, and noticed the generic machinery presumes all by-val structs are by hidden pointer. The documentation even warns:

'Note that the code generator does not yet fully support va_arg on many targets. Also, it does not currently support va_arg with aggregate types on any target.'

While this assert would not have triggered in our case, it was surprising to not see a check. So here's one. Of course it doesn't trigger :)


Full diff: https://github.com/llvm/llvm-project/pull/67148.diff

1 Files Affected:

  • (modified) llvm/include/llvm/IR/Instructions.h (+4)
diff --git a/llvm/include/llvm/IR/Instructions.h b/llvm/include/llvm/IR/Instructions.h
index a0c8406fe4ec1e5..cfe90ecdef3fd5a 100644
--- a/llvm/include/llvm/IR/Instructions.h
+++ b/llvm/include/llvm/IR/Instructions.h
@@ -1796,12 +1796,16 @@ class VAArgInst : public UnaryInstruction {
   VAArgInst(Value *List, Type *Ty, const Twine &NameStr = "",
              Instruction *InsertBefore = nullptr)
     : UnaryInstruction(Ty, VAArg, List, InsertBefore) {
+    assert(!Ty->isStructTy() && !Ty->isArrayTy() &&
+           "by-val structures and arrays unsupported");
     setName(NameStr);
   }
 
   VAArgInst(Value *List, Type *Ty, const Twine &NameStr,
             BasicBlock *InsertAtEnd)
     : UnaryInstruction(Ty, VAArg, List, InsertAtEnd) {
+    assert(!Ty->isStructTy() && !Ty->isArrayTy() &&
+           "by-val structures and arrays unsupported");
     setName(NameStr);
   }
 

@urnathan urnathan marked this pull request as ready for review October 13, 2023 16:17
@urnathan urnathan closed this Mar 22, 2024
@urnathan urnathan deleted the push/vaarg branch March 22, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants