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.

Sign up for GitHub

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

Jump to bottom

Fix UB in LLVM FFI when passing zero or >1 bundle #123941

Merged
merged 1 commit into from Apr 15, 2024
Merged

Fix UB in LLVM FFI when passing zero or >1 bundle #123941

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 29 additions & 6 deletions compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp
View file
Open in desktop
Expand Up @@ -1524,13 +1524,21 @@ extern "C" void LLVMRustFreeOperandBundleDef(OperandBundleDef *Bundle) {

extern "C" LLVMValueRef LLVMRustBuildCall(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extern "C" LLVMValueRef LLVMRustBuildCall(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
// OpBundlesIndirect is an array of pointers (*not* a pointer to an array).
extern "C" LLVMValueRef LLVMRustBuildCall(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,

Something like, maybe, for the comment?

"Pointer to array" is how the old code treated it.

LLVMValueRef *Args, unsigned NumArgs,
OperandBundleDef **OpBundles,
OperandBundleDef **OpBundlesIndirect,
unsigned NumOpBundles) {
Value *Callee = unwrap(Fn);
FunctionType *FTy = unwrap<FunctionType>(Ty);

// FIXME: Is there a way around this?
SmallVector<OperandBundleDef> OpBundles;
OpBundles.reserve(NumOpBundles);
for (unsigned i = 0; i < NumOpBundles; ++i) {
OpBundles.push_back(*OpBundlesIndirect[i]);
}

return wrap(unwrap(B)->CreateCall(
FTy, Callee, ArrayRef<Value*>(unwrap(Args), NumArgs),
ArrayRef<OperandBundleDef>(*OpBundles, NumOpBundles)));
ArrayRef<OperandBundleDef>(OpBundles)));
}

extern "C" LLVMValueRef LLVMRustGetInstrProfIncrementIntrinsic(LLVMModuleRef M) {
Expand Down Expand Up @@ -1570,13 +1578,21 @@ extern "C" LLVMValueRef
LLVMRustBuildInvoke(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
LLVMValueRef *Args, unsigned NumArgs,
LLVMBasicBlockRef Then, LLVMBasicBlockRef Catch,
OperandBundleDef **OpBundles, unsigned NumOpBundles,
OperandBundleDef **OpBundlesIndirect, unsigned NumOpBundles,
const char *Name) {
Value *Callee = unwrap(Fn);
FunctionType *FTy = unwrap<FunctionType>(Ty);

// FIXME: Is there a way around this?
SmallVector<OperandBundleDef> OpBundles;
OpBundles.reserve(NumOpBundles);
for (unsigned i = 0; i < NumOpBundles; ++i) {
OpBundles.push_back(*OpBundlesIndirect[i]);
}

return wrap(unwrap(B)->CreateInvoke(FTy, Callee, unwrap(Then), unwrap(Catch),
ArrayRef<Value*>(unwrap(Args), NumArgs),
ArrayRef<OperandBundleDef>(*OpBundles, NumOpBundles),
ArrayRef<OperandBundleDef>(OpBundles),
Name));
}

Expand All @@ -1585,7 +1601,7 @@ LLVMRustBuildCallBr(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
LLVMBasicBlockRef DefaultDest,
LLVMBasicBlockRef *IndirectDests, unsigned NumIndirectDests,
LLVMValueRef *Args, unsigned NumArgs,
OperandBundleDef **OpBundles, unsigned NumOpBundles,
OperandBundleDef **OpBundlesIndirect, unsigned NumOpBundles,
const char *Name) {
Value *Callee = unwrap(Fn);
FunctionType *FTy = unwrap<FunctionType>(Ty);
Expand All @@ -1597,11 +1613,18 @@ LLVMRustBuildCallBr(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
IndirectDestsUnwrapped.push_back(unwrap(IndirectDests[i]));
}

// FIXME: Is there a way around this?
Copy link
Member

@RalfJung RalfJung Apr 15, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to make sure I understand correctly... in Rust we have this data as an array of pointers such that the actual OperandBundleDef are non-contiguous, but LLVM wants them as a single flat array with all the OperandBundleDef next to each other? So really in Rust we'd want Vec<OperandBundleDef> instead of Vec<&OperandBundleDef>? But for some reason this can't be by-val in Rust, despite LLVM requiring this as a packed array (which can only be created by-val)?

May be worth adding some comment to explain this...

The fact that in Rust we have two types called OperandBundleDef where one is a pointer to the other does not help either.^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. OperandBundleDef is a C++ class - we can't easily pass that around by value in Rust without something like cxx to produce the right bindings (and even then it might not be possible if anything isn't trivially movable inside it).

I'm not sure there's a good place for a comment. I think long-term if we want to optimize this the right strategy is to have our wrapper code allocate the Vec and let Rust push into that Vec (roughly speaking).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we have two types on the rust side (can't grep right now). The OperandBundleDef I'm aware of is an extern type (I.e. lacks a size) so it can only be held by reference. It's the same thing as the C++ OperandBundleDef though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we have two, see here.

SmallVector<OperandBundleDef> OpBundles;
OpBundles.reserve(NumOpBundles);
for (unsigned i = 0; i < NumOpBundles; ++i) {
OpBundles.push_back(*OpBundlesIndirect[i]);
}

return wrap(unwrap(B)->CreateCallBr(
FTy, Callee, unwrap(DefaultDest),
ArrayRef<BasicBlock*>(IndirectDestsUnwrapped),
ArrayRef<Value*>(unwrap(Args), NumArgs),
ArrayRef<OperandBundleDef>(*OpBundles, NumOpBundles),
ArrayRef<OperandBundleDef>(OpBundles),
Name));
}

Expand Down