On Wed, Sep 04, 2024 at 11:06:22AM -0400, Jason Merrill wrote:
> On 9/2/24 1:49 PM, Jakub Jelinek wrote:
> > Hi!
> >
> > The following testcase is miscompiled, because
> > get_member_function_from_ptrfunc
> > emits something like
> > (((FUNCTION.__pfn & 1) != 0)
> > ? ptr + FUNCTION.__delta + FUNCTION.__pfn - 1
> > : FUNCTION.__pfn) (ptr + FUNCTION.__delta, ...)
> > or so, so FUNCTION tree is used there 5 times. There is
> > if (TREE_SIDE_EFFECTS (function)) function = save_expr (function);
> > but in this case function doesn't have side-effects, just nested ARRAY_REFs.
> > Now, if all the FUNCTION trees would be shared, it would work fine,
> > FUNCTION is evaluated in the first operand of COND_EXPR; but unfortunately
> > that isn't the case, both the BIT_AND_EXPR shortening and conversion to
> > bool done for build_conditional_expr actually unshare_expr that first
> > expression, but none of the other 4 are unshared. With -fsanitize=bounds,
> > .UBSAN_BOUNDS calls are added to the ARRAY_REFs and use SAVE_EXPRs to avoid
> > evaluating the argument multiple times, but because that FUNCTION tree is
> > first used in the second argument of COND_EXPR (i.e. conditionally), the
> > SAVE_EXPR initialization is done just there and then the third argument
> > of COND_EXPR just uses the uninitialized temporary and so does the first
> > argument computation as well.
>
> If there are SAVE_EXPRs in FUNCTION, why is TREE_SIDE_EFFECTS false?
They aren't there when this function is called, they are added only during
cp_genericize when instrumenting the ARRAY_REFs with UBSAN.
And unlike this function, e.g. ubsan_instrument_bounds just calls save_expr
on the index and not if (TREE_SIDE_EFFECTS (index)) index = save_expr
(index).
> It seems desirable to only do the bounds-checking once.
So, one possibility would be to call save_expr unconditionally in
get_member_function_from_ptrfunc as well.
Or build a TARGET_EXPR (force_target_expr or similar).
Jakub
