-
Notifications
You must be signed in to change notification settings - Fork 14.1k
CodeGen: Fix implementation of __builtin_trivially_relocate. #140312
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
CodeGen: Fix implementation of __builtin_trivially_relocate. #140312
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Peter Collingbourne (pcc) ChangesThe builtin is documented to copy Full diff: https://github.com/llvm/llvm-project/pull/140312.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 48cfbda12b2ac..0cfb88a9d9789 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4425,6 +4425,14 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
Address Dest = EmitPointerWithAlignment(E->getArg(0));
Address Src = EmitPointerWithAlignment(E->getArg(1));
Value *SizeVal = EmitScalarExpr(E->getArg(2));
+ if (BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_trivially_relocate)
+ SizeVal = Builder.CreateMul(
+ SizeVal,
+ ConstantInt::get(
+ SizeVal->getType(),
+ getContext()
+ .getTypeSizeInChars(E->getArg(0)->getType()->getPointeeType())
+ .getQuantity()));
EmitArgCheck(TCK_Store, Dest, E->getArg(0), 0);
EmitArgCheck(TCK_Load, Src, E->getArg(1), 1);
Builder.CreateMemMove(Dest, Src, SizeVal, false);
diff --git a/clang/test/CodeGenCXX/cxx2c-trivially-relocatable.cpp b/clang/test/CodeGenCXX/cxx2c-trivially-relocatable.cpp
index 17144cffb6476..63f3ba8e74ed5 100644
--- a/clang/test/CodeGenCXX/cxx2c-trivially-relocatable.cpp
+++ b/clang/test/CodeGenCXX/cxx2c-trivially-relocatable.cpp
@@ -8,7 +8,7 @@ struct S trivially_relocatable_if_eligible {
};
// CHECK: @_Z4testP1SS0_
-// CHECK: call void @llvm.memmove.p0.p0.i64
+// CHECK: call void @llvm.memmove.p0.p0.i64({{.*}}, i64 8
// CHECK-NOT: __builtin
// CHECK: ret
void test(S* source, S* dest) {
|
@@ -4425,6 +4425,14 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID, | |||
Address Dest = EmitPointerWithAlignment(E->getArg(0)); | |||
Address Src = EmitPointerWithAlignment(E->getArg(1)); | |||
Value *SizeVal = EmitScalarExpr(E->getArg(2)); | |||
if (BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_trivially_relocate) | |||
SizeVal = Builder.CreateMul( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this multiply trigger some sort of ubsan check if it overflows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an overflow here can only result from a call to std::trivially_relocate(first, last, result)
with first > last
. I feel like it would probably be better to report the error at the caller so that we can provide a better error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think generally, there is the question of whether we want UBSAN to enforce the preconditions of trivially_relocate (object completeness, valid ranges), However, because the intent is for that builtin to be wrapped in a standard function, it's probably better to only do the checks there, it would integrate better with hardening/contracts, and we don't (afaik) have precondition checks for builtins
@@ -8,7 +8,7 @@ struct S trivially_relocatable_if_eligible { | |||
}; | |||
|
|||
// CHECK: @_Z4testP1SS0_ | |||
// CHECK: call void @llvm.memmove.p0.p0.i64 | |||
// CHECK: call void @llvm.memmove.p0.p0.i64({{.*}}, i64 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this was a size issue, I think it's important this memmove check has the expected count, so can we have a few tests for multiple different counts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Created using spr 1.3.6-beta.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this bug!
Created using spr 1.3.6-beta.1 [skip ci]
The builtin is documented to copy `count` elements, but the implementation copies `count` bytes. Fix that. Reviewers: cor3ntin, ojhunt Pull Request: llvm/llvm-project#140312
The builtin is documented to copy `count` elements, but the implementation copies `count` bytes. Fix that. Reviewers: cor3ntin, ojhunt Pull Request: llvm#140312
The builtin is documented to copy
count
elements, but the implementationcopies
count
bytes. Fix that.