* [PATCH] Don't expand string/memory builtins if ASan is enabled. [not found] <5441008A.2010706@partner.samsung.com> @ 2014-10-17 11:47 ` Maxim Ostapenko 2014-10-17 12:29 ` Jakub Jelinek 0 siblings, 1 reply; 10+ messages in thread From: Maxim Ostapenko @ 2014-10-17 11:47 UTC (permalink / raw) To: GCC Patches; +Cc: Jakub Jelinek, Yury Gribov, Slava Garbuzov, Maxim Ostapenko [-- Attachment #1: Type: text/plain, Size: 954 bytes --] Hi, this patch disables string/memory builtin functions inlining if ASan is enabled. As described in my previous post (https://gcc.gnu.org/ml/gcc/2014-09/msg00020.html), this allow us to be sure that some dangerous builtins (strcpy, stpcpy, etc) will be handled correctly. Also, some redundant checks will be removed for builtin functions, that are instrumented but later not inlined for some reason. Patch also changes logic in asan_mem_ref_hash updating. I eliminated memory ref access size from hash computing, so all accesses for same memory reference have the same hash. Updating of asan_mem_ref_hash occurs only if new access size is greater then saved one. I've provided some performance testing (spec2006 v1.1) on x86_64-unknown-linux-gnu and attached results in test.res (sorry for this, I couldn't make my Thunderbird make a pretty table). Regtested / bootstrapped on x86_64-unknown-linux-gnu. Does this patch look sane? -Maxim [-- Attachment #2: test.res --] [-- Type: text/plain, Size: 1595 bytes --] $ ~/install/master-x86_64/bin/gcc -v Using built-in specs. COLLECT_GCC=/home/max/install/master-x86_64/bin/gcc COLLECT_LTO_WRAPPER=/home/max/install/master-x86_64/libexec/gcc/x86_64-unknown-linux-gnu/5.0.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: /home/max/workspace/downloads/gcc/configure --enable-multilib --enable-checking --target=x86_64-unknown-linux-gnu --host=x86_64-unknown-linux-gnu --build=x86_64-unknown-linux-gnu --prefix=/home/max/install/master-x86_64 --disable-bootstrap --enable-languages=c,c++ Thread model: posix gcc version 5.0.0 20141014 (experimental) (GCC) Compile options: -O3 -fsanitize=address -static-libasan. test master nobuiltin % of slowdown 400.perlbench 1044 1050 0,5747 401.bzip2 682 676 -0,8798 403.gcc 497 495 -0,4024 429.mcf 488 489 0,2049 445.gobmk 723 724 0,1383 456.hmmer 783 750 -4,2146 458.sjeng 887 880 -0,7892 462.libquantum 330 323 -2,1212 464.h264ref 1108 1154 4,1516 471.omnetpp 545 559 2,5688 473.astar 490 480 -2,0408 483.xalancbmk 411 400 -2,6764 433.milc 517 509 -1,5474 444.namd 419 419 0,0000 450.soplex 310 299 -3,5484 453.povray 287 276 -3,8328 470.lbm 299 306 2,3411 482.sphinx3 777 804 3,4749 Geomean: master nobuiltin % of increase 540 538 -0,50 [-- Attachment #3: builtins-12.diff --] [-- Type: text/x-patch, Size: 26496 bytes --] gcc/ChangeLog: 2014-10-17 Max Ostapenko <m.ostapenko@partner.samsung.com> * asan.c (asan_mem_ref_hasher::hash): Remove MEM_REF access size from hash value construction. Call iterative_hash_expr instead of explicit hash building. (asan_mem_ref_hasher::equal): Change condition. (has_mem_ref_been_instrumented): Likewise. (update_mem_ref_hash_table): Likewise. (maybe_update_mem_ref_hash_table): New function. (instrument_strlen_call): Removed. (instrument_mem_region_access): Likewise. (instrument_builtin_call): Call maybe_update_mem_ref_hash_table instead of instrument_mem_region_access. * builtins.c (is_memory_builtin): New function. (expand_builtin): Don't expand string/memory builtin functions if ASan is enabled. * builtins.def: Add comment. gcc/testsuite/ChangeLog: 2014-10-17 Max Ostapenko <m.ostapenko@partner.samsung.com> * c-c++-common/asan/no-redundant-instrumentation-1.c: Updated test. * c-c++-common/asan/no-redundant-instrumentation-4.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-2.c: Removed. * c-c++-common/asan/no-redundant-instrumentation-9.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-10.c: New test. * c-c++-common/asan/no-redundant-instrumentation-11.c: Likewise. diff --git a/gcc/asan.c b/gcc/asan.c index 2a61a82..391f693 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -352,10 +352,7 @@ struct asan_mem_ref_hasher inline hashval_t asan_mem_ref_hasher::hash (const asan_mem_ref *mem_ref) { - inchash::hash hstate; - inchash::add_expr (mem_ref->start, hstate); - hstate.add_wide_int (mem_ref->access_size); - return hstate.end (); + return iterative_hash_expr (mem_ref->start, 0); } /* Compare two memory references. We accept the length of either @@ -365,8 +362,7 @@ inline bool asan_mem_ref_hasher::equal (const asan_mem_ref *m1, const asan_mem_ref *m2) { - return (m1->access_size == m2->access_size - && operand_equal_p (m1->start, m2->start, 0)); + return operand_equal_p (m1->start, m2->start, 0); } static hash_table<asan_mem_ref_hasher> *asan_mem_ref_ht; @@ -417,7 +413,8 @@ has_mem_ref_been_instrumented (tree ref, HOST_WIDE_INT access_size) asan_mem_ref r; asan_mem_ref_init (&r, ref, access_size); - return (get_mem_ref_hash_table ()->find (&r) != NULL); + asan_mem_ref *saved_ref = get_mem_ref_hash_table ()->find (&r); + return saved_ref && (saved_ref->access_size >= access_size); } /* Return true iff the memory reference REF has been instrumented. */ @@ -434,19 +431,11 @@ has_mem_ref_been_instrumented (const asan_mem_ref *ref) static bool has_mem_ref_been_instrumented (const asan_mem_ref *ref, tree len) { - /* First let's see if the address of the beginning of REF has been - instrumented. */ - if (!has_mem_ref_been_instrumented (ref)) - return false; - - if (len != 0) - { - /* Let's see if the end of the region has been instrumented. */ - if (!has_mem_ref_been_instrumented (asan_mem_ref_get_end (ref, len), - ref->access_size)) - return false; - } - return true; + HOST_WIDE_INT size_in_bytes + = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; + if (size_in_bytes != -1) + return has_mem_ref_been_instrumented (ref->start, size_in_bytes); + return false; } /* Set REF to the memory reference present in a gimple assignment @@ -870,7 +859,7 @@ update_mem_ref_hash_table (tree ref, HOST_WIDE_INT access_size) asan_mem_ref_init (&r, ref, access_size); asan_mem_ref **slot = ht->find_slot (&r, INSERT); - if (*slot == NULL) + if ((*slot == NULL) || ((*slot)->access_size < access_size)) *slot = asan_mem_ref_new (ref, access_size); } @@ -1784,113 +1773,21 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t, } -/* Instrument an access to a contiguous memory region that starts at - the address pointed to by BASE, over a length of LEN (expressed in - the sizeof (*BASE) bytes). ITER points to the instruction before - which the instrumentation instructions must be inserted. LOCATION - is the source location that the instrumentation instructions must - have. If IS_STORE is true, then the memory access is a store; - otherwise, it's a load. */ +/* Insert a memory reference into the hash table if access length + can be determined in compile time. */ static void -instrument_mem_region_access (tree base, tree len, - gimple_stmt_iterator *iter, - location_t location, bool is_store) +maybe_update_mem_ref_hash_table (tree base, tree len) { if (!POINTER_TYPE_P (TREE_TYPE (base)) - || !INTEGRAL_TYPE_P (TREE_TYPE (len)) - || integer_zerop (len)) + || !INTEGRAL_TYPE_P (TREE_TYPE (len))) return; - /* If the beginning of the memory region has already been - instrumented, do not instrument it. */ - bool start_instrumented = has_mem_ref_been_instrumented (base, 1); - - /* If the end of the memory region has already been instrumented, do - not instrument it. */ - tree end = asan_mem_ref_get_end (base, len); - bool end_instrumented = has_mem_ref_been_instrumented (end, 1); - HOST_WIDE_INT size_in_bytes = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; - build_check_stmt (location, base, len, size_in_bytes, iter, - /*is_non_zero_len*/size_in_bytes > 0, /*before_p*/true, - is_store, /*is_scalar_access*/false, /*align*/0, - start_instrumented, end_instrumented); - - update_mem_ref_hash_table (base, 1); - if (size_in_bytes != -1) - update_mem_ref_hash_table (end, 1); - - *iter = gsi_for_stmt (gsi_stmt (*iter)); -} - -/* Instrument the call (to the builtin strlen function) pointed to by - ITER. - - This function instruments the access to the first byte of the - argument, right before the call. After the call it instruments the - access to the last byte of the argument; it uses the result of the - call to deduce the offset of that last byte. - - Upon completion, iff the call has actually been instrumented, this - function returns TRUE and *ITER points to the statement logically - following the built-in strlen function call *ITER was initially - pointing to. Otherwise, the function returns FALSE and *ITER - remains unchanged. */ - -static bool -instrument_strlen_call (gimple_stmt_iterator *iter) -{ - gimple g; - gimple call = gsi_stmt (*iter); - gcc_assert (is_gimple_call (call)); - - tree callee = gimple_call_fndecl (call); - gcc_assert (is_builtin_fn (callee) - && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL - && DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN); - - location_t loc = gimple_location (call); - - tree len = gimple_call_lhs (call); - if (len == NULL) - /* Some passes might clear the return value of the strlen call; - bail out in that case. Return FALSE as we are not advancing - *ITER. */ - return false; - gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (len))); - - len = maybe_cast_to_ptrmode (loc, len, iter, /*before_p*/false); - - tree str_arg = gimple_call_arg (call, 0); - bool start_instrumented = has_mem_ref_been_instrumented (str_arg, 1); - - tree cptr_type = build_pointer_type (char_type_node); - g = gimple_build_assign_with_ops (NOP_EXPR, - make_ssa_name (cptr_type, NULL), - str_arg, NULL); - gimple_set_location (g, loc); - gsi_insert_before (iter, g, GSI_SAME_STMT); - str_arg = gimple_assign_lhs (g); - - build_check_stmt (loc, str_arg, NULL_TREE, 1, iter, - /*is_non_zero_len*/true, /*before_p=*/true, - /*is_store=*/false, /*is_scalar_access*/true, /*align*/0, - start_instrumented, start_instrumented); - - g = gimple_build_assign_with_ops (POINTER_PLUS_EXPR, - make_ssa_name (cptr_type, NULL), - str_arg, - len); - gimple_set_location (g, loc); - gsi_insert_after (iter, g, GSI_NEW_STMT); - - build_check_stmt (loc, gimple_assign_lhs (g), NULL_TREE, 1, iter, - /*is_non_zero_len*/true, /*before_p=*/false, - /*is_store=*/false, /*is_scalar_access*/true, /*align*/0); - - return true; + if ((size_in_bytes != -1) + && !has_mem_ref_been_instrumented (base, size_in_bytes)) + update_mem_ref_hash_table (base, size_in_bytes); } /* Instrument the call to a built-in memory access function that is @@ -1910,50 +1807,41 @@ instrument_builtin_call (gimple_stmt_iterator *iter) gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL)); - tree callee = gimple_call_fndecl (call); location_t loc = gimple_location (call); - if (DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN) - iter_advanced_p = instrument_strlen_call (iter); - else - { - asan_mem_ref src0, src1, dest; - asan_mem_ref_init (&src0, NULL, 1); - asan_mem_ref_init (&src1, NULL, 1); - asan_mem_ref_init (&dest, NULL, 1); + asan_mem_ref src0, src1, dest; + asan_mem_ref_init (&src0, NULL, 1); + asan_mem_ref_init (&src1, NULL, 1); + asan_mem_ref_init (&dest, NULL, 1); - tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; - bool src0_is_store = false, src1_is_store = false, - dest_is_store = false, dest_is_deref = false; + tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; + bool src0_is_store = false, src1_is_store = false, dest_is_store = false; + bool dest_is_deref = false; - if (get_mem_refs_of_builtin_call (call, - &src0, &src0_len, &src0_is_store, - &src1, &src1_len, &src1_is_store, - &dest, &dest_len, &dest_is_store, - &dest_is_deref)) + if (get_mem_refs_of_builtin_call (call, + &src0, &src0_len, &src0_is_store, + &src1, &src1_len, &src1_is_store, + &dest, &dest_len, &dest_is_store, + &dest_is_deref)) + { + if (dest_is_deref) { - if (dest_is_deref) - { - instrument_derefs (iter, dest.start, loc, dest_is_store); - gsi_next (iter); - iter_advanced_p = true; - } - else if (src0_len || src1_len || dest_len) - { - if (src0.start != NULL_TREE) - instrument_mem_region_access (src0.start, src0_len, - iter, loc, /*is_store=*/false); - if (src1.start != NULL_TREE) - instrument_mem_region_access (src1.start, src1_len, - iter, loc, /*is_store=*/false); - if (dest.start != NULL_TREE) - instrument_mem_region_access (dest.start, dest_len, - iter, loc, /*is_store=*/true); - *iter = gsi_for_stmt (call); - gsi_next (iter); - iter_advanced_p = true; - } + instrument_derefs (iter, dest.start, loc, dest_is_store); + gsi_next (iter); + iter_advanced_p = true; } + else + { + gsi_next (iter); + iter_advanced_p = true; + + if (src0.start != NULL_TREE) + maybe_update_mem_ref_hash_table (src0.start, src0_len); + if (src1.start != NULL_TREE) + maybe_update_mem_ref_hash_table (src1.start, src1_len); + if (dest.start != NULL_TREE) + maybe_update_mem_ref_hash_table (dest.start, dest_len); + } } return iter_advanced_p; } diff --git a/gcc/builtins.c b/gcc/builtins.c index 975f696..f32057b3 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -5747,6 +5747,15 @@ expand_stack_save (void) return ret; } +/* Returns TRUE if given FCODE corresponds to string or memory builtin function. + */ + +static inline bool +is_memory_builtin (enum built_in_function fcode) +{ + return fcode <= BUILT_IN_STRSTR && fcode >= BUILT_IN_BCMP; +} + /* Expand an expression EXP that calls a built-in function, with result going to TARGET if that's convenient (and in mode MODE if that's convenient). @@ -5762,6 +5771,10 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode, enum machine_mode target_mode = TYPE_MODE (TREE_TYPE (exp)); int flags; + if ((flag_sanitize & SANITIZE_ADDRESS) + && is_memory_builtin (fcode)) + return expand_call (exp, target, ignore); + if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD) return targetm.expand_builtin (exp, target, subtarget, mode, ignore); diff --git a/gcc/builtins.def b/gcc/builtins.def index cd823a3..a76c937 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -569,7 +569,11 @@ DEF_C99_COMPL_BUILTIN (BUILT_IN_CTANL, "ctanl", BT_FN_COMPLEX_LONGDOUBLE_ /* Category: string/memory builtins. */ /* bcmp, bcopy and bzero have traditionally accepted NULL pointers - when the length parameter is zero, so don't apply attribute "nonnull". */ + when the length parameter is zero, so don't apply attribute "nonnull". + Do not reorder the BUILT_IN_* builtins, e.g. builtins.c relies on this + order. If you want to add a new string/memory builtin, please insert + it after BUILT_IN_BCMP and before BUILT_IN_STRSTR. */ + DEF_EXT_LIB_BUILTIN (BUILT_IN_BCMP, "bcmp", BT_FN_INT_CONST_PTR_CONST_PTR_SIZE, ATTR_PURE_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN_BCOPY, "bcopy", BT_FN_VOID_CONST_PTR_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN_BZERO, "bzero", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST) diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c index 028f8d7..baacb1e 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c @@ -6,7 +6,7 @@ /* { dg-do compile } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ -extern char tab[4]; +extern char tab[6]; static int test0 () @@ -35,17 +35,10 @@ test1 (int i) the initialization. */ foo[i] = 1; - /*__builtin___asan_report_store_n called once here to instrument - the store to the memory region of tab. */ + /* Instrument tab memory region. */ __builtin_memset (tab, 3, sizeof (tab)); - /* There is no instrumentation for the two memset calls below. */ - __builtin_memset (tab, 4, sizeof (tab)); - __builtin_memset (tab, 5, sizeof (tab)); - - /* There is a call to __builtin___asan_report_store_n and a call - to __builtin___asan_report_load_n to instrument the store to - (subset of) the memory region of tab. */ + /* Instrument tab[1] with access size 3. */ __builtin_memcpy (&tab[1], foo + i, 3); /* This should not generate a __builtin___asan_report_load1 because @@ -63,6 +56,5 @@ main () } /* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 3 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_load1" "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c new file mode 100644 index 0000000..24dfcfe --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c @@ -0,0 +1,18 @@ +/* { dg-options "-fdump-tree-sanopt" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +extern __UINT32_TYPE__ a; + +void +foo () +{ + /* Instrument a with access size 3. */ + int d = __builtin_memcmp (&a, "123", 3); + /* This should generate a __builtin___asan_report_store4, because + the reference to a has been instrumented above with access size 3. */ + a = 1; +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store4" 1 "sanopt" } } */ +/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c new file mode 100644 index 0000000..4082f32 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c @@ -0,0 +1,20 @@ +/* { dg-options "-fdump-tree-sanopt" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +extern __UINT32_TYPE__ a; + +void +foo () +{ + /* Instrument a with access size 5. */ + int d = __builtin_memcmp (&a, "12345", 4); + /* This should not generate a __builtin___asan_report_store4 because + the reference to a has been already instrumented above with access + size 5. */ + a = 1; +} + +/* { dg-final { scan-tree-dump-not "& 7" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "sanopt" } } */ +/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c deleted file mode 100644 index a58411c..0000000 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c +++ /dev/null @@ -1,26 +0,0 @@ -/* This tests that when faced with two references to the same memory - location in the same basic block, the second reference should not - be instrumented by the Address Sanitizer. But in case of access to - overlapping regions we must be precise. */ - -/* { dg-options "-fdump-tree-sanopt" } */ -/* { dg-do compile } */ -/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ - -int -main () -{ - char tab[5]; - - /* Here, we instrument the access at offset 0 and access at offset - 4. */ - __builtin_memset (tab, 1, sizeof (tab)); - /* We instrumented access at offset 0 above already, so only access - at offset 3 is instrumented. */ - __builtin_memset (tab, 1, 3); -} - -/* { dg-final { scan-tree-dump-times "& 7" 3 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report" 2 "sanopt" } } */ -/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c index c3632aa..a613b92 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c @@ -5,13 +5,13 @@ void foo (int *a, char *b, char *c) { - /* One check for c[0], one check for a[], one check for c, two checks for b. */ + /* One check for c[0], one check for a[]. */ __builtin_memmove (c, b, a[c[0]]); - /* For a total of 5 checks. */ + /* For a total of 2 checks. */ + int d = c[0] == 1; } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 2 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c index 077ea34..f4ca603 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c @@ -5,14 +5,12 @@ void foo (int *a, char *b, char *c) { - /* One check for b[0], one check for a[], 2 checks for c and one checks for b. */ - __builtin_memmove (c, b, a[b[0]]); - /* For a total of 5 checks. */ + /* One check for a[]. */ + __builtin_memmove (c, b, a[0]); + /* For a total of 1 checks. */ + int d = a[0] == 0; } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 1 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c index 6d87104..757f0ee 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c @@ -5,16 +5,15 @@ void foo (int *a, char *b, char *c) { - /* One check for c[0], one check for a[], one check for c and 2 checks for b. */ + /* One check for c[0], one check for a[]. */ __builtin_memmove (c, b, a[c[0]]); - /* One check for a[], one check for c and one check for b. */ + /* One check for b[0], one check for a[]. */ __builtin_memmove (c, b, a[b[0]]); - /* For a total of 8 checks. */ + /* For a total of 4 checks. */ + int d = c[0] == b[0]; } -/* { dg-final { scan-tree-dump-times "& 7" 8 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 4 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c index 5baa10d..0c50145 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c @@ -2,26 +2,22 @@ /* { dg-do compile } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ -char e[200]; +char e[5]; -struct S +extern struct S { - char a[100]; - char b[100]; + int a; + char b; } s; int foo (int *a, char *b, char *c) { - /* 2 checks for s.a, 2 checks for e. */ - int d = __builtin_memcmp (s.a, e, 100); - /* One check for s.a and one check for e. */ - d += __builtin_memcmp (s.a, e, 200); - /* For a total of 6 checks. */ - return d; + int d = __builtin_memcmp (&s.a, e, 4); + /* No check because s.a was instrumented above with access size 4. */ + return s.a; } -/* { dg-final { scan-tree-dump-times "& 7" 6 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 4 "sanopt" } } */ -/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "& 7" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_load4" "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c index 2a4c081..4eeedce 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c @@ -5,16 +5,16 @@ char foo (int *a, char *b, char *c) { - /* One check for b[0], one check for a[], two checks for c and one check for b. */ + /* One check for b[0], one check for a[]. */ __builtin_memmove (c, b, a[b[0]]); + /* One check for c[0], one check for a[]. */ + __builtin_memmove (b, c, a[c[0]]); /* No checks here. */ return c[0] + b[0]; - /* For a total of 5 checks. */ + /* For a total of 4 checks. */ } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 4 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 2 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c deleted file mode 100644 index 9449de5..0000000 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c +++ /dev/null @@ -1,13 +0,0 @@ -/* { dg-options "-fdump-tree-sanopt" } */ -/* { dg-do compile } */ -/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ - -__SIZE_TYPE__ -f (char *a) -{ - a[0] = '1'; - return __builtin_strlen (a); -} - -/* { dg-final { scan-tree-dump-times "__asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c index f58f554..0f49286 100644 --- a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c +++ b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c @@ -9,14 +9,8 @@ char a[2] = "0"; #ifdef __cplusplus extern "C" #endif - -__attribute__((no_sanitize_address, noinline)) __SIZE_TYPE__ -strlen (const char *p) { - - __SIZE_TYPE__ n = 0; - for (; *p; ++n, ++p); - return n; -} +__SIZE_TYPE__ +strlen (const char *p); int main () { char *p = &a[0]; @@ -25,6 +19,6 @@ int main () { return __builtin_strlen (a); } -/* { dg-output "READ of size 1 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */ -/* { dg-output " #0 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:25|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 1 bytes inside of global variable" } */ +/* { dg-output "READ of size 2 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */ +/* { dg-output " #1 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:19|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 0 bytes to the right of global variable" } */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Don't expand string/memory builtins if ASan is enabled. 2014-10-17 11:47 ` [PATCH] Don't expand string/memory builtins if ASan is enabled Maxim Ostapenko @ 2014-10-17 12:29 ` Jakub Jelinek 2014-10-17 13:03 ` Yury Gribov 0 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2014-10-17 12:29 UTC (permalink / raw) To: Maxim Ostapenko; +Cc: GCC Patches, Yury Gribov, Slava Garbuzov, Maxim Ostapenko On Fri, Oct 17, 2014 at 03:45:52PM +0400, Maxim Ostapenko wrote: > Patch also changes logic in asan_mem_ref_hash updating. I eliminated memory > ref access size from hash computing, so all accesses for same memory > reference have the same hash. Updating of asan_mem_ref_hash occurs only if > new access size is greater then saved one. I guess that is reasonable. > -/* Instrument an access to a contiguous memory region that starts at > - the address pointed to by BASE, over a length of LEN (expressed in > - the sizeof (*BASE) bytes). ITER points to the instruction before > - which the instrumentation instructions must be inserted. LOCATION > - is the source location that the instrumentation instructions must > - have. If IS_STORE is true, then the memory access is a store; > - otherwise, it's a load. */ > +/* Insert a memory reference into the hash table if access length > + can be determined in compile time. */ ... If you don't expand the memops builtins inline, I'd expect you start with get_mem_refs_of_builtin_call and remove all the builtins you stop expanding specially (i.e. emit a libcall instead unconditionally) that are handled by libsanitizer (only a subset of them are apparently, perhaps something to fix) from there. There are builtins that must be kept instrumented (e.g. all the sync/atomic builtins). There are builtins which might need first additions to libsanitizer (e.g. I see no __*_chk functions in libsanitizer). > +/* Returns TRUE if given FCODE corresponds to string or memory builtin function. > + */ > + > +static inline bool > +is_memory_builtin (enum built_in_function fcode) > +{ > + return fcode <= BUILT_IN_STRSTR && fcode >= BUILT_IN_BCMP; This is too fragile and ugly. IMHO you should list (supposedly not in a special inline, but directly where you use it) in a switch all the builtins you don't want to expand. Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Don't expand string/memory builtins if ASan is enabled. 2014-10-17 12:29 ` Jakub Jelinek @ 2014-10-17 13:03 ` Yury Gribov 2014-10-17 13:08 ` Jakub Jelinek 0 siblings, 1 reply; 10+ messages in thread From: Yury Gribov @ 2014-10-17 13:03 UTC (permalink / raw) To: Jakub Jelinek, Maxim Ostapenko Cc: GCC Patches, Slava Garbuzov, Maxim Ostapenko On 10/17/2014 04:24 PM, Jakub Jelinek wrote: >> +/* Returns TRUE if given FCODE corresponds to string or memory builtin function. >> + */ >> + >> +static inline bool >> +is_memory_builtin (enum built_in_function fcode) >> +{ >> + return fcode <= BUILT_IN_STRSTR && fcode >= BUILT_IN_BCMP; > > This is too fragile and ugly. > IMHO you should list (supposedly not in a special inline, but directly > where you use it) in a switch all the builtins you don't want to expand. We already do this for BUILT_IN_ASAN_REPORT_LOAD1 ... BUILT_IN_ASAN_STOREN but I agree that this one is more ugly. -Y ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Don't expand string/memory builtins if ASan is enabled. 2014-10-17 13:03 ` Yury Gribov @ 2014-10-17 13:08 ` Jakub Jelinek 2014-10-21 13:38 ` [PATCHv2] " Maxim Ostapenko 0 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2014-10-17 13:08 UTC (permalink / raw) To: Yury Gribov; +Cc: Maxim Ostapenko, GCC Patches, Slava Garbuzov, Maxim Ostapenko On Fri, Oct 17, 2014 at 05:01:33PM +0400, Yury Gribov wrote: > On 10/17/2014 04:24 PM, Jakub Jelinek wrote: > >>+/* Returns TRUE if given FCODE corresponds to string or memory builtin function. > >>+ */ > >>+ > >>+static inline bool > >>+is_memory_builtin (enum built_in_function fcode) > >>+{ > >>+ return fcode <= BUILT_IN_STRSTR && fcode >= BUILT_IN_BCMP; > > > >This is too fragile and ugly. > >IMHO you should list (supposedly not in a special inline, but directly > >where you use it) in a switch all the builtins you don't want to expand. > > We already do this for BUILT_IN_ASAN_REPORT_LOAD1 ... BUILT_IN_ASAN_STOREN I know, but it is still a coherent sent of builtins for very similar purposes, many of them sorted by increasing size number. > but I agree that this one is more ugly. The memops builtins are just random bag of them, it is expected many people will add builtins into that range and outside of that range. Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] Don't expand string/memory builtins if ASan is enabled. 2014-10-17 13:08 ` Jakub Jelinek @ 2014-10-21 13:38 ` Maxim Ostapenko 2014-10-23 8:51 ` Yury Gribov 0 siblings, 1 reply; 10+ messages in thread From: Maxim Ostapenko @ 2014-10-21 13:38 UTC (permalink / raw) To: Jakub Jelinek, Yury Gribov; +Cc: GCC Patches, Slava Garbuzov, Maxim Ostapenko [-- Attachment #1: Type: text/plain, Size: 1732 bytes --] Hi, this is the second version of the patch. Here the major changes from the previous one: 1) Added a new intercepted_p parameter in get_mem_refs_of_builtin_call to decide whether builtin function should/shouldn't be instrumented. 2) Changed instrument_mem_region_access function. Now, we update asan_mem_ref_ht with (base, size_in_bytes), if we can determine access size during compile time. 3) Removed ASAN_CHECK_START_INSTRUMENTED and ASAN_CHECK_END_INSTRUMENTED from asan_check_flags since we don't instrument base and end of memory region with access size 1 anymore. 4) Specified builtins that shouldn't be expanded explicitly in gcc/builtins.c. Regtested / bootrapped on x86_64-unknown-linux-gnu. -Maxim On 10/17/2014 05:03 PM, Jakub Jelinek wrote: > On Fri, Oct 17, 2014 at 05:01:33PM +0400, Yury Gribov wrote: >> On 10/17/2014 04:24 PM, Jakub Jelinek wrote: >>>> +/* Returns TRUE if given FCODE corresponds to string or memory builtin function. >>>> + */ >>>> + >>>> +static inline bool >>>> +is_memory_builtin (enum built_in_function fcode) >>>> +{ >>>> + return fcode <= BUILT_IN_STRSTR && fcode >= BUILT_IN_BCMP; >>> This is too fragile and ugly. >>> IMHO you should list (supposedly not in a special inline, but directly >>> where you use it) in a switch all the builtins you don't want to expand. >> We already do this for BUILT_IN_ASAN_REPORT_LOAD1 ... BUILT_IN_ASAN_STOREN > I know, but it is still a coherent sent of builtins for very similar > purposes, many of them sorted by increasing size number. > >> but I agree that this one is more ugly. > The memops builtins are just random bag of them, it is expected many people > will add builtins into that range and outside of that range. > > Jakub > [-- Attachment #2: builtins-17.diff --] [-- Type: text/x-patch, Size: 35202 bytes --] gcc/ChangeLog: 2014-10-21 Max Ostapenko <m.ostapenko@partner.samsung.com> * asan.c (asan_mem_ref_hasher::hash): Remove MEM_REF access size from hash value construction. Call iterative_hash_expr instead of explicit hash building. (asan_mem_ref_hasher::equal): Change condition. (has_mem_ref_been_instrumented): Likewise. (update_mem_ref_hash_table): Likewise. (maybe_update_mem_ref_hash_table): New function. (instrument_strlen_call): Removed. (get_mem_refs_of_builtin_call): Handle new parameter. (instrument_builtin_call): Call maybe_update_mem_ref_hash_table instead of instrument_mem_region_access if intercepted_p is true. (instrument_mem_region_access): Instrument only base with len instead of base and end with 1. (build_check_stmt): Remove start_instrumented and end_instrumented parameters. (enum asan_check_flags): Remove ASAN_CHECK_START_INSTRUMENTED and ASAN_CHECK_END_INSTRUMENTED. Change ASAN_CHECK_LAST. (asan_expand_check_ifn): Remove start_instrumented and end_instrumented. * builtins.c (expand_builtin): Don't expand string/memory builtin functions that have interceptors in libsanitizer if ASan is enabled. gcc/testsuite/ChangeLog: 2014-10-21 Max Ostapenko <m.ostapenko@partner.samsung.com> * c-c++-common/asan/no-redundant-instrumentation-1.c: Updated test. * c-c++-common/asan/no-redundant-instrumentation-4.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-2.c: Removed. * c-c++-common/asan/no-redundant-instrumentation-9.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-10.c: New test. * c-c++-common/asan/no-redundant-instrumentation-11.c: Likewise. diff --git a/gcc/asan.c b/gcc/asan.c index 2a61a82..a9eb9aa 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -253,9 +253,7 @@ enum asan_check_flags ASAN_CHECK_STORE = 1 << 0, ASAN_CHECK_SCALAR_ACCESS = 1 << 1, ASAN_CHECK_NON_ZERO_LEN = 1 << 2, - ASAN_CHECK_START_INSTRUMENTED = 1 << 3, - ASAN_CHECK_END_INSTRUMENTED = 1 << 4, - ASAN_CHECK_LAST + ASAN_CHECK_LAST = 1 << 3 }; /* Hashtable support for memory references used by gimple @@ -352,10 +350,7 @@ struct asan_mem_ref_hasher inline hashval_t asan_mem_ref_hasher::hash (const asan_mem_ref *mem_ref) { - inchash::hash hstate; - inchash::add_expr (mem_ref->start, hstate); - hstate.add_wide_int (mem_ref->access_size); - return hstate.end (); + return iterative_hash_expr (mem_ref->start, 0); } /* Compare two memory references. We accept the length of either @@ -365,8 +360,7 @@ inline bool asan_mem_ref_hasher::equal (const asan_mem_ref *m1, const asan_mem_ref *m2) { - return (m1->access_size == m2->access_size - && operand_equal_p (m1->start, m2->start, 0)); + return operand_equal_p (m1->start, m2->start, 0); } static hash_table<asan_mem_ref_hasher> *asan_mem_ref_ht; @@ -417,7 +411,8 @@ has_mem_ref_been_instrumented (tree ref, HOST_WIDE_INT access_size) asan_mem_ref r; asan_mem_ref_init (&r, ref, access_size); - return (get_mem_ref_hash_table ()->find (&r) != NULL); + asan_mem_ref *saved_ref = get_mem_ref_hash_table ()->find (&r); + return saved_ref && saved_ref->access_size >= access_size; } /* Return true iff the memory reference REF has been instrumented. */ @@ -434,19 +429,11 @@ has_mem_ref_been_instrumented (const asan_mem_ref *ref) static bool has_mem_ref_been_instrumented (const asan_mem_ref *ref, tree len) { - /* First let's see if the address of the beginning of REF has been - instrumented. */ - if (!has_mem_ref_been_instrumented (ref)) - return false; + HOST_WIDE_INT size_in_bytes + = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; - if (len != 0) - { - /* Let's see if the end of the region has been instrumented. */ - if (!has_mem_ref_been_instrumented (asan_mem_ref_get_end (ref, len), - ref->access_size)) - return false; - } - return true; + return size_in_bytes != -1 + && has_mem_ref_been_instrumented (ref->start, size_in_bytes); } /* Set REF to the memory reference present in a gimple assignment @@ -492,7 +479,8 @@ get_mem_refs_of_builtin_call (const gimple call, asan_mem_ref *dst, tree *dst_len, bool *dst_is_store, - bool *dest_is_deref) + bool *dest_is_deref, + bool *intercepted_p) { gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL)); @@ -506,6 +494,7 @@ get_mem_refs_of_builtin_call (const gimple call, { /* (s, s, n) style memops. */ case BUILT_IN_BCMP: + *intercepted_p = false; case BUILT_IN_MEMCMP: source0 = gimple_call_arg (call, 0); source1 = gimple_call_arg (call, 1); @@ -514,18 +503,20 @@ get_mem_refs_of_builtin_call (const gimple call, /* (src, dest, n) style memops. */ case BUILT_IN_BCOPY: + *intercepted_p = false; source0 = gimple_call_arg (call, 0); dest = gimple_call_arg (call, 1); len = gimple_call_arg (call, 2); break; /* (dest, src, n) style memops. */ - case BUILT_IN_MEMCPY: case BUILT_IN_MEMCPY_CHK: - case BUILT_IN_MEMMOVE: + case BUILT_IN_MEMPCPY_CHK: case BUILT_IN_MEMMOVE_CHK: case BUILT_IN_MEMPCPY: - case BUILT_IN_MEMPCPY_CHK: + *intercepted_p = false; + case BUILT_IN_MEMCPY: + case BUILT_IN_MEMMOVE: dest = gimple_call_arg (call, 0); source0 = gimple_call_arg (call, 1); len = gimple_call_arg (call, 2); @@ -533,13 +524,15 @@ get_mem_refs_of_builtin_call (const gimple call, /* (dest, n) style memops. */ case BUILT_IN_BZERO: + *intercepted_p = false; dest = gimple_call_arg (call, 0); len = gimple_call_arg (call, 1); break; /* (dest, x, n) style memops*/ - case BUILT_IN_MEMSET: case BUILT_IN_MEMSET_CHK: + *intercepted_p = false; + case BUILT_IN_MEMSET: dest = gimple_call_arg (call, 0); len = gimple_call_arg (call, 2); break; @@ -759,6 +752,7 @@ get_mem_refs_of_builtin_call (const gimple call, gcc_unreachable (); access_size = int_size_in_bytes (TREE_TYPE (dest)); + *intercepted_p = false; } default: @@ -834,12 +828,12 @@ has_stmt_been_instrumented_p (gimple stmt) tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; bool src0_is_store = false, src1_is_store = false, - dest_is_store = false, dest_is_deref = false; + dest_is_store = false, dest_is_deref = false, intercepted_p = true; if (get_mem_refs_of_builtin_call (stmt, &src0, &src0_len, &src0_is_store, &src1, &src1_len, &src1_is_store, &dest, &dest_len, &dest_is_store, - &dest_is_deref)) + &dest_is_deref, &intercepted_p)) { if (src0.start != NULL_TREE && !has_mem_ref_been_instrumented (&src0, src0_len)) @@ -870,7 +864,7 @@ update_mem_ref_hash_table (tree ref, HOST_WIDE_INT access_size) asan_mem_ref_init (&r, ref, access_size); asan_mem_ref **slot = ht->find_slot (&r, INSERT); - if (*slot == NULL) + if (*slot == NULL || (*slot)->access_size < access_size) *slot = asan_mem_ref_new (ref, access_size); } @@ -1608,22 +1602,13 @@ static void build_check_stmt (location_t loc, tree base, tree len, HOST_WIDE_INT size_in_bytes, gimple_stmt_iterator *iter, bool is_non_zero_len, bool before_p, bool is_store, - bool is_scalar_access, unsigned int align = 0, - bool start_instrumented = false, - bool end_instrumented = false) + bool is_scalar_access, unsigned int align = 0) { gimple_stmt_iterator gsi = *iter; gimple g; gcc_assert (!(size_in_bytes > 0 && !is_non_zero_len)); - if (start_instrumented && end_instrumented) - { - if (!before_p) - gsi_next (iter); - return; - } - gsi = *iter; base = unshare_expr (base); @@ -1666,10 +1651,6 @@ build_check_stmt (location_t loc, tree base, tree len, flags |= ASAN_CHECK_NON_ZERO_LEN; if (is_scalar_access) flags |= ASAN_CHECK_SCALAR_ACCESS; - if (start_instrumented) - flags |= ASAN_CHECK_START_INSTRUMENTED; - if (end_instrumented) - flags |= ASAN_CHECK_END_INSTRUMENTED; g = gimple_build_call_internal (IFN_ASAN_CHECK, 4, build_int_cst (integer_type_node, flags), @@ -1784,6 +1765,22 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t, } +/* Insert a memory reference into the hash table if access length + can be determined in compile time. */ + +static void +maybe_update_mem_ref_hash_table (tree base, tree len) +{ + if (!POINTER_TYPE_P (TREE_TYPE (base)) + || !INTEGRAL_TYPE_P (TREE_TYPE (len))) + return; + + HOST_WIDE_INT size_in_bytes = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; + + if (size_in_bytes != -1) + update_mem_ref_hash_table (base, size_in_bytes); +} + /* Instrument an access to a contiguous memory region that starts at the address pointed to by BASE, over a length of LEN (expressed in the sizeof (*BASE) bytes). ITER points to the instruction before @@ -1802,97 +1799,20 @@ instrument_mem_region_access (tree base, tree len, || integer_zerop (len)) return; - /* If the beginning of the memory region has already been - instrumented, do not instrument it. */ - bool start_instrumented = has_mem_ref_been_instrumented (base, 1); - - /* If the end of the memory region has already been instrumented, do - not instrument it. */ - tree end = asan_mem_ref_get_end (base, len); - bool end_instrumented = has_mem_ref_been_instrumented (end, 1); - HOST_WIDE_INT size_in_bytes = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; - build_check_stmt (location, base, len, size_in_bytes, iter, - /*is_non_zero_len*/size_in_bytes > 0, /*before_p*/true, - is_store, /*is_scalar_access*/false, /*align*/0, - start_instrumented, end_instrumented); - - update_mem_ref_hash_table (base, 1); - if (size_in_bytes != -1) - update_mem_ref_hash_table (end, 1); + if ((size_in_bytes == -1) + || !has_mem_ref_been_instrumented (base, size_in_bytes)) + { + build_check_stmt (location, base, len, size_in_bytes, iter, + /*is_non_zero_len*/size_in_bytes > 0, /*before_p*/true, + is_store, /*is_scalar_access*/false, /*align*/0); + } + maybe_update_mem_ref_hash_table (base, len); *iter = gsi_for_stmt (gsi_stmt (*iter)); } -/* Instrument the call (to the builtin strlen function) pointed to by - ITER. - - This function instruments the access to the first byte of the - argument, right before the call. After the call it instruments the - access to the last byte of the argument; it uses the result of the - call to deduce the offset of that last byte. - - Upon completion, iff the call has actually been instrumented, this - function returns TRUE and *ITER points to the statement logically - following the built-in strlen function call *ITER was initially - pointing to. Otherwise, the function returns FALSE and *ITER - remains unchanged. */ - -static bool -instrument_strlen_call (gimple_stmt_iterator *iter) -{ - gimple g; - gimple call = gsi_stmt (*iter); - gcc_assert (is_gimple_call (call)); - - tree callee = gimple_call_fndecl (call); - gcc_assert (is_builtin_fn (callee) - && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL - && DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN); - - location_t loc = gimple_location (call); - - tree len = gimple_call_lhs (call); - if (len == NULL) - /* Some passes might clear the return value of the strlen call; - bail out in that case. Return FALSE as we are not advancing - *ITER. */ - return false; - gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (len))); - - len = maybe_cast_to_ptrmode (loc, len, iter, /*before_p*/false); - - tree str_arg = gimple_call_arg (call, 0); - bool start_instrumented = has_mem_ref_been_instrumented (str_arg, 1); - - tree cptr_type = build_pointer_type (char_type_node); - g = gimple_build_assign_with_ops (NOP_EXPR, - make_ssa_name (cptr_type, NULL), - str_arg, NULL); - gimple_set_location (g, loc); - gsi_insert_before (iter, g, GSI_SAME_STMT); - str_arg = gimple_assign_lhs (g); - - build_check_stmt (loc, str_arg, NULL_TREE, 1, iter, - /*is_non_zero_len*/true, /*before_p=*/true, - /*is_store=*/false, /*is_scalar_access*/true, /*align*/0, - start_instrumented, start_instrumented); - - g = gimple_build_assign_with_ops (POINTER_PLUS_EXPR, - make_ssa_name (cptr_type, NULL), - str_arg, - len); - gimple_set_location (g, loc); - gsi_insert_after (iter, g, GSI_NEW_STMT); - - build_check_stmt (loc, gimple_assign_lhs (g), NULL_TREE, 1, iter, - /*is_non_zero_len*/true, /*before_p=*/false, - /*is_store=*/false, /*is_scalar_access*/true, /*align*/0); - - return true; -} - /* Instrument the call to a built-in memory access function that is pointed to by the iterator ITER. @@ -1910,49 +1830,54 @@ instrument_builtin_call (gimple_stmt_iterator *iter) gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL)); - tree callee = gimple_call_fndecl (call); location_t loc = gimple_location (call); - if (DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN) - iter_advanced_p = instrument_strlen_call (iter); - else - { - asan_mem_ref src0, src1, dest; - asan_mem_ref_init (&src0, NULL, 1); - asan_mem_ref_init (&src1, NULL, 1); - asan_mem_ref_init (&dest, NULL, 1); + asan_mem_ref src0, src1, dest; + asan_mem_ref_init (&src0, NULL, 1); + asan_mem_ref_init (&src1, NULL, 1); + asan_mem_ref_init (&dest, NULL, 1); - tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; - bool src0_is_store = false, src1_is_store = false, - dest_is_store = false, dest_is_deref = false; + tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; + bool src0_is_store = false, src1_is_store = false, dest_is_store = false, + dest_is_deref = false, intercepted_p = true; - if (get_mem_refs_of_builtin_call (call, - &src0, &src0_len, &src0_is_store, - &src1, &src1_len, &src1_is_store, - &dest, &dest_len, &dest_is_store, - &dest_is_deref)) + if (get_mem_refs_of_builtin_call (call, + &src0, &src0_len, &src0_is_store, + &src1, &src1_len, &src1_is_store, + &dest, &dest_len, &dest_is_store, + &dest_is_deref, &intercepted_p)) + { + if (dest_is_deref) { - if (dest_is_deref) - { - instrument_derefs (iter, dest.start, loc, dest_is_store); - gsi_next (iter); - iter_advanced_p = true; - } - else if (src0_len || src1_len || dest_len) - { - if (src0.start != NULL_TREE) - instrument_mem_region_access (src0.start, src0_len, - iter, loc, /*is_store=*/false); - if (src1.start != NULL_TREE) - instrument_mem_region_access (src1.start, src1_len, - iter, loc, /*is_store=*/false); - if (dest.start != NULL_TREE) - instrument_mem_region_access (dest.start, dest_len, - iter, loc, /*is_store=*/true); - *iter = gsi_for_stmt (call); - gsi_next (iter); - iter_advanced_p = true; - } + instrument_derefs (iter, dest.start, loc, dest_is_store); + gsi_next (iter); + iter_advanced_p = true; + } + else if (!intercepted_p + && (src0_len || src1_len || dest_len)) + { + if (src0.start != NULL_TREE) + instrument_mem_region_access (src0.start, src0_len, + iter, loc, /*is_store=*/false); + if (src1.start != NULL_TREE) + instrument_mem_region_access (src1.start, src1_len, + iter, loc, /*is_store=*/false); + if (dest.start != NULL_TREE) + instrument_mem_region_access (dest.start, dest_len, + iter, loc, /*is_store=*/true); + + *iter = gsi_for_stmt (call); + gsi_next (iter); + iter_advanced_p = true; + } + else + { + if (src0.start != NULL_TREE) + maybe_update_mem_ref_hash_table (src0.start, src0_len); + if (src1.start != NULL_TREE) + maybe_update_mem_ref_hash_table (src1.start, src1_len); + if (dest.start != NULL_TREE) + maybe_update_mem_ref_hash_table (dest.start, dest_len); } } return iter_advanced_p; @@ -2507,8 +2432,6 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) bool is_scalar_access = (flags & ASAN_CHECK_SCALAR_ACCESS) != 0; bool is_store = (flags & ASAN_CHECK_STORE) != 0; bool is_non_zero_len = (flags & ASAN_CHECK_NON_ZERO_LEN) != 0; - bool start_instrumented = (flags & ASAN_CHECK_START_INSTRUMENTED) != 0; - bool end_instrumented = (flags & ASAN_CHECK_END_INSTRUMENTED) != 0; tree base = gimple_call_arg (g, 1); tree len = gimple_call_arg (g, 2); @@ -2613,46 +2536,42 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) else { /* Slow path for 1, 2 and 4 byte accesses. */ - - if (!start_instrumented) + /* Test (shadow != 0) + & ((base_addr & 7) + (real_size_in_bytes - 1)) >= shadow). */ + tree shadow = build_shadow_mem_access (&gsi, loc, base_addr, + shadow_ptr_type); + gimple shadow_test = build_assign (NE_EXPR, shadow, 0); + gimple_seq seq = NULL; + gimple_seq_add_stmt (&seq, shadow_test); + /* Aligned (>= 8 bytes) can test just + (real_size_in_bytes - 1 >= shadow), as base_addr & 7 is known + to be 0. */ + if (align < 8) { - /* Test (shadow != 0) - & ((base_addr & 7) + (real_size_in_bytes - 1)) >= shadow). */ - tree shadow = build_shadow_mem_access (&gsi, loc, base_addr, - shadow_ptr_type); - gimple shadow_test = build_assign (NE_EXPR, shadow, 0); - gimple_seq seq = NULL; - gimple_seq_add_stmt (&seq, shadow_test); - /* Aligned (>= 8 bytes) can test just - (real_size_in_bytes - 1 >= shadow), as base_addr & 7 is known - to be 0. */ - if (align < 8) - { - gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, - base_addr, 7)); - gimple_seq_add_stmt (&seq, - build_type_cast (shadow_type, - gimple_seq_last (seq))); - if (real_size_in_bytes > 1) - gimple_seq_add_stmt (&seq, - build_assign (PLUS_EXPR, - gimple_seq_last (seq), - real_size_in_bytes - 1)); - t = gimple_assign_lhs (gimple_seq_last_stmt (seq)); - } - else - t = build_int_cst (shadow_type, real_size_in_bytes - 1); - gimple_seq_add_stmt (&seq, build_assign (GE_EXPR, t, shadow)); - gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test, - gimple_seq_last (seq))); - t = gimple_assign_lhs (gimple_seq_last (seq)); - gimple_seq_set_location (seq, loc); - gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); + gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, + base_addr, 7)); + gimple_seq_add_stmt (&seq, + build_type_cast (shadow_type, + gimple_seq_last (seq))); + if (real_size_in_bytes > 1) + gimple_seq_add_stmt (&seq, + build_assign (PLUS_EXPR, + gimple_seq_last (seq), + real_size_in_bytes - 1)); + t = gimple_assign_lhs (gimple_seq_last_stmt (seq)); } + else + t = build_int_cst (shadow_type, real_size_in_bytes - 1); + gimple_seq_add_stmt (&seq, build_assign (GE_EXPR, t, shadow)); + gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test, + gimple_seq_last (seq))); + t = gimple_assign_lhs (gimple_seq_last (seq)); + gimple_seq_set_location (seq, loc); + gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); /* For non-constant, misaligned or otherwise weird access sizes, - check first and last byte. */ - if (size_in_bytes == -1 && !end_instrumented) + check first and last byte. */ + if (size_in_bytes == -1) { g = gimple_build_assign_with_ops (MINUS_EXPR, make_ssa_name (pointer_sized_int_node, NULL), @@ -2683,9 +2602,8 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) shadow)); gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test, gimple_seq_last (seq))); - if (!start_instrumented) - gimple_seq_add_stmt (&seq, build_assign (BIT_IOR_EXPR, t, - gimple_seq_last (seq))); + gimple_seq_add_stmt (&seq, build_assign (BIT_IOR_EXPR, t, + gimple_seq_last (seq))); t = gimple_assign_lhs (gimple_seq_last (seq)); gimple_seq_set_location (seq, loc); gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); diff --git a/gcc/builtins.c b/gcc/builtins.c index 975f696..530b5a4 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -5762,6 +5762,37 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode, enum machine_mode target_mode = TYPE_MODE (TREE_TYPE (exp)); int flags; + /* When ASan is enabled, we don't want to expand some memory/string + builtins and rely on libsanitizer's hooks. This allows us to avoid + redundant checks and be sure, that possible overflow will be detected + by ASan. */ + + if (flag_sanitize & SANITIZE_ADDRESS) + switch (fcode) + { + case BUILT_IN_INDEX: + case BUILT_IN_MEMCHR: + case BUILT_IN_MEMCMP: + case BUILT_IN_MEMCPY: + case BUILT_IN_MEMMOVE: + case BUILT_IN_MEMSET: + case BUILT_IN_STRCASECMP: + case BUILT_IN_STRCAT: + case BUILT_IN_STRCHR: + case BUILT_IN_STRCMP: + case BUILT_IN_STRCPY: + case BUILT_IN_STRDUP: + case BUILT_IN_STRLEN: + case BUILT_IN_STRNCASECMP: + case BUILT_IN_STRNCAT: + case BUILT_IN_STRNCMP: + case BUILT_IN_STRNCPY: + return expand_call (exp, target, ignore); + + default: + break; + } + if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD) return targetm.expand_builtin (exp, target, subtarget, mode, ignore); diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c index 028f8d7..baacb1e 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c @@ -6,7 +6,7 @@ /* { dg-do compile } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ -extern char tab[4]; +extern char tab[6]; static int test0 () @@ -35,17 +35,10 @@ test1 (int i) the initialization. */ foo[i] = 1; - /*__builtin___asan_report_store_n called once here to instrument - the store to the memory region of tab. */ + /* Instrument tab memory region. */ __builtin_memset (tab, 3, sizeof (tab)); - /* There is no instrumentation for the two memset calls below. */ - __builtin_memset (tab, 4, sizeof (tab)); - __builtin_memset (tab, 5, sizeof (tab)); - - /* There is a call to __builtin___asan_report_store_n and a call - to __builtin___asan_report_load_n to instrument the store to - (subset of) the memory region of tab. */ + /* Instrument tab[1] with access size 3. */ __builtin_memcpy (&tab[1], foo + i, 3); /* This should not generate a __builtin___asan_report_load1 because @@ -63,6 +56,5 @@ main () } /* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 3 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_load1" "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c new file mode 100644 index 0000000..24dfcfe --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c @@ -0,0 +1,18 @@ +/* { dg-options "-fdump-tree-sanopt" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +extern __UINT32_TYPE__ a; + +void +foo () +{ + /* Instrument a with access size 3. */ + int d = __builtin_memcmp (&a, "123", 3); + /* This should generate a __builtin___asan_report_store4, because + the reference to a has been instrumented above with access size 3. */ + a = 1; +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store4" 1 "sanopt" } } */ +/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c new file mode 100644 index 0000000..4082f32 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c @@ -0,0 +1,20 @@ +/* { dg-options "-fdump-tree-sanopt" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +extern __UINT32_TYPE__ a; + +void +foo () +{ + /* Instrument a with access size 5. */ + int d = __builtin_memcmp (&a, "12345", 4); + /* This should not generate a __builtin___asan_report_store4 because + the reference to a has been already instrumented above with access + size 5. */ + a = 1; +} + +/* { dg-final { scan-tree-dump-not "& 7" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "sanopt" } } */ +/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c deleted file mode 100644 index a58411c..0000000 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c +++ /dev/null @@ -1,26 +0,0 @@ -/* This tests that when faced with two references to the same memory - location in the same basic block, the second reference should not - be instrumented by the Address Sanitizer. But in case of access to - overlapping regions we must be precise. */ - -/* { dg-options "-fdump-tree-sanopt" } */ -/* { dg-do compile } */ -/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ - -int -main () -{ - char tab[5]; - - /* Here, we instrument the access at offset 0 and access at offset - 4. */ - __builtin_memset (tab, 1, sizeof (tab)); - /* We instrumented access at offset 0 above already, so only access - at offset 3 is instrumented. */ - __builtin_memset (tab, 1, 3); -} - -/* { dg-final { scan-tree-dump-times "& 7" 3 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report" 2 "sanopt" } } */ -/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c index c3632aa..a613b92 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c @@ -5,13 +5,13 @@ void foo (int *a, char *b, char *c) { - /* One check for c[0], one check for a[], one check for c, two checks for b. */ + /* One check for c[0], one check for a[]. */ __builtin_memmove (c, b, a[c[0]]); - /* For a total of 5 checks. */ + /* For a total of 2 checks. */ + int d = c[0] == 1; } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 2 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c index 077ea34..f4ca603 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c @@ -5,14 +5,12 @@ void foo (int *a, char *b, char *c) { - /* One check for b[0], one check for a[], 2 checks for c and one checks for b. */ - __builtin_memmove (c, b, a[b[0]]); - /* For a total of 5 checks. */ + /* One check for a[]. */ + __builtin_memmove (c, b, a[0]); + /* For a total of 1 checks. */ + int d = a[0] == 0; } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 1 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c index 6d87104..757f0ee 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c @@ -5,16 +5,15 @@ void foo (int *a, char *b, char *c) { - /* One check for c[0], one check for a[], one check for c and 2 checks for b. */ + /* One check for c[0], one check for a[]. */ __builtin_memmove (c, b, a[c[0]]); - /* One check for a[], one check for c and one check for b. */ + /* One check for b[0], one check for a[]. */ __builtin_memmove (c, b, a[b[0]]); - /* For a total of 8 checks. */ + /* For a total of 4 checks. */ + int d = c[0] == b[0]; } -/* { dg-final { scan-tree-dump-times "& 7" 8 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 4 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c index 5baa10d..0c50145 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c @@ -2,26 +2,22 @@ /* { dg-do compile } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ -char e[200]; +char e[5]; -struct S +extern struct S { - char a[100]; - char b[100]; + int a; + char b; } s; int foo (int *a, char *b, char *c) { - /* 2 checks for s.a, 2 checks for e. */ - int d = __builtin_memcmp (s.a, e, 100); - /* One check for s.a and one check for e. */ - d += __builtin_memcmp (s.a, e, 200); - /* For a total of 6 checks. */ - return d; + int d = __builtin_memcmp (&s.a, e, 4); + /* No check because s.a was instrumented above with access size 4. */ + return s.a; } -/* { dg-final { scan-tree-dump-times "& 7" 6 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 4 "sanopt" } } */ -/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "& 7" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_load4" "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c index 2a4c081..4eeedce 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c @@ -5,16 +5,16 @@ char foo (int *a, char *b, char *c) { - /* One check for b[0], one check for a[], two checks for c and one check for b. */ + /* One check for b[0], one check for a[]. */ __builtin_memmove (c, b, a[b[0]]); + /* One check for c[0], one check for a[]. */ + __builtin_memmove (b, c, a[c[0]]); /* No checks here. */ return c[0] + b[0]; - /* For a total of 5 checks. */ + /* For a total of 4 checks. */ } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 4 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 2 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c deleted file mode 100644 index 9449de5..0000000 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c +++ /dev/null @@ -1,13 +0,0 @@ -/* { dg-options "-fdump-tree-sanopt" } */ -/* { dg-do compile } */ -/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ - -__SIZE_TYPE__ -f (char *a) -{ - a[0] = '1'; - return __builtin_strlen (a); -} - -/* { dg-final { scan-tree-dump-times "__asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c index f58f554..0f49286 100644 --- a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c +++ b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c @@ -9,14 +9,8 @@ char a[2] = "0"; #ifdef __cplusplus extern "C" #endif - -__attribute__((no_sanitize_address, noinline)) __SIZE_TYPE__ -strlen (const char *p) { - - __SIZE_TYPE__ n = 0; - for (; *p; ++n, ++p); - return n; -} +__SIZE_TYPE__ +strlen (const char *p); int main () { char *p = &a[0]; @@ -25,6 +19,6 @@ int main () { return __builtin_strlen (a); } -/* { dg-output "READ of size 1 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */ -/* { dg-output " #0 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:25|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 1 bytes inside of global variable" } */ +/* { dg-output "READ of size 2 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */ +/* { dg-output " #1 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:19|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 0 bytes to the right of global variable" } */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Don't expand string/memory builtins if ASan is enabled. 2014-10-21 13:38 ` [PATCHv2] " Maxim Ostapenko @ 2014-10-23 8:51 ` Yury Gribov 2014-10-27 14:37 ` Jakub Jelinek 0 siblings, 1 reply; 10+ messages in thread From: Yury Gribov @ 2014-10-23 8:51 UTC (permalink / raw) To: Maxim Ostapenko, Jakub Jelinek Cc: GCC Patches, Slava Garbuzov, Maxim Ostapenko On 10/21/2014 05:26 PM, Maxim Ostapenko wrote: > Hi, > > this is the second version of the patch. Here the major changes from the > previous one: [snip] > case BUILT_IN_BCMP: > + *intercepted_p = false; The code which identifies interceptors seems to be duplicated in expand_builtin. What about factoring this out to some is_asan_intercepted() in asan.h? -Y ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Don't expand string/memory builtins if ASan is enabled. 2014-10-23 8:51 ` Yury Gribov @ 2014-10-27 14:37 ` Jakub Jelinek 2014-10-28 9:27 ` Maxim Ostapenko 0 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2014-10-27 14:37 UTC (permalink / raw) To: Yury Gribov; +Cc: Maxim Ostapenko, GCC Patches, Slava Garbuzov, Maxim Ostapenko On Thu, Oct 23, 2014 at 12:26:48PM +0400, Yury Gribov wrote: > On 10/21/2014 05:26 PM, Maxim Ostapenko wrote: > >Hi, > > > >this is the second version of the patch. Here the major changes from the > >previous one: > > [snip] > > > case BUILT_IN_BCMP: > >+ *intercepted_p = false; > > The code which identifies interceptors seems to be duplicated in > expand_builtin. What about factoring this out to some is_asan_intercepted() > in asan.h? I agree. BTW, when you handle a builtin, but set *intercepted_p to false, is the point just that you don't instrument e.g. scalar accesses to that memory region afterwards? I mean: memcpy (p, "abc", 3); p[0] = 'd'; you don't really have to instrument p[0] store, because supposedly memcpy call has already verified p[0] through p[2] is writable. Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Don't expand string/memory builtins if ASan is enabled. 2014-10-27 14:37 ` Jakub Jelinek @ 2014-10-28 9:27 ` Maxim Ostapenko 2014-10-28 9:35 ` Jakub Jelinek 2014-11-11 23:51 ` H.J. Lu 0 siblings, 2 replies; 10+ messages in thread From: Maxim Ostapenko @ 2014-10-28 9:27 UTC (permalink / raw) To: Jakub Jelinek, Yury Gribov; +Cc: GCC Patches, Slava Garbuzov, Maxim Ostapenko [-- Attachment #1: Type: text/plain, Size: 900 bytes --] >> The code which identifies interceptors seems to be duplicated in >> expand_builtin. What about factoring this out to some is_asan_intercepted() >> in asan.h? > I agree. Fixed this. > > BTW, when you handle a builtin, but set *intercepted_p to false, is the > point just that you don't instrument e.g. scalar accesses to that memory > region afterwards? I mean: > memcpy (p, "abc", 3); > p[0] = 'd'; > you don't really have to instrument p[0] store, because supposedly memcpy > call has already verified p[0] through p[2] is writable. > > Jakub > Yes, in this case p[0] store is not instrumented we use either intercepted_p builtin or not intercepted_p one. But access with some offset e.g. to p[1] will be instrumented in both cases. I'll probably fix this next time. As Yury mentioned, I've also added a test case for pr63638 in this patch. Tested on x86_64-unknown-inux-gnu. -Maxim [-- Attachment #2: builtins-18.diff --] [-- Type: text/x-patch, Size: 34973 bytes --] gcc/ChangeLog: 2014-10-28 Max Ostapenko <m.ostapenko@partner.samsung.com> * asan.h (is_asan_intercepted): New function. * asan.c (asan_mem_ref_hasher::hash): Remove MEM_REF access size from hash value construction. Call iterative_hash_expr instead of explicit hash building. (asan_mem_ref_hasher::equal): Change condition. (has_mem_ref_been_instrumented): Likewise. (update_mem_ref_hash_table): Likewise. (maybe_update_mem_ref_hash_table): New function. (instrument_strlen_call): Removed. (get_mem_refs_of_builtin_call): Handle new parameter. (instrument_builtin_call): Call maybe_update_mem_ref_hash_table instead of instrument_mem_region_access if intercepted_p is true. (instrument_mem_region_access): Instrument only base with len instead of base and end with 1. (build_check_stmt): Remove start_instrumented and end_instrumented parameters. (enum asan_check_flags): Remove ASAN_CHECK_START_INSTRUMENTED and ASAN_CHECK_END_INSTRUMENTED. Change ASAN_CHECK_LAST. (asan_expand_check_ifn): Remove start_instrumented and end_instrumented. * builtins.c (expand_builtin): Include asan.h. Don't expand string/memory builtin functions that have interceptors if ASan is enabled. gcc/testsuite/ChangeLog: 2014-10-28 Max Ostapenko <m.ostapenko@partner.samsung.com> * c-c++-common/asan/no-redundant-instrumentation-1.c: Updated test. * c-c++-common/asan/no-redundant-instrumentation-4.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-2.c: Removed. * c-c++-common/asan/no-redundant-instrumentation-9.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-10.c: New test. * c-c++-common/asan/no-redundant-instrumentation-11.c: Likewise. * c-c++-common/asan/pr63638.c: Likewise. diff --git a/gcc/asan.c b/gcc/asan.c index d50e9e0..878fe48 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -264,9 +264,7 @@ enum asan_check_flags ASAN_CHECK_STORE = 1 << 0, ASAN_CHECK_SCALAR_ACCESS = 1 << 1, ASAN_CHECK_NON_ZERO_LEN = 1 << 2, - ASAN_CHECK_START_INSTRUMENTED = 1 << 3, - ASAN_CHECK_END_INSTRUMENTED = 1 << 4, - ASAN_CHECK_LAST + ASAN_CHECK_LAST = 1 << 3 }; /* Hashtable support for memory references used by gimple @@ -363,10 +361,7 @@ struct asan_mem_ref_hasher inline hashval_t asan_mem_ref_hasher::hash (const asan_mem_ref *mem_ref) { - inchash::hash hstate; - inchash::add_expr (mem_ref->start, hstate); - hstate.add_wide_int (mem_ref->access_size); - return hstate.end (); + return iterative_hash_expr (mem_ref->start, 0); } /* Compare two memory references. We accept the length of either @@ -376,8 +371,7 @@ inline bool asan_mem_ref_hasher::equal (const asan_mem_ref *m1, const asan_mem_ref *m2) { - return (m1->access_size == m2->access_size - && operand_equal_p (m1->start, m2->start, 0)); + return operand_equal_p (m1->start, m2->start, 0); } static hash_table<asan_mem_ref_hasher> *asan_mem_ref_ht; @@ -428,7 +422,8 @@ has_mem_ref_been_instrumented (tree ref, HOST_WIDE_INT access_size) asan_mem_ref r; asan_mem_ref_init (&r, ref, access_size); - return (get_mem_ref_hash_table ()->find (&r) != NULL); + asan_mem_ref *saved_ref = get_mem_ref_hash_table ()->find (&r); + return saved_ref && saved_ref->access_size >= access_size; } /* Return true iff the memory reference REF has been instrumented. */ @@ -445,19 +440,11 @@ has_mem_ref_been_instrumented (const asan_mem_ref *ref) static bool has_mem_ref_been_instrumented (const asan_mem_ref *ref, tree len) { - /* First let's see if the address of the beginning of REF has been - instrumented. */ - if (!has_mem_ref_been_instrumented (ref)) - return false; + HOST_WIDE_INT size_in_bytes + = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; - if (len != 0) - { - /* Let's see if the end of the region has been instrumented. */ - if (!has_mem_ref_been_instrumented (asan_mem_ref_get_end (ref, len), - ref->access_size)) - return false; - } - return true; + return size_in_bytes != -1 + && has_mem_ref_been_instrumented (ref->start, size_in_bytes); } /* Set REF to the memory reference present in a gimple assignment @@ -503,7 +490,8 @@ get_mem_refs_of_builtin_call (const gimple call, asan_mem_ref *dst, tree *dst_len, bool *dst_is_store, - bool *dest_is_deref) + bool *dest_is_deref, + bool *intercepted_p) { gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL)); @@ -513,6 +501,8 @@ get_mem_refs_of_builtin_call (const gimple call, bool is_store = true, got_reference_p = false; HOST_WIDE_INT access_size = 1; + *intercepted_p = is_asan_intercepted ((DECL_FUNCTION_CODE (callee))); + switch (DECL_FUNCTION_CODE (callee)) { /* (s, s, n) style memops. */ @@ -845,12 +835,12 @@ has_stmt_been_instrumented_p (gimple stmt) tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; bool src0_is_store = false, src1_is_store = false, - dest_is_store = false, dest_is_deref = false; + dest_is_store = false, dest_is_deref = false, intercepted_p = true; if (get_mem_refs_of_builtin_call (stmt, &src0, &src0_len, &src0_is_store, &src1, &src1_len, &src1_is_store, &dest, &dest_len, &dest_is_store, - &dest_is_deref)) + &dest_is_deref, &intercepted_p)) { if (src0.start != NULL_TREE && !has_mem_ref_been_instrumented (&src0, src0_len)) @@ -881,7 +871,7 @@ update_mem_ref_hash_table (tree ref, HOST_WIDE_INT access_size) asan_mem_ref_init (&r, ref, access_size); asan_mem_ref **slot = ht->find_slot (&r, INSERT); - if (*slot == NULL) + if (*slot == NULL || (*slot)->access_size < access_size) *slot = asan_mem_ref_new (ref, access_size); } @@ -1619,22 +1609,13 @@ static void build_check_stmt (location_t loc, tree base, tree len, HOST_WIDE_INT size_in_bytes, gimple_stmt_iterator *iter, bool is_non_zero_len, bool before_p, bool is_store, - bool is_scalar_access, unsigned int align = 0, - bool start_instrumented = false, - bool end_instrumented = false) + bool is_scalar_access, unsigned int align = 0) { gimple_stmt_iterator gsi = *iter; gimple g; gcc_assert (!(size_in_bytes > 0 && !is_non_zero_len)); - if (start_instrumented && end_instrumented) - { - if (!before_p) - gsi_next (iter); - return; - } - gsi = *iter; base = unshare_expr (base); @@ -1677,10 +1658,6 @@ build_check_stmt (location_t loc, tree base, tree len, flags |= ASAN_CHECK_NON_ZERO_LEN; if (is_scalar_access) flags |= ASAN_CHECK_SCALAR_ACCESS; - if (start_instrumented) - flags |= ASAN_CHECK_START_INSTRUMENTED; - if (end_instrumented) - flags |= ASAN_CHECK_END_INSTRUMENTED; g = gimple_build_call_internal (IFN_ASAN_CHECK, 4, build_int_cst (integer_type_node, flags), @@ -1795,6 +1772,22 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t, } +/* Insert a memory reference into the hash table if access length + can be determined in compile time. */ + +static void +maybe_update_mem_ref_hash_table (tree base, tree len) +{ + if (!POINTER_TYPE_P (TREE_TYPE (base)) + || !INTEGRAL_TYPE_P (TREE_TYPE (len))) + return; + + HOST_WIDE_INT size_in_bytes = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; + + if (size_in_bytes != -1) + update_mem_ref_hash_table (base, size_in_bytes); +} + /* Instrument an access to a contiguous memory region that starts at the address pointed to by BASE, over a length of LEN (expressed in the sizeof (*BASE) bytes). ITER points to the instruction before @@ -1813,97 +1806,20 @@ instrument_mem_region_access (tree base, tree len, || integer_zerop (len)) return; - /* If the beginning of the memory region has already been - instrumented, do not instrument it. */ - bool start_instrumented = has_mem_ref_been_instrumented (base, 1); - - /* If the end of the memory region has already been instrumented, do - not instrument it. */ - tree end = asan_mem_ref_get_end (base, len); - bool end_instrumented = has_mem_ref_been_instrumented (end, 1); - HOST_WIDE_INT size_in_bytes = tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1; - build_check_stmt (location, base, len, size_in_bytes, iter, - /*is_non_zero_len*/size_in_bytes > 0, /*before_p*/true, - is_store, /*is_scalar_access*/false, /*align*/0, - start_instrumented, end_instrumented); - - update_mem_ref_hash_table (base, 1); - if (size_in_bytes != -1) - update_mem_ref_hash_table (end, 1); + if ((size_in_bytes == -1) + || !has_mem_ref_been_instrumented (base, size_in_bytes)) + { + build_check_stmt (location, base, len, size_in_bytes, iter, + /*is_non_zero_len*/size_in_bytes > 0, /*before_p*/true, + is_store, /*is_scalar_access*/false, /*align*/0); + } + maybe_update_mem_ref_hash_table (base, len); *iter = gsi_for_stmt (gsi_stmt (*iter)); } -/* Instrument the call (to the builtin strlen function) pointed to by - ITER. - - This function instruments the access to the first byte of the - argument, right before the call. After the call it instruments the - access to the last byte of the argument; it uses the result of the - call to deduce the offset of that last byte. - - Upon completion, iff the call has actually been instrumented, this - function returns TRUE and *ITER points to the statement logically - following the built-in strlen function call *ITER was initially - pointing to. Otherwise, the function returns FALSE and *ITER - remains unchanged. */ - -static bool -instrument_strlen_call (gimple_stmt_iterator *iter) -{ - gimple g; - gimple call = gsi_stmt (*iter); - gcc_assert (is_gimple_call (call)); - - tree callee = gimple_call_fndecl (call); - gcc_assert (is_builtin_fn (callee) - && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL - && DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN); - - location_t loc = gimple_location (call); - - tree len = gimple_call_lhs (call); - if (len == NULL) - /* Some passes might clear the return value of the strlen call; - bail out in that case. Return FALSE as we are not advancing - *ITER. */ - return false; - gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (len))); - - len = maybe_cast_to_ptrmode (loc, len, iter, /*before_p*/false); - - tree str_arg = gimple_call_arg (call, 0); - bool start_instrumented = has_mem_ref_been_instrumented (str_arg, 1); - - tree cptr_type = build_pointer_type (char_type_node); - g = gimple_build_assign_with_ops (NOP_EXPR, - make_ssa_name (cptr_type, NULL), - str_arg, NULL); - gimple_set_location (g, loc); - gsi_insert_before (iter, g, GSI_SAME_STMT); - str_arg = gimple_assign_lhs (g); - - build_check_stmt (loc, str_arg, NULL_TREE, 1, iter, - /*is_non_zero_len*/true, /*before_p=*/true, - /*is_store=*/false, /*is_scalar_access*/true, /*align*/0, - start_instrumented, start_instrumented); - - g = gimple_build_assign_with_ops (POINTER_PLUS_EXPR, - make_ssa_name (cptr_type, NULL), - str_arg, - len); - gimple_set_location (g, loc); - gsi_insert_after (iter, g, GSI_NEW_STMT); - - build_check_stmt (loc, gimple_assign_lhs (g), NULL_TREE, 1, iter, - /*is_non_zero_len*/true, /*before_p=*/false, - /*is_store=*/false, /*is_scalar_access*/true, /*align*/0); - - return true; -} - /* Instrument the call to a built-in memory access function that is pointed to by the iterator ITER. @@ -1921,49 +1837,54 @@ instrument_builtin_call (gimple_stmt_iterator *iter) gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL)); - tree callee = gimple_call_fndecl (call); location_t loc = gimple_location (call); - if (DECL_FUNCTION_CODE (callee) == BUILT_IN_STRLEN) - iter_advanced_p = instrument_strlen_call (iter); - else - { - asan_mem_ref src0, src1, dest; - asan_mem_ref_init (&src0, NULL, 1); - asan_mem_ref_init (&src1, NULL, 1); - asan_mem_ref_init (&dest, NULL, 1); + asan_mem_ref src0, src1, dest; + asan_mem_ref_init (&src0, NULL, 1); + asan_mem_ref_init (&src1, NULL, 1); + asan_mem_ref_init (&dest, NULL, 1); - tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; - bool src0_is_store = false, src1_is_store = false, - dest_is_store = false, dest_is_deref = false; + tree src0_len = NULL_TREE, src1_len = NULL_TREE, dest_len = NULL_TREE; + bool src0_is_store = false, src1_is_store = false, dest_is_store = false, + dest_is_deref = false, intercepted_p = true; - if (get_mem_refs_of_builtin_call (call, - &src0, &src0_len, &src0_is_store, - &src1, &src1_len, &src1_is_store, - &dest, &dest_len, &dest_is_store, - &dest_is_deref)) + if (get_mem_refs_of_builtin_call (call, + &src0, &src0_len, &src0_is_store, + &src1, &src1_len, &src1_is_store, + &dest, &dest_len, &dest_is_store, + &dest_is_deref, &intercepted_p)) + { + if (dest_is_deref) { - if (dest_is_deref) - { - instrument_derefs (iter, dest.start, loc, dest_is_store); - gsi_next (iter); - iter_advanced_p = true; - } - else if (src0_len || src1_len || dest_len) - { - if (src0.start != NULL_TREE) - instrument_mem_region_access (src0.start, src0_len, - iter, loc, /*is_store=*/false); - if (src1.start != NULL_TREE) - instrument_mem_region_access (src1.start, src1_len, - iter, loc, /*is_store=*/false); - if (dest.start != NULL_TREE) - instrument_mem_region_access (dest.start, dest_len, - iter, loc, /*is_store=*/true); - *iter = gsi_for_stmt (call); - gsi_next (iter); - iter_advanced_p = true; - } + instrument_derefs (iter, dest.start, loc, dest_is_store); + gsi_next (iter); + iter_advanced_p = true; + } + else if (!intercepted_p + && (src0_len || src1_len || dest_len)) + { + if (src0.start != NULL_TREE) + instrument_mem_region_access (src0.start, src0_len, + iter, loc, /*is_store=*/false); + if (src1.start != NULL_TREE) + instrument_mem_region_access (src1.start, src1_len, + iter, loc, /*is_store=*/false); + if (dest.start != NULL_TREE) + instrument_mem_region_access (dest.start, dest_len, + iter, loc, /*is_store=*/true); + + *iter = gsi_for_stmt (call); + gsi_next (iter); + iter_advanced_p = true; + } + else + { + if (src0.start != NULL_TREE) + maybe_update_mem_ref_hash_table (src0.start, src0_len); + if (src1.start != NULL_TREE) + maybe_update_mem_ref_hash_table (src1.start, src1_len); + if (dest.start != NULL_TREE) + maybe_update_mem_ref_hash_table (dest.start, dest_len); } } return iter_advanced_p; @@ -2518,8 +2439,6 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) bool is_scalar_access = (flags & ASAN_CHECK_SCALAR_ACCESS) != 0; bool is_store = (flags & ASAN_CHECK_STORE) != 0; bool is_non_zero_len = (flags & ASAN_CHECK_NON_ZERO_LEN) != 0; - bool start_instrumented = (flags & ASAN_CHECK_START_INSTRUMENTED) != 0; - bool end_instrumented = (flags & ASAN_CHECK_END_INSTRUMENTED) != 0; tree base = gimple_call_arg (g, 1); tree len = gimple_call_arg (g, 2); @@ -2624,46 +2543,42 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) else { /* Slow path for 1, 2 and 4 byte accesses. */ - - if (!start_instrumented) + /* Test (shadow != 0) + & ((base_addr & 7) + (real_size_in_bytes - 1)) >= shadow). */ + tree shadow = build_shadow_mem_access (&gsi, loc, base_addr, + shadow_ptr_type); + gimple shadow_test = build_assign (NE_EXPR, shadow, 0); + gimple_seq seq = NULL; + gimple_seq_add_stmt (&seq, shadow_test); + /* Aligned (>= 8 bytes) can test just + (real_size_in_bytes - 1 >= shadow), as base_addr & 7 is known + to be 0. */ + if (align < 8) { - /* Test (shadow != 0) - & ((base_addr & 7) + (real_size_in_bytes - 1)) >= shadow). */ - tree shadow = build_shadow_mem_access (&gsi, loc, base_addr, - shadow_ptr_type); - gimple shadow_test = build_assign (NE_EXPR, shadow, 0); - gimple_seq seq = NULL; - gimple_seq_add_stmt (&seq, shadow_test); - /* Aligned (>= 8 bytes) can test just - (real_size_in_bytes - 1 >= shadow), as base_addr & 7 is known - to be 0. */ - if (align < 8) - { - gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, - base_addr, 7)); - gimple_seq_add_stmt (&seq, - build_type_cast (shadow_type, - gimple_seq_last (seq))); - if (real_size_in_bytes > 1) - gimple_seq_add_stmt (&seq, - build_assign (PLUS_EXPR, - gimple_seq_last (seq), - real_size_in_bytes - 1)); - t = gimple_assign_lhs (gimple_seq_last_stmt (seq)); - } - else - t = build_int_cst (shadow_type, real_size_in_bytes - 1); - gimple_seq_add_stmt (&seq, build_assign (GE_EXPR, t, shadow)); - gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test, - gimple_seq_last (seq))); - t = gimple_assign_lhs (gimple_seq_last (seq)); - gimple_seq_set_location (seq, loc); - gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); + gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, + base_addr, 7)); + gimple_seq_add_stmt (&seq, + build_type_cast (shadow_type, + gimple_seq_last (seq))); + if (real_size_in_bytes > 1) + gimple_seq_add_stmt (&seq, + build_assign (PLUS_EXPR, + gimple_seq_last (seq), + real_size_in_bytes - 1)); + t = gimple_assign_lhs (gimple_seq_last_stmt (seq)); } + else + t = build_int_cst (shadow_type, real_size_in_bytes - 1); + gimple_seq_add_stmt (&seq, build_assign (GE_EXPR, t, shadow)); + gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test, + gimple_seq_last (seq))); + t = gimple_assign_lhs (gimple_seq_last (seq)); + gimple_seq_set_location (seq, loc); + gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); /* For non-constant, misaligned or otherwise weird access sizes, - check first and last byte. */ - if (size_in_bytes == -1 && !end_instrumented) + check first and last byte. */ + if (size_in_bytes == -1) { g = gimple_build_assign_with_ops (MINUS_EXPR, make_ssa_name (pointer_sized_int_node, NULL), @@ -2694,9 +2609,8 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) shadow)); gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test, gimple_seq_last (seq))); - if (!start_instrumented) - gimple_seq_add_stmt (&seq, build_assign (BIT_IOR_EXPR, t, - gimple_seq_last (seq))); + gimple_seq_add_stmt (&seq, build_assign (BIT_IOR_EXPR, t, + gimple_seq_last (seq))); t = gimple_assign_lhs (gimple_seq_last (seq)); gimple_seq_set_location (seq, loc); gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); diff --git a/gcc/asan.h b/gcc/asan.h index 198433f..46a4f18 100644 --- a/gcc/asan.h +++ b/gcc/asan.h @@ -76,4 +76,28 @@ asan_red_zone_size (unsigned int size) return c ? 2 * ASAN_RED_ZONE_SIZE - c : ASAN_RED_ZONE_SIZE; } +/* Return TRUE if builtin with given FCODE will be intercepted by + libasan. */ + +static inline bool +is_asan_intercepted (enum built_in_function fcode) +{ + return fcode == BUILT_IN_INDEX + || fcode == BUILT_IN_MEMCHR + || fcode == BUILT_IN_MEMCMP + || fcode == BUILT_IN_MEMCPY + || fcode == BUILT_IN_MEMMOVE + || fcode == BUILT_IN_MEMSET + || fcode == BUILT_IN_STRCASECMP + || fcode == BUILT_IN_STRCAT + || fcode == BUILT_IN_STRCHR + || fcode == BUILT_IN_STRCMP + || fcode == BUILT_IN_STRCPY + || fcode == BUILT_IN_STRDUP + || fcode == BUILT_IN_STRLEN + || fcode == BUILT_IN_STRNCASECMP + || fcode == BUILT_IN_STRNCAT + || fcode == BUILT_IN_STRNCMP + || fcode == BUILT_IN_STRNCPY; +} #endif /* TREE_ASAN */ diff --git a/gcc/builtins.c b/gcc/builtins.c index 6eeb3d9..5396de9 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -62,6 +62,7 @@ along with GCC; see the file COPYING3. If not see #include "value-prof.h" #include "diagnostic-core.h" #include "builtins.h" +#include "asan.h" #include "ubsan.h" #include "cilk.h" @@ -5765,6 +5766,14 @@ expand_builtin (tree exp, rtx target, rtx subtarget, enum machine_mode mode, enum machine_mode target_mode = TYPE_MODE (TREE_TYPE (exp)); int flags; + /* When ASan is enabled, we don't want to expand some memory/string + builtins and rely on libsanitizer's hooks. This allows us to avoid + redundant checks and be sure, that possible overflow will be detected + by ASan. */ + + if ((flag_sanitize & SANITIZE_ADDRESS) && is_asan_intercepted (fcode)) + return expand_call (exp, target, ignore); + if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD) return targetm.expand_builtin (exp, target, subtarget, mode, ignore); diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c index 028f8d7..baacb1e 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c @@ -6,7 +6,7 @@ /* { dg-do compile } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ -extern char tab[4]; +extern char tab[6]; static int test0 () @@ -35,17 +35,10 @@ test1 (int i) the initialization. */ foo[i] = 1; - /*__builtin___asan_report_store_n called once here to instrument - the store to the memory region of tab. */ + /* Instrument tab memory region. */ __builtin_memset (tab, 3, sizeof (tab)); - /* There is no instrumentation for the two memset calls below. */ - __builtin_memset (tab, 4, sizeof (tab)); - __builtin_memset (tab, 5, sizeof (tab)); - - /* There is a call to __builtin___asan_report_store_n and a call - to __builtin___asan_report_load_n to instrument the store to - (subset of) the memory region of tab. */ + /* Instrument tab[1] with access size 3. */ __builtin_memcpy (&tab[1], foo + i, 3); /* This should not generate a __builtin___asan_report_load1 because @@ -63,6 +56,5 @@ main () } /* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 3 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_load1" "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c new file mode 100644 index 0000000..24dfcfe --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-10.c @@ -0,0 +1,18 @@ +/* { dg-options "-fdump-tree-sanopt" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +extern __UINT32_TYPE__ a; + +void +foo () +{ + /* Instrument a with access size 3. */ + int d = __builtin_memcmp (&a, "123", 3); + /* This should generate a __builtin___asan_report_store4, because + the reference to a has been instrumented above with access size 3. */ + a = 1; +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store4" 1 "sanopt" } } */ +/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c new file mode 100644 index 0000000..4082f32 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-11.c @@ -0,0 +1,20 @@ +/* { dg-options "-fdump-tree-sanopt" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +extern __UINT32_TYPE__ a; + +void +foo () +{ + /* Instrument a with access size 5. */ + int d = __builtin_memcmp (&a, "12345", 4); + /* This should not generate a __builtin___asan_report_store4 because + the reference to a has been already instrumented above with access + size 5. */ + a = 1; +} + +/* { dg-final { scan-tree-dump-not "& 7" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "sanopt" } } */ +/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c deleted file mode 100644 index a58411c..0000000 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c +++ /dev/null @@ -1,26 +0,0 @@ -/* This tests that when faced with two references to the same memory - location in the same basic block, the second reference should not - be instrumented by the Address Sanitizer. But in case of access to - overlapping regions we must be precise. */ - -/* { dg-options "-fdump-tree-sanopt" } */ -/* { dg-do compile } */ -/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ - -int -main () -{ - char tab[5]; - - /* Here, we instrument the access at offset 0 and access at offset - 4. */ - __builtin_memset (tab, 1, sizeof (tab)); - /* We instrumented access at offset 0 above already, so only access - at offset 3 is instrumented. */ - __builtin_memset (tab, 1, 3); -} - -/* { dg-final { scan-tree-dump-times "& 7" 3 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report" 2 "sanopt" } } */ -/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c index c3632aa..a613b92 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c @@ -5,13 +5,13 @@ void foo (int *a, char *b, char *c) { - /* One check for c[0], one check for a[], one check for c, two checks for b. */ + /* One check for c[0], one check for a[]. */ __builtin_memmove (c, b, a[c[0]]); - /* For a total of 5 checks. */ + /* For a total of 2 checks. */ + int d = c[0] == 1; } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 2 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c index 077ea34..f4ca603 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c @@ -5,14 +5,12 @@ void foo (int *a, char *b, char *c) { - /* One check for b[0], one check for a[], 2 checks for c and one checks for b. */ - __builtin_memmove (c, b, a[b[0]]); - /* For a total of 5 checks. */ + /* One check for a[]. */ + __builtin_memmove (c, b, a[0]); + /* For a total of 1 checks. */ + int d = a[0] == 0; } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 1 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c index 6d87104..757f0ee 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c @@ -5,16 +5,15 @@ void foo (int *a, char *b, char *c) { - /* One check for c[0], one check for a[], one check for c and 2 checks for b. */ + /* One check for c[0], one check for a[]. */ __builtin_memmove (c, b, a[c[0]]); - /* One check for a[], one check for c and one check for b. */ + /* One check for b[0], one check for a[]. */ __builtin_memmove (c, b, a[b[0]]); - /* For a total of 8 checks. */ + /* For a total of 4 checks. */ + int d = c[0] == b[0]; } -/* { dg-final { scan-tree-dump-times "& 7" 8 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 4 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "sanopt" } } */ /* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 2 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 2 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c index 5baa10d..0c50145 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c @@ -2,26 +2,22 @@ /* { dg-do compile } */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ -char e[200]; +char e[5]; -struct S +extern struct S { - char a[100]; - char b[100]; + int a; + char b; } s; int foo (int *a, char *b, char *c) { - /* 2 checks for s.a, 2 checks for e. */ - int d = __builtin_memcmp (s.a, e, 100); - /* One check for s.a and one check for e. */ - d += __builtin_memcmp (s.a, e, 200); - /* For a total of 6 checks. */ - return d; + int d = __builtin_memcmp (&s.a, e, 4); + /* No check because s.a was instrumented above with access size 4. */ + return s.a; } -/* { dg-final { scan-tree-dump-times "& 7" 6 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 4 "sanopt" } } */ -/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "& 7" "sanopt" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_load4" "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c index 2a4c081..4eeedce 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c @@ -5,16 +5,16 @@ char foo (int *a, char *b, char *c) { - /* One check for b[0], one check for a[], two checks for c and one check for b. */ + /* One check for b[0], one check for a[]. */ __builtin_memmove (c, b, a[b[0]]); + /* One check for c[0], one check for a[]. */ + __builtin_memmove (b, c, a[c[0]]); /* No checks here. */ return c[0] + b[0]; - /* For a total of 5 checks. */ + /* For a total of 4 checks. */ } -/* { dg-final { scan-tree-dump-times "& 7" 5 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load_n" 1 "sanopt" } } */ -/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store_n" 1 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "& 7" 4 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "sanopt" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load4" 2 "sanopt" } } */ /* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c deleted file mode 100644 index 9449de5..0000000 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-9.c +++ /dev/null @@ -1,13 +0,0 @@ -/* { dg-options "-fdump-tree-sanopt" } */ -/* { dg-do compile } */ -/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ - -__SIZE_TYPE__ -f (char *a) -{ - a[0] = '1'; - return __builtin_strlen (a); -} - -/* { dg-final { scan-tree-dump-times "__asan_report_load1" 1 "sanopt" } } */ -/* { dg-final { cleanup-tree-dump "sanopt" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/pr63638.c b/gcc/testsuite/c-c++-common/asan/pr63638.c new file mode 100644 index 0000000..a8bafc5 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/pr63638.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ + +extern +#ifdef __cplusplus +"C" +#endif +void *memcpy (void *, const void *, __SIZE_TYPE__); + +struct S{ + long d0, d1, d2, d3, d4, d5, d6; +}; + +struct S s[6]; + +int f(struct S *p) +{ + memcpy(p, &s[2], sizeof(*p)); + memcpy(p, &s[1], sizeof(*p)); +} + diff --git a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c index f58f554..0f49286 100644 --- a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c +++ b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c @@ -9,14 +9,8 @@ char a[2] = "0"; #ifdef __cplusplus extern "C" #endif - -__attribute__((no_sanitize_address, noinline)) __SIZE_TYPE__ -strlen (const char *p) { - - __SIZE_TYPE__ n = 0; - for (; *p; ++n, ++p); - return n; -} +__SIZE_TYPE__ +strlen (const char *p); int main () { char *p = &a[0]; @@ -25,6 +19,6 @@ int main () { return __builtin_strlen (a); } -/* { dg-output "READ of size 1 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */ -/* { dg-output " #0 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:25|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 1 bytes inside of global variable" } */ +/* { dg-output "READ of size 2 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */ +/* { dg-output " #1 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:19|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 0 bytes to the right of global variable" } */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Don't expand string/memory builtins if ASan is enabled. 2014-10-28 9:27 ` Maxim Ostapenko @ 2014-10-28 9:35 ` Jakub Jelinek 2014-11-11 23:51 ` H.J. Lu 1 sibling, 0 replies; 10+ messages in thread From: Jakub Jelinek @ 2014-10-28 9:35 UTC (permalink / raw) To: Maxim Ostapenko; +Cc: Yury Gribov, GCC Patches, Slava Garbuzov, Maxim Ostapenko On Tue, Oct 28, 2014 at 12:21:25PM +0400, Maxim Ostapenko wrote: > 2014-10-28 Max Ostapenko <m.ostapenko@partner.samsung.com> > > * asan.h (is_asan_intercepted): New function. I'd call it asan_intercepted_p instead. Can you please check if: void foo (char *p) { volatile int zero = 0; memcpy (p, "abc", zero); p[0] = 'd'; } does not remove instrumentation from p[0] store (i.e. the builtin store length might (and this case actually is) zero, so no checking is done for any byte. Ditto for explicit s/zero)/0)/, and for s/zero)/2)/ check if the instrumentation is removed. If that works properly, the patch is ok with the routine name change. Thanks. Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Don't expand string/memory builtins if ASan is enabled. 2014-10-28 9:27 ` Maxim Ostapenko 2014-10-28 9:35 ` Jakub Jelinek @ 2014-11-11 23:51 ` H.J. Lu 1 sibling, 0 replies; 10+ messages in thread From: H.J. Lu @ 2014-11-11 23:51 UTC (permalink / raw) To: Maxim Ostapenko Cc: Jakub Jelinek, Yury Gribov, GCC Patches, Slava Garbuzov, Maxim Ostapenko On Tue, Oct 28, 2014 at 1:21 AM, Maxim Ostapenko <m.ostapenko@partner.samsung.com> wrote: > >>> The code which identifies interceptors seems to be duplicated in >>> expand_builtin. What about factoring this out to some >>> is_asan_intercepted() >>> in asan.h? >> >> I agree. > > > Fixed this. >> >> >> BTW, when you handle a builtin, but set *intercepted_p to false, is the >> point just that you don't instrument e.g. scalar accesses to that memory >> region afterwards? I mean: >> memcpy (p, "abc", 3); >> p[0] = 'd'; >> you don't really have to instrument p[0] store, because supposedly memcpy >> call has already verified p[0] through p[2] is writable. >> >> Jakub >> > > Yes, in this case p[0] store is not instrumented we use either intercepted_p > builtin or not intercepted_p one. But access with some offset e.g. to p[1] > will be instrumented in both cases. I'll probably fix this next time. > > As Yury mentioned, I've also added a test case for pr63638 in this patch. > > Tested on x86_64-unknown-inux-gnu. > > -Maxim Your checkin: https://gcc.gnu.org/ml/gcc-cvs/2014-10/msg01046.html contains changes to c-c++-common/asan/strlen-overflow-1.c which isn't listed in ChangeLog. -- H.J. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-11-11 23:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <5441008A.2010706@partner.samsung.com>
2014-10-17 11:47 ` [PATCH] Don't expand string/memory builtins if ASan is enabled Maxim Ostapenko
2014-10-17 12:29 ` Jakub Jelinek
2014-10-17 13:03 ` Yury Gribov
2014-10-17 13:08 ` Jakub Jelinek
2014-10-21 13:38 ` [PATCHv2] " Maxim Ostapenko
2014-10-23 8:51 ` Yury Gribov
2014-10-27 14:37 ` Jakub Jelinek
2014-10-28 9:27 ` Maxim Ostapenko
2014-10-28 9:35 ` Jakub Jelinek
2014-11-11 23:51 ` H.J. Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).