From 2d80dbdfc80832217fb4158c3eb0628e360d9c71 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 24 Jun 2020 13:02:43 -0400 Subject: [PATCH 247/315] FIXME: fixes to leak-detection --- gcc/analyzer/engine.cc | 42 ++++++++---- gcc/analyzer/program-state.cc | 40 +++++++++-- gcc/analyzer/program-state.h | 16 +++++ gcc/analyzer/region-model2.cc | 84 +++++++++++++++++++----- gcc/analyzer/region-model2.h | 15 +++-- gcc/testsuite/gcc.dg/analyzer/malloc-1.c | 6 +- 6 files changed, 159 insertions(+), 44 deletions(-) diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index d4c0a58c700..8259c9f022c 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -2138,22 +2138,40 @@ exploded_node::detect_leaks (exploded_graph &eg) const gcc_assert (get_point ().get_stack_depth () == 1); const program_state &old_state = get_state (); - if (!old_state.m_region_model) - return; /* Work with a temporary copy of the state: pop the frame, and see what leaks (via purge_unused_svalues). */ program_state new_state (old_state); - gcc_assert (new_state.m_region_model); - - purge_stats stats; - impl_region_model_context ctxt (eg, this, - &old_state, &new_state, - NULL, - get_stmt ()); - new_state.m_region_model->pop_frame (region_id::null (), - true, &stats, &ctxt); + if (!old_state.m_region_model) + { + gcc_assert (new_state.m_region_model2); + +#if 1 + purge_stats2 stats; + impl_region_model2_context ctxt (eg, this, + &old_state, &new_state, + NULL, + get_stmt ()); + const svalue2 *result = NULL; + new_state.m_region_model2->pop_frame (NULL, true, &stats, &result, + &ctxt); + program_state::detect_leaks2 (old_state, new_state, result, + eg.get_ext_state (), &ctxt); +#endif + } + else + { + gcc_assert (new_state.m_region_model); + + purge_stats stats; + impl_region_model_context ctxt (eg, this, + &old_state, &new_state, + NULL, + get_stmt ()); + new_state.m_region_model->pop_frame (region_id::null (), + true, &stats, &ctxt); + } } /* Dump the successors and predecessors of this enode to OUTF. */ @@ -3384,7 +3402,7 @@ exploded_graph::process_node (exploded_node *node) impl_region_model2_context ctxt (*this, node, &old_state, &next_state, &change, stmt); - program_state::detect_leaks2 (old_state, next_state, + program_state::detect_leaks2 (old_state, next_state, NULL, get_ext_state (), &ctxt); } diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index 742afc21417..e8bb20eb185 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -1679,7 +1679,30 @@ program_state::prune_for_point2 (exploded_graph &eg, = pm->get_data_for_ssa_name (ssa_name); if (!per_ssa.needed_at_point_p (point.get_function_point ())) { - // TODO: see logic below on sm-state + /* Don't purge bindings of SSA names to svalues + that have unpurgable sm-state, so that leaks are + reported at the end of the function, rather than + at the last place that such an SSA name is referred to. + + But do purge them for temporaries (when SSA_NAME_VAR is + NULL), so that we report for cases where a leak happens when + a variable is overwritten with another value, so that the leak + is reported at the point of overwrite, rather than having + temporaries keep the value reachable until the frame is + popped. */ + const svalue2 *sval + = new_state.m_region_model2->get_store_value (reg); + if (!new_state.can_purge_p2 (eg.get_ext_state (), sval) + && SSA_NAME_VAR (ssa_name)) + { + /* (currently only state maps can keep things + alive). */ + if (logger) + logger->log ("not purging binding for %qE" + " (used by state map)", ssa_name); + continue; + } + new_state.m_region_model2->purge_region (reg); num_ssas_purged++; // TODO: update stats @@ -1771,7 +1794,7 @@ program_state::prune_for_point2 (exploded_graph &eg, &new_state, change, point.get_stmt ()); - detect_leaks2 (*this, new_state, eg.get_ext_state (), &ctxt); + detect_leaks2 (*this, new_state, NULL, eg.get_ext_state (), &ctxt); } } @@ -2006,6 +2029,7 @@ log_set_of_svalue2s (logger *logger, const char *name, void program_state::detect_leaks2 (const program_state &src_state, const program_state &dest_state, + const svalue2 *extra_sval, const extrinsic_state &ext_state, region_model2_context *ctxt) { @@ -2022,13 +2046,21 @@ program_state::detect_leaks2 (const program_state &src_state, pp_string (pp, "dest_state: "); dest_state.dump_to_pp (ext_state, true, false, pp); logger->end_log_line (); + if (extra_sval) + { + logger->start_log_line (); + pp_string (pp, "extra_sval: "); + extra_sval->dump_to_pp (pp, true); + logger->end_log_line (); + } } /* Get svalues reachable from each of src_state and dst_state. */ hash_set src_svalues; hash_set dest_svalues; - src_state.m_region_model2->get_reachable_svalues (&src_svalues); - dest_state.m_region_model2->get_reachable_svalues (&dest_svalues); + src_state.m_region_model2->get_reachable_svalues (&src_svalues, NULL); + dest_state.m_region_model2->get_reachable_svalues (&dest_svalues, + extra_sval); if (logger) { diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h index fa854c1afcb..8eff3856ae3 100644 --- a/gcc/analyzer/program-state.h +++ b/gcc/analyzer/program-state.h @@ -403,6 +403,21 @@ public: } return true; } + bool can_purge_p2 (const extrinsic_state &ext_state, + const svalue2 *sval) + { + /* Don't purge vars that have non-purgeable sm state, to avoid + generating false "leak" complaints. */ + int i; + sm_state_map2 *smap; + FOR_EACH_VEC_ELT (m_checker_states2, i, smap) + { + const state_machine &sm = ext_state.get_sm (i); + if (!sm.can_purge_p (smap->get_state (sval))) + return false; + } + return true; + } bool can_merge_with_p (const program_state &other, const extrinsic_state &ext_state, @@ -417,6 +432,7 @@ public: static void detect_leaks2 (const program_state &src_state, const program_state &dest_state, + const svalue2 *extra_sval, const extrinsic_state &ext_state, region_model2_context *ctxt); diff --git a/gcc/analyzer/region-model2.cc b/gcc/analyzer/region-model2.cc index 7665e0029db..a24780feca2 100644 --- a/gcc/analyzer/region-model2.cc +++ b/gcc/analyzer/region-model2.cc @@ -5445,10 +5445,15 @@ add_svalues_to_set_cb (const svalue2 *sval, hash_set *out) Traverse the region2s in this model, determining what region2s are reachable from the store. + If EXTRA_SVAL is non-NULL, treat it as an additional "root" + for reachability (for handling return values from functions when + analyzing return of the only function on the stack). + Find svalues that haven't leaked. */ void -region_model2::get_reachable_svalues (hash_set *out) +region_model2::get_reachable_svalues (hash_set *out, + const svalue2 *extra_sval) { reachable_region2s reachable_regs (&m_store, m_mgr); @@ -5457,6 +5462,9 @@ region_model2::get_reachable_svalues (hash_set *out) m_store.for_each_cluster (reachable_region2s::init_cluster_cb, &reachable_regs); + if (extra_sval) + reachable_regs.handle_sval (extra_sval); + #if 0 /* Mark all locals in all frames (and what's reachable from them). */ for (const frame_region2 *iter_frame = get_current_frame (); @@ -5498,6 +5506,15 @@ region_model2::get_reachable_svalues (hash_set *out) still have their initial values (but not for uninit values). */ #endif +#if 1 + /* Populate *OUT based on the values that were reachable. */ + for (hash_set::iterator iter + = reachable_regs.begin_reachable_svals (); + iter != reachable_regs.end_reachable_svals (); ++iter) + { + out->add (*iter); + } +#else /* Populate *OUT based on the clusters that were reachable. */ for (hash_set::iterator iter = reachable_regs.begin (); iter != reachable_regs.end (); ++iter) @@ -5514,6 +5531,7 @@ region_model2::get_reachable_svalues (hash_set *out) out->add (init); } } +#endif } /* Update this model for the RETURN_STMT, using CTXT to report any @@ -5587,7 +5605,7 @@ region_model2::on_longjmp (const gcall *longjmp_call, const gcall *setjmp_call, while (get_stack_depth () > setjmp_stack_depth) { /* Don't purge unused svalue2s yet, as we're using fake_retval_sval. */ - pop_frame (NULL, false, NULL, ctxt); + pop_frame (NULL, false, NULL, NULL, ctxt); } gcc_assert (get_stack_depth () == setjmp_stack_depth); @@ -7157,7 +7175,7 @@ region_model2::update_for_return_superedge (const return_superedge &return_edge, } purge_stats2 stats; - pop_frame (result_dst_reg, true, &stats, ctxt); + pop_frame (result_dst_reg, true, &stats, NULL, ctxt); // TODO: do something with the stats? @@ -7374,11 +7392,27 @@ region_model2::get_current_function () const } /* Pop the topmost frame_region2 from this region_model2's stack; - see the comment for stack_region2::pop_frame. */ + + FIXME: + If RESULT_DST_RID is non-null, copy any return value from the frame + into RESULT_DST_RID's region. + + FIXME: + Purge the frame region and all its descendent regions. + Convert any pointers that point into such regions into + POISON_KIND_POPPED_STACK svalues. + + FIXME: + If PURGE, then purge all unused svalues, with the exception of any + returned values. + + FIXME: + Accumulate stats on purged entities into STATS. */ void region_model2::pop_frame (const region2 *result_dst_reg, bool purge, purge_stats2 *out, + const svalue2 **out_result, region_model2_context *ctxt) { gcc_assert (m_current_frame); @@ -7399,6 +7433,8 @@ region_model2::pop_frame (const region2 *result_dst_reg, get_lvalue (result, ctxt), ctxt); } + if (out_result) + *out_result = get_rvalue (result, ctxt); if (purge) { #if 0 @@ -7423,7 +7459,7 @@ region_model2::pop_frame (const region2 *result_dst_reg, /* Pop the frame. */ m_current_frame = m_current_frame->get_calling_frame (); -#if 0 +#if 1 unbind_region2_and_descendents (frame_reg, POISON_KIND_POPPED_STACK, out, @@ -7742,28 +7778,40 @@ region_model2::get_descendents (region2 *reg, region2_id_set *out, } } #endif + +// FIXME /* Delete RID and all descendent region2s. Find any pointers to such region2s; convert them to poisoned values of kind PKIND. Accumulate stats on purged entities into STATS. */ -#if 0 + void -region_model2::delete_region2_and_descendents (region2 *reg, - enum poison_kind pkind, - purge_stats *stats, - logger *logger) +region_model2::unbind_region2_and_descendents (const region2 *reg, + enum poison_kind pkind, + purge_stats2 *stats, + logger *logger) { - /* Find all child and descendent region2s. */ - region2_id_set descendents (this); - get_descendents (rid, &descendents, region2_id::null ()); + /* Gather a set of base regions to be unbound. */ + hash_set base_regs; + for (store2::cluster_map_t::iterator iter = m_store.begin (); + iter != m_store.end (); ++iter) + { + const region2 *iter_base_reg = (*iter).first; + if (iter_base_reg->descendent_of_p (reg)) + base_regs.add (iter_base_reg); + } + for (hash_set::iterator iter = base_regs.begin (); + iter != base_regs.end (); ++iter) + m_store.purge_region (m_mgr->get_store2_manager (), (*iter)); + // TODO: unit test for descendent_of_p + // TODO: poison pointers to regions that are descendents of REG. +#if 0 /* Find any pointers to such region2s; convert to poisoned. */ poison_any_pointers_to_bad_region2s (descendents, pkind); - - /* Delete all such region2s. */ - purge_region2s (descendents, stats, logger); -} #endif +} + /* Find any pointers to region2s within BAD_REGION2S; convert them to poisoned values of kind PKIND. */ #if 0 @@ -9395,7 +9443,7 @@ test_stack_frames () /* Pop the "child_fn" frame from the stack. */ purge_stats2 purged; - model.pop_frame (NULL, true, &purged, &ctxt); + model.pop_frame (NULL, true, &purged, NULL, &ctxt); #if 0 /* We should have purged the unknown values for x and y. */ diff --git a/gcc/analyzer/region-model2.h b/gcc/analyzer/region-model2.h index 97ac2bced52..70f98cae282 100644 --- a/gcc/analyzer/region-model2.h +++ b/gcc/analyzer/region-model2.h @@ -2175,7 +2175,8 @@ class region_model2 #endif void handle_unrecognized_call (const gcall *call, region_model2_context *ctxt); - void get_reachable_svalues (hash_set *out); + void get_reachable_svalues (hash_set *out, + const svalue2 *extra_sval); bool on_greturn (const greturn *stmt, region_model2_context *ctxt); void on_setjmp (const gcall *stmt, const exploded_node *enode, @@ -2201,6 +2202,7 @@ class region_model2 function * get_current_function () const; void pop_frame (const region2 *result_dst, bool purge, purge_stats2 *stats, + const svalue2 **out_result, region_model2_context *ctxt); int get_stack_depth () const; const frame_region2 *get_frame_at_index (int index) const; @@ -2291,12 +2293,13 @@ class region_model2 #if 0 void get_descendents (region2_id rid, region2_id_set *out, region2_id exclude_rid) const; - - void delete_region2_and_descendents (region2_id rid, - enum poison_kind pkind, - purge_stats *stats, - logger *logger); #endif + + void unbind_region2_and_descendents (const region2 *reg, + enum poison_kind pkind, + purge_stats2 *stats, + logger *logger); + bool can_merge_with_p (const region_model2 &other_model, const program_point &point, region_model2 *out_model) const; diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c index 3024e546137..fe4e55d24c6 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c @@ -529,10 +529,8 @@ struct link global_link; void test_43 (void) { global_link.m_ptr = malloc (sizeof (struct link)); /* { dg-message "allocated here" } */ - global_link.m_ptr = NULL; -} /* { dg-warning "leak of ''" } */ -/* TODO: should be more precise than just '', and - ideally would be at the assigment to NULL. */ + global_link.m_ptr = NULL; /* { dg-warning "leak of 'global_link.m_ptr'" } */ +} struct link *global_ptr; -- 2.26.2