On 08/22/18 00:25, Jeff Law wrote:
> On 08/21/2018 02:43 AM, Richard Biener wrote:
>> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
> [ snip. ]
>
>>> Yes, I found some peanuts on my way.
>>>
>>> For instance this fix for PR middle-end/86121 survives bootstrap on
>>> it's own, and fixes one xfail.
>>>
>>> Is it OK for trunk?
>>
>> Yes, that's OK for trunk.
> Agreed. Seems like a nice independent bugfix and I don't think it
> adversely affects anything else under current discussion. In fact, not
> folding here makes it easier to warn about incorrect code elsewhere.
>
>>
>>>> Btw, I don't think we want sth like
>>>> flag_assume_zero_terminated_char_arrays or even make it default at
>>>> -Ofast.
>>>>
>>>
>>> Yes, I agree. Is there a consensus about this?
>>
>> Well, it's my own opinion of course. Show me a benchmark that
>> improves with -fassume-zero-terminated-char-arrays. Certainly
>> for security reasons it sounds a dangerous thing (and the documentation
>> needs a more thorough description of what it really means).
> I certainly don't want to see a flag. We've already got way too many;
> adding another for marginal behavior just seems wrong.
>
>>
>>> If yes, I go ahead and remove that option again.
>>>
>>> BTW: I needed this option in a few test cases, that insist in checking the
>>> optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).
>>
>> Any example?
>>
>>> But we can as well remove those test cases.
> Bernd, if there are specific tests that you want to see removed, we
> should discuss them.
>
The test cases are:
gcc.dg/strlenopt-36.c
gcc.dg/strlenopt-40.c
gcc.dg/strlenopt-45.c
gcc.dg/strlenopt-48.c
gcc.dg/strlenopt-51.c
I see no way how to fix those, as they test the information flow
from the get_range_string to VRP info, which has to go away.
For the developing this patch it was fine to tweak the test cases
with the compiler flag, but I'd prefer to get rid of them.
There is one test that tests a warning, gcc.dg/pr83373.c.
I used the flag there, but could as well have simply xfailed that:
size_t len = __builtin_strlen (src);
if (len < size)
__builtin_memcpy (dst, src, len + 1);
else
{
__builtin_memcpy (dst, src, size - 1); /* { dg-bogus
"\\\[-Wstringop-oveflow]" } */
dst[size - 1] = '\0';
}
I have not fully debugged that, but believe the test case works because unsafe
range infos are used to eliminate the memcpy call before it is diagnosed.
> I think we can all agree that if a test depends on C semantics rather
> than GIMPLE semantics for optimization/codegen, then removing it seems
> like the right thing to do as the test is just wrong for GCC.
>
> If the test is meant to issue a warning or avoid a false positive, then
> we should defer -- I'd like to not lose the warnings if at all possible.
>
yes, this patch is looking pretty healthy right now.
except the one regression above, there are no further regressions on
the warnings. Just the code generation does change.
There are however still more correctness issues:
There is still an information flow from
get_range_strlen -> sprintf return code -> VRP.
And generally, even if using the range info from the sprintf function
is fine from the posix spec, I still have a bad feeling about that,
because "sprintf" is a rather complex piece of software that can even
have bugs or implementation details, I think of non-posix environments
like linux or ecos which come with an own implementation of sprintf
and those may have subtle differences, or simply bugs, but this
optimization takes those the right away to return an error code.
> A test which verifies correct optimization seems like it should be
> discussed. I'd be more inclined to xfail those rather than remove them
> completely -- particularly for a test which isn't a good indicator of
> the real world code typically seen by gcc.
>
xfail would work for me, but I doubt we will ever be able to fix a test case
that does so many different things at the same time, and checks just if
all was folded or nothing.
Bernd.
