[PATCH] D131575: [RISC-V][HWASAN] Add support for HWASAN code instrumentation for RISC-V

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:838
+  // In case of RISC-V always use shortgranules
   IRB.CreateCall(Intrinsic::getDeclaration(
+                     M, UseShortGranules || TargetTriple.isRISCV64()
----------------
smd wrote:
> vitalybuka wrote:
> > Code should respect ClUseShortGranules, even if it's not useful mode.
> > Otherwise UseShortGranules should be true for non android
> > 
> @vitalybuka 
> Could you please clarify:
> If I get you right, you're suggesting allowing users to override UseShortGranules and make it false, even if the platform doesn't support this mode, right? 
> For cases rather than this, short granules would be always used. If the user decides to use non-short granules for riscv, in this case the compilation would fail with:
> ```
> fatal error: error in backend: Cannot select: intrinsic %llvm.hwasan.check.memaccess
> ```
> which is sort of ok, I guess. But maybe an explicit check like (isRISCV64() && !UseShortGranules) with an error message would be a better idea?
> @vitalybuka 
> Could you please clarify:
> If I get you right, you're suggesting allowing users to override UseShortGranules and make it false, even if the platform doesn't support this mode, right? 
> For cases rather than this, short granules would be always used. If the user decides to use non-short granules for riscv, in this case the compilation would fail with:
> ```
> fatal error: error in backend: Cannot select: intrinsic %llvm.hwasan.check.memaccess
> ```
> which is sort of ok, I guess. But maybe an explicit check like (isRISCV64() && !UseShortGranules) with an error message would be a better idea?

I assume hwasan.check.memaccess is just not implemented yet?
copt<> are internal flags, if some one using them, it's their responsibility to understand consequences.
So I would prefer we respect user provided ClUseShortGranules and special error is not needed.

in this case these lines not need to change 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131575