Re: [PATCH] consider MIN_EXPR in get_size_range() (PR 85888)

[Richard Biener <richard dot guenther at gmail dot com>]

On May 24, 2018 7:02:17 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
>On 05/24/2018 03:39 AM, Richard Biener wrote:
>> On Thu, May 24, 2018 at 12:50 AM Martin Sebor <msebor@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. 

>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