[PATCH] Fix memory leaks in tree-ssa-uninit.c

Martin Liška mliska@suse.cz
Fri Nov 20 11:15:00 GMT 2015


On 11/20/2015 03:14 AM, Bernd Schmidt wrote:
> BTW, I'm with whoever said absolutely no way to the idea of making automatic changes like this as part of a commit hook.
> 
> I think the whitespace change can go in if it hasn't already, but I think the other one still has enough problems that I'll say - leave it for the next stage 1.
> 
>> @@ -178,8 +173,9 @@ warn_uninitialized_vars (bool warn_possibly_uninitialized)
>>
>>    FOR_EACH_BB_FN (bb, cfun)
>>      {
>> -      bool always_executed = dominated_by_p (CDI_POST_DOMINATORS,
>> -                         single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb);
>> +      bool always_executed
>> +    = dominated_by_p (CDI_POST_DOMINATORS,
>> +              single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb);
>>        for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>      {
> 
> Better to pull the single_succ into its own variable perhaps?
> 
>> @@ -1057,7 +1039,8 @@ prune_uninit_phi_opnds_in_unrealizable_paths (gphi *phi,
>>           *visited_flag_phis = BITMAP_ALLOC (NULL);
>>
>>         if (bitmap_bit_p (*visited_flag_phis,
>> -                SSA_NAME_VERSION (gimple_phi_result (flag_arg_def))))
>> +                SSA_NAME_VERSION (
>> +                  gimple_phi_result (flag_arg_def))))
>>           return false;
>>
>>         bitmap_set_bit (*visited_flag_phis,
> 
> Pull the gimple_phi_result into a separate variable, or just leave it unchanged for now, the modified version is too ugly to live.
> 
>>         bitmap_clear_bit (*visited_flag_phis,
>> -                SSA_NAME_VERSION (gimple_phi_result (flag_arg_def)));
>> +                SSA_NAME_VERSION (
>> +                  gimple_phi_result (flag_arg_def)));
>>         continue;
>>       }
> 
> Here too.
> 
>> -  all_pruned = prune_uninit_phi_opnds_in_unrealizable_paths (phi,
>> -                                 uninit_opnds,
>> -                                 as_a <gphi *> (flag_def),
>> -                                 boundary_cst,
>> -                                 cmp_code,
>> -                                 visited_phis,
>> -                                 &visited_flag_phis);
>> +  all_pruned = prune_uninit_phi_opnds_in_unrealizable_paths
>> +    (phi, uninit_opnds, as_a<gphi *> (flag_def), boundary_cst, cmp_code,
>> +     visited_phis, &visited_flag_phis);
> 
> I'd rather shorten the name of the function, even if it goes against the spirit of the novel writing month.
> 
>> -      if (gphi *use_phi = dyn_cast <gphi *> (use_stmt))
>> -    use_bb = gimple_phi_arg_edge (use_phi,
>> -                      PHI_ARG_INDEX_FROM_USE (use_p))->src;
>> +      if (gphi *use_phi = dyn_cast<gphi *> (use_stmt))
>> +    use_bb
>> +      = gimple_phi_arg_edge (use_phi, PHI_ARG_INDEX_FROM_USE (use_p))->src;
>>         else
>>       use_bb = gimple_bb (use_stmt);
> 
> There are some changes of this nature and I'm not sure it's an improvement. Leave them out for now?
> 
>> @@ -2345,8 +2309,8 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
>>       }
>>
>>     /* Now check if we have any use of the value without proper guard.  */
>> -  uninit_use_stmt = find_uninit_use (phi, uninit_opnds,
>> -                     worklist, added_to_worklist);
>> +  uninit_use_stmt
>> +    = find_uninit_use (phi, uninit_opnds, worklist, added_to_worklist);
>>
>>     /* All uses are properly guarded.  */
>>     if (!uninit_use_stmt)
> 
> Here too.
> 
>> @@ -2396,12 +2359,24 @@ public:
>>     {}
>>
>>     /* opt_pass methods: */
>> -  opt_pass * clone () { return new pass_late_warn_uninitialized (m_ctxt); }
>> -  virtual bool gate (function *) { return gate_warn_uninitialized (); }
>> +  opt_pass *clone ();
>> +  virtual bool gate (function *);
> 
> This may technically violate our coding standards, but it's consistent with a lot of similar cases. Since coding standards are about enforcing consistency, I'd drop this change.
> 
>>   namespace {
>>
>>   const pass_data pass_data_early_warn_uninitialized =
>>   {
>> -  GIMPLE_PASS, /* type */
>> +  GIMPLE_PASS,               /* type */
>>     "*early_warn_uninitialized", /* name */
>> -  OPTGROUP_NONE, /* optinfo_flags */
>> -  TV_TREE_UNINIT, /* tv_id */
>> -  PROP_ssa, /* properties_required */
>> -  0, /* properties_provided */
>> -  0, /* properties_destroyed */
>> -  0, /* todo_flags_start */
>> -  0, /* todo_flags_finish */
>> +  OPTGROUP_NONE,           /* optinfo_flags */
>> +  TV_TREE_UNINIT,           /* tv_id */
>> +  PROP_ssa,               /* properties_required */
>> +  0,                   /* properties_provided */
>> +  0,                   /* properties_destroyed */
>> +  0,                   /* todo_flags_start */
>> +  0,                   /* todo_flags_finish */
>>   };
> 
> Likewise. Seems to be done practically nowhere.
> 
>>   class pass_early_warn_uninitialized : public gimple_opt_pass
>> @@ -2519,14 +2491,23 @@ public:
>>     {}
>>
>>     /* opt_pass methods: */
>> -  virtual bool gate (function *) { return gate_warn_uninitialized (); }
>> -  virtual unsigned int execute (function *)
>> -    {
>> -      return execute_early_warn_uninitialized ();
>> -    }
>> +  virtual bool gate (function *);
>> +  virtual unsigned int execute (function *);
> 
> Likewise.
> 
> 
> Bernd

Hi.

Enhanced patch should cover all notes pointed in the previous email.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Manual-changes-to-GCC-coding-style-in-tree-ssa-unini.patch
Type: text/x-patch
Size: 40924 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20151120/f3169fbb/attachment.bin>


More information about the Gcc-patches mailing list