[PATCH] D114558: [ASan] Shared optimized callbacks implementation.

[Kirill Stoimenov via Phabricator via llvm-commits]

kstoimenov added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_rtl_x86_64.S:18
+#define BEGINF(reg, op, s, i) \
+.globl  FNAME(reg, op, s, i) ;\
+ASM_TYPE_FUNCTION(FNAME(reg, op, s, i)) ;\
----------------
morehouse wrote:
> Nit:  Can we omit the semicolons?
Unfortunately no. The issue is that macros are expanded as single line and ; is used instead of a new line character. 


================
Comment at: compiler-rt/lib/asan/asan_rtl_x86_64.S:42
+        pop    %rcx ;\
+        jl     RLABEL(reg, op, 1, i);\
+        mov    %##reg,%rdi ;\
----------------
morehouse wrote:
> Maybe I've confused myself, but isn't this condition opposite of what we want?
> 
> ```
> if (last_accessed_byte >= shadow_value) crash()
> ```
> would translate to
> ```
> cmp %ecx, %r8d
> ...
> jl RLABEL(...)
> ```
> or 
> ```
> cmp %r8d, %ecx
> ...
> jg RLABEL(...)
> ```
Here is the disassembled version of __asan_load1: 

        mov    %rdi,%rax
        shr    $0x3,%rax
        movsbl 0x7fff8000(%rax),%eax
        test   %eax,%eax
        jne    <__asan_load1+0x13>
        ret    
        mov    %edi,%ecx
        and    $0x7,%ecx
        cmp    %eax,%ecx
        jl     <__asan_load1+0x12>
        xor    %esi,%esi
        mov    $0x1,%edx
        xor    %ecx,%ecx
        mov    $0x1,%r8d
        jmp    <_ZN6__asanL25ReportGenericErrorWrapperEmbiib>


================
Comment at: compiler-rt/lib/asan/asan_rtl_x86_64.S:99
+        mov    %##reg,%rdi ;\
+        jmp    __asan_report_##op##8 ;\
+
----------------
morehouse wrote:
> I think we also need to handle the 16 byte case.
Good catch! 


================
Comment at: compiler-rt/lib/asan/asan_shadow_defines.h:31-49
+#  if SANITIZER_RISCV64
+#    define SHADOW_OFFSET_CONST 0x55550000
+#  elif defined(__aarch64__)
+#    define SHADOW_OFFSET_CONST 0x1000000000
+#  elif defined(__powerpc64__)
+#    define SHADOW_OFFSET_CONST 0x100000000000
+#  elif defined(__s390x__)
----------------
Fixed 0 constants. @morehouse PTAL.


================
Comment at: compiler-rt/lib/asan/tests/asan_noinst_test.cpp:351
+
+#if defined(SHADOW_OFFSET_STR)
+TEST(AddressSanitizer, TestShadowGlobalVar) {
----------------
vitalybuka wrote:
> I see  SHADOW_OFFSET_STR neither in existing code nor in the patch
> Is some file is missing or another patch.
It is supposed to be SHADOW_OFFSET. Good catch. 


================
Comment at: compiler-rt/test/asan/TestCases/Linux/interface_symbols_linux.cpp:4
+// RUN: %clangxx -x c++-header -o - -E %p/../../../../lib/asan/asan_interface.inc  \
+// RUN:  >  %t.asan_interface.inc
 // RUN: %clangxx_asan -O2 %s -o %t.exe
----------------
morehouse wrote:
> Why is this step required now?
It runs the preprocessor, which will produce platform specific files for the platform as they are guarded by #ifdef in the asan_interface.inc file. 


================
Comment at: llvm/test/CodeGen/X86/asan-check-memaccess-add.ll:61
 ; CHECK-NOT:          push    %rbp
-; CHECK:              callq   __asan_check_load16_rn[[RN16:.*]]
-; CHECK:              callq   __asan_check_store16_rn[[RN16]]
+; CHECK:              callq   __asan_check_x86_64_load_add_16_[[REG16:.*]]
+; CHECK:              callq   __asan_check_x86_64_store_add_16_[[REG16]]
----------------
vitalybuka wrote:
> why do you need x86_64 in nname?
No, it doesn't. Removed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114558