On May 25, 2018 7:31:43 PM GMT+02:00, Martin Sebor <mse...@gmail.com> wrote:
>On 05/25/2018 01:02 AM, Richard Biener wrote:
>> On Thu, May 24, 2018 at 11:22 PM Martin Sebor <mse...@gmail.com>
>wrote:
>>
>>> On 05/24/2018 11:15 AM, Richard Biener wrote:
>>>> On May 24, 2018 7:02:17 PM GMT+02:00, Martin Sebor
><mse...@gmail.com>
>> wrote:
>>>>> On 05/24/2018 03:39 AM, Richard Biener wrote:
>>>>>> On Thu, May 24, 2018 at 12:50 AM Martin Sebor <mse...@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>> The attached patch enhances the get_size_range() function to
>>>>>>> extract more accurate ranges out of MIN_EXPR and MAX_EXPR nodes
>>>>>>> with SSA_NAME operand(s). This helps -Wstringop-overflow avoid
>>>>>>> false positives on some targets in cases like some of those
>>>>>>> reported in bug 85623 - strncmp() warns about attribute
>nonstring
>>>>>>> incorrectly in -Wstringop-overflow
>>>>>>
>>>>>>> When first trying to expand calls to __builtin_strncmp, most
>back
>>>>>>> ends succeed even when the expansion results in a call to the
>>>>> library
>>>>>>> function. The powerpc64 back-end, however, fails and and allows
>>>>>>> the generic expansion to take place. There is a subtle
>difference
>>>>>>> between the two cases that shows up when examining the bound of
>>>>>>> the strncmp call expression (the third argument): in the
>original
>>>>>>> call expression (in expand_builtin_strncmp) the bound is or can
>be
>>>>>>> an SSA_NAME. But when the early expansion fails,
>>>>>>> expand_builtin_strncmp transforms the original call expression
>into
>>>>>>> a new one, substituting MIN_EXPR (bound, CST) for the bound
>where
>>>>>>> CST is the constant result of strlen (STR), with STR being
>either
>>>>>>> the first or second strncmp argument. When the bound is an
>SSA_NAME
>>>>>>> the subsequent attempt to determine its range from the MIN_EXPR
>>>>> fails.
>>>>>>
>>>>>> Hmm, wouldn't sth extracted from the attached random patch I have
>>>>> lying
>>>>>> around be more useful in general? That is, refactor the GENERIC
>>>>>> expression walk in determine_value_range_1 to sth that can be
>used
>>>>>> in the current get_range_info () interface? Or if we don't want
>to
>>>>> change
>>>>>> that to accept non-SSA names add a corresponding
>get_expr_range_info
>>>>> ()
>>>>>> interface.
>>>>>
>>>>> Thanks. It is certainly more general. I'm just not sure how much
>>>>> use the generality can be put to because...
>>>>>
>>>>>> If you do that please cut out the dominator/loop condition
>handling
>>>>> and
>>>>>> refrain from the idea of looking at SSA def stmts.
>>>>>
>>>>> ...I've done that but I'm having trouble exercising the code
>except
>>>>> for this bug. (I've run the C testsuite but the only tests that
>end
>>>>> up in it call it with uninteresting arguments like VAR_DECL). Do
>>>>> you have any suggestions?
>>>>
>>>> Well, what builds the MIN_EXPR for your testcase? I orinigally used
>it
>> for niters analysis which deals with complex GENERIC expressions. If
>you
>> just changed get_range_info then of course callers are guarded with
>SSA
>> name checks.
>>
>>> In pr85888 it's built by expand_builtin_strncmp (so very late).
>>> It doesn't seem like a good use case because of this code:
>>
>>> /* Expand the library call ourselves using a stabilized argument
>>> list to avoid re-evaluating the function's arguments twice.
>*/
>>> tree fn = build_call_nofold_loc (loc, fndecl, 3, arg1, arg2,
>len);
>>> gcc_assert (TREE_CODE (fn) == CALL_EXPR);
>>> CALL_EXPR_TAILCALL (fn) = CALL_EXPR_TAILCALL (exp);
>>> return expand_call (fn, target, target == const0_rtx);
>>
>>> AFAICS, the new call expression is the same as the old one, the only
>>> difference is the LEN argument which is the MIN_EXPR.
>>
>>> The code was added in r72380 where the stabilization referred to
>>> calling save_expr() on the arguments. That code has been gone
>>> since r242556 so I think a better/more targeted solution to fix
>>> pr85888 than either of our approaches might be to simply call
>>> expand_call() on the original CALL_EXPR with the original
>>> arguments, including ARG3.
>>
>>> I don't fully understand the point of the save_expr() calls there
>>> (and they're still in builtin_expand_strcmp), so if they need to
>>> be restored then they would presumably include ARG3. I.e.,
>>> expand_call() would be called using ARG3 in the original SSA_NAME
>>> form rather than the MIN_EXPR wrapper created by the function.
>>
>>> Attached is a patch that implements the first approach above.
>>
>>> If the SAVE_EXPRs are needed for some reason, it would be good
>>> to have a test case to verify it. I haven't been able to come
>>> up with one (or find an existing test that fails due to their
>>> removal) but that could very well be because my limited
>>> understanding of what they're for (and poor testsuite coverage).
>>
>>> I'm also up for taking your patch and trying to make use of it
>>> for other trees where get_range_info() is currently being called
>>> for SSA_NAMEs only. But it feels like a separate project from
>>> this bug fix.
>>
>> But your patch makes the whole 'len' computation after
>>
>> /* If we don't have a constant length for the first, use the
>length
>> of the second, if we know it. If neither string is constant
>length,
>> use the given length argument. We don't require a constant for
>> this case; some cost analysis could be done if both are
>available
>> but neither is constant. For now, assume they're equally
>cheap,
>> unless one has side effects. If both strings have constant
>lengths,
>> use the smaller. */
>>
>> if (!len1 && !len2)
>> len = len3;
>> else if (!len1)
>> ...
>> /* If we are not using the given length, we must incorporate it
>here.
>> The actual new length parameter will be MIN(len,arg3) in this
>case. */
>> if (len != len3)
>> len = fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (len), len,
>len3);
>>
>> dead if expand_cmpstrn_or_cmpmem doesn't expand things inline.
>> I believe the idea is to possibly reduce 'len' for expansion of the
>> libcall.
>>
>> OTOH all this "optimization" should have been done as folding
>> and not at RTL expansion time. Thus if the issue is getting
>> a warning from the recursive re-expansion of the adjusted
>> call then removing this "postmature" optimization and making
>> sure it happens earlier as folding might be indeed a good thing.
>>
>> But your patch as-is looks like it loses some optimization
>> (and I'm not caring about "folding" at -O0).
>
>On the one affected target (as far as I know) it increases
>the code size by adding a branch. I wouldn't have thought
>of that as an optimization. Since it bloats code, at
>a minimum I'd expect the decision to be gated for -Os. But
>I'm not sure the branch is necessarily a win either -- it
>will certainly depend on whether strncpy makes use of it.
>
>At the same time, if you believe this code is deliberately
>crafted this way I wouldn't want to undo it just to suppress
>a warning. So I'm back to the question: is my first patch
>good enough for now to suppress the warning or do you want
>me to use your patch?
I don't like to have random pieces of range propagation code spread throughout
the compiler so no, your first patch isn't OK. A variant using a new
get_expr_range_info is preferred.
Richard.
>I don't have a problem using yours but in the first pass
>I wouldn't want to do more than just fix the warning; with
>it fixed, I could the look into removing some of the guards
>for SSA_NAME around calls to get_range_info().
>
>Martin
>
>> Richard.
>>
>>> Martin
>>
>>>>
>>>>> Martin
>>>>>
>>>>> PS I was able to create a test that at -O0 could call it with
>>>>> a more interesting argument such as:
>>>>>
>>>>> const __SIZE_TYPE__ i = 3, j = 5;
>>>>>
>>>>> int f (void)
>>>>> {
>>>>> return __builtin_strncmp ("1234", "123", i < j ? i : j);
>>>>> }
>>>>>
>>>>> Here, the first call to gimple_fold_builtin_string_compare() that
>>>>> I picked as a test case sees:
>>>>>
>>>>> j.0_1 = 5;
>>>>> i.1_2 = 3;
>>>>> _3 = MIN_EXPR <j.0_1, i.1_2>;
>>>>> D.1963 = __builtin_strncmp ("1234", "123", _3);
>>>>>
>>>>> Above, _3's range info is not available yet at this point so
>>>>> the expression could be evaluated but only by looking at def
>stmts.
>>>>> Why is it not a good idea?
>>>>
>>>> Because it raises complexity concerns unless you also populate
>> intermediate SSA range Infos and stop at present ones. And you can't
>deal
>> with conditional info as the patch tries to do (but I wouldn't keep
>that
>> either initially).
>>>>
>>>> Richard.
>>>>
>>>>>
>>>>> Martin
>>>>
