[PATCH] D107850: [asan] Implemented intrinsic for the custom calling convention similar used by HWASan for X86.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h:150
 
+namespace AsanAccessInfo {
+
----------------
kstoimenov wrote:
> vitalybuka wrote:
> > It's not how enums described here https://llvm.org/docs/CodingStandards.html#id43
> > Also it's common to use enum class nowerdays.
> > 
> > 
> > However this one does not need to be enum and just "constexpr size_t"
> I wanted to be as close as possible to the HWASan style. Please let me know if you want me to change it. 
They are quite unrelated, no need to violate coding style to match them.

Also packing and unpacking implementation unnecessary separated.
Maybe something like this and keep bit arithmetic in a single cpp file:
```
struct AsanAccessInfo {
  uint8_t AccessSize;
  bool CompileKernel;
  bool IsWrite;
...

  explicit AsanAccessInfo(int64_t packed);
  int64_t Pack() const;
}
```

Also math a little bit simpler if 4bit field is the last


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1426
+  OutStreamer->emitInstruction(MCInstBuilder(X86::AND32ri8)
+                                   .addReg(X86::ECX)
+                                   .addReg(X86::ECX)
----------------
what is in ECX register here?


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1553-1555
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_ELF_TypeFunction);
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_Weak);
+    OutStreamer->emitSymbolAttribute(Sym, MCSA_Hidden);
----------------
Tests do not check these attributes.


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1561-1564
+    bool CompileKernel = (AccessInfo >> ASanAccessInfo::CompileKernelShift) & 1;
+    int32_t AccessSizeIndex =
+        (AccessInfo >> ASanAccessInfo::AccessSizeShift) & 0xf;
+    bool IsWrite = (AccessInfo >> ASanAccessInfo::IsWriteShift) & 1;
----------------
as-is shifts are in the header, and masks are here, which is not nice.


================
Comment at: llvm/test/CodeGen/X86/asan-check-memaccess-add.ll:6
+define void @load1(i8* nocapture readonly %x) {
+; CHECK:              callq   __asan_check_load1_rn[[RN1:.*]]
+; CHECK:              callq   __asan_check_store1_rn[[RN1]]
----------------
should we also check how we load registers before callq?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107850