[RISCV][ASAN] implementation for previous/next pc routines for riscv64

[EccoTheDolphin]

Summary

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

Depends On D87575

[luismarques]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

88–95

A word-sized instruction (4 bytes) seems like the safest choice, if there is a safe choice at all, but I'm not sure this is even appropriate for RISC-V. Isn't this only for frame-pointer-based unwinds? Generally you'd use a CFI-based unwinder, not a frame-pointer-based one. If following this approach you could still do some sanity checking and verify if it points to something with a valid/plausible instruction length.

[EccoTheDolphin]

[EccoTheDolphin]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

88–95

A word-sized instruction (4 bytes) seems like the safest choice, if there is a safe choice at all, but I'm not sure this is even appropriate for RISC-V. Isn't this only for frame-pointer-based unwinds? Generally you'd use a CFI-based unwinder, not a frame-pointer-based one.

This code is still used in the existing lit/ult tests. To print the backtrace. From the codebase, it seems that we don't really need the exact value - it just needs to be somewhat accurate.

I can add sanity checks. But unfortunately, they won't be 100% reliable in the general case.

[eugenis]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

88–95

Right, ideally it should return the first byte of the previous instruction because otherwise the output could be a bit confusing. When it's hard or not possible, we simply do -1 (like on x86) so that when symbolized, that offset translates to the right code line / function at least.

-2 is also fine if all instructions are at least 2 bytes in size.

[EccoTheDolphin]

address perfomance concern with the order of checking

[EccoTheDolphin]

addressing comments/continue

[EccoTheDolphin]

addressing changes

[EccoTheDolphin]

@reviewers: I believe we are done here. Since we can't add a reliable check to figure out the exactly length - I guess the current implementation should serve fine.

[EccoTheDolphin]

rebasing

[luismarques]

I haven't had the opportunity to personally test this, but looking at the code and the information you have provided I think it's probably fine. Please just adjust the code comment.
LGTM.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

89

Some nitpicking about this block of comments.

• Typo "instruciton".
• RV-64: the base RV64 ISA doesn't have variable instruction length, only when you add the C extension. For sanitizers that you'd run in a POSIX environment, I think it's reasonable to assume RV64GC, as the GC extensions are required for Linux. This is also in seeming contradiction with the line below that says that RV-64 has 4-byte instructions, but it becomes fine if you adjust this first line to say RV64GC. Also, drop the hyphen, to be consistent with the ISA string format.

[EccoTheDolphin]

addressed review comments

[EccoTheDolphin]

@reviewers: if you have no further objections, can I proceed with committing this one? (Could you mark the patch as "accepted" once again?)

[luismarques]

LGTM. I'll commit this.

[EccoTheDolphin]

In D87577#2301159, @luismarques wrote:

LGTM. I'll commit this.

Thanks!

By the way - to my surprise, Chris gave me write permissions - So I can commit too.

[luismarques]

In D87577#2301202, @EccoTheDolphin wrote:

By the way - to my surprise, Chris gave me write permissions - So I can commit too.

That's great to hear -- and not that surprising :-)
Please go ahead and commit it yourself, then.

[vitalybuka]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

#if defined(__arm__)
   // so we return (pc-2) in that case in order to be safe.
   // For A32 mode we return (pc-4) because all instructions are 32 bit long.
   return (pc - 3) & (~1);
 #elif defined(__powerpc__) || defined(__powerpc64__) || defined(__aarch64__)
   // PCs are always 4 byte aligned.
   return pc - 4;
 #elif defined(__sparc__) || defined(__mips__)
   return pc - 8;
+#elif SANITIZER_RISCV64
+  // RV-64 has variable instruciton length...
+  // C extentions gives us 2-byte instructoins
+  // RV-64 has 4-byte instructions
+  // + RISCV architecture allows instructions up to 8 bytes
+  // It seems difficult to figure out the exact instruction length -
+  // pc - 2 seems like a safe option for the purposes of stack tracing
+  return pc - 2;
 #else
   return pc - 1;
 #endif
 }
 
 // StackTrace that owns the buffer used to store the addresses.
 struct BufferedStackTrace : public StackTrace {
   uptr trace_buffer[kStackTraceMax];

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

 
 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();
 }