Page MenuHomePhabricator

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

Authored by EccoTheDolphin on Sat, Sep 12, 6:12 PM.

Details

Summary

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

Depends On D87575

Diff Detail

Event Timeline

EccoTheDolphin created this revision.Sat, Sep 12, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Sep 12, 6:12 PM
Herald added subscribers: Restricted Project, evandro, luismarques and 11 others. · View Herald Transcript
luismarques added inline comments.Mon, Sep 14, 6:56 AM
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.

smd added a subscriber: smd.Mon, Sep 14, 9:38 AM
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 accepted this revision.Wed, Sep 16, 2:18 PM
eugenis added a subscriber: 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.

This revision is now accepted and ready to land.Wed, Sep 16, 2:18 PM

address perfomance concern with the order of checking

EccoTheDolphin retitled this revision from [RISCV][ASAN] implementation for previous/next pc to [RISCV][ASAN] implementation for previous/next pc routines for riscv64.Sun, Sep 20, 11:33 PM
EccoTheDolphin edited the summary of this revision. (Show Details)
EccoTheDolphin marked 2 inline comments as done.Sun, Sep 20, 11:50 PM

addressing comments/continue

EccoTheDolphin edited the summary of this revision. (Show Details)Tue, Sep 22, 11:40 AM

addressing changes

@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.

luismarques accepted this revision.Thu, Sep 24, 2:31 AM

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.

addressed review comments

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