Skip to content

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

Conversation

pcc
Copy link
Contributor

@pcc pcc commented May 16, 2025

The builtin is documented to copy count elements, but the implementation
copies count bytes. Fix that.

pcc and others added 2 commits May 16, 2025 14:37
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1

[skip ci]
@pcc pcc requested a review from cor3ntin May 16, 2025 21:37
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels May 16, 2025
@llvmbot
Copy link
Member

llvmbot commented May 16, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Peter Collingbourne (pcc)

Changes

The builtin is documented to copy count elements, but the implementation
copies count bytes. Fix that.


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (+8)
  • (modified) clang/test/CodeGenCXX/cxx2c-trivially-relocatable.cpp (+1-1)
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(
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

@cor3ntin cor3ntin left a 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!

cor3ntin and others added 2 commits May 23, 2025 17:01
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@pcc pcc changed the base branch from users/pcc/spr/main.codegen-fix-implementation-of-__builtin_trivially_relocate to main May 24, 2025 00:02
@pcc pcc merged commit b208016 into main May 24, 2025
7 of 15 checks passed
@pcc pcc deleted the users/pcc/spr/codegen-fix-implementation-of-__builtin_trivially_relocate branch May 24, 2025 00:02
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 24, 2025
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
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
The builtin is documented to copy `count` elements, but the implementation
copies `count` bytes. Fix that.

Reviewers: cor3ntin, ojhunt

Pull Request: llvm#140312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants