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 <mse...@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.
Thanks, jeff
