| Message ID | 52825A74.8070806@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 12 Nov 2013, Jeff Law wrote: > Here's a patch which arranges to leave a null pointer dereferece in the IL. > For a memory load through a null pointer, we just leave the statement as-is > since the LHS is going to be a throw-away SSA_NAME. For a store through a > null pointer, we try to simplify the RHS when it's trivially easy to do so > (when the RHS is an integral type). This allows DCE to sometimes do a better > job if the RHS is something that was computed on the isolated path. This > could be easily extended to floating types, vectors and structures if someone > is interested. We issue the builtin_trap after the null dereference. You didn't like Jakub's comment about __builtin_unreachable? > - /* Now delete all remaining statements in this block. */ > + If the dereference is a load, then there's nothing to do as the > + LHS will be a throw-away SSA_NAME and the LHS is the NULL dereference. I assume the second LHS should be RHS. Don't you want to mark it somehow so the next DCE doesn't remove it? For PR59083, signals as yet another control flow mechanism are a pain...
On 11/12/13 10:14, Marc Glisse wrote: > On Tue, 12 Nov 2013, Jeff Law wrote: > >> Here's a patch which arranges to leave a null pointer dereferece in >> the IL. For a memory load through a null pointer, we just leave the >> statement as-is since the LHS is going to be a throw-away SSA_NAME. >> For a store through a null pointer, we try to simplify the RHS when >> it's trivially easy to do so (when the RHS is an integral type). This >> allows DCE to sometimes do a better job if the RHS is something that >> was computed on the isolated path. This could be easily extended to >> floating types, vectors and structures if someone is interested. We >> issue the builtin_trap after the null dereference. > > You didn't like Jakub's comment about __builtin_unreachable? No, it's certainly not appropriate for this optimization. The problem with using builtin_unreachable is if you do reach that point, you fall into an unrelated blob of code in the executable. That is a huge security issue. > >> - /* Now delete all remaining statements in this block. */ >> + If the dereference is a load, then there's nothing to do as the >> + LHS will be a throw-away SSA_NAME and the LHS is the NULL >> dereference. > > I assume the second LHS should be RHS. Don't you want to mark it somehow > so the next DCE doesn't remove it? I'll fix the comment. Good point about DCE possibly removing the code. I'll have to take a look at that. > > For PR59083, signals as yet another control flow mechanism are a pain... Yup. The ability to catch the null dereference in a signal handler is the primary motivation behind keeping the dereference in the IL. jeff
On Tue, 12 Nov 2013, Jeff Law wrote: > On 11/12/13 10:14, Marc Glisse wrote: >> You didn't like Jakub's comment about __builtin_unreachable? > No, it's certainly not appropriate for this optimization. The problem with > using builtin_unreachable is if you do reach that point, you fall into an > unrelated blob of code in the executable. That is a huge security issue. I guess this will end up as a flag and the debate will only be about the default value of the flag? (-fsanitize=Idontrememberwhat would already be such a flag ;-) but you may prefer a more specific one) Note that security is a concern for some applications, but for others it is irrelevant and performance trumps it. I would even consider a flag that turns abort, trap, etc into __builtin_unreachable. Anyway, thanks for the optimization, I am only discussing the details because I like it.
On Tue, Nov 12, 2013 at 07:08:22PM +0100, Marc Glisse wrote: > On Tue, 12 Nov 2013, Jeff Law wrote: > > >On 11/12/13 10:14, Marc Glisse wrote: > >>You didn't like Jakub's comment about __builtin_unreachable? > >No, it's certainly not appropriate for this optimization. The > >problem with using builtin_unreachable is if you do reach that > >point, you fall into an unrelated blob of code in the executable. > >That is a huge security issue. > > I guess this will end up as a flag and the debate will only be about > the default value of the flag? > > (-fsanitize=Idontrememberwhat would already be such a flag ;-) but > you may prefer a more specific one) Incidentally, I'll post a patch that implements -fsanitize=null in a bit. Well, I yet have to write ChangeLogs, so don't hold your breath ;). -fsanitize=null will call an ubsan builtin if the control flow reaches the point where we're loading from/storing to a NULL pointer. The maybe bad thing is that these builtins are not noreturn, so after issuing an error message, we happily dereference the NULL pointer (I did it this way because it's what clang does). Changing these builtins to be noreturn is possible, though not as easy as it seems. Marek
On 11/12/13 10:14, Marc Glisse wrote: > > I assume the second LHS should be RHS. Don't you want to mark it somehow > so the next DCE doesn't remove it? Sigh, yes DCE is removing the load. That can be fixed by just marking the MEM_REF as volatile and updating the statement. It's a pretty trivial change. Jeff
On Tue, Nov 12, 2013 at 8:42 AM, Jeff Law <law@redhat.com> wrote: > On 11/10/13 20:11, Jeff Law wrote: >> >> OK. It sounds like there's a pretty general consensus that we ought ot >> go ahead and leave in a load/store of a NULL pointer. I'll go ahead and >> run with that. I'll probably just emit SSA_NAME = *0, unless someone >> thinks we ought ot distinguish between loads and stores, emitting >> SSA_NAME = *0 and *0 = 0 for the two cases respectively. > > [ ... ] > Here's a patch which arranges to leave a null pointer dereferece in the IL. > For a memory load through a null pointer, we just leave the statement as-is > since the LHS is going to be a throw-away SSA_NAME. For a store through a > null pointer, we try to simplify the RHS when it's trivially easy to do so > (when the RHS is an integral type). This allows DCE to sometimes do a > better job if the RHS is something that was computed on the isolated path. > This could be easily extended to floating types, vectors and structures if > someone is interested. We issue the builtin_trap after the null > dereference. > > For other cases (invalid NULL argument, invalid NULL return), we continue to > issue the builtin trap in the same manner we did before. > > I've adjusted the existing tests and added a new one which verifies the RHS > of a store through a null pointer is simplified. > > Bootstrapped and regression tested on x86_64-unknown-linux-gnu. Installed on > the trunk. > > > > * gimple-ssa-isolate-paths.c (check_loadstore): New function. > (insert_trap_and_remove_trailing_statements): New argument OP which > is the NULL pointer. Emit the trap after the load/store through > the NULL pointer. Simplify the RHS of a store through a NULL > pointer > when trivial to do so. > (isolate_path): Corresponding changes. > (gimple_ssa_isolate_erroneous_path): Likewise. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59127
diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c index 983ed4d..1203cfc 100644 --- a/gcc/gimple-ssa-isolate-paths.c +++ b/gcc/gimple-ssa-isolate-paths.c @@ -39,17 +39,65 @@ along with GCC; see the file COPYING3. If not see static bool cfg_altered; -/* Insert a trap before SI and remove SI and all statements after SI. */ +/* Callback for walk_stmt_load_store_ops. + + Return TRUE if OP will dereference the tree stored in DATA, FALSE + otherwise. + + This routine only makes a superficial check for a dereference. Thus, + it must only be used if it is safe to return a false negative. */ +static bool +check_loadstore (gimple stmt ATTRIBUTE_UNUSED, tree op, void *data) +{ + if ((TREE_CODE (op) == MEM_REF || TREE_CODE (op) == TARGET_MEM_REF) + && operand_equal_p (TREE_OPERAND (op, 0), (tree)data, 0)) + return true; + return false; +} + +/* Insert a trap after SI and remove SI and all statements after the trap. */ static void -insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p) +insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p, tree op) { - gimple_seq seq = NULL; - gimple stmt = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); - gimple_seq_add_stmt (&seq, stmt); - gsi_insert_before (si_p, seq, GSI_SAME_STMT); + /* We want the NULL pointer dereference to actually occur so that + code that wishes to catch the signal can do so. - /* Now delete all remaining statements in this block. */ + If the dereference is a load, then there's nothing to do as the + LHS will be a throw-away SSA_NAME and the LHS is the NULL dereference. + + If the dereference is a store and we can easily transform the RHS, + then simplify the RHS to enable more DCE. */ + gimple stmt = gsi_stmt (*si_p); + if (walk_stmt_load_store_ops (stmt, (void *)op, NULL, check_loadstore) + && INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))) + { + /* We just need to turn the RHS into zero converted to the proper + type. */ + tree type = TREE_TYPE (gimple_assign_lhs (stmt)); + gimple_assign_set_rhs_code (stmt, INTEGER_CST); + gimple_assign_set_rhs1 (stmt, fold_convert (type, integer_zero_node)); + update_stmt (stmt); + } + + gimple new_stmt + = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); + gimple_seq seq = NULL; + gimple_seq_add_stmt (&seq, new_stmt); + + /* If we had a NULL pointer dereference, then we want to insert the + __builtin_trap after the statement, for the other cases we want + to insert before the statement. */ + if (walk_stmt_load_store_ops (stmt, (void *)op, + check_loadstore, + check_loadstore)) + gsi_insert_after (si_p, seq, GSI_NEW_STMT); + else + gsi_insert_before (si_p, seq, GSI_NEW_STMT); + + /* The iterator points to the __builtin_trap. Advance the iterator + and delete everything else in the block. */ + gsi_next (si_p); for (; !gsi_end_p (*si_p);) { stmt = gsi_stmt (*si_p); @@ -73,7 +121,8 @@ insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p) Return BB'. */ basic_block -isolate_path (basic_block bb, basic_block duplicate, edge e, gimple stmt) +isolate_path (basic_block bb, basic_block duplicate, + edge e, gimple stmt, tree op) { gimple_stmt_iterator si, si2; edge_iterator ei; @@ -133,7 +182,7 @@ isolate_path (basic_block bb, basic_block duplicate, edge e, gimple stmt) SI2 points to the duplicate of STMT in DUPLICATE. Insert a trap before SI2 and remove SI2 and all trailing statements. */ if (!gsi_end_p (si2)) - insert_trap_and_remove_trailing_statements (&si2); + insert_trap_and_remove_trailing_statements (&si2, op); return duplicate; } @@ -224,7 +273,7 @@ gimple_ssa_isolate_erroneous_paths (void) if (infer_nonnull_range (use_stmt, lhs)) { duplicate = isolate_path (bb, duplicate, - e, use_stmt); + e, use_stmt, lhs); /* When we remove an incoming edge, we need to reprocess the Ith element. */ @@ -247,7 +296,8 @@ gimple_ssa_isolate_erroneous_paths (void) where a non-NULL value is required. */ if (infer_nonnull_range (stmt, null_pointer_node)) { - insert_trap_and_remove_trailing_statements (&si); + insert_trap_and_remove_trailing_statements (&si, + null_pointer_node); /* And finally, remove all outgoing edges from BB. */ edge e; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c b/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c index 6b779b4..33745ba 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c @@ -43,12 +43,14 @@ d_type (struct d_info *di) return ret; } -/* We're testing two aspects of isolation here. First that isolation +/* We're testing three aspects of isolation here. First that isolation occurs, second that if we have two null dereferences in a block that that we delete everything from the first dereferece to the end of the - block, regardless of which comes first in the immediate use iterator. */ + block, regardless of which comes first in the immediate use iterator + and finally that we set the RHS of the store to zero. */ /* { dg-final { scan-tree-dump-times "__builtin_trap" 1 "isolate-paths"} } */ -/* { dg-final { scan-tree-dump-times "->type" 1 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->type = 42" 1 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->type = 0" 1 "isolate-paths"} } */ /* { dg-final { scan-tree-dump-times "->zzz" 1 "isolate-paths"} } */ /* { dg-final { cleanup-tree-dump "isolate-paths" } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c b/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c new file mode 100644 index 0000000..a70871e --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c @@ -0,0 +1,60 @@ + +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-isolate-paths" } */ + + +struct demangle_component +{ + + int type; + int zzz; + +}; + + +struct d_info +{ + struct demangle_component *comps; + int next_comp; + int num_comps; +}; + + +static struct demangle_component * +d_make_empty (struct d_info *di) +{ + struct demangle_component *p; + + if (di->next_comp >= di->num_comps) + return ((void *)0); + p = &di->comps[di->next_comp]; + return p; +} + + + +struct demangle_component * +d_type (struct d_info *di) +{ + struct demangle_component *ret; + ret = d_make_empty (di); + foo (ret->type); + bar (ret->zzz); + return ret; +} + +/* We're testing two aspects of isolation here. First that isolation + occurs, second that if we have two null dereferences in a block that + that we delete everything from the first dereferece to the end of the + block, regardless of which comes first in the immediate use iterator. + + We leave the 0->type in the IL, so expect to see ->type twice. */ +/* { dg-final { scan-tree-dump-times "__builtin_trap" 1 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->type" 2 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->zzz" 1 "isolate-paths"} } */ +/* { dg-final { cleanup-tree-dump "isolate-paths" } } */ + + + + +