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

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h:150
 
+namespace AsanAccessInfo {
+
----------------
vitalybuka wrote:
> 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
I like the idea. Keeps things well encapsulated in the implementation file. Done. 


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


================
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;
----------------
vitalybuka wrote:
> as-is shifts are in the header, and masks are here, which is not nice.
Encapsulated in the implementation file as you have recommended. 


================
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]]
----------------
vitalybuka wrote:
> should we also check how we load registers before callq?
The issue is that there is no load instruction, because the registers are implied.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107850