[PATCH] make POINTER_PLUS offset sizetype (PR 97956)

[Richard Biener]

On Wed, Nov 25, 2020 at 7:22 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 11/25/20 2:31 AM, Richard Biener wrote:
> > On Wed, Nov 25, 2020 at 1:45 AM Martin Sebor via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> Offsets in pointer expressions are signed but GCC prefers to
> >> represent them as sizetype instead, and sometimes (though not
> >> always) crashes during GIMPLE verification when they're not.
> >> The sometimes-but-not-always part makes it easy for mistakes
> >> to slip in and go undetected for months, until someone either
> >> trips over it by accident, or deliberately tries to break
> >> things (the test case in the bug relies on declaring memchr
> >> with the third argument of type signed long which is what's
> >> apparently needed to trigger the ICE).  The attached patch
> >> corrects a couple of such mistakes.
> >>
> >> Martin
> >>
> >> PS It would save us the time and effort dealing with these
> >> bugs to either detect (or even correct) the mistakes early,
> >> at the time the POINTER_PLUS_EXPR is built.  Adding an assert
> >> to gimple_build_assign()) to verify that it has the expected
> >> type (or converting the operand to sizetype) as in the change
> >> below does that.  I'm pretty sure I submitted a patch like it
> >> in the past but it was rejected.  If I'm wrong or if there are
> >> no objections to it now I'll be happy to commit it as well.
> >
> > We already verify this in verify_gimple_assign_binary after
> > each pass.  Iff then this would argue for verifying all built
> > stmts immediately, assigns with verify_gimple_assign.
> > But I think this is overkill - your testcase is already catched
> > by the IL verification.
>
> You're right, having the check wouldn't have prevented this bug.
> But I'm not worried about this test case.  What I'd like to do
> is reduce the risk of similar problems happening in the future
> where the check would help.  Catching problems earlier by having
> functions verify their pre- and postconditions is good practice.
> So yes, I think all these build() functions should do that (not
> necessarily to the same extent as the full-blown IL verification
> but at least the basics).
>
> >
> > Btw, are you sure the offset returned by constant_byte_string
> > is never checked to be positive in callers?
>
> The function sets *PTR_OFFSET to sizetype in all but this one
> case (actually, it also sets it to integer_zero_one).  Callers
> then typically compare it to the length of the string to see
> if it's less.  If not, the result is discarded because it refers
> outside the string.  It's tested for equality to zero but I don't
> see it being checked to see if it's positive and I'm not sure to
> what end.  What's your concern?

My concern was that code might do

 /* Handle broken code.  */
 if (tree_int_cst_sgn (offset) < 0)
  return NULL_TREE;

.. expect offset to be within the string ..

but I didn't look at much context.  If the function uses sizetype
everywhere else that concern is moot and thus this hunk is OK
as well.

Thanks,
Richard.

> Anyway, as an experiment, I've changed the function to set
> the offset to ssizetype instead of sizetype and reran a subset
> of the test suite with the check in gimple_build_assign and it
> didn't trigger.  So I guess the sloppiness here doesn't matter.
>
> That said, there is a bug in the function that I noticed while
> making this change so it wasn't a completely pointless exercise.
> The function should call itself recursively but instead it calls
> string_constant.  I'll resubmit the sizetype change with the fix
> for this bug
>
> Martin
>
> >
> > The gimple-fold.c hunk and the new testcase are OK.
> >
> > Richard.
> >
> >> Both patches were tested on x86_64-linux.
> >>
> >> diff --git a/gcc/gimple.c b/gcc/gimple.c
> >> index e3e508daf2f..8e88bab9e41 100644
> >> --- a/gcc/gimple.c
> >> +++ b/gcc/gimple.c
> >> @@ -489,6 +489,9 @@ gassign *
> >>    gimple_build_assign (tree lhs, enum tree_code subcode, tree op1,
> >>                        tree op2 MEM_STAT_DECL)
> >>    {
> >> +  if (subcode == POINTER_PLUS_EXPR)
> >> +    gcc_checking_assert (ptrofftype_p (TREE_TYPE (op2)));
> >> +
> >>      return gimple_build_assign_1 (lhs, subcode, op1, op2, NULL_TREE
> >>                                   PASS_MEM_STAT);
> >>    }
>