[clang][Sparc] Fix __builtin_extract_return_addr etc.

[ro]

Summary

While investigating the failures of symbolize_pc.cpp and symbolize_pc_inline.cpp on SPARC (both Solaris and Linux), I noticed that __builtin_extract_return_addr is a no-op in clang on all targets, while gcc has non-default implementations for arm, mips, s390, and sparc.

This patch provides the SPARC implementation. For background see SparcISelLowering.cpp (SparcTargetLowering::LowerReturn_32) , the SPARC psABI p.3-12, %i7 and p.3-16/17, and SCD 2.4.1, p.3P-10, %i7 and p.3P-15.

Tested (after enabling the sanitizer_common tests on SPARC) on sparcv9-sun-solaris2.11.

[ro]

Ping. This hasn't been reviewed in more than a year and is still required for D91608.

[efriedma]

Testcase?

Do you need to ptrtoint/inttoptr? I would expect that the address is an i8*, so you can just GEP an appropriate number of bytes.

[ro]

In D91607#3280808, @efriedma wrote:

Testcase?

I thought the 3 testcases adjusted in D91608 to use __builtin_extract_return_addr and fixed by this patch were enough. Otherwise, should I just augment clang/test/CodeGen/builtins-sparc.c or better create a new test?

Do you need to ptrtoint/inttoptr? I would expect that the address is an i8*, so you can just GEP an appropriate number of bytes.

TBH, I know practically nothing about LLVM codegen, so I rely heavily on guidance. IIRC this patch was developed by searching for similar code in TargetInfo.cpp and modifying it until it did what I needed. Is this the place to read on GEP?

[efriedma]

In D91607#3283350, @ro wrote:

In D91607#3280808, @efriedma wrote:

Testcase?

I thought the 3 testcases adjusted in D91608 to use __builtin_extract_return_addr and fixed by this patch were enough. Otherwise, should I just augment clang/test/CodeGen/builtins-sparc.c or better create a new test?

I'd prefer to have coverage in the clang regression tests, so developers can catch regressions and easily check the expected codegen. builtins-sparc.c is fine.

Do you need to ptrtoint/inttoptr? I would expect that the address is an i8*, so you can just GEP an appropriate number of bytes.

TBH, I know practically nothing about LLVM codegen, so I rely heavily on guidance. IIRC this patch was developed by searching for similar code in TargetInfo.cpp and modifying it until it did what I needed. Is this the place to read on GEP?

That's a good starting place for understanding the complexity of GEP... but you don't need that much here. Here, we just want to pass a single index; that's equivalent to pointer addition in C.

You should be able to just drop the calls to CreatePtrToInt and CreateIntToPtr, and replace CreateAdd with CreateGEP.

[ro]

In D91607#3285825, @efriedma wrote:

In D91607#3283350, @ro wrote:

In D91607#3280808, @efriedma wrote:

Testcase?

I thought the 3 testcases adjusted in D91608 to use __builtin_extract_return_addr and fixed by this patch were enough. Otherwise, should I just augment clang/test/CodeGen/builtins-sparc.c or better create a new test?

I'd prefer to have coverage in the clang regression tests, so developers can catch regressions and easily check the expected codegen. builtins-sparc.c is fine.

Understood: this way you don't rely on native builds and can test SPARC V8 structure return which isn't exercised by the sanitzer tests.

Do you need to ptrtoint/inttoptr? I would expect that the address is an i8*, so you can just GEP an appropriate number of bytes.

TBH, I know practically nothing about LLVM codegen, so I rely heavily on guidance. IIRC this patch was developed by searching for similar code in TargetInfo.cpp and modifying it until it did what I needed. Is this the place to read on GEP?

That's a good starting place for understanding the complexity of GEP... but you don't need that much here. Here, we just want to pass a single index; that's equivalent to pointer addition in C.

You should be able to just drop the calls to CreatePtrToInt and CreateIntToPtr, and replace CreateAdd with CreateGEP.

Cool: I'd been struggling with CreateConstInBoundsByteGEP which seemed what I need, but had me fighting with conversion between llvm::Value * and Address.

[ro]

• Use CreateGEP
• Add testcase

[efriedma]

LGTM

clang/lib/CodeGen/TargetInfo.cpp

for (auto &Arg : FI.arguments())
     Arg.info = classifyArgumentType(Arg.type);
 }
 
 namespace {
 class SparcV8TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   SparcV8TargetCodeGenInfo(CodeGenTypes &CGT)
       : TargetCodeGenInfo(std::make_unique<SparcV8ABIInfo>(CGT)) {}
+
+  llvm::Value *decodeReturnAddress(CodeGen::CodeGenFunction &CGF,
+                                   llvm::Value *Address) const override {
+    int Offset;
+    if (isAggregateTypeForABI(CGF.CurFnInfo->getReturnType()))
+      Offset = 12;
+    else
+      Offset = 8;
+    return CGF.Builder.CreateGEP(CGF.Int8Ty, Address,
+                                 llvm::ConstantInt::get(CGF.Int32Ty, Offset));
+  }
+
+  llvm::Value *encodeReturnAddress(CodeGen::CodeGenFunction &CGF,
+                                   llvm::Value *Address) const override {
+    int Offset;
+    if (isAggregateTypeForABI(CGF.CurFnInfo->getReturnType()))
+      Offset = -12;
+    else
+      Offset = -8;
+    return CGF.Builder.CreateGEP(CGF.Int8Ty, Address,
+                                 llvm::ConstantInt::get(CGF.Int32Ty, Offset));
+  }
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // SPARC v9 ABI Implementation.
 // Based on the SPARC Compliance Definition version 2.4.1.
 //
 // Function arguments a mapped to a nominal "parameter array" and promoted to
SparcV9TargetCodeGenInfo(CodeGenTypes &CGT)
       : TargetCodeGenInfo(std::make_unique<SparcV9ABIInfo>(CGT)) {}
 
   int getDwarfEHStackPointer(CodeGen::CodeGenModule &M) const override {
     return 14;
   }
 
   bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                llvm::Value *Address) const override;
+
+  llvm::Value *decodeReturnAddress(CodeGen::CodeGenFunction &CGF,
+                                   llvm::Value *Address) const override {
+    return CGF.Builder.CreateGEP(CGF.Int8Ty, Address,
+                                 llvm::ConstantInt::get(CGF.Int32Ty, 8));
+  }
+
+  llvm::Value *encodeReturnAddress(CodeGen::CodeGenFunction &CGF,
+                                   llvm::Value *Address) const override {
+    return CGF.Builder.CreateGEP(CGF.Int8Ty, Address,
+                                 llvm::ConstantInt::get(CGF.Int32Ty, -8));
+  }
 };
 } // end anonymous namespace
 
 bool
 SparcV9TargetCodeGenInfo::initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                                 llvm::Value *Address) const {
   // This is calculated from the LLVM and GCC tables and verified
   // against gcc output.  AFAIK all ABIs use the same encoding.

clang/test/CodeGen/builtins-sparc.c

 // REQUIRES: sparc-registered-target
 // RUN: %clang_cc1 -triple sparc-unknown-unknown -emit-llvm %s -o - | FileCheck %s
-// RUN: %clang_cc1 -triple sparc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple sparc64-unknown-unknown -emit-llvm %s -o - | FileCheck -check-prefix CHECK-V9 %s
 
 void test_eh_return_data_regno(void)
 {
   volatile int res;
-  res = __builtin_eh_return_data_regno(0);  // CHECK: store volatile i32 24
-  res = __builtin_eh_return_data_regno(1);  // CHECK: store volatile i32 25
+  res = __builtin_eh_return_data_regno(0);  // CHECK,CHECKV9: store volatile i32 24
+  res = __builtin_eh_return_data_regno(1);  // CHECK,CHECKV9: store volatile i32 25
+}
+
+void *test_extract_return_address(void)
+{
+  // CHECK,CHECKV9: getelementptr i8, i8* %0, i32 8
+  return __builtin_extract_return_addr(__builtin_return_address(0));
+}
+
+struct s {
+  void *p;
+};
+
+struct s test_extract_struct_return_address(void)
+{
+  struct s s;
+  // CHECK:    getelementptr i8, i8* %0, i32 12
+  // CHECK-V9: getelementptr i8, i8* %0, i32 8
+  s.p = __builtin_extract_return_addr(__builtin_return_address(0));
+  return s;
 }