Page MenuHomePhabricator

[clang][Sparc] Fix __builtin_extract_return_addr etc.
ClosedPublic

Authored by ro on Nov 17 2020, 3:12 AM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
370 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

ro created this revision.Nov 17 2020, 3:12 AM
ro requested review of this revision.Nov 17 2020, 3:12 AM
ro added a comment.Jan 28 2022, 3:44 AM

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

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 added a comment.Jan 31 2022, 2:11 AM

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?

In D91607#3283350, @ro 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 added a comment.Feb 1 2022, 7:53 AM
In D91607#3283350, @ro 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 updated this revision to Diff 404939.Feb 1 2022, 7:53 AM
  • Use CreateGEP
  • Add testcase
efriedma accepted this revision.Feb 2 2022, 9:53 AM

LGTM

This revision is now accepted and ready to land.Feb 2 2022, 9:53 AM
This revision was automatically updated to reflect the committed changes.