[PATCH] D131341: [RISC-V][HWASAN] Add tag mismatch routines for HWASAN required for RISC-V

[Alexey Baturo via Phabricator via llvm-commits]

smd added a comment.

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



================
Comment at: compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S:81
+  // This function is defined in hwasan_interceptors.cc
+  j    __sigjmp_save
+  CFI_ENDPROC
----------------
jrtc27 wrote:
> 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)
Fixed, thanks


================
Comment at: compiler-rt/lib/hwasan/hwasan_setjmp_riscv64.S:99
+
+GNU_PROPERTY_BTI_PAC
----------------
jrtc27 wrote:
> 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).
Fixed, thanks


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S:69
+.global __hwasan_tag_mismatch_v2
+.type __hwasan_tag_mismatch_v2, %function
+__hwasan_tag_mismatch_v2:
----------------
jrtc27 wrote:
> ASM_TYPE_FUNCTION?
Fixed, thanks


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S:124
+
+  call __hwasan_tag_mismatch4
+  CFI_ENDPROC
----------------
jrtc27 wrote:
> 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...
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


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_riscv64.S:127-128
+
+.Lfunc_end0:
+  .size __hwasan_tag_mismatch_v2, .Lfunc_end0-__hwasan_tag_mismatch_v2
+
----------------
jrtc27 wrote:
> ASM_SIZE?
Fixed, thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131341/new/

https://reviews.llvm.org/D131341