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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
