On 08/20/18 12:23, Richard Biener wrote:
> On Sun, 19 Aug 2018, Bernd Edlinger wrote:
>
>> Hi,
>>
>>
>> I rebased my range computation patch to current trunk,
>> and updated it according to what was discussed here.
>>
>> That means get_range_strlen has already a parameter
>> that is used to differentiate between ranges for warnings
>> and ranges for code-gen.
>>
>> That is called "strict", in the 4-parameter overload
>> and "fuzzy" in the internally used 7-parameter overload.
>>
>> So I added an "optimistic" parameter to my
>> get_inner_char_array_unless_typecast helper function.
>> That's it.
>>
>> Therefore at this time, there is only one warning regression
>> in one test case and one xfailed warning test case fixed.
>>
>> So that is par on the warning regression side.
>>
>> The failed test case is gcc/testsuite/gcc.dg/pr83373.c which
>> uses -fassume-zero-terminated-char-arrays, to enable the
>> (unsafe) feedback from string-length information to VRP to
>> suppress the warning.
>>
>> The 5 test cases that were designed to check the optimized
>> tree dump have to use the new -fassume-zero-terminated-char-arrays
>> option, but that is what we agreed upon.
>>
>> The patch is not dependent on any other patches.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>
> + tree base = arg;
> + while (TREE_CODE (base) == ARRAY_REF
> + || TREE_CODE (base) == ARRAY_RANGE_REF
> + || TREE_CODE (base) == COMPONENT_REF)
> + base = TREE_OPERAND (base, 0);
> +
> + /* If this looks like a type cast don't assume anything. */
> + if ((TREE_CODE (base) == MEM_REF
> + && (! integer_zerop (TREE_OPERAND (base, 1))
> + || TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (TREE_OPERAND (base,
> 0))))
> + != TYPE_MAIN_VARIANT (TREE_TYPE (base))))
>
> I'm not convinced you are testing anything useful here.
> TREE_OPERAND (base, 1) might be a pointer which means it's type
> doesn't have any semantics so you are testing the access type
> against "random". If you'd restrict this to ADDR_EXPRs and
> look at the objects declared type then you'd still miss
> type-changes from a dynamic type that is different from what
> is declared.
>
This whole function is only used for warnings or if the test case
asks for unsafe optimization via -ffassume-zero-terminated-char-arrays.
So yes, it is understood, but it has proven to be an oracle with
99.9% likelihood to give the right answer.
> So my conclusion is if you really want to not want to return
> arg for things that look like a type cast then you have to
> unconditionally return NULL_TREE.
Yes. :-)
>
> + || TREE_CODE (base) == VIEW_CONVERT_EXPR
> + /* Or other stuff that would be handled by get_inner_reference. */
>
> simply use || handled_component_p (base) for the above and the rest
> to be sure to handle everything that is not stripped above.
>
> + || TREE_CODE (base) == BIT_FIELD_REF
> + || TREE_CODE (base) == REALPART_EXPR
> + || TREE_CODE (base) == IMAGPART_EXPR)
> + return NULL_TREE;
>
Yes, good point.
> Btw, you are always returning the passed arg or NULL_TREE so
> formulating this as a predicate function makes uses easier.
> Not sure why it is called "inner" char array?
>
;
Yes, in a previous version of this patch, this function actually
walked towards the innermost array, and returned that, but I dropped
that part, as it caused too many test cases regress.
So agreed, I think I will convert that to a _p function and think
of a better name.
> There do seem to be independently useful fixes in the patch that
> I'd approve immediately.
>
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?
> 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?
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).
But we can as well remove those test cases.
Bernd.
2018-08-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/86121
* tree-ssa-strlen.c (adjust_last_stmt): Avoid folding away undefined
behaviour.
testsuite:
2018-08-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/86121
* gcc.dg/Wstringop-overflow-6.c: Remove xfail.
diff -ur gcc/testsuite/gcc.dg/Wstringop-overflow-6.c gcc/testsuite/gcc.dg/Wstringop-overflow-6.c
--- gcc/testsuite/gcc.dg/Wstringop-overflow-6.c 2018-06-12 20:05:13.000000000 +0200
+++ gcc/testsuite/gcc.dg/Wstringop-overflow-6.c 2018-08-20 14:53:55.605350343 +0200
@@ -25,7 +25,7 @@
void test_strcpy_strcat_2 (void)
{
- strcpy (a2, "12"), strcat (a2, "3"); /* { dg-warning "\\\[-Wstringop-overflow=]" "bug 86121" { xfail *-*-* } } */
+ strcpy (a2, "12"), strcat (a2, "3"); /* { dg-warning "\\\[-Wstringop-overflow=]" } */
}
void test_strcpy_strcat_3 (void)
diff -ur gcc/tree-ssa-strlen.c gcc/tree-ssa-strlen.c
--- gcc/tree-ssa-strlen.c 2018-08-02 01:39:35.000000000 +0200
+++ gcc/tree-ssa-strlen.c 2018-08-20 12:41:23.352955874 +0200
@@ -1107,6 +1107,13 @@
to store the extra '\0' in that case. */
if ((tree_to_uhwi (len) & 3) == 0)
return;
+
+ /* Don't fold away an out of bounds access, as this defeats proper
+ warnings. */
+ tree dst = gimple_call_arg (last.stmt, 0);
+ tree size = compute_objsize (dst, 0);
+ if (size && tree_int_cst_lt (size, len))
+ return;
}
else if (TREE_CODE (len) == SSA_NAME)
{
