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))
+ 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);
> 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.
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.
--
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
