[PING #4][PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)

Christophe Lyon christophe.lyon@linaro.org
Thu Dec 6 13:00:00 GMT 2018


On Thu, 6 Dec 2018 at 00:11, Jeff Law <law@redhat.com> wrote:
>
> On 11/29/18 4:43 PM, Martin Sebor wrote:
> > On 11/29/18 4:07 PM, Jeff Law wrote:
> >> On 11/29/18 1:34 PM, Martin Sebor wrote:
> >>> On 11/16/2018 02:07 AM, Richard Biener wrote:
> >>>> On Fri, Nov 16, 2018 at 4:12 AM Martin Sebor <msebor@gmail.com> wrote:
> >>>>>
> >>>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html
> >>>>>
> >>>>> Please let me know if there is something I need to change here
> >>>>> to make the fix acceptable or if I should stop trying.
> >>>>
> >>>> I have one more comment about
> >>>>
> >>>> +  /* Defer warning (and folding) until the next statement in the basic
> >>>> +     block is reachable.  */
> >>>> +  if (!gimple_bb (stmt))
> >>>> +    return false;
> >>>> +
> >>>>
> >>>> it's not about the next statement in the basic-block being "reachable"
> >>>> (even w/o a CFG you can use gsi_next()) but rather that the next
> >>>> stmt isn't yet gimplified and thus not inserted into the gimple sequence,
> >>>>
> >>>> right?
> >>>
> >>> No, it's about the current statement not being associated with
> >>> a basic block yet when the warning code runs for the first time
> >>> (during gimplify_expr), and so gsi_next() returning null.
> >>>
> >>>> You apply this to gimple_fold_builtin_strncpy but I'd rather
> >>>> see us not sprinkling this over gimple-fold.c but instead do this
> >>>> in gimplify.c:maybe_fold_stmt, delaying folding until say lowering.
> >>>>
> >>>> See the attached (untested).
> >>>
> >>> I would also prefer this solution.  I had tested it (in response
> >>> to you first mentioning it back in September) and it causes quite
> >>> a bit of fallout in tests that look for the folding to take place
> >>> very early.  See the end of my reply here:
> >>>
> >>>    https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01248.html
> >>>
> >>> But I'm willing to do the test suite cleanup if you think it's
> >>> suitable for GCC 9.  (If you're thinking GCC 10 please let me
> >>> know now.)
> >> The fallout on existing tests is minimal.  What's more concerning is
> >> that it doesn't actually pass the new test from Martin's original
> >> submission.  We get bogus warnings.
> >>
> >> At least part of the problem is weakness in maybe_diag_stxncpy_trunc.
> >> It can't handle something like this:
> >>
> >> test_literal (char * d, struct S * s)
> >> {
> >>    strncpy (d, "1234567890", 3);
> >>    _1 = d + 3;
> >>    *_1 = 0;
> >> }
> >>
> >>
> >> Note the pointer arithmetic between the strncpy and storing the NUL
> >> terminator.
> >
> > Right.  I'm less concerned about this case because it involves
> > a literal that's obviously longer than the destination but it
> > would be nice if the suppression worked here as well in case
> > the literal comes from macro expansion.  It will require
> > another tweak.
> >
> > But the test from my patch passes with the changes to calls.c
> > from my patch, so that's an improvement.
> >
> > I have done the test suite cleanup in the attached patch.  It
> > was indeed minimal -- not sure why I saw so many failures with
> > my initial approach.
> >
> > I can submit a patch to handle the literal case above as
> > a followup unless you would prefer it done at the same time.
> >
> > Martin
> >
> > gcc-87028.diff
> >
> > PR tree-optimization/87028 - false positive -Wstringop-truncation strncpy with global variable source string
> >
> > gcc/ChangeLog:
> >
> >       PR tree-optimization/87028
> >       * calls.c (get_attr_nonstring_decl): Avoid setting *REF to
> >       SSA_NAME_VAR.
> >       * gcc/gimple-low.c (lower_stmt): Delay foldin built-ins.
> >       * gimplify (maybe_fold_stmt): Avoid folding statements that
> >       don't belong to a basic block.
> >       * tree.h (SSA_NAME_VAR): Update comment.
> >       * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Simplify.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR tree-optimization/87028
> >       * c-c++-common/Wstringop-truncation.c: Remove xfails.
> >       * gcc.dg/Wstringop-truncation-5.c: New test.
> >       * gcc.dg/strcmpopt_1.c: Adjust.
> >       * gcc.dg/tree-ssa/pr79697.c: Same.
> I fixed up the ChangeLog a little and installed the patch.
>

Hi,
The new test (Wstringop-truncation-5.c ) fails at least on arm and aarch64:
FAIL: gcc.dg/Wstringop-truncation-5.c (test for excess errors)
Excess errors:
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:74:8: error:
redefinition of 'struct S'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:79:12: error:
redefinition of 'arr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:80:19: error:
redefinition of 'ptr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:82:12: error:
redefinition of 'arr2'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:84:6: error:
conflicting types for 'test_literal'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:90:6: error:
conflicting types for 'test_global_arr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:96:6: error:
conflicting types for 'test_global_arr2'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:104:6: error:
conflicting types for 'test_global_ptr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:110:6: error:
conflicting types for 'test_local_arr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:117:6: error:
conflicting types for 'test_local_ptr'
/gcc/testsuite/gcc.dg/Wstringop-truncation-5.c:124:6: error:
conflicting types for 'test_compound_literal'

> Thanks,
> jeff



More information about the Gcc-patches mailing list