[ifcombine] out-of-bounds bitfield refs can trap [PR118514]

  On Jan 21, 2025, Richard Biener <richard.guenther@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.

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.


[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
  

On Thu, Jan 23, 2025 at 1:08 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jan 21, 2025, Richard Biener <richard.guenther@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