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

[Alexey Baturo via Phabricator via llvm-commits]

smd 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()
----------------
vitalybuka wrote:
> 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 
> 
>I assume hwasan.check.memaccess is just not implemented yet?
Indeed it's not implemented, but to be honest I didn't think it should be implemented. I got an impression that short_granules(v2) is a preferred way of checking tags, while v1 function is kept for older targets.
Or am I wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131575