On Fri, Nov 14, 2014 at 12:25:37PM +0100, Dodji Seketeli wrote:
> > + /* True if there is a block with HAS_FREEING_CALL_P flag set
> > + on any a path between imm(BB) and BB. */
>
> s/a//.
>
> Also, I'd rather say "on any path between an immediate dominator of
> BB, denoted imm(BB), and BB". That way, subsequent uses of imm(BB)
> makes sense more for the new comer. This is only a suggestion. If
> you feel that formulation is obvious enough, then please ignore my
> comment.
Ok.
> > +/* Return true if there might be any call to free/munmap operation
> > + on any path in between DOM (which should be imm(BB)) and BB. */
> > +
> > +static bool
> > +imm_dom_path_with_freeing_call (basic_block bb, basic_block dom)
> > +{
>
> To ease maintainability, maybe we could assert that:
>
> gcc_assert (dom == get_immediate_dominator(CDI_DOMINATORS, bb));
Well, that would make the dom argument useless, the point of passing it
around was that because we have to call it in the caller already,
there is no point in calling it again the the callee.
So if we as well call it (most people don't use --disable-checking),
we could just drop the argument.
> ?
>
> And thus remove the assert that is at the caller site of this
> function, later in maybe_optimize_asan_check_ifn:
>
> > + basic_block imm = get_immediate_dominator (CDI_DOMINATORS, last_bb);
> > + gcc_assert (imm);
The assert is very much intentional here, want to double-check the algorithm
that we indeed always reach gbb through the imm dominator walk (unless
bailing out early, but if we wouldn't, we'd reach it).
> > + if (imm_dom_path_with_freeing_call (last_bb, imm))
> > + break;
>
>
> Also, when the 'dom' basic block is NULL, couldn't we just return
> immediately?
get_immediate_dominator (CDI_DOMINATORS, ) should return NULL
just for the ENTRY block, it would be a bug to reach that bb.
> > +/* Optimize away redundant ASAN_CHECK calls. */
> > +
> > +static bool
> > +maybe_optimize_asan_check_ifn (struct sanopt_ctx *ctx, gimple stmt)
> > +{
> > + gcc_assert (gimple_call_num_args (stmt) == 4);
> > + tree ptr = gimple_call_arg (stmt, 1);
> > + tree len = gimple_call_arg (stmt, 2);
> > + basic_block bb = gimple_bb (stmt);
> > + sanopt_info *info = (sanopt_info *) bb->aux;
> > +
> > + if (TREE_CODE (len) != INTEGER_CST)
> > + return false;
> > + if (integer_zerop (len))
> > + return false;
> > +
> > + gimple_set_uid (stmt, info->freeing_call_events);
>
> I am not sure, but I am wondering if we shouldn't save the previous uid
> of 'stmt' here before setting it, and then restore it before getting out
> of this function.
No, gimple uids are AFAIK undefined at the start of passes, passes that use
them are supposed to initialize them before use (new statements created
during the pass will get 0 there by default), and don't have to clean them
up anyway at the end of pass.
> > @@ -89,111 +402,77 @@ sanopt_optimize_walker (basic_block bb,
>
> I think the comment of this sanopt_optimize_walker function should now
> be adapted to say that it optimizes away redundant UBSAN_NULL *and*
> ASAN_CHECK internal function calls.
Ok, will do.
Jakub
