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

[Jeff Law <law@redhat.com>]

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.

Thanks,
jeff