On Wed, Oct 23, 2024 at 12:45:53PM -0400, Jason Merrill wrote:
> On 10/23/24 12:33 PM, Jakub Jelinek wrote:
> > On Wed, Oct 23, 2024 at 12:27:32PM -0400, Jason Merrill wrote:
> > > On 10/22/24 2:17 PM, Jakub Jelinek wrote:
> > > > The following testcase shows that the previous
> > > > get_member_function_from_ptrfunc
> > > > changes weren't sufficient and we still have cases where
> > > > -fsanitize=undefined with pointers to member functions can cause wrong
> > > > code
> > > > being generated and related false positive warnings.
> > > >
> > > > The problem is that save_expr doesn't always create SAVE_EXPR, it can
> > > > skip
> > > > some invariant arithmetics and in the end it could be really large
> > > > expressions which would be evaluated several times (and what is worse,
> > > > with
> > > > -fsanitize=undefined those expressions then can have SAVE_EXPRs added to
> > > > their subparts for -fsanitize=bounds or -fsanitize=null or
> > > > -fsanitize=alignment instrumentation). Tried to just build1 a SAVE_EXPR
> > > > + add TREE_SIDE_EFFECTS instead of save_expr, but that doesn't work
> > > > either,
> > > > because cp_fold happily optimizes those SAVE_EXPRs away when it sees
> > > > SAVE_EXPR operand is tree_invariant_p.
> > >
> > > Hmm, when is that be a problem? I wouldn't expect instance_ptr to be
> > > tree_invariant_p.
> >
> > E.g. TREE_READONLY !TREE_SIDE_EFFECTS ARRAY_REF (with some const array first
> > operand and some VAR_DECL etc. second operand).
>
> That seems like a bug in tree_invariant_p.
save_expr has been doing that at least since 1992, likely before that.
Though, that
4073 /* Array ref is const/volatile if the array elements are
4074 or if the array is.. */
4075 TREE_READONLY (rval)
4076 |= (CP_TYPE_CONST_P (type) | TREE_READONLY (array));
is done in the C++ FE also since 1994-ish.
I'm afraid what will break especially in Ada if we change it.
Though, unsure even to what.
Are the TREE_READONLY flags needed on ARRAY_REFs/COMPONENT_REFs with
ARRAY_REF bases etc.?
If yes, are ARRAY_REFs/ARRAY_RANGE_REFs with non-invariant index (or
possibly also non-invariant 3rd/4th argument or ARRAY_RANGE_REFs with
non-invariant type size) the only problematic cases?
Say TREE_READONLY COMPONENT_REF with VAR_DECL base should be invariant
I'd hope.
So, should we for the (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
case walk the tree, looking for the ARRAY_REFs etc. and checking if that
is really invariant?
Jakub
