| Message ID | 20121011163847.GE584@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2012-10-11 12:38 , Jakub Jelinek wrote: > - gimple_seq seq, stmts; > - tree shadow_type = size_in_bytes == 16 ? > - short_integer_type_node : char_type_node; > - tree shadow_ptr_type = build_pointer_type (shadow_type); > - tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode, > - /*unsignedp=*/true); > + tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16]; Add '? 1 : 0' in the array index expression. > /* Build > - (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ > + (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ Hm, I wonder if this is a documentation problem or we're generating bad runtime code. Wei, you tested the runtime and it was working with the GCC generated code, right? In any case, we can adjust the expression later. > + if (shadow_ptr_types[0] == NULL_TREE) > + { > + alias_set_type set = new_alias_set (); > + shadow_ptr_types[0] > + = build_distinct_type_copy (unsigned_char_type_node); > + TYPE_ALIAS_SET (shadow_ptr_types[0]) = set; > + shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]); > + shadow_ptr_types[1] > + = build_distinct_type_copy (short_unsigned_type_node); > + TYPE_ALIAS_SET (shadow_ptr_types[1]) = set; > + shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]); > + } Move this to an initialization function, please. OK with those changes. Diego.
On Thu, Oct 11, 2012 at 01:14:31PM -0400, Diego Novillo wrote: > On 2012-10-11 12:38 , Jakub Jelinek wrote: > > >- gimple_seq seq, stmts; > >- tree shadow_type = size_in_bytes == 16 ? > >- short_integer_type_node : char_type_node; > >- tree shadow_ptr_type = build_pointer_type (shadow_type); > >- tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode, > >- /*unsignedp=*/true); > >+ tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16]; > > Add '? 1 : 0' in the array index expression. Ok. > > /* Build > >- (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ > >+ (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ > > Hm, I wonder if this is a documentation problem or we're generating > bad runtime code. Wei, you tested the runtime and it was working > with the GCC generated code, right? The asan web pages document |, the old tree-asan.c emitted +, I've changed it to BIT_IOR_EXPR, but that resulted in worse assembly, and I believe at least for the current x86_64 and i686 address ranges and shadow offset values it actually doesn't matter. On x86_64 stack is like 0x7ffff6e00000, shifted down by 3 is still smaller than 1L << 44 that is ored or added to it. And the negative half of the address space is used by the kernel, nothing is mapped into it (besides vsyscall page) and neither | nor + of 1L << 44 to it would work well. On i386, | and + works the same for all addresses, as 0xffffffffU >> 3 is still smaller than 1 << 29. The reason why + generates better code on x86_64/i686 is that one can use e.g. movzbl (%r1, %r2), %r3 instead of orq %r2, %r1; movzb (%r1), %r3. > >+ if (shadow_ptr_types[0] == NULL_TREE) > >+ { > >+ alias_set_type set = new_alias_set (); > >+ shadow_ptr_types[0] > >+ = build_distinct_type_copy (unsigned_char_type_node); > >+ TYPE_ALIAS_SET (shadow_ptr_types[0]) = set; > >+ shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]); > >+ shadow_ptr_types[1] > >+ = build_distinct_type_copy (short_unsigned_type_node); > >+ TYPE_ALIAS_SET (shadow_ptr_types[1]) = set; > >+ shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]); > >+ } > > Move this to an initialization function, please. Okay. Jakub
On Thu, Oct 11, 2012 at 9:38 AM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > Building trees, then gimplifying it, is unnecessarily expensive. > This patch changes build_check_stmt to emit GIMPLE directly, and > a couple of small cleanups here and there. Also, I'm using a different > alias set for the shadow memory accesses, those obviously can't alias any > other accesses. > > Ok for asan? > > 2012-10-11 Jakub Jelinek <jakub@redhat.com> > > * Makefile.in (GTFILES): Add $(srcdir)/asan.c. > * asan.c (shadow_ptr_types): New variable. > (report_error_func): Change is_store argument to bool, don't append > newline to function name. > (PROB_VERY_UNLIKELY, PROB_ALWAYS): Define. > (build_check_stmt): Change is_store argument to bool. Emit GIMPLE > directly instead of creating trees and gimplifying them. Mark > the error reporting function as very unlikely. > (instrument_derefs): Change is_store argument to bool. Use > int_size_in_bytes to compute size_in_bytes, simplify size check. > Use build_fold_addr_expr instead of build_addr. > (transform_statements): Adjust instrument_derefs caller. > Use gimple_assign_single_p as stmt test. Don't look at MEM refs > in rhs2. > (asan_instrument): Don't push/pop gimplify context. > Initialize shadow_ptr_types if not yet initialized. > * asan.h (ASAN_SHADOW_SHIFT): Adjust comment. > > --- gcc/Makefile.in.jj 2012-10-11 16:09:02.000000000 +0200 > +++ gcc/Makefile.in 2012-10-11 16:51:59.666672099 +0200 > @@ -3681,6 +3681,7 @@ GTFILES = $(CPP_ID_DATA_H) $(srcdir)/inp > $(srcdir)/lto-streamer.h \ > $(srcdir)/target-globals.h \ > $(srcdir)/ipa-inline.h \ > + $(srcdir)/asan.c \ > @all_gtfiles@ > > # Compute the list of GT header files from the corresponding C sources, > --- gcc/asan.c.jj 2012-10-11 16:33:58.000000000 +0200 > +++ gcc/asan.c 2012-10-11 18:20:35.265675838 +0200 > @@ -79,18 +79,20 @@ along with GCC; see the file COPYING3. > to create redzones for stack and global object and poison them. > */ > > +static GTY(()) tree shadow_ptr_types[2]; > + > /* Construct a function tree for __asan_report_{load,store}{1,2,4,8,16}. > IS_STORE is either 1 (for a store) or 0 (for a load). > SIZE_IN_BYTES is one of 1, 2, 4, 8, 16. */ > > static tree > -report_error_func (int is_store, int size_in_bytes) > +report_error_func (bool is_store, int size_in_bytes) > { > tree fn_type; > tree def; > char name[100]; > > - sprintf (name, "__asan_report_%s%d\n", > + sprintf (name, "__asan_report_%s%d", > is_store ? "store" : "load", size_in_bytes); > fn_type = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE); > def = build_fn_decl (name, fn_type); > @@ -118,6 +120,9 @@ asan_init_func (void) > } > > > +#define PROB_VERY_UNLIKELY (REG_BR_PROB_BASE / 2000 - 1) > +#define PROB_ALWAYS (REG_BR_PROB_BASE) > + Does it belong here ? -- looks like they can be generally useful for others. > /* Instrument the memory access instruction BASE. > Insert new statements before ITER. > LOCATION is source code location. > @@ -127,21 +132,17 @@ asan_init_func (void) > static void > build_check_stmt (tree base, > gimple_stmt_iterator *iter, > - location_t location, int is_store, int size_in_bytes) > + location_t location, bool is_store, int size_in_bytes) > { > gimple_stmt_iterator gsi; > basic_block cond_bb, then_bb, join_bb; > edge e; > - tree cond, t, u; > - tree base_addr; > - tree shadow_value; > + tree t, base_addr, shadow; > gimple g; > - gimple_seq seq, stmts; > - tree shadow_type = size_in_bytes == 16 ? > - short_integer_type_node : char_type_node; > - tree shadow_ptr_type = build_pointer_type (shadow_type); > - tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode, > - /*unsignedp=*/true); > + tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16]; > + tree shadow_type = TREE_TYPE (shadow_ptr_type); > + tree uintptr_type > + = build_nonstandard_integer_type (TYPE_PRECISION (TREE_TYPE (base)), 1); > > /* We first need to split the current basic block, and start altering > the CFG. This allows us to insert the statements we're about to > @@ -166,14 +167,15 @@ build_check_stmt (tree base, > > /* Create the bb that contains the crash block. */ > then_bb = create_empty_bb (cond_bb); Missing frequency update for then_bb ? > - make_edge (cond_bb, then_bb, EDGE_TRUE_VALUE); > + e = make_edge (cond_bb, then_bb, EDGE_TRUE_VALUE); > + e->probability = PROB_VERY_UNLIKELY; > make_single_succ_edge (then_bb, join_bb, EDGE_FALLTHRU); > > /* Mark the pseudo-fallthrough edge from cond_bb to join_bb. */ > e = find_edge (cond_bb, join_bb); > e->flags = EDGE_FALSE_VALUE; > e->count = cond_bb->count; > - e->probability = REG_BR_PROB_BASE; > + e->probability = PROB_ALWAYS - PROB_VERY_UNLIKELY; > > /* Update dominance info. Note that bb_join's data was > updated by split_block. */ > @@ -183,75 +185,123 @@ build_check_stmt (tree base, > set_immediate_dominator (CDI_DOMINATORS, join_bb, cond_bb); > } > > - base_addr = create_tmp_reg (uintptr_type, "__asan_base_addr"); > + gsi = gsi_last_bb (cond_bb); > + g = gimple_build_assign_with_ops (TREE_CODE (base), > + make_ssa_name (TREE_TYPE (base), NULL), > + base, NULL_TREE); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > > - seq = NULL; > - t = fold_convert_loc (location, uintptr_type, > - unshare_expr (base)); > - t = force_gimple_operand (t, &stmts, false, NULL_TREE); > - gimple_seq_add_seq (&seq, stmts); > - g = gimple_build_assign (base_addr, t); > + g = gimple_build_assign_with_ops (NOP_EXPR, > + make_ssa_name (uintptr_type, NULL), > + gimple_assign_lhs (g), NULL_TREE); > gimple_set_location (g, location); > - gimple_seq_add_stmt (&seq, g); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + base_addr = gimple_assign_lhs (g); > Set base_addr name here? thanks, David > /* Build > - (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ > + (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ > > - t = build2 (RSHIFT_EXPR, uintptr_type, base_addr, > - build_int_cst (uintptr_type, ASAN_SHADOW_SHIFT)); > - t = build2 (PLUS_EXPR, uintptr_type, t, > - build_int_cst (uintptr_type, targetm.asan_shadow_offset ())); > - t = build1 (INDIRECT_REF, shadow_type, > - build1 (VIEW_CONVERT_EXPR, shadow_ptr_type, t)); > - t = force_gimple_operand (t, &stmts, false, NULL_TREE); > - gimple_seq_add_seq (&seq, stmts); > - shadow_value = create_tmp_reg (shadow_type, "__asan_shadow"); > - g = gimple_build_assign (shadow_value, t); > + t = build_int_cst (uintptr_type, ASAN_SHADOW_SHIFT); > + g = gimple_build_assign_with_ops (RSHIFT_EXPR, > + make_ssa_name (uintptr_type, NULL), > + base_addr, t); > gimple_set_location (g, location); > - gimple_seq_add_stmt (&seq, g); > - t = build2 (NE_EXPR, boolean_type_node, shadow_value, > - build_int_cst (shadow_type, 0)); > - if (size_in_bytes < 8) > - { > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > > - /* Slow path for 1-, 2- and 4- byte accesses. > - Build ((base_addr & 7) + (size_in_bytes - 1)) >= shadow_value. */ > - > - u = build2 (BIT_AND_EXPR, uintptr_type, > - base_addr, > - build_int_cst (uintptr_type, 7)); > - u = build1 (CONVERT_EXPR, shadow_type, u); > - u = build2 (PLUS_EXPR, shadow_type, u, > - build_int_cst (shadow_type, size_in_bytes - 1)); > - u = build2 (GE_EXPR, uintptr_type, u, shadow_value); > - } > - else > - u = build_int_cst (boolean_type_node, 1); > - t = build2 (TRUTH_AND_EXPR, boolean_type_node, t, u); > - t = force_gimple_operand (t, &stmts, false, NULL_TREE); > - gimple_seq_add_seq (&seq, stmts); > - cond = create_tmp_reg (boolean_type_node, "__asan_crash_cond"); > - g = gimple_build_assign (cond, t); > + t = build_int_cst (uintptr_type, targetm.asan_shadow_offset ()); > + g = gimple_build_assign_with_ops (PLUS_EXPR, > + make_ssa_name (uintptr_type, NULL), > + gimple_assign_lhs (g), t); > gimple_set_location (g, location); > - gimple_seq_add_stmt (&seq, g); > - g = gimple_build_cond (NE_EXPR, cond, boolean_false_node, NULL_TREE, > - NULL_TREE); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + > + g = gimple_build_assign_with_ops (NOP_EXPR, > + make_ssa_name (shadow_ptr_type, NULL), > + gimple_assign_lhs (g), NULL_TREE); > gimple_set_location (g, location); > - gimple_seq_add_stmt (&seq, g); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > > - /* Generate call to the run-time library (e.g. __asan_report_load8). */ > + t = build2 (MEM_REF, shadow_type, gimple_assign_lhs (g), > + build_int_cst (shadow_ptr_type, 0)); > + g = gimple_build_assign_with_ops (MEM_REF, > + make_ssa_name (shadow_type, NULL), > + t, NULL_TREE); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + shadow = gimple_assign_lhs (g); > > - gsi = gsi_last_bb (cond_bb); > - gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); > - seq = NULL; > - g = gimple_build_call (report_error_func (is_store, size_in_bytes), > - 1, base_addr); > - gimple_seq_add_stmt (&seq, g); > + if (size_in_bytes < 8) > + { > + /* Slow path for 1, 2 and 4 byte accesses. > + Test (shadow != 0) > + & ((base_addr & 7) + (size_in_bytes - 1)) >= shadow). */ > + g = gimple_build_assign_with_ops (NE_EXPR, > + make_ssa_name (boolean_type_node, > + NULL), > + shadow, > + build_int_cst (shadow_type, 0)); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + t = gimple_assign_lhs (g); > + > + g = gimple_build_assign_with_ops (BIT_AND_EXPR, > + make_ssa_name (uintptr_type, > + NULL), > + base_addr, > + build_int_cst (uintptr_type, 7)); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + > + g = gimple_build_assign_with_ops (NOP_EXPR, > + make_ssa_name (shadow_type, > + NULL), > + gimple_assign_lhs (g), NULL_TREE); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + > + if (size_in_bytes > 1) > + { > + g = gimple_build_assign_with_ops (PLUS_EXPR, > + make_ssa_name (shadow_type, > + NULL), > + gimple_assign_lhs (g), > + build_int_cst (shadow_type, > + size_in_bytes - 1)); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + } > + > + g = gimple_build_assign_with_ops (GE_EXPR, > + make_ssa_name (boolean_type_node, > + NULL), > + gimple_assign_lhs (g), > + shadow); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + > + g = gimple_build_assign_with_ops (BIT_AND_EXPR, > + make_ssa_name (boolean_type_node, > + NULL), > + t, gimple_assign_lhs (g)); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > + t = gimple_assign_lhs (g); > + } > + else > + t = shadow; > > - /* Insert the check code in the THEN block. */ > + g = gimple_build_cond (NE_EXPR, t, build_int_cst (TREE_TYPE (t), 0), > + NULL_TREE, NULL_TREE); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > > + /* Generate call to the run-time library (e.g. __asan_report_load8). */ > gsi = gsi_start_bb (then_bb); > - gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); > + g = gimple_build_call (report_error_func (is_store, size_in_bytes), > + 1, base_addr); > + gimple_set_location (g, location); > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > > *iter = gsi_start_bb (join_bb); > } > @@ -262,14 +312,12 @@ build_check_stmt (tree base, > > static void > instrument_derefs (gimple_stmt_iterator *iter, tree t, > - location_t location, int is_store) > + location_t location, bool is_store) > { > tree type, base; > - int size_in_bytes; > + HOST_WIDE_INT size_in_bytes; > > type = TREE_TYPE (t); > - if (type == error_mark_node) > - return; > switch (TREE_CODE (t)) > { > case ARRAY_REF: > @@ -280,25 +328,25 @@ instrument_derefs (gimple_stmt_iterator > default: > return; > }
Hi Diego, >> /* Build >> - (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). >> */ >> + (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). >> */ > > > Hm, I wonder if this is a documentation problem or we're generating bad > runtime code. Wei, you tested the runtime and it was working with the GCC > generated code, right? > > In any case, we can adjust the expression later. I only tested my smallcase and it worked. Because usually the redzone are not a very small areas (more than 4K), so I think it is possible the smallcase works even if the shadow addr calculation is incorrect and has small deviation. Thanks, Wei.
On Thu, Oct 11, 2012 at 10:31:58AM -0700, Xinliang David Li wrote: > > +#define PROB_VERY_UNLIKELY (REG_BR_PROB_BASE / 2000 - 1) > > +#define PROB_ALWAYS (REG_BR_PROB_BASE) > > + > > > Does it belong here ? -- looks like they can be generally useful for others. Perhaps, but then it would need to go on mainline first, and wait for a merge from the trunk to asan. So, IMHO better to put it in now this way and if mainline gets the macros moved from profile.c to a header, asan branch can be then adjusted. > > @@ -166,14 +167,15 @@ build_check_stmt (tree base, > > > > /* Create the bb that contains the crash block. */ > > then_bb = create_empty_bb (cond_bb); > > Missing frequency update for then_bb ? I don't see other places which create very unlikely edges doing that (e.g. trans-mem.c, omp-low.c, ...). Shall it be that then_bb->frequency = cond_bb->frequency * PROB_VERY_UNLIKELY / REG_BR_PROB_BASE; and join_bb->frequency -= then_bb->frequency; ? join_bb is clearly misnamed, as then_bb is noreturn, so there is no joining... > > gimple_set_location (g, location); > > - gimple_seq_add_stmt (&seq, g); > > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); > > + base_addr = gimple_assign_lhs (g); > > > > > Set base_addr name here? Why? I think nameless SSA_NAMEs are just fine for this. Jakub
On Thu, Oct 11, 2012 at 11:12 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Oct 11, 2012 at 10:31:58AM -0700, Xinliang David Li wrote: >> > +#define PROB_VERY_UNLIKELY (REG_BR_PROB_BASE / 2000 - 1) >> > +#define PROB_ALWAYS (REG_BR_PROB_BASE) >> > + >> >> >> Does it belong here ? -- looks like they can be generally useful for others. > > Perhaps, but then it would need to go on mainline first, and wait for a > merge from the trunk to asan. So, IMHO better to put it in now this way > and if mainline gets the macros moved from profile.c to a header, asan > branch can be then adjusted. reasonable. The existing definitions are in predict.c. > >> > @@ -166,14 +167,15 @@ build_check_stmt (tree base, >> > >> > /* Create the bb that contains the crash block. */ >> > then_bb = create_empty_bb (cond_bb); >> >> Missing frequency update for then_bb ? > > I don't see other places which create very unlikely edges doing > that (e.g. trans-mem.c, omp-low.c, ...). Shall it be that > then_bb->frequency = cond_bb->frequency * PROB_VERY_UNLIKELY > / REG_BR_PROB_BASE; > and join_bb->frequency -= then_bb->frequency; ? That looks good to me. > join_bb is clearly misnamed, as then_bb is noreturn, so there is no > joining... > >> > gimple_set_location (g, location); >> > - gimple_seq_add_stmt (&seq, g); >> > + gsi_insert_after (&gsi, g, GSI_NEW_STMT); >> > + base_addr = gimple_assign_lhs (g); >> > >> >> >> Set base_addr name here? > > Why? I think nameless SSA_NAMEs are just fine for this. It makes the GIMPLE dump slightly more readable. thanks, David > > Jakub
Hi, Here is the initial test results of gcc asan patch, and it shows us some missing features in gcc but existing in llvm. [1]. gcc regression test for gcc-asan passes. [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests written in google test and 24 failures in 28 for tests written in lit tests. gcc missing features: 1. gcc implementation doesn't support stack/global overflow check 1. gcc implementation doesn't support some attributes, such as __attribute__((no_address_safety_analysis)), which llvm does 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy for heap allocated memory or string, which llvm does 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm -asan-initialization-order 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit tests contain checks from -O0 to -O3, which makes gcc fail. 6. $HOME/llvm/trunk/projects/compiler-rt/lib/asan/scripts/asan_symbolize.py could generate valid source locations from virtual addresses for llvm binary, but fail to do that for gcc binary. example, llvm result #1 0x402694 in main heap-overflow.cc:23 .vs. gcc result: #1 0x402694 in main ??:0. Some FileCheck in llvm lit tests expect the valid source locations. Thanks, Wei. On Thu, Oct 11, 2012 at 10:31 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Oct 11, 2012 at 01:14:31PM -0400, Diego Novillo wrote: >> On 2012-10-11 12:38 , Jakub Jelinek wrote: >> >> >- gimple_seq seq, stmts; >> >- tree shadow_type = size_in_bytes == 16 ? >> >- short_integer_type_node : char_type_node; >> >- tree shadow_ptr_type = build_pointer_type (shadow_type); >> >- tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode, >> >- /*unsignedp=*/true); >> >+ tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16]; >> >> Add '? 1 : 0' in the array index expression. > > Ok. > >> > /* Build >> >- (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ >> >+ (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ >> >> Hm, I wonder if this is a documentation problem or we're generating >> bad runtime code. Wei, you tested the runtime and it was working >> with the GCC generated code, right? > > The asan web pages document |, the old tree-asan.c emitted +, I've changed > it to BIT_IOR_EXPR, but that resulted in worse assembly, and I believe at > least for the current x86_64 and i686 address ranges and shadow offset > values it actually doesn't matter. > On x86_64 stack is like 0x7ffff6e00000, shifted down by 3 is still smaller > than 1L << 44 that is ored or added to it. And the negative half of the > address space is used by the kernel, nothing is mapped into it (besides > vsyscall page) and neither | nor + of 1L << 44 to it would work well. > On i386, | and + works the same for all addresses, as 0xffffffffU >> 3 > is still smaller than 1 << 29. > The reason why + generates better code on x86_64/i686 is that one can use > e.g. movzbl (%r1, %r2), %r3 instead of orq %r2, %r1; movzb (%r1), %r3. > >> >+ if (shadow_ptr_types[0] == NULL_TREE) >> >+ { >> >+ alias_set_type set = new_alias_set (); >> >+ shadow_ptr_types[0] >> >+ = build_distinct_type_copy (unsigned_char_type_node); >> >+ TYPE_ALIAS_SET (shadow_ptr_types[0]) = set; >> >+ shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]); >> >+ shadow_ptr_types[1] >> >+ = build_distinct_type_copy (short_unsigned_type_node); >> >+ TYPE_ALIAS_SET (shadow_ptr_types[1]) = set; >> >+ shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]); >> >+ } >> >> Move this to an initialization function, please. > > Okay. > > Jakub
On Thu, Oct 11, 2012 at 04:19:18PM -0700, Wei Mi wrote: > Here is the initial test results of gcc asan patch, and it shows us > some missing features in gcc but existing in llvm. > [1]. gcc regression test for gcc-asan passes. > [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests > written in google test and 24 failures in 28 for tests written in lit > tests. > > gcc missing features: I think LLVM calls the option -faddress-sanitizer, perhaps we should rename -fasan to that too for some compatibility. For the varios knobs LLVM asan has we could add params to params.def if we want to support them. > 1. gcc implementation doesn't support stack/global overflow check Yeah, I think the stack check shouldn't be that hard and can hack it up, I'll perhaps leave the global vars stuff to Dodji or others if he has time. > 1. gcc implementation doesn't support some attributes, such as > __attribute__((no_address_safety_analysis)), which llvm does Yeah, shouldn't be hard. > 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does Testcase (for everything testcases would be useful, and of course the runtime library too)? Yeah, the code currently punts on those, the question is how to handle them. > 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy > for heap allocated memory or string, which llvm does That is easy (we can easily handle instrumenting lots of builtins). Just a big switch on DECL_FUNCTION_CODE, collecting src/dst addresses and length for each of them and adding the instrumentation for first and last bytes in the strings. > 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm > -asan-initialization-order I must say I don't like the -asan-blacklist option at all, IMHO it is much saner to just ask users to use attributes (or pragmas around whole headers). > 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit > tests contain checks from -O0 to -O3, which makes gcc fail. That is because of the place where the instrumentation is scheduled right now in the pass queue. We can easily add a pass_asan_O0 that will run say close to pass_lower_complex_O0. Jakub
On Thu, Oct 11, 2012 at 7:31 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Oct 11, 2012 at 01:14:31PM -0400, Diego Novillo wrote: >> On 2012-10-11 12:38 , Jakub Jelinek wrote: >> >> >- gimple_seq seq, stmts; >> >- tree shadow_type = size_in_bytes == 16 ? >> >- short_integer_type_node : char_type_node; >> >- tree shadow_ptr_type = build_pointer_type (shadow_type); >> >- tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode, >> >- /*unsignedp=*/true); >> >+ tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16]; >> >> Add '? 1 : 0' in the array index expression. > > Ok. > >> > /* Build >> >- (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ >> >+ (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ >> >> Hm, I wonder if this is a documentation problem or we're generating >> bad runtime code. Wei, you tested the runtime and it was working >> with the GCC generated code, right? > > The asan web pages document |, the old tree-asan.c emitted +, I've changed > it to BIT_IOR_EXPR, but that resulted in worse assembly, and I believe at > least for the current x86_64 and i686 address ranges and shadow offset > values it actually doesn't matter. > On x86_64 stack is like 0x7ffff6e00000, shifted down by 3 is still smaller > than 1L << 44 that is ored or added to it. And the negative half of the > address space is used by the kernel, nothing is mapped into it (besides > vsyscall page) and neither | nor + of 1L << 44 to it would work well. > On i386, | and + works the same for all addresses, as 0xffffffffU >> 3 > is still smaller than 1 << 29. > The reason why + generates better code on x86_64/i686 is that one can use > e.g. movzbl (%r1, %r2), %r3 instead of orq %r2, %r1; movzb (%r1), %r3. > >> >+ if (shadow_ptr_types[0] == NULL_TREE) >> >+ { >> >+ alias_set_type set = new_alias_set (); >> >+ shadow_ptr_types[0] >> >+ = build_distinct_type_copy (unsigned_char_type_node); >> >+ TYPE_ALIAS_SET (shadow_ptr_types[0]) = set; >> >+ shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]); >> >+ shadow_ptr_types[1] >> >+ = build_distinct_type_copy (short_unsigned_type_node); >> >+ TYPE_ALIAS_SET (shadow_ptr_types[1]) = set; >> >+ shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]); >> >+ } >> >> Move this to an initialization function, please. > > Okay. That doesn't work with LTO btw, so make sure you instrument during LTRANS. Richard. > Jakub
On 2012-10-11 19:19 , Wei Mi wrote:> Hi, > > Here is the initial test results of gcc asan patch, and it shows us > some missing features in gcc but existing in llvm. > [1]. gcc regression test for gcc-asan passes. > [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests > written in google test and 24 failures in 28 for tests written in lit > tests. > > gcc missing features: > 1. gcc implementation doesn't support stack/global overflow check > 1. gcc implementation doesn't support some attributes, such as > __attribute__((no_address_safety_analysis)), which llvm does > 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does > 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy > for heap allocated memory or string, which llvm does > 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm > -asan-initialization-order > 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit > tests contain checks from -O0 to -O3, which makes gcc fail. > 6. $HOME/llvm/trunk/projects/compiler-rt/lib/asan/scripts/asan_symbolize.py > could generate valid source locations from virtual addresses for llvm > binary, but fail to do that for gcc binary. example, llvm result #1 > 0x402694 in main heap-overflow.cc:23 .vs. gcc result: #1 0x402694 in > main ??:0. Some FileCheck in llvm lit tests expect the valid source > locations. Thanks for the analysis Wei. We need to decide what's the best way of dealing with the runtime. My inclination is to treat it the same way we treat gmp, mpfr, et al: 1- If libasan/ exists, we enable asan as a feature at configure time. 2- libasan/ is always a pristine copy of the LLVM repository. We could have tarballs in ftp://gcc.gnu.org/pub/gcc/infrastructure/ which are brought in by contrib/download_prerequisites. 3- To run ASAN's testsuite, I propose a simple wrapper script that executes it using the just-built gcc. I don't think it's worth the pain to convert the testsuite into DejaGNU. Comments? Thanks. Diego.
Diego Novillo <dnovillo@google.com> writes: > 3- To run ASAN's testsuite, I propose a simple wrapper script that executes > it using the just-built gcc. I don't think it's worth the pain to convert > the testsuite into DejaGNU. If the testsuite is not converted (which can be ugly for maintainers since it needs separate mechanisms for everything Dejagnu already deals with, like timeouts, target-specific skipping), at the very least make sure that the make check output is in the same format produced by Dejagnu so it's picked up make make mail-report.log, otherwise failures are almost guaranteed to be overlooked. Rainer
On 2012-10-12 11:01 , Rainer Orth wrote: > Diego Novillo <dnovillo@google.com> writes: > >> 3- To run ASAN's testsuite, I propose a simple wrapper script that executes >> it using the just-built gcc. I don't think it's worth the pain to convert >> the testsuite into DejaGNU. > > If the testsuite is not converted (which can be ugly for maintainers > since it needs separate mechanisms for everything Dejagnu already deals > with, like timeouts, target-specific skipping), at the very least make The thing is that if we convert the testsuite, then taking pristine tarballs is out of the question and we end up with a library like boehm-gc or zlib. I would prefer to have something completely separate that we can just drop in. > sure that the make check output is in the same format produced by > Dejagnu so it's picked up make make mail-report.log, otherwise failures > are almost guaranteed to be overlooked. Sure. That's a good idea. Diego.
On Fri, Oct 12, 2012 at 12:15 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Oct 11, 2012 at 04:19:18PM -0700, Wei Mi wrote: >> Here is the initial test results of gcc asan patch, and it shows us >> some missing features in gcc but existing in llvm. >> [1]. gcc regression test for gcc-asan passes. >> [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests >> written in google test and 24 failures in 28 for tests written in lit >> tests. >> >> gcc missing features: > > I think LLVM calls the option -faddress-sanitizer, perhaps we should rename > -fasan to that too for some compatibility. For the varios knobs LLVM asan > has we could add params to params.def if we want to support them. > >> 1. gcc implementation doesn't support stack/global overflow check > > Yeah, I think the stack check shouldn't be that hard and can hack it up, > I'll perhaps leave the global vars stuff to Dodji or others if he has time. Since the stack part is relative self contained, might it be better for Wei (he is new) to tackle it with guidance from you? Global handling needs people more experienced with varasm stuff. > >> 1. gcc implementation doesn't support some attributes, such as >> __attribute__((no_address_safety_analysis)), which llvm does > > Yeah, shouldn't be hard. > yes -- that is simple. >> 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does > > Testcase (for everything testcases would be useful, and of course the > runtime library too)? Yeah, the code currently punts on those, the question > is how to handle them. The code should sythensize the smallest containing byte/word/doubleword/qword of the bitfield and then compute the shadow address. > >> 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy >> for heap allocated memory or string, which llvm does > > That is easy (we can easily handle instrumenting lots of builtins). > Just a big switch on DECL_FUNCTION_CODE, collecting src/dst addresses and > length for each of them and adding the instrumentation for first and last > bytes in the strings. Yes. > >> 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm >> -asan-initialization-order > > I must say I don't like the -asan-blacklist option at all, IMHO it is much > saner to just ask users to use attributes (or pragmas around whole headers). But the option is easy to have too. > >> 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit >> tests contain checks from -O0 to -O3, which makes gcc fail. > > That is because of the place where the instrumentation is scheduled right > now in the pass queue. We can easily add a pass_asan_O0 that will run say > close to pass_lower_complex_O0. Right. thanks, David > > Jakub
On Fri, Oct 12, 2012 at 09:30:33AM -0700, Xinliang David Li wrote: > > Yeah, I think the stack check shouldn't be that hard and can hack it up, > > I'll perhaps leave the global vars stuff to Dodji or others if he has time. > > Since the stack part is relative self contained, might it be better > for Wei (he is new) to tackle it with guidance from you? Too late, already posted (just is missing actually pushing the cleanup sequence in the right spots). Jakub
On Fri, Oct 12, 2012 at 9:32 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Oct 12, 2012 at 09:30:33AM -0700, Xinliang David Li wrote: >> > Yeah, I think the stack check shouldn't be that hard and can hack it up, >> > I'll perhaps leave the global vars stuff to Dodji or others if he has time. >> >> Since the stack part is relative self contained, might it be better >> for Wei (he is new) to tackle it with guidance from you? > > Too late, already posted (just is missing actually pushing the cleanup > sequence in the right spots). that is fast :) thanks, David > > Jakub
On Fri, Oct 12, 2012 at 12:26 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Oct 11, 2012 at 10:05:54PM -0700, Xinliang David Li wrote: >> Was: >> Type global; >> Now: >> struct { // at least 32-byte aligned >> Type orig; >> char redzone[32 + required_for_alignment]; >> } global; >> >> All uses of global in the module should be replaced with global.orig >> >> (Note that the actual implementation does not need to change global >> type nor references -- just adding the padding space) > > Yeah, IMHO it is easier to hook into varasm.c and just force the higher > alignment and append padding after globals to be instrumented. > The vars can be exported, we don't want to affect their .size, etc. > I think so too. Changing types and IR references will be too heavy weight for it. >> 4) Implement stack redzones: >> >> Collect all local variables that are still on stack (not on virtual registers). >> Create a new local variable of size enough to hold all current locals >> and their redzones. >> Replace uses of locals with the new variable (Note that this does not >> need to happen in IR, only add guard space in stack layout) >> Poison the stack at function entry (one or two stores per local variable). >> Unpoison the stack at function exits (maybe including all cleanup landing pads?) >> Was: >> int foo() { >> Type1 a; >> Type2 b; >> <code that uses 'a' and 'b'; they are not on virtual registers> >> return; >> } >> Now: >> int foo() { >> struct { // 32-byte aligned >> char redzone0[32]; >> Type1 a; >> char redzone1[32 + required_alignment_a]; >> Type2 b; >> char redzone2[32 + required_alignment_b]; >> } locals; >> >> int *shadow_base = ((&locals.redzone0)>>3)+kAsanOffset; >> shadow_base[0] = 0xffffffff; // poison redzone0 >> // poison the rest >> >> <code that uses 'locals.a' and 'locals.b'> >> >> shadow_base[0] = 0; // unpoison redzone0 >> // unpoison the rest >> return; >> } > > We don't need to do this at the GIMPLE level, instead we can hook into > cfgexpand.c var layout code. Yes. > BTW, I wonder if for ASAN_SHADDOW_SHIFT 3 > we really need to force 32-byte alignment for the stack vars (as opposed > to use the highest alignment among the protected vars). 32-byte alignment > is fairly expensive, and if none of the vars actually need 32-byte > alignment, I'd hope that 16-byte alignment should be enough. The redzones > would still be 32-byte etc. > I wonder about the 32 byte alignment requirement too. thanks, David By doing this during expand we can keep say -fstack-protector to do its job > too. > LLVM seems to emit 0xf1, 0xf2, 0xf3 etc. bytes into shadow memory for > various parts of the padding (left, middle, right). > > Jakub
On Fri, Oct 12, 2012 at 11:51:22AM -0400, Diego Novillo wrote: > On 2012-10-12 11:01 , Rainer Orth wrote: > >Diego Novillo <dnovillo@google.com> writes: > > > >>3- To run ASAN's testsuite, I propose a simple wrapper script that executes > >>it using the just-built gcc. I don't think it's worth the pain to convert > >>the testsuite into DejaGNU. > > > >If the testsuite is not converted (which can be ugly for maintainers > >since it needs separate mechanisms for everything Dejagnu already deals > >with, like timeouts, target-specific skipping), at the very least make > > The thing is that if we convert the testsuite, then taking pristine > tarballs is out of the question and we end up with a library like > boehm-gc or zlib. I would prefer to have something completely > separate that we can just drop in. I don't see how can their testcase be used if not converted to Dejagnu though, most of their testcases are full of LLVM testcase markup (// RUN, // CHECK*, etc.). So, if we import the library unmodified, we'll need to setup some directory under gcc/testsuite/ for asan tests and arrange for *.exp to find the libraries. Jakub
On 2012-10-12 12:40 , Jakub Jelinek wrote: > On Fri, Oct 12, 2012 at 11:51:22AM -0400, Diego Novillo wrote: >> On 2012-10-12 11:01 , Rainer Orth wrote: >>> Diego Novillo <dnovillo@google.com> writes: >>> >>>> 3- To run ASAN's testsuite, I propose a simple wrapper script that executes >>>> it using the just-built gcc. I don't think it's worth the pain to convert >>>> the testsuite into DejaGNU. >>> >>> If the testsuite is not converted (which can be ugly for maintainers >>> since it needs separate mechanisms for everything Dejagnu already deals >>> with, like timeouts, target-specific skipping), at the very least make >> >> The thing is that if we convert the testsuite, then taking pristine >> tarballs is out of the question and we end up with a library like >> boehm-gc or zlib. I would prefer to have something completely >> separate that we can just drop in. > > I don't see how can their testcase be used if not converted to Dejagnu > though, most of their testcases are full of LLVM testcase markup > (// RUN, // CHECK*, etc.). So, if we import the library unmodified, > we'll need to setup some directory under gcc/testsuite/ for asan tests > and arrange for *.exp to find the libraries. Hm, perhaps that would be the way then. Have the testsuite in gcc/testsuite/asan and only run it if ASAN was configured. We could adapt the basic tests from libasan and over time add more into gcc/testsuite/asan. Would that work? Diego.
On Fri, Oct 12, 2012 at 9:40 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > I don't see how can their testcase be used if not converted to Dejagnu > though, most of their testcases are full of LLVM testcase markup > (// RUN, // CHECK*, etc.). So, if we import the library unmodified, > we'll need to setup some directory under gcc/testsuite/ for asan tests > and arrange for *.exp to find the libraries. Yes, that's more or less what the Go testsuite is like. I just have a complicated gcc/testsuite/go.test/go-test.exp that does the right thing to handle the Go test cases as DejaGNU test cases. Ian
Ian Lance Taylor <iant@google.com> writes: > On Fri, Oct 12, 2012 at 9:40 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> >> I don't see how can their testcase be used if not converted to Dejagnu >> though, most of their testcases are full of LLVM testcase markup >> (// RUN, // CHECK*, etc.). So, if we import the library unmodified, >> we'll need to setup some directory under gcc/testsuite/ for asan tests >> and arrange for *.exp to find the libraries. > > Yes, that's more or less what the Go testsuite is like. I just have a > complicated gcc/testsuite/go.test/go-test.exp that does the right > thing to handle the Go test cases as DejaGNU test cases. I'd prefer if LLVM would accept the (sometimes more expressive and, to GCC maintainers, well-known) Dejagnu annotations in addition to their own ones, so the .exp files could live in the gcc tree only, while the testcases themselves would be imported from upstream as is. The same holds for gcc/testsuite/go.*, btw. Rainer
On Mon, Oct 15, 2012 at 7:31 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > Ian Lance Taylor <iant@google.com> writes: > >> On Fri, Oct 12, 2012 at 9:40 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> >>> I don't see how can their testcase be used if not converted to Dejagnu >>> though, most of their testcases are full of LLVM testcase markup >>> (// RUN, // CHECK*, etc.). So, if we import the library unmodified, >>> we'll need to setup some directory under gcc/testsuite/ for asan tests >>> and arrange for *.exp to find the libraries. >> >> Yes, that's more or less what the Go testsuite is like. I just have a >> complicated gcc/testsuite/go.test/go-test.exp that does the right >> thing to handle the Go test cases as DejaGNU test cases. > > I'd prefer if LLVM would accept the (sometimes more expressive and, to > GCC maintainers, well-known) Dejagnu annotations in addition to their > own ones, so the .exp files could live in the gcc tree only, while the > testcases themselves would be imported from upstream as is. > > The same holds for gcc/testsuite/go.*, btw. In my opinion, supporting the full range of GCC testsuite annotations means imposing a lot of mechanism that the Go testsuite does not require. It would complicate the Go testsuite for no benefit. Anybody who can understand the GCC testsuite annotations can understand the much simpler Go testsuite annotations. Ian
On Mon, Oct 15, 2012 at 11:55 AM, Ian Lance Taylor <iant@google.com> wrote: > In my opinion, supporting the full range of GCC testsuite annotations > means imposing a lot of mechanism that the Go testsuite does not > require. It would complicate the Go testsuite for no benefit. > Anybody who can understand the GCC testsuite annotations can > understand the much simpler Go testsuite annotations. Agreed. The fact that we have to suffer DejaGNU does not gives the right to make other projects miserable. Diego.
Another error checking feature is to poison stack vars on entry and
exit of the lexical scope to catch uninit variable reference and out
of scope references:
S* sp;
{
S s;
sp = &s;
}
.. *sp ...
This is relatively easy to do in gcc thanks to the clobber statement.
In Clang/LLVM, it is in the wishlist:
http://code.google.com/p/address-sanitizer/issues/detail?id=83
Might be good consider this feature.
David
On Fri, Oct 12, 2012 at 9:30 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Oct 12, 2012 at 12:15 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Thu, Oct 11, 2012 at 04:19:18PM -0700, Wei Mi wrote:
>>> Here is the initial test results of gcc asan patch, and it shows us
>>> some missing features in gcc but existing in llvm.
>>> [1]. gcc regression test for gcc-asan passes.
>>> [2]. llvm regression tests for gcc-asan: 18 failures in 123 for tests
>>> written in google test and 24 failures in 28 for tests written in lit
>>> tests.
>>>
>>> gcc missing features:
>>
>> I think LLVM calls the option -faddress-sanitizer, perhaps we should rename
>> -fasan to that too for some compatibility. For the varios knobs LLVM asan
>> has we could add params to params.def if we want to support them.
>>
>>> 1. gcc implementation doesn't support stack/global overflow check
>>
>> Yeah, I think the stack check shouldn't be that hard and can hack it up,
>> I'll perhaps leave the global vars stuff to Dodji or others if he has time.
>
> Since the stack part is relative self contained, might it be better
> for Wei (he is new) to tackle it with guidance from you?
>
> Global handling needs people more experienced with varasm stuff.
>
>>
>>> 1. gcc implementation doesn't support some attributes, such as
>>> __attribute__((no_address_safety_analysis)), which llvm does
>>
>> Yeah, shouldn't be hard.
>>
>
> yes -- that is simple.
>
>
>>> 2. gcc doesn't detect out-of-bound bitfield access of heap, which llvm does
>>
>> Testcase (for everything testcases would be useful, and of course the
>> runtime library too)? Yeah, the code currently punts on those, the question
>> is how to handle them.
>
> The code should sythensize the smallest containing
> byte/word/doubleword/qword of the bitfield and then compute the shadow
> address.
>
>>
>>> 3. gcc doesn't detect out-of-bound memset, memcopy, strcat and strcpy
>>> for heap allocated memory or string, which llvm does
>>
>> That is easy (we can easily handle instrumenting lots of builtins).
>> Just a big switch on DECL_FUNCTION_CODE, collecting src/dst addresses and
>> length for each of them and adding the instrumentation for first and last
>> bytes in the strings.
>
> Yes.
>
>>
>>> 4. gcc doesn't contain similar options: -mllvm -asan-blacklist, -mllvm
>>> -asan-initialization-order
>>
>> I must say I don't like the -asan-blacklist option at all, IMHO it is much
>> saner to just ask users to use attributes (or pragmas around whole headers).
>
> But the option is easy to have too.
>
>>
>>> 5. gcc -fasan doesn't take effect at -O0, but llvm does. Most lit
>>> tests contain checks from -O0 to -O3, which makes gcc fail.
>>
>> That is because of the place where the instrumentation is scheduled right
>> now in the pass queue. We can easily add a pass_asan_O0 that will run say
>> close to pass_lower_complex_O0.
>
> Right.
>
> thanks,
>
> David
>>
>> Jakub
On Mon, Oct 15, 2012 at 10:48:13PM -0700, Xinliang David Li wrote: > Another error checking feature is to poison stack vars on entry and > exit of the lexical scope to catch uninit variable reference and out > of scope references: > > S* sp; > { > S s; > sp = &s; > } > .. *sp ... > > This is relatively easy to do in gcc thanks to the clobber statement. > In Clang/LLVM, it is in the wishlist: > http://code.google.com/p/address-sanitizer/issues/detail?id=83 That is not easy at all unfortunately, CLOBBER isn't sufficient for that. You have the points where the variable looses value, but there aren't similar markup statement where it gets into scope again. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54770#c3 Jakub
Diego Novillo <dnovillo@google.com> writes: > On Mon, Oct 15, 2012 at 11:55 AM, Ian Lance Taylor <iant@google.com> wrote: > >> In my opinion, supporting the full range of GCC testsuite annotations >> means imposing a lot of mechanism that the Go testsuite does not >> require. It would complicate the Go testsuite for no benefit. >> Anybody who can understand the GCC testsuite annotations can >> understand the much simpler Go testsuite annotations. > > Agreed. The fact that we have to suffer DejaGNU does not gives the > right to make other projects miserable. But importing different upstream testsuites with different annotation systems allows them to make GCC maintainer's lives miserable? If this trend continues, maintainers need to cope with several different annoation systems with different capabilites instead of a single one, some of them lacking necessary features which are already present in DejaGnu (which leads to handling stuff that's just a simple annotation in DejaGnu in the testsuite drivers instead). I'm not asking upstreams to deal with DejaGnu themselves, just to accept that the dg annotations live in their repos. Rainer
On 2012-10-16 07:05 , Rainer Orth wrote: > Diego Novillo <dnovillo@google.com> writes: > >> On Mon, Oct 15, 2012 at 11:55 AM, Ian Lance Taylor <iant@google.com> wrote: >> >>> In my opinion, supporting the full range of GCC testsuite annotations >>> means imposing a lot of mechanism that the Go testsuite does not >>> require. It would complicate the Go testsuite for no benefit. >>> Anybody who can understand the GCC testsuite annotations can >>> understand the much simpler Go testsuite annotations. >> >> Agreed. The fact that we have to suffer DejaGNU does not gives the >> right to make other projects miserable. > > But importing different upstream testsuites with different annotation > systems allows them to make GCC maintainer's lives miserable? Yes, absolutely. We have to adapt to upstream's conventions. If that means putting translation layers, the onus is on us. Alternately, we could import libasan the same way we import things like zlib or boehm-gc and then re-write the testsuite. That makes it harder to import newer versions, however. Finally, we could simply duplicate libasan's testsuite inside gcc/testsuite/asan and add our own tests as well. This may be the more practical choice. Diego.
On Tue, Oct 16, 2012 at 4:05 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > > But importing different upstream testsuites with different annotation > systems allows them to make GCC maintainer's lives miserable? If this > trend continues, maintainers need to cope with several different > annoation systems with different capabilites instead of a single one, > some of them lacking necessary features which are already present in > DejaGnu (which leads to handling stuff that's just a simple annotation > in DejaGnu in the testsuite drivers instead). Yes, that is true. But when you say "if this trend continues" you are making a slippery slope argument that I don't think applies. To date, the trend consists of a single example. We are discussing adding a second example, and we may decide against it. There are no current prospects of a third example. > I'm not asking upstreams > to deal with DejaGnu themselves, just to accept that the dg annotations > live in their repos. Where it would be untested and unmaintained. Do you think we would be happy adding additional annotations to our testsuite for the benefit of some other project? Ian
On Mon, Oct 15, 2012 at 11:12 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Oct 15, 2012 at 10:48:13PM -0700, Xinliang David Li wrote: >> Another error checking feature is to poison stack vars on entry and >> exit of the lexical scope to catch uninit variable reference and out >> of scope references: >> >> S* sp; >> { >> S s; >> sp = &s; >> } >> .. *sp ... >> >> This is relatively easy to do in gcc thanks to the clobber statement. >> In Clang/LLVM, it is in the wishlist: >> http://code.google.com/p/address-sanitizer/issues/detail?id=83 > > That is not easy at all unfortunately, CLOBBER isn't sufficient for that. > You have the points where the variable looses value, but there aren't > similar markup statement where it gets into scope again. > See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54770#c3 I don't get it. Clobber marks the end of lifetime of a variable so it is safe to emit code to really clobber its value -- otherwise how would clobber based slot sharing work? David > > Jakub
On Tue, Oct 16, 2012 at 09:06:22AM -0700, Xinliang David Li wrote: > I don't get it. Clobber marks the end of lifetime of a variable so it > is safe to emit code to really clobber its value -- otherwise how > would clobber based slot sharing work? If you at CLOBBER protect the var in the shadow mem as unaccessible, the thing is where you make it accessible again. Consider: int i; for (i = 0; i < 2; i++) { int tmp; bar (); baz (&tmp); bar (); } where the first bar () can't possibly access tmp, but the second bar () can. Now we unroll it and have: bar (); baz (&tmp); bar (); tmp ={v} {CLOBBER}; bar (); baz (&tmp); bar (); tmp ={v} {CLOBBER}; So, if you want to *mem_to_shadow (&tmp) = 0xff; at {CLOBBER} time, where and how you would insert *mem_to_shadow (&tmp) = 0; again? In the above code it would need to go before the second baz (&tmp);, the question is how you'd find that out... Jakub
The way to do this is not to use the shadow memory, but to clobber directly the variable itself with specified byte pattern -- though error diagnostics won't be as nice. To use shadow memory, the scope start marker is also needed. David On Tue, Oct 16, 2012 at 9:12 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Oct 16, 2012 at 09:06:22AM -0700, Xinliang David Li wrote: >> I don't get it. Clobber marks the end of lifetime of a variable so it >> is safe to emit code to really clobber its value -- otherwise how >> would clobber based slot sharing work? > > If you at CLOBBER protect the var in the shadow mem as unaccessible, the > thing is where you make it accessible again. > Consider: > > int i; > for (i = 0; i < 2; i++) > { > int tmp; > bar (); > baz (&tmp); > bar (); > } > > where the first bar () can't possibly access tmp, but the second bar () can. > Now we unroll it and have: > bar (); > baz (&tmp); > bar (); > tmp ={v} {CLOBBER}; > bar (); > baz (&tmp); > bar (); > tmp ={v} {CLOBBER}; > So, if you want to *mem_to_shadow (&tmp) = 0xff; at {CLOBBER} time, where > and how you would insert *mem_to_shadow (&tmp) = 0; again? In the above > code it would need to go before the second baz (&tmp);, the question is > how you'd find that out... > > Jakub
On Tue, Oct 16, 2012 at 09:48:28AM -0700, Xinliang David Li wrote: > The way to do this is not to use the shadow memory, but to clobber > directly the variable itself with specified byte pattern -- though > error diagnostics won't be as nice. Clobbering the memory directly is definitely easy, but doesn't have anything to do how shadow memory poisioning is done for stack vars. Guess should be done depending on some special option, not unconditionally implied by -fasan though. Jakub
On Tue, Oct 16, 2012 at 10:40 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Oct 16, 2012 at 09:48:28AM -0700, Xinliang David Li wrote: >> The way to do this is not to use the shadow memory, but to clobber >> directly the variable itself with specified byte pattern -- though >> error diagnostics won't be as nice. > > Clobbering the memory directly is definitely easy, but doesn't have anything > to do how shadow memory poisioning is done for stack vars. > Guess should be done depending on some special option, not unconditionally > implied by -fasan though. yes -- agree. Longer term this functionality should be using the shadow memory scheme though. David > > Jakub
> Yes, that is true. But when you say "if this trend continues" you are > making a slippery slope argument that I don't think applies. To date, > the trend consists of a single example. We are discussing adding a > second example, and we may decide against it. There are no current > prospects of a third example. You can hardly counter a slippery slope argument by saying "we're adding one, but, for the time being, we won't add more" since that's precisely what is denounced: you will say it for every new addition and this will never end. IMHO we need to decide what to do in this case once for all.
On Tue, Oct 16, 2012 at 2:33 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Yes, that is true. But when you say "if this trend continues" you are >> making a slippery slope argument that I don't think applies. To date, >> the trend consists of a single example. We are discussing adding a >> second example, and we may decide against it. There are no current >> prospects of a third example. > > You can hardly counter a slippery slope argument by saying "we're adding one, > but, for the time being, we won't add more" since that's precisely what is > denounced: you will say it for every new addition and this will never end. The slippery slope argument is that this will become easier and easier over time, and that the earlier examples will serve to pave the way for the later examples. I don't see any reason to assume that is true. There is no slope here. > IMHO we need to decide what to do in this case once for all. That is fine with me as long as we acknowledge that the upstream sources don't care about GCC and will think it is absurd that they should modify their code to carry untested and unmaintained GCC-specific annotations. It would be one thing if the GCC-specific annotations were clearly better, but in fact I would say that they are clearly worse. Ian
On 2012-10-16 18:50 , Ian Lance Taylor wrote: > That is fine with me as long as we acknowledge that the upstream > sources don't care about GCC and will think it is absurd that they > should modify their code to carry untested and unmaintained > GCC-specific annotations. It would be one thing if the GCC-specific > annotations were clearly better, but in fact I would say that they are > clearly worse. Precisely. Eric, Rainer, what do you think of the other two options I outlined in my earlier message? 1- Copy the upstream testsuite into gcc/testsuite/asan. This gives us the flexibility of adding new tests as the GCC implementation matures. 2- Deal with libasan as we deal with zlib/boehm-gc. I prefer option #1, personally. Diego.
> Eric, Rainer, what do you think of the other two options I outlined in > my earlier message? > > 1- Copy the upstream testsuite into gcc/testsuite/asan. This gives us > the flexibility of adding new tests as the GCC implementation matures. > > 2- Deal with libasan as we deal with zlib/boehm-gc. > > I prefer option #1, personally. I feel a bit umconfortable saying this given the gcc/testsuite/ada/acats precedent, but I don't think we should import foreing testsuites into gcc/testsuite, unless they are adapted to our harness. I don't think we want to run again into the issues we had with acats (Rainer puts a lot of efforts to clean up the mess here).
--- gcc/Makefile.in.jj 2012-10-11 16:09:02.000000000 +0200 +++ gcc/Makefile.in 2012-10-11 16:51:59.666672099 +0200 @@ -3681,6 +3681,7 @@ GTFILES = $(CPP_ID_DATA_H) $(srcdir)/inp $(srcdir)/lto-streamer.h \ $(srcdir)/target-globals.h \ $(srcdir)/ipa-inline.h \ + $(srcdir)/asan.c \ @all_gtfiles@ # Compute the list of GT header files from the corresponding C sources, --- gcc/asan.c.jj 2012-10-11 16:33:58.000000000 +0200 +++ gcc/asan.c 2012-10-11 18:20:35.265675838 +0200 @@ -79,18 +79,20 @@ along with GCC; see the file COPYING3. to create redzones for stack and global object and poison them. */ +static GTY(()) tree shadow_ptr_types[2]; + /* Construct a function tree for __asan_report_{load,store}{1,2,4,8,16}. IS_STORE is either 1 (for a store) or 0 (for a load). SIZE_IN_BYTES is one of 1, 2, 4, 8, 16. */ static tree -report_error_func (int is_store, int size_in_bytes) +report_error_func (bool is_store, int size_in_bytes) { tree fn_type; tree def; char name[100]; - sprintf (name, "__asan_report_%s%d\n", + sprintf (name, "__asan_report_%s%d", is_store ? "store" : "load", size_in_bytes); fn_type = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE); def = build_fn_decl (name, fn_type); @@ -118,6 +120,9 @@ asan_init_func (void) } +#define PROB_VERY_UNLIKELY (REG_BR_PROB_BASE / 2000 - 1) +#define PROB_ALWAYS (REG_BR_PROB_BASE) + /* Instrument the memory access instruction BASE. Insert new statements before ITER. LOCATION is source code location. @@ -127,21 +132,17 @@ asan_init_func (void) static void build_check_stmt (tree base, gimple_stmt_iterator *iter, - location_t location, int is_store, int size_in_bytes) + location_t location, bool is_store, int size_in_bytes) { gimple_stmt_iterator gsi; basic_block cond_bb, then_bb, join_bb; edge e; - tree cond, t, u; - tree base_addr; - tree shadow_value; + tree t, base_addr, shadow; gimple g; - gimple_seq seq, stmts; - tree shadow_type = size_in_bytes == 16 ? - short_integer_type_node : char_type_node; - tree shadow_ptr_type = build_pointer_type (shadow_type); - tree uintptr_type = lang_hooks.types.type_for_mode (ptr_mode, - /*unsignedp=*/true); + tree shadow_ptr_type = shadow_ptr_types[size_in_bytes == 16]; + tree shadow_type = TREE_TYPE (shadow_ptr_type); + tree uintptr_type + = build_nonstandard_integer_type (TYPE_PRECISION (TREE_TYPE (base)), 1); /* We first need to split the current basic block, and start altering the CFG. This allows us to insert the statements we're about to @@ -166,14 +167,15 @@ build_check_stmt (tree base, /* Create the bb that contains the crash block. */ then_bb = create_empty_bb (cond_bb); - make_edge (cond_bb, then_bb, EDGE_TRUE_VALUE); + e = make_edge (cond_bb, then_bb, EDGE_TRUE_VALUE); + e->probability = PROB_VERY_UNLIKELY; make_single_succ_edge (then_bb, join_bb, EDGE_FALLTHRU); /* Mark the pseudo-fallthrough edge from cond_bb to join_bb. */ e = find_edge (cond_bb, join_bb); e->flags = EDGE_FALSE_VALUE; e->count = cond_bb->count; - e->probability = REG_BR_PROB_BASE; + e->probability = PROB_ALWAYS - PROB_VERY_UNLIKELY; /* Update dominance info. Note that bb_join's data was updated by split_block. */ @@ -183,75 +185,123 @@ build_check_stmt (tree base, set_immediate_dominator (CDI_DOMINATORS, join_bb, cond_bb); } - base_addr = create_tmp_reg (uintptr_type, "__asan_base_addr"); + gsi = gsi_last_bb (cond_bb); + g = gimple_build_assign_with_ops (TREE_CODE (base), + make_ssa_name (TREE_TYPE (base), NULL), + base, NULL_TREE); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); - seq = NULL; - t = fold_convert_loc (location, uintptr_type, - unshare_expr (base)); - t = force_gimple_operand (t, &stmts, false, NULL_TREE); - gimple_seq_add_seq (&seq, stmts); - g = gimple_build_assign (base_addr, t); + g = gimple_build_assign_with_ops (NOP_EXPR, + make_ssa_name (uintptr_type, NULL), + gimple_assign_lhs (g), NULL_TREE); gimple_set_location (g, location); - gimple_seq_add_stmt (&seq, g); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + base_addr = gimple_assign_lhs (g); /* Build - (base_addr >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ + (base_addr >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ - t = build2 (RSHIFT_EXPR, uintptr_type, base_addr, - build_int_cst (uintptr_type, ASAN_SHADOW_SHIFT)); - t = build2 (PLUS_EXPR, uintptr_type, t, - build_int_cst (uintptr_type, targetm.asan_shadow_offset ())); - t = build1 (INDIRECT_REF, shadow_type, - build1 (VIEW_CONVERT_EXPR, shadow_ptr_type, t)); - t = force_gimple_operand (t, &stmts, false, NULL_TREE); - gimple_seq_add_seq (&seq, stmts); - shadow_value = create_tmp_reg (shadow_type, "__asan_shadow"); - g = gimple_build_assign (shadow_value, t); + t = build_int_cst (uintptr_type, ASAN_SHADOW_SHIFT); + g = gimple_build_assign_with_ops (RSHIFT_EXPR, + make_ssa_name (uintptr_type, NULL), + base_addr, t); gimple_set_location (g, location); - gimple_seq_add_stmt (&seq, g); - t = build2 (NE_EXPR, boolean_type_node, shadow_value, - build_int_cst (shadow_type, 0)); - if (size_in_bytes < 8) - { + gsi_insert_after (&gsi, g, GSI_NEW_STMT); - /* Slow path for 1-, 2- and 4- byte accesses. - Build ((base_addr & 7) + (size_in_bytes - 1)) >= shadow_value. */ - - u = build2 (BIT_AND_EXPR, uintptr_type, - base_addr, - build_int_cst (uintptr_type, 7)); - u = build1 (CONVERT_EXPR, shadow_type, u); - u = build2 (PLUS_EXPR, shadow_type, u, - build_int_cst (shadow_type, size_in_bytes - 1)); - u = build2 (GE_EXPR, uintptr_type, u, shadow_value); - } - else - u = build_int_cst (boolean_type_node, 1); - t = build2 (TRUTH_AND_EXPR, boolean_type_node, t, u); - t = force_gimple_operand (t, &stmts, false, NULL_TREE); - gimple_seq_add_seq (&seq, stmts); - cond = create_tmp_reg (boolean_type_node, "__asan_crash_cond"); - g = gimple_build_assign (cond, t); + t = build_int_cst (uintptr_type, targetm.asan_shadow_offset ()); + g = gimple_build_assign_with_ops (PLUS_EXPR, + make_ssa_name (uintptr_type, NULL), + gimple_assign_lhs (g), t); gimple_set_location (g, location); - gimple_seq_add_stmt (&seq, g); - g = gimple_build_cond (NE_EXPR, cond, boolean_false_node, NULL_TREE, - NULL_TREE); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + + g = gimple_build_assign_with_ops (NOP_EXPR, + make_ssa_name (shadow_ptr_type, NULL), + gimple_assign_lhs (g), NULL_TREE); gimple_set_location (g, location); - gimple_seq_add_stmt (&seq, g); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); - /* Generate call to the run-time library (e.g. __asan_report_load8). */ + t = build2 (MEM_REF, shadow_type, gimple_assign_lhs (g), + build_int_cst (shadow_ptr_type, 0)); + g = gimple_build_assign_with_ops (MEM_REF, + make_ssa_name (shadow_type, NULL), + t, NULL_TREE); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + shadow = gimple_assign_lhs (g); - gsi = gsi_last_bb (cond_bb); - gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); - seq = NULL; - g = gimple_build_call (report_error_func (is_store, size_in_bytes), - 1, base_addr); - gimple_seq_add_stmt (&seq, g); + if (size_in_bytes < 8) + { + /* Slow path for 1, 2 and 4 byte accesses. + Test (shadow != 0) + & ((base_addr & 7) + (size_in_bytes - 1)) >= shadow). */ + g = gimple_build_assign_with_ops (NE_EXPR, + make_ssa_name (boolean_type_node, + NULL), + shadow, + build_int_cst (shadow_type, 0)); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + t = gimple_assign_lhs (g); + + g = gimple_build_assign_with_ops (BIT_AND_EXPR, + make_ssa_name (uintptr_type, + NULL), + base_addr, + build_int_cst (uintptr_type, 7)); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + + g = gimple_build_assign_with_ops (NOP_EXPR, + make_ssa_name (shadow_type, + NULL), + gimple_assign_lhs (g), NULL_TREE); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + + if (size_in_bytes > 1) + { + g = gimple_build_assign_with_ops (PLUS_EXPR, + make_ssa_name (shadow_type, + NULL), + gimple_assign_lhs (g), + build_int_cst (shadow_type, + size_in_bytes - 1)); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + } + + g = gimple_build_assign_with_ops (GE_EXPR, + make_ssa_name (boolean_type_node, + NULL), + gimple_assign_lhs (g), + shadow); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + + g = gimple_build_assign_with_ops (BIT_AND_EXPR, + make_ssa_name (boolean_type_node, + NULL), + t, gimple_assign_lhs (g)); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + t = gimple_assign_lhs (g); + } + else + t = shadow; - /* Insert the check code in the THEN block. */ + g = gimple_build_cond (NE_EXPR, t, build_int_cst (TREE_TYPE (t), 0), + NULL_TREE, NULL_TREE); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); + /* Generate call to the run-time library (e.g. __asan_report_load8). */ gsi = gsi_start_bb (then_bb); - gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); + g = gimple_build_call (report_error_func (is_store, size_in_bytes), + 1, base_addr); + gimple_set_location (g, location); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); *iter = gsi_start_bb (join_bb); } @@ -262,14 +312,12 @@ build_check_stmt (tree base, static void instrument_derefs (gimple_stmt_iterator *iter, tree t, - location_t location, int is_store) + location_t location, bool is_store) { tree type, base; - int size_in_bytes; + HOST_WIDE_INT size_in_bytes; type = TREE_TYPE (t); - if (type == error_mark_node) - return; switch (TREE_CODE (t)) { case ARRAY_REF: @@ -280,25 +328,25 @@ instrument_derefs (gimple_stmt_iterator default: return; } - size_in_bytes = tree_low_cst (TYPE_SIZE (type), 0) / BITS_PER_UNIT; - if (size_in_bytes != 1 && size_in_bytes != 2 && - size_in_bytes != 4 && size_in_bytes != 8 && size_in_bytes != 16) - return; - { - /* For now just avoid instrumenting bit field acceses. + + size_in_bytes = int_size_in_bytes (type); + if ((size_in_bytes & (size_in_bytes - 1)) != 0 + || (unsigned HOST_WIDE_INT) size_in_bytes - 1 >= 16) + return; + + /* For now just avoid instrumenting bit field acceses. Fixing it is doable, but expected to be messy. */ - HOST_WIDE_INT bitsize, bitpos; - tree offset; - enum machine_mode mode; - int volatilep = 0, unsignedp = 0; - get_inner_reference (t, &bitsize, &bitpos, &offset, - &mode, &unsignedp, &volatilep, false); - if (bitpos != 0 || bitsize != size_in_bytes * BITS_PER_UNIT) - return; - } + HOST_WIDE_INT bitsize, bitpos; + tree offset; + enum machine_mode mode; + int volatilep = 0, unsignedp = 0; + get_inner_reference (t, &bitsize, &bitpos, &offset, + &mode, &unsignedp, &volatilep, false); + if (bitpos != 0 || bitsize != size_in_bytes * BITS_PER_UNIT) + return; - base = build_addr (t, current_function_decl); + base = build_fold_addr_expr (t); build_check_stmt (base, iter, location, is_store, size_in_bytes); } @@ -314,7 +362,6 @@ transform_statements (void) basic_block bb; gimple_stmt_iterator i; int saved_last_basic_block = last_basic_block; - enum gimple_rhs_class grhs_class; FOR_EACH_BB (bb) { @@ -322,16 +369,12 @@ transform_statements (void) for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) { gimple s = gsi_stmt (i); - if (gimple_code (s) != GIMPLE_ASSIGN) - continue; + if (!gimple_assign_single_p (s)) + continue; instrument_derefs (&i, gimple_assign_lhs (s), - gimple_location (s), 1); + gimple_location (s), true); instrument_derefs (&i, gimple_assign_rhs1 (s), - gimple_location (s), 0); - grhs_class = get_gimple_rhs_class (gimple_assign_rhs_code (s)); - if (grhs_class == GIMPLE_BINARY_RHS) - instrument_derefs (&i, gimple_assign_rhs2 (s), - gimple_location (s), 0); + gimple_location (s), false); } } } @@ -356,10 +399,19 @@ asan_finish_file (void) static unsigned int asan_instrument (void) { - struct gimplify_ctx gctx; - push_gimplify_context (&gctx); + if (shadow_ptr_types[0] == NULL_TREE) + { + alias_set_type set = new_alias_set (); + shadow_ptr_types[0] + = build_distinct_type_copy (unsigned_char_type_node); + TYPE_ALIAS_SET (shadow_ptr_types[0]) = set; + shadow_ptr_types[0] = build_pointer_type (shadow_ptr_types[0]); + shadow_ptr_types[1] + = build_distinct_type_copy (short_unsigned_type_node); + TYPE_ALIAS_SET (shadow_ptr_types[1]) = set; + shadow_ptr_types[1] = build_pointer_type (shadow_ptr_types[1]); + } transform_statements (); - pop_gimplify_context (NULL); return 0; } @@ -385,6 +437,8 @@ struct gimple_opt_pass pass_asan = 0, /* properties_destroyed */ 0, /* todo_flags_start */ TODO_verify_flow | TODO_verify_stmts - | TODO_update_ssa /* todo_flags_finish */ + | TODO_update_ssa /* todo_flags_finish */ } }; + +#include "gt-asan.h" --- gcc/asan.h.jj 2012-10-11 16:04:28.000000000 +0200 +++ gcc/asan.h 2012-10-11 18:09:07.430449499 +0200 @@ -24,7 +24,7 @@ along with GCC; see the file COPYING3. extern void asan_finish_file(void); /* Shadow memory is found at - (address >> ASAN_SHADOW_SHIFT) | targetm.asan_shadow_offset (). */ + (address >> ASAN_SHADOW_SHIFT) + targetm.asan_shadow_offset (). */ #define ASAN_SHADOW_SHIFT 3 #endif /* TREE_ASAN */