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

[Jeff Law <law at redhat dot com>]

On 12/6/18 6:00 AM, Christophe Lyon wrote:
> 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)
I must have applied the hunk more than once because the contents of the
test are duplicated resulting in the errors.  I removed the duplicate
copy of the test and that should fix this problem.
jeff