[PATCH] D107850: [asan] [Patch 1/3] Implemented custom calling convention similar to the one used by HWASan for X86.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

Can you please use "Edit related revisions" and link other patches of the features into the stack?



================
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:272-274
+  [(int_asan_check_memaccess GR64NoR8:$addr, (i64 timm:$shadowbase),
+    (i32 timm:$iswrite), (i32 timm:$accesssizeindex), (i32 timm:$mappingscale),
+    (i32 timm:$orshadowoffset))]>,
----------------
shadowbase, mappingscale and orshadowoffset are known to runtime, we should not have them as part of intrinsic


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1330
+  if (!TM.getTargetTriple().isOSBinFormatELF()) {
+    report_fatal_error("llvm.hwa`san.check.memaccess only supported on ELF");
+    return;
----------------
This does not look right


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1334
+
+  unsigned Reg = MI.getOperand(0).getReg().id();
+  uint64_t ShadowBase = MI.getOperand(1).getImm();
----------------
Why this called reg when it's address?


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1340
+  MCSymbol *&Sym = AsanMemaccessSymbols[{
+      Reg, ShadowBase, IsWrite, AccessSizeIndex, MI.getOperand(4).getImm(),
+      MI.getOperand(5).getImm()}];
----------------
Why shadow is an argument? Both runtime and instrumented code know the shadow.


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1340-1341
+  MCSymbol *&Sym = AsanMemaccessSymbols[{
+      Reg, ShadowBase, IsWrite, AccessSizeIndex, MI.getOperand(4).getImm(),
+      MI.getOperand(5).getImm()}];
+  if (!Sym) {
----------------
vitalybuka wrote:
> Why shadow is an argument? Both runtime and instrumented code know the shadow.
could you please unpack these to into local variables for consistency?


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1369
+                                   .addReg(X86::R8)
+                                   .addImm(MappingScale),
+                               STI);
----------------
I assumed the goal to move this shadow math into runtime code
so we have less instructions in the instrumented code.

Also MappingScale and OrShadowOffset are constants.
It's a little bit not nice that ShadowBase can be dynamic, so each __asan_check_ will have to load it,
but let's for now care about fixed shadow which is constant.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107850