[PATCH] D131343: [RISC-V][HWASAN] Add support for lowering HWASAN intrinsic for RISC-V

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:120
+
   if (!lowerRISCVMachineInstrToMCInst(MI, TmpInst, *this))
     EmitToStreamer(*OutStreamer, TmpInst);
----------------
vitalybuka wrote:
> This "if" looks to random here?
> File structure is very different from AArch64 version, so not sure if this is the best place for the code.
> 
lowerRISCVMachineInstrToMCInst seems like the right place for it. That's where PATCHABLE_FUNCTION_ENTER gets handled. Given that this node is a copy of AArch64's and every architecture would presumably use the same thing I do wonder if it should be under TargetOpcode, too...


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:451
+
+    // Intentionally load the GOT entry and branch to it, rather than possibly
+    // late binding the function, which may clobber the registers before we have
----------------
This is what STO_RISCV_VARIANT_CC is for


================
Comment at: llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll:1
+; RUN: llc -mtriple=riscv64 < %s | FileCheck %s
+
----------------
Use update_llc_test_checks.py. Also test PIC too if you have PIC-specific code.


================
Comment at: llvm/test/CodeGen/RISCV/hwasan-check-memaccess.ll:3
+
+target triple = "riscv64-unknown-linux"
+
----------------
Redundant, you have -mtriple in the command line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131343