On Thu, Jan 23, 2025 at 1:08 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jan 21, 2025, Richard Biener <richard.guent...@gmail.com> wrote:
>
> > 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.
>
> It does. But my intuition isn't happy with this. I don't get why only
> DECLs need to be checked here, and why we should check the type size
> rather than the decl size if we're only checking decls.
We only check DECLs because non-DECLs are covered by the MEM_REF
checking (which will make them possibly trap). And sure, the check then
could also use DECL_SIZE but for a DECL I hope it's size matches that of
the declared type ;)
That said, this patch is OK (with TYPE_SIZE or DECL_SIZE - whatever
you like better).
> I have another patch coming up that doesn't raise concerns for me, so
> I'll hold off from installing the above, in case you also prefer the
> other one.
I'll have a look at it when I see it.
Richard.
>
> [ifcombine] out-of-bounds bitfield refs can trap [PR118514]
>
> Check that BIT_FIELD_REFs of DECLs are in range before deciding they
> don't trap.
>
> Check that a replacement bitfield load is as trapping as the replaced
> load.
>
>
> for gcc/ChangeLog
>
> PR tree-optimization/118514
> * tree-eh.cc (bit_field_ref_in_bounds_p): New.
> (tree_could_trap_p) <BIT_FIELD_REF>: Call it.
> * gimple-fold.cc (make_bit_field_load): Check trapping status
> of replacement load against original load.
>
> for gcc/testsuite/ChangeLog
>
> PR tree-optimization/118514
> * gcc.dg/field-merge-23.c: New.
> ---
> gcc/gimple-fold.cc | 5 +++++
> gcc/testsuite/gcc.dg/field-merge-23.c | 19 +++++++++++++++++++
> gcc/tree-eh.cc | 29 ++++++++++++++++++++++++++++-
> 3 files changed, 52 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/field-merge-23.c
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 3c971a29ef045..01c4d076af26c 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7859,6 +7859,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/testsuite/gcc.dg/field-merge-23.c
> b/gcc/testsuite/gcc.dg/field-merge-23.c
> new file mode 100644
> index 0000000000000..d60f76206ebea
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/field-merge-23.c
> @@ -0,0 +1,19 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +
> +/* PR tree-optimization/118514 */
> +
> +/* Check that we don't pull optimized references that could trap out of
> + loops. */
> +
> +int a, c = 1;
> +unsigned char b[1], d;
> +int main() {
> + while (a || !c) {
> + signed char e = b[1000000000];
> + d = e < 0 || b[1000000000] > 1;
> + if (d)
> + __builtin_abort ();
> + }
> + return 0;
> +}
> diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> index 1033b124e4df3..27a4b2b5b1665 100644
> --- a/gcc/tree-eh.cc
> +++ b/gcc/tree-eh.cc
> @@ -2646,6 +2646,29 @@ 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))
> + return false;
> +
> + min = bit_field_offset (expr);
> + wid = bit_field_size (expr);
> + 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 +2711,14 @@ tree_could_trap_p (tree expr)
> restart:
> switch (code)
> {
> + case BIT_FIELD_REF:
> + if (DECL_P (TREE_OPERAND (expr, 0)) && !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);
>
>
> --
> 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
