This is an archive of the discontinued LLVM Phabricator instance.

Differential D75168

[sanitizer][RISCV] Implement SignalContext::GetWriteFlag for RISC-V
ClosedPublic

Authored by luismarques on Feb 26 2020, 5:08 AM.

Details

Reviewers
lenary
asb
Commits
rGad1466f8cbc5: [sanitizer][RISCV] Implement SignalContext::GetWriteFlag for RISC-V
Summary

This patch follows the approach also used for MIPS, where we decode the offending instruction to determine if the fault was caused by a read or write operation, as that seems to be the only relevant information we have in the signal context structure to determine that.

Diff Detail

Event Timeline

luismarques created this revision.Feb 26 2020, 5:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 26 2020, 5:08 AM
Herald added subscribers: llvm-commits, Restricted Project, evandro and 13 others. · View Herald Transcript
luismarques updated this revision to Diff 246702.Feb 26 2020, 6:33 AM

Fix preprocessor conditional compilation block conditions.

Harbormaster completed remote builds in B47297: Diff 246702.Feb 26 2020, 6:55 AM
asb added a comment.Mar 5 2020, 9:49 AM

There are some cases where you need to perform more checking in order to be future proof for possible new standard extensions. Specifically, review the RVC Instruction Set Listings in the RISC-V spec and check for where an encoding is marked as "RES" (reserved). e.g. C.LWSP is valid only if rd!=0, and the version with rd=0 is a reserved encoding. Similar with C.LDSP. I think it's just those two.

luismarques added a comment.Mar 6 2020, 1:15 AM
In D75168#1907941, @asb wrote:

There are some cases where you need to perform more checking in order to be future proof for possible new standard extensions. Specifically, review the RVC Instruction Set Listings in the RISC-V spec and check for where an encoding is marked as "RES" (reserved). e.g. C.LWSP is valid only if rd!=0, and the version with rd=0 is a reserved encoding. Similar with C.LDSP. I think it's just those two.

I'm not sure that's something we want to do. Those are reserved for HINT instructions. The spec says:

This HINT encoding has been chosen so that simple implementations can ignore HINTs alto-
gether, and instead execute a HINT as a regular computational instruction that happens not to
mutate the architectural state.

If the core truly supports those HINTs then it shouldn't trap. If we are trapping presumably it's because the core is just executing the HINT as a plain load/store. That would typically be a NOP, but the address must be invalid and so we are trapping. In that case we are correctly identifying it as a READ or WRITE.

asb added a comment.Mar 19 2020, 6:37 AM
In D75168#1909097, @luismarques wrote:
In D75168#1907941, @asb wrote:

There are some cases where you need to perform more checking in order to be future proof for possible new standard extensions. Specifically, review the RVC Instruction Set Listings in the RISC-V spec and check for where an encoding is marked as "RES" (reserved). e.g. C.LWSP is valid only if rd!=0, and the version with rd=0 is a reserved encoding. Similar with C.LDSP. I think it's just those two.

I'm not sure that's something we want to do. Those are reserved for HINT instructions. The spec says:

This HINT encoding has been chosen so that simple implementations can ignore HINTs alto-
gether, and instead execute a HINT as a regular computational instruction that happens not to
mutate the architectural state.

If the core truly supports those HINTs then it shouldn't trap. If we are trapping presumably it's because the core is just executing the HINT as a plain load/store. That would typically be a NOP, but the address must be invalid and so we are trapping. In that case we are correctly identifying it as a READ or WRITE.

Compressed instruction encodings marked 'RES' are not the same as those marked 'HINT', so I think my reasoning for checking for those reserved encodings remains valid. From the spec:

Several instructions are only valid for certain operands; when invalid, they are marked either RES to indicate that the opcode is reserved for future standard extensions; NSE to indicate that the opcode is reserved for custom extensions; or HINT to indicate that the opcode is reserved for microarchitectural hints

luismarques updated this revision to Diff 252027.Mar 23 2020, 7:22 AM

Properly handle compressed reserved instructions.

luismarques updated this revision to Diff 252037.Mar 23 2020, 7:47 AM

Fix copy paste issue in the updated patch.

asb accepted this revision.Mar 26 2020, 4:58 AM

This LGTM, thanks Luis!

This revision is now accepted and ready to land.Mar 26 2020, 4:58 AM
Closed by commit rGad1466f8cbc5: [sanitizer][RISCV] Implement SignalContext::GetWriteFlag for RISC-V (authored by luismarques). · Explain WhyMar 26 2020, 1:03 PM
This revision was automatically updated to reflect the committed changes.
schwab added a subscriber: schwab.Jun 7 2020, 12:29 AM

../../../../libsanitizer/sanitizer_common/sanitizer_linux.cpp:1880:16: error: missing terminating ' character
1880 | case 0b10'010: // c.lwsp (rd != x0)

|                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
luismarques added a comment.Jun 7 2020, 12:33 AM
In D75168#2078362, @schwab wrote:

../../../../libsanitizer/sanitizer_common/sanitizer_linux.cpp:1880:16: error: missing terminating ' character
1880 | case 0b10'010: // c.lwsp (rd != x0)

|                ^~~~~~~~~~~~~~~~~~~~~~~~~~~

That sounds like you are compiling with a compiler that doesn't recognize the ' digit separator. That (and the binary literals) should be supported since C++14. Let me know if that addresses the issue.

schwab added a comment.Jun 7 2020, 3:57 AM

C++14 is much too recent.

luismarques added a comment.Jun 7 2020, 4:20 AM
In D75168#2078385, @schwab wrote:

C++14 is much too recent.

The LLVM documentation says:

Unless otherwise documented, LLVM subprojects are written using standard C++14 code and avoid unnecessary vendor-specific extensions.

I'm not finding any indication that compiler-rt or the sanitizers differ from that.

schwab added a comment.Jun 7 2020, 6:01 AM

It's breaking users.

luismarques added a comment.Jun 7 2020, 9:09 AM
In D75168#2078418, @schwab wrote:

It's breaking users.

If you wish to argue for a different policy regarding the adoption of C++ standards the correct place for that is the llvm-dev mailing list.

luismarques mentioned this in D87576: [RISCV][ASAN] implementation of SignalContext::GetWriteFlag.Sep 14 2020, 7:12 AM

Revision Contents

PathSize
compiler-rt/
lib/
sanitizer_common/
99 lines

Diff 252959

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

Show First 20 Lines • Show All 1,855 Lines • ▼ Show 20 Lines
#if defined(__arch64__) #if defined(__arch64__)
uptr pc = scontext->sigc_regs.tpc; uptr pc = scontext->sigc_regs.tpc;
#else #else
uptr pc = scontext->si_regs.pc; uptr pc = scontext->si_regs.pc;
#endif #endif
#endif #endif
u32 instr = *(u32 *)pc; u32 instr = *(u32 *)pc;
return (instr >> 21) & 1 ? WRITE: READ; return (instr >> 21) & 1 ? WRITE: READ;
#elif defined(__riscv)
unsigned long pc = ucontext->uc_mcontext.__gregs[REG_PC];
unsigned faulty_instruction = *(uint16_t *)pc;
#if defined(__riscv_compressed)
if ((faulty_instruction & 0x3) != 0x3) { // it's a compressed instruction
// set op_bits to the instruction bits [1, 0, 15, 14, 13]
unsigned op_bits =
((faulty_instruction & 0x3) << 3) | (faulty_instruction >> 13);
unsigned rd = faulty_instruction & 0xF80; // bits 7-11, inclusive
switch (op_bits) {
case 0b10'010: // c.lwsp (rd != x0)
#if __riscv_xlen == 64
case 0b10'011: // c.ldsp (rd != x0)
#endif
return rd ? SignalContext::READ : SignalContext::UNKNOWN;
case 0b00'010: // c.lw
#if __riscv_flen >= 32 && __riscv_xlen == 32
case 0b10'011: // c.flwsp
#endif
#if __riscv_flen >= 32 || __riscv_xlen == 64
case 0b00'011: // c.flw / c.ld
#endif
#if __riscv_flen == 64
case 0b00'001: // c.fld
case 0b10'001: // c.fldsp
#endif
return SignalContext::READ;
case 0b00'110: // c.sw
case 0b10'110: // c.swsp
#if __riscv_flen >= 32 || __riscv_xlen == 64
case 0b00'111: // c.fsw / c.sd
case 0b10'111: // c.fswsp / c.sdsp
#endif
#if __riscv_flen == 64
case 0b00'101: // c.fsd
case 0b10'101: // c.fsdsp
#endif
return SignalContext::WRITE;
default:
return SignalContext::UNKNOWN;
}
}
#endif
unsigned opcode = faulty_instruction & 0x7f; // lower 7 bits
unsigned funct3 = (faulty_instruction >> 12) & 0x7; // bits 12-14, inclusive
switch (opcode) {
case 0b0000011: // loads
switch (funct3) {
case 0b000: // lb
case 0b001: // lh
case 0b010: // lw
#if __riscv_xlen == 64
case 0b011: // ld
#endif
case 0b100: // lbu
case 0b101: // lhu
return SignalContext::READ;
default:
return SignalContext::UNKNOWN;
}
case 0b0100011: // stores
switch (funct3) {
case 0b000: // sb
case 0b001: // sh
case 0b010: // sw
#if __riscv_xlen == 64
case 0b011: // sd
#endif
return SignalContext::WRITE;
default:
return SignalContext::UNKNOWN;
}
#if __riscv_flen >= 32
case 0b0000111: // floating-point loads
switch (funct3) {
case 0b010: // flw
#if __riscv_flen == 64
case 0b011: // fld
#endif
return SignalContext::READ;
default:
return SignalContext::UNKNOWN;
}
case 0b0100111: // floating-point stores
switch (funct3) {
case 0b010: // fsw
#if __riscv_flen == 64
case 0b011: // fsd
#endif
return SignalContext::WRITE;
default:
return SignalContext::UNKNOWN;
}
#endif
default:
return SignalContext::UNKNOWN;
}
#else #else
(void)ucontext; (void)ucontext;
return UNKNOWN; // FIXME: Implement. return UNKNOWN; // FIXME: Implement.
#endif #endif
} }
bool SignalContext::IsTrueFaultingAddress() const { bool SignalContext::IsTrueFaultingAddress() const {
auto si = static_cast<const siginfo_t *>(siginfo); auto si = static_cast<const siginfo_t *>(siginfo);
▲ Show 20 Lines • Show All 293 Lines • Show Last 20 Lines