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

[Clang] Add GCC's __builtin_stack_address() to Clang. #121332

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aalhwc
Copy link

@aalhwc aalhwc commented Dec 30, 2024

This new builtin returns the value of the stack pointer register, mirroring GCC's __builtin_stack_address(). This implementation initially supports only the x86 and x86_64 architectures. Support for other architectures can be added in future patches.

Referring: #82632

This new builtin returns the value of the stack pointer register,
mirroring GCC's __builtin_stack_address(). This implementation initially
supports only the x86 and x86_64 architectures. Support for other
architectures can be added in future patches.
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen labels Dec 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 30, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: None (aalhwc)

Changes

This new builtin returns the value of the stack pointer register, mirroring GCC's __builtin_stack_address(). This implementation initially supports only the x86 and x86_64 architectures. Support for other architectures can be added in future patches.


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

3 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+6)
  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (+24)
  • (added) clang/test/CodeGen/builtin-stackaddress.c (+13)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index b5b47ae2746011..69aed2e6b2f0ca 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -899,6 +899,12 @@ def FrameAddress : Builtin {
   let Prototype = "void*(_Constant unsigned int)";
 }
 
+def StackAddress : Builtin {
+  let Spellings = ["__builtin_stack_address"];
+  let Attributes = [NoThrow];
+  let Prototype = "void*()";
+}
+
 def ClearCache : Builtin {
   let Spellings = ["__builtin___clear_cache"];
   let Attributes = [NoThrow];
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 4d4b7428abd505..d691958852699e 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4782,6 +4782,30 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
     Function *F = CGM.getIntrinsic(Intrinsic::frameaddress, AllocaInt8PtrTy);
     return RValue::get(Builder.CreateCall(F, Depth));
   }
+  case Builtin::BI__builtin_stack_address: {
+    IntegerType *SPRegType;
+    StringRef SPRegName;
+    switch (getTarget().getTriple().getArch()) {
+    case Triple::x86:
+      SPRegType = Int32Ty;
+      SPRegName = "esp";
+      break;
+    case Triple::x86_64:
+      SPRegType = Int64Ty;
+      SPRegName = "rsp";
+      break;
+    default:
+      llvm_unreachable("Intrinsic __builtin_stack_address is not supported for "
+                       "the target architecture");
+    }
+    Function *F = CGM.getIntrinsic(Intrinsic::read_register, {SPRegType});
+    Value *SPRegValue = MetadataAsValue::get(
+        getLLVMContext(),
+        MDNode::get(getLLVMContext(),
+                    {MDString::get(getLLVMContext(), SPRegName)}));
+    Value *Call = Builder.CreateCall(F, SPRegValue);
+    return RValue::get(Builder.CreateIntToPtr(Call, Int8PtrTy));
+  }
   case Builtin::BI__builtin_extract_return_addr: {
     Value *Address = EmitScalarExpr(E->getArg(0));
     Value *Result = getTargetHooks().decodeReturnAddress(*this, Address);
diff --git a/clang/test/CodeGen/builtin-stackaddress.c b/clang/test/CodeGen/builtin-stackaddress.c
new file mode 100644
index 00000000000000..5b1ddad45f21a1
--- /dev/null
+++ b/clang/test/CodeGen/builtin-stackaddress.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple i686-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=X86 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=X86_64 %s
+
+void* a() {
+  // X86_64: [[INT_SP:%.*]] = call i64 @llvm.read_register.i64(metadata [[SPREG:![0-9]+]])
+  // X86_64: inttoptr i64 [[INT_SP]] 
+  // X86_64: [[SPREG]] = !{!"rsp"}
+  //
+  // X86: [[INT_SP:%.*]] = call i32 @llvm.read_register.i32(metadata [[SPREG:![0-9]+]])
+  // X86: inttoptr i32 [[INT_SP]] 
+  // X86: [[SPREG]] = !{!"esp"}
+  return __builtin_stack_address();
+}

@aalhwc aalhwc changed the title [Clang] Add GCC's __builtin_stack_address() to Clang (#82632). [Clang] Add GCC's __builtin_stack_address() to Clang. Dec 30, 2024
Comment on lines +4798 to +4800
llvm_unreachable("Intrinsic __builtin_stack_address is not supported for "
"the target architecture");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is it ensured that this is actually unreachable?

Copy link
Author

@aalhwc aalhwc Dec 31, 2024

Choose a reason for hiding this comment

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

Maybe llvm_unreachable is inappropriate in this case.

The purpose of that llvm::unreachable statement is to indicate that __builtin_stack_address is either: (1) supported by the target architecture but not yet implemented in LLVM (I plan to add support for more architectures in future patches) or (2) not supported by the target architecture (e.g. there's no stack pointer register). The llvm_unreachable will fire when either (1) or (2) is true.

Would it be more appropriate to llvm::report_fatal_error or simply assert in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably make this a target-specific builtin. The compiler shouldn't crash just because someone used an attribute on a platform where it's not supported.

Copy link
Author

Choose a reason for hiding this comment

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

The compiler shouldn't crash just because someone used an attribute on a platform where it's not supported.

I agree. It's probably more appropriate to report an error instead of crashing?

You should probably make this a target-specific builtin

I see. I made this a general builtin because I noticed that __builtin_frame_address and __builtin_return_address are defined as general builtin's so I thought __builtin_stack_address should follow the same approach?

Making this a target-specific builtin is a good idea, but what do you think about keeping __builtin_stack_address as a general builtin and generating a Cannot compile this __builtin_stack_address yet compile error) when the target arch is unsupported? The compile error will look like the following:

llvm-project/clang/test/CodeGen/builtin-stackaddress.c:17:10: error: cannot compile this __builtin_stack_address yet
   17 |   return __builtin_stack_address();
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Insights are appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants