Page MenuHomePhabricator

[RISC-V][HWASAN] Add tag mismatch routines for HWASAN required for RISC-V
ClosedPublic

Authored by smd on Aug 6 2022, 2:45 PM.

Details

Diff Detail

Event Timeline

smd created this revision.Aug 6 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 2:45 PM
smd added a project: Restricted Project.Aug 6 2022, 3:07 PM
smd published this revision for review.Aug 7 2022, 3:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 3:29 AM
Herald added subscribers: llvm-commits, Restricted Project, pcwang-thead. · View Herald Transcript
craig.topper added inline comments.
compiler-rt/lib/hwasan/CMakeLists.txt
21

this should be alphabetized above x86_64

llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn
73

alphabetize

kito-cheng added inline comments.Aug 7 2022, 7:27 PM
compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S
63

That might not work correctly for rv64if / lp64f, but that is rare combination, so I would suggest you add some #if/#error here to make sure that won't happen :)

smd updated this revision to Diff 450792.Aug 8 2022, 6:54 AM

Addressing comments

smd added inline comments.Aug 8 2022, 6:57 AM
compiler-rt/lib/hwasan/CMakeLists.txt
21

Fixed, thanks

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S
63

Addressed this issue, thanks

llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn
73

Fixed, thanks

vitalybuka accepted this revision.Aug 9 2022, 5:46 PM
vitalybuka added a reviewer: kito-cheng.

LGTM, but someone who work RISC-V should take a look

jrtc27 added inline comments.Aug 9 2022, 6:12 PM
compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S
78

FLEN is irrelevant, only the floating-point ABI matters. This also doesn't fail for lp64q. What you want is:

  • __riscv_float_abi_soft - do nothing
  • __riscv_float_abi_double - save/restore
  • anything else - error
smd updated this revision to Diff 451463.Aug 10 2022, 7:57 AM

Addressing comments

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S
78

Makes sense, fixed. Thanks

smd edited the summary of this revision. (Show Details)Aug 10 2022, 7:58 AM
smd added a comment.Aug 16 2022, 11:49 AM

LGTM, but someone who work RISC-V should take a look

@kito-cheng @craig.topper could you please help to identify someone who could help with reviewing riscv part of these patch series?
Thanks

LGTM, but someone who work RISC-V should take a look

@kito-cheng @craig.topper could you please help to identify someone who could help with reviewing riscv part of these patch series?
Thanks

This is new code, which should not regress existing stuff.
So I am OK if you remove "blocker" I added to @kito-cheng.

jrtc27 added inline comments.Aug 17 2022, 2:36 PM
compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S
48

The size of jmp_buf, and the order of fields within it, is implementation-specific. Moreover, the latter is not part of the ABI, it can change AFAIK. Since longjmp gets intercepted too this is just about ok (though you haven't modified InternalLongjmp, and I don't get why that's implemented in C, that seems dodgy, then again so does this whole design that you're adding RISC-V support to), though technically you could end up storing to padding bytes in jmp_buf here that don't get copied if someone copies a jmp_buf (real-world code does copy jmp_buf's; tcsh is one example, speaking from experience of having broken jmp_buf copying).

82

Tail calls must use tail not j otherwise you run into issues once the object this is linked into gets too big as JAL only has a 20-bit immediate (the exception is for jumping to functions within the same section in the same file, as those end up in a single input section and can't be split up so will be near each other in the output)

100

This is an AArch64-ism (BTI and PAC are AArch64 extension names) so doesn't belong in RISC-V-specific code (though does *work* because it's #define'd to nothing on non-AArch64 so it can be used in architecture-independent assembly files, currently just the vfork interceptor wrapper).

compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S
70

ASM_TYPE_FUNCTION?

125

call or tail? Bit weird to have a call at the end of a function, returning to it isn't useful and will fall off the end, though the AArch64 code does use bl not b so the equivalent is call...

128–129

ASM_SIZE?

smd updated this revision to Diff 453633.Aug 18 2022, 5:40 AM

Addressing comments

smd updated this revision to Diff 453635.Aug 18 2022, 5:46 AM

Fixed missing label

smd added a comment.Aug 18 2022, 6:07 AM

@jrtc27 I would like to thank you for your such elaborate comments, your help is much appreciated. Thanks!

compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S
82

Fixed, thanks

100

Fixed, thanks

compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S
70

Fixed, thanks

125

call here is used intentionally to get the address of previous frame: later while printing backtrace it allows to skip all service hwasan frames. I agree it's not obvious, maybe I should add a comment there? Thanks

128–129

Fixed, thanks

smd removed 1 blocking reviewer(s): kito-cheng.Aug 18 2022, 7:27 AM
This revision is now accepted and ready to land.Aug 18 2022, 7:27 AM