On Thu, 6 Dec 2018 at 00:11, Jeff Law <l...@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 <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. >
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
