This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)


On Sun, Aug 26, 2018 at 7:26 AM Jeff Law <law@redhat.com> wrote:
>
> On 08/24/2018 09:58 AM, Martin Sebor wrote:
> > The warning suppression for -Wstringop-truncation looks for
> > the next statement after a truncating strncpy to see if it
> > adds a terminating nul.  This only works when the next
> > statement can be reached using the Gimple statement iterator
> > which isn't until after gimplification.  As a result, strncpy
> > calls that truncate their constant argument that are being
> > folded to memcpy this early get diagnosed even if they are
> > followed by the nul assignment:
> >
> >   const char s[] = "12345";
> >   char d[3];
> >
> >   void f (void)
> >   {
> >     strncpy (d, s, sizeof d - 1);   // -Wstringop-truncation
> >     d[sizeof d - 1] = 0;
> >   }
> >
> > To avoid the warning I propose to defer folding strncpy to
> > memcpy until the pointer to the basic block the strnpy call
> > is in can be used to try to reach the next statement (this
> > happens as early as ccp1).  I'm aware of the preference to
> > fold things early but in the case of strncpy (a relatively
> > rarely used function that is often misused), getting
> > the warning right while folding a bit later but still fairly
> > early on seems like a reasonable compromise.  I fear that
> > otherwise, the false positives will drive users to adopt
> > other unsafe solutions (like memcpy) where these kinds of
> > bugs cannot be as readily detected.
> >
> > Tested on x86_64-linux.
> >
> > Martin
> >
> > PS There still are outstanding cases where the warning can
> > be avoided.  I xfailed them in the test for now but will
> > still try to get them to work for GCC 9.
> >
> > gcc-87028.diff
> >
> >
> > PR tree-optimization/87028 - false positive -Wstringop-truncation strncpy with global variable source string
> > gcc/ChangeLog:
> >
> >       PR tree-optimization/87028
> >       * gimple-fold.c (gimple_fold_builtin_strncpy): Avoid folding when
> >       statement doesn't belong to a basic block.
> >       * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Handle MEM_REF on
> >       the left hand side of assignment.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR tree-optimization/87028
> >       * c-c++-common/Wstringop-truncation.c: Remove xfails.
> >       * gcc.dg/Wstringop-truncation-5.c: New test.
> >
> > diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> > index 07341eb..284c2fb 100644
> > --- a/gcc/gimple-fold.c
> > +++ b/gcc/gimple-fold.c
> > @@ -1702,6 +1702,11 @@ gimple_fold_builtin_strncpy (gimple_stmt_iterator *gsi,
> >    if (tree_int_cst_lt (ssize, len))
> >      return false;
> >
> > +  /* Defer warning (and folding) until the next statement in the basic
> > +     block is reachable.  */
> > +  if (!gimple_bb (stmt))
> > +    return false;
> I think you want cfun->cfg as the test here.  They should be equivalent
> in practice.

Please do not add 'cfun' references.  Note that the next stmt is also accessible
when there is no CFG.  I guess the issue is that we fold this during
gimplification
where the next stmt is not yet "there" (but still in GENERIC)?

We generally do not want to have unfolded stmts in the IL when we can avoid that
which is why we fold most stmts during gimplification.  We also do that because
we now do less folding on GENERIC.

There may be the possibility to refactor gimplification time folding to what we
do during inlining - queue stmts we want to fold and perform all
folding delayed.
This of course means bigger compile-time due to cache effects.

>
> > diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
> > index d0792aa..f1988f6 100644
> > --- a/gcc/tree-ssa-strlen.c
> > +++ b/gcc/tree-ssa-strlen.c
> > @@ -1981,6 +1981,23 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
> >         && known_eq (dstoff, lhsoff)
> >         && operand_equal_p (dstbase, lhsbase, 0))
> >       return false;
> > +
> > +      if (code == MEM_REF
> > +       && TREE_CODE (lhsbase) == SSA_NAME
> > +       && known_eq (dstoff, lhsoff))
> > +     {
> > +       /* Extract the referenced variable from something like
> > +            MEM[(char *)d_3(D) + 3B] = 0;  */
> > +       gimple *def = SSA_NAME_DEF_STMT (lhsbase);
> > +       if (gimple_nop_p (def))
> > +         {
> > +           lhsbase = SSA_NAME_VAR (lhsbase);
> > +           if (lhsbase
> > +               && dstbase
> > +               && operand_equal_p (dstbase, lhsbase, 0))
> > +             return false;
> > +         }
> > +     }
> If you find yourself looking at SSA_NAME_VAR, you're usually barking up
> the wrong tree.  It'd be easier to suggest something here if I could see
> the gimple (with virtual operands).  BUt at some level what you really
> want to do is make sure the base of the MEM_REF is the same as what got
> passed as the destination of the strncpy.  You'd want to be testing
> SSA_NAMEs in that case.

Yes.  Why not simply compare the SSA names?  Why would it be
not OK to do that when !lhsbase?

Richard.

>
> Jeff
>
> Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]