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