[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 {
+    llvm::Value *AddrAsInt = Address;
+    AddrAsInt = CGF.Builder.CreatePtrToInt(AddrAsInt, CGF.IntPtrTy);
+    AddrAsInt = CGF.Builder.CreateAdd(AddrAsInt,
+                                      llvm::ConstantInt::get(CGF.IntPtrTy, 8));
+    if (isAggregateTypeForABI(CGF.CurFnInfo->getReturnType()))
+      AddrAsInt = CGF.Builder.CreateAdd(
+          AddrAsInt, llvm::ConstantInt::get(CGF.IntPtrTy, 4));
+    AddrAsInt = CGF.Builder.CreateIntToPtr(AddrAsInt, Address->getType());
+    return AddrAsInt;
+  }
+
+  llvm::Value *encodeReturnAddress(CodeGen::CodeGenFunction &CGF,
+                                   llvm::Value *Address) const override {
+    llvm::Value *AddrAsInt = Address;
+    AddrAsInt = CGF.Builder.CreatePtrToInt(AddrAsInt, CGF.IntPtrTy);
+    AddrAsInt = CGF.Builder.CreateAdd(AddrAsInt,
+                                      llvm::ConstantInt::get(CGF.IntPtrTy, -8));
+    if (isAggregateTypeForABI(CGF.CurFnInfo->getReturnType()))
+      AddrAsInt = CGF.Builder.CreateAdd(
+          AddrAsInt, llvm::ConstantInt::get(CGF.IntPtrTy, -4));
+    AddrAsInt = CGF.Builder.CreateIntToPtr(AddrAsInt, Address->getType());
+    return AddrAsInt;
+  }
 };
 } // 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 {
+    llvm::Value *AddrAsInt = Address;
+    AddrAsInt = CGF.Builder.CreatePtrToInt(AddrAsInt, CGF.IntPtrTy);
+    AddrAsInt = CGF.Builder.CreateAdd(AddrAsInt,
+                                      llvm::ConstantInt::get(CGF.IntPtrTy, 8));
+    AddrAsInt = CGF.Builder.CreateIntToPtr(AddrAsInt, Address->getType());
+    return AddrAsInt;
+  }
+
+  llvm::Value *encodeReturnAddress(CodeGen::CodeGenFunction &CGF,
+                                   llvm::Value *Address) const override {
+    llvm::Value *AddrAsInt = Address;
+    AddrAsInt = CGF.Builder.CreatePtrToInt(AddrAsInt, CGF.IntPtrTy);
+    AddrAsInt = CGF.Builder.CreateAdd(AddrAsInt,
+                                      llvm::ConstantInt::get(CGF.IntPtrTy, -8));
+    AddrAsInt = CGF.Builder.CreateIntToPtr(AddrAsInt, Address->getType());
+    return AddrAsInt;
+  }
 };
 } // 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.