[RISCV][ASAN] unwind fixup

[EccoTheDolphin]

Summary

[8/11] patch series to port ASAN for riscv64

Depends On D87577

[luismarques]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

23–48

This block is incorrectly marked in the patch as already existing code, instead of new code.
For performance, it should probably do the checks in order of most to least likely case. That's probably 4, 2, 6, 8.

120

Where does this -1 value come from?

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

120

I'm not familiar with the ISA at all, but if I understand this correctly, return address is at frame[-1] and previous frame pointer is at frame[-2], so perhaps GetCanonicFrame must be updated as well?

2:   1141                    addi    sp,sp,-16
4:   e406                    sd      ra,8(sp)
6:   e022                    sd      s0,0(sp)
8:   0800                    addi    s0,sp,16

134–140

What I meant earlier is that is needs to be frame[-2] maybe?

[EccoTheDolphin]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

120

Yes, this the case. The proposed adjustment seems to be valid. We've checked it on a manually written test and the produced backtrace looks much better. I have strong suspicion that the adjustment in question should fix most of the remaining tests (the ones marked as haveing "backtrace" issues).

I've prepared relevant patches but I'd like to re-run all tests once again before pushing the fix (re-building and re-running tests takes time )

[EccoTheDolphin]

more adjustments to UnwindFast + added more comments

[EccoTheDolphin]

rebasing

[eugenis]

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

[EccoTheDolphin]

In D87579#2288622, @eugenis wrote:

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

I'll double-check.

[EccoTheDolphin]

In D87579#2288656, @EccoTheDolphin wrote:

In D87579#2288622, @eugenis wrote:

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

I'll double-check.

Looks like it does.

clang -O0
	int foo(int):
        addi    sp, sp, -32
        sd      ra, 24(sp)
        sd      s0, 16(sp)
        addi    s0, sp, 32
gcc -O0 -march=rv64imafdc -mabi=lp64d
	int foo(int):
        addi    sp,sp,-32
        sd      ra,24(sp)
        sd      s0,16(sp)
        addi    s0,sp,32

Example with gcc/clang (-O2 -fno-omit-frame-pointer')

https://godbolt.org/z/3GeET1

They match too.

[lenary]

In D87579#2289461, @EccoTheDolphin wrote:

In D87579#2288656, @EccoTheDolphin wrote:

In D87579#2288622, @eugenis wrote:

LGTM

Does gcc use the same frame record layout? We've had issues like that with Arm, see GetCanonicFrame. If not, maybe it's not too late to fix?

I'll double-check.

Looks like it does.

Is this something it would be useful for the psABI to specify? I can look at an addition along the lines of "If you save ra, it must go in a certain place in the frame. If you save fp, it also has to go in a certain place." I think other ABIs require this too, but I could be wrong.

[eugenis]

Documenting this would be a good idea.
You can say something along the lines of:
if a function has a frame pointer, it must point one byte past the end of a record that contains the previous frame pointer in the lower word, and the previous frame return address in the high word.

[EccoTheDolphin]

rebasing

[EccoTheDolphin]

@eugenis, @luismarques - I believe all concerns for this one were addressed. Do you mind if I commit this as is?

[eugenis]

I have no objections.

[vitalybuka]

In D87579#2305247, @EccoTheDolphin wrote:

@eugenis, @luismarques - I believe all concerns for this one were addressed. Do you mind if I commit this as is?

If some revision of the patch is approved, and all comments are addressed no need to wait for re-approval.
If you still want another look, it's better to somehow attract reviewer's attention, as phabricator hides patches with previously approved revisions from "Ready to Review" list.

[EccoTheDolphin]

In D87579#2307439, @vitalybuka wrote:

In D87579#2305247, @EccoTheDolphin wrote:

@eugenis, @luismarques - I believe all concerns for this one were addressed. Do you mind if I commit this as is?

If some revision of the patch is approved, and all comments are addressed no need to wait for re-approval.
If you still want another look, it's better to somehow attract reviewer's attention, as phabricator hides patches with previously approved revisions from "Ready to Review" list.

Got it. Thank you very much for clarifying!

[EccoTheDolphin]

rebasing

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

 #include "sanitizer_stacktrace.h"
 
 namespace __sanitizer {
 
 uptr StackTrace::GetNextInstructionPc(uptr pc) {
 #if defined(__sparc__) || defined(__mips__)
   return pc + 8;
 #elif defined(__powerpc__) || defined(__arm__) || defined(__aarch64__)
   return pc + 4;
 #elif SANITIZER_RISCV64
   // Current check order is 4 -> 2 -> 6 -> 8
   u8 InsnByte = *(u8 *)(pc);
   if (((InsnByte & 0x3) == 0x3) && ((InsnByte & 0x1c) != 0x1c)) {
     // xxxxxxxxxxxbbb11 | 32 bit | bbb != 111
     return pc + 4;
   }
   if ((InsnByte & 0x3) != 0x3) {
     // xxxxxxxxxxxxxxaa | 16 bit | aa != 11
     return pc + 2;
   }
   // RISC-V encoding allows instructions to be up to 8 bytes long
   if ((InsnByte & 0x3f) == 0x1f) {
     // xxxxxxxxxx011111 | 48 bit |
     return pc + 6;
   }
   if ((InsnByte & 0x7f) == 0x3f) {
     // xxxxxxxxx0111111 | 64 bit |
     return pc + 8;
   }
   // bail-out if could not figure out the instruction size
   return 0;
 #else
   return pc + 1;
 #endif
 }
 
 uptr StackTrace::GetCurrentPc() {
   return GET_CALLER_PC();
 }
 
 void BufferedStackTrace::Init(const uptr *pcs, uptr cnt, uptr extra_top_pc) {
   size = cnt + !!extra_top_pc;
#ifdef __powerpc__
     // back chain to find the caller frame before extracting it.
     uhwptr *caller_frame = (uhwptr*)frame[0];
     if (!IsValidFrame((uptr)caller_frame, stack_top, bottom) ||
         !IsAligned((uptr)caller_frame, sizeof(uhwptr)))
       break;
     uhwptr pc1 = caller_frame[2];
 #elif defined(__s390__)
     uhwptr pc1 = frame[14];
+#elif defined(__riscv)
+    // frame[-1] contains the return address
+    uhwptr pc1 = frame[-1];
 #else
     uhwptr pc1 = frame[1];
 #endif
     // Let's assume that any pointer in the 0th page (i.e. <0x1000 on i386 and
     // x86_64) is invalid and stop unwinding here.  If we're adding support for
     // a platform where this isn't true, we need to reconsider this check.
     if (pc1 < kPageSize)
       break;
     if (pc1 != pc) {
       trace_buffer[size++] = (uptr) pc1;
     }
     bottom = (uptr)frame;
-    frame = GetCanonicFrame((uptr)frame[0], stack_top, bottom);
+#if defined(__riscv)
+    // frame[-2] contain fp of the previous frame
+    uptr new_bp = (uptr)frame[-2];
+#else
+    uptr new_bp = (uptr)frame[0];
+#endif
+    frame = GetCanonicFrame(new_bp, stack_top, bottom);
   }
 }
 
 #endif  // !defined(__sparc__)
 
 void BufferedStackTrace::PopStackFrames(uptr count) {
   CHECK_LT(count, size);
   size -= count;