On Mon, Jan 20, 2025 at 8:28 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jan 20, 2025, Richard Biener <richard.guent...@gmail.com> wrote:
>
> > I think this is a bug in tree_could_trap_p which is indeed not considering
> > out-of-bound accesses of a VAR_DECL (or any decl), but very conservatively
> > handle ARRAY_REFs.
>
> Yeah. I even had a patch to fix it, but it wasn't enough to retain the
> same trapping status for field merging, because get_inner_reference
> would drop conversions that would have to be somehow retained to avoid
> mismatches. This (manually reduced, so the hashes won't match) patchlet
> would hit bootstrap errors when building libgnat IIRC.
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 3c971a29ef045..db4eb2a255cca 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7859,6 +7860,11 @@ make_bit_field_load (location_t loc, tree inner, tree
> orig_inner, tree type,
> gimple *new_stmt = gsi_stmt (i);
> if (gimple_has_mem_ops (new_stmt))
> gimple_set_vuse (new_stmt, reaching_vuse);
> + gcc_checking_assert (! (gimple_assign_load_p (point)
> + && gimple_assign_load_p (new_stmt))
> + || (tree_could_trap_p (gimple_assign_rhs1 (point))
> + == tree_could_trap_p (gimple_assign_rhs1
> + (new_stmt))));
> }
>
> gimple_stmt_iterator gsi = gsi_for_stmt (point);
> diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> index 1033b124e4df3..a92eccb4bb6db 100644
> --- a/gcc/tree-eh.cc
> +++ b/gcc/tree-eh.cc
> @@ -2646,6 +2646,30 @@ range_in_array_bounds_p (tree ref)
> return true;
> }
>
> +/* Return true iff EXPR, a BIT_FIELD_REF, accesses a bit range that is known
> to
> + be in bounds for the referred operand type. */
> +
> +static bool
> +bit_field_ref_in_bounds_p (tree expr)
> +{
> + tree size_tree;
> + poly_uint64 size_max, min, wid, max;
> +
> + size_tree = TYPE_SIZE (TREE_TYPE (TREE_OPERAND (expr, 0)));
> + if (!size_tree
> + || !poly_int_tree_p (size_tree, &size_max)
> + || !poly_int_tree_p (TREE_OPERAND (expr, 2), &min)
> + || !poly_int_tree_p (TREE_OPERAND (expr, 1), &wid))
you can use bit_field_size () and bit_field_offset () unconditionally,
> + return false;
> +
> + max = min + wid;
> + if (maybe_lt (max, min)
> + || maybe_lt (size_max, max))
> + return false;
> +
> + return true;
> +}
> +
> /* Return true if EXPR can trap, as in dereferencing an invalid pointer
> location or floating point arithmetic. C.f. the rtl version, may_trap_p.
> This routine expects only GIMPLE lhs or rhs input. */
> @@ -2688,10 +2712,14 @@ tree_could_trap_p (tree expr)
> restart:
> switch (code)
> {
> + case BIT_FIELD_REF:
> + if (!bit_field_ref_in_bounds_p (expr))
> + return true;
> + /* Fall through. */
> +
> case COMPONENT_REF:
> case REALPART_EXPR:
> case IMAGPART_EXPR:
> - case BIT_FIELD_REF:
> case VIEW_CONVERT_EXPR:
> case WITH_SIZE_EXPR:
> expr = TREE_OPERAND (expr, 0);
I guess the point is that all handled_component_p are supposed to
subset the referenced object. Given MEM_REF can pun - consider
int x;
struct Y { int y1; int y2; };
foo ()
{
... = ((struct Y *)&x)->y2;
}
which will look like MEM [&x].y2, here the COMPONENT_REF is the
sub-ref advancing the offset of the access to outside of 'x' and
the MEM is accessing sizeof(Y) at offset zero from 'x'. It's enough
to check that MEM doesn't access beyond the objects size and we
do that.
Now, we don't have the same handling on BIT_FIELD_REFs but it
seems it's enough to apply the check to those with a DECL as
object to operate on.
So - your fix looks almost good, I'd adjust it to
> + case BIT_FIELD_REF:
> + if (DECL_P (TREE_OPERAND (expr, 0))
> + && !bit_field_ref_in_bounds_p (expr))
> + return true;
> + /* Fall through. */
OK if that works.
>
> > So you fear of missed optimizations when allowing variable size types?
>
> Yeah, IMHO it would be unfortunate if an optimization that could be
> applied to a fixed-size struct couldn't be applied to VLAs thereof.
> Unfortunately, checking BIT_FIELD_REF bounds isn't enough to enable
> that, and it occurred to me that perhaps BIT_FIELD_REFs were *intended*
> to never trap due to the bitrange alone, because they were meant to be
> presumed in range.
Note we assume that x.a[i] (variable index) may trap, handling other cases
where variable size/offset is involved in the same conservative manner looks
reasomable.
>
> OTOH, the cases that are really problematic for this optimization are
> those in which we'd *drop* a could-trap, and those are easy to detect in
> advance. And, conversely, when the BIT_FIELD_REF would seem potentially
> trapping for being out-of-bounds, even where the original reference
> wouldn't, maybe we could annotate it with TREE_NO_TRAP (it would have to
> be extended to apply to BIT_FIELD_REFs).
>
>
> > I'd have said we should have proof that the generated BIT_FIELD_REF
> > does not access out-of-bounds, in particular the
>
> > tree_could_trap_p (gimple_assign_rhs1 (load))
> > & !tree_could_trap_p (inner)
>
> > (&& btw) looks like bad style.
>
> Oops, yeah, typo (keyboard woes), thanks.
>
> > Did you consider not applying this ifcombine to tree_could_trap_p original
> > refs
> > at all?
>
> Yeah. It works. But then I figured we could take a safe step further
> and ended up with what I posted.
What about that short-circuit argument? How do we end up combining
two refs that could trap and emit that to a block that's no longer guarded
by the original separate conditions?
Richard.
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
