On 06/13/2018 01:58 AM, Jakub Jelinek wrote:
On Wed, Jun 13, 2018 at 09:46:28AM +0200, Richard Biener wrote:
gcc/ChangeLog:
PR c/85931
* fold-const.c (operand_equal_p): Handle SAVE_EXPR.
gcc/testsuite/ChangeLog:
PR c/85931
* gcc.dg/Wstringop-truncation-3.c: New test.
OK for the trunk. Richi and Jakub have the final say for the branch.
I'm a bit surprised that you don't just use operand_equal_p for both the
INTEGER_CST and !INTEGER_CST cases when testing dstsz == srcsz
It seemed that since the CST case compared the values of the size
expressions and the other case their lexicographic form they needed
to be different. But operand_equal_p() does value comparison for
constants so the conditional can be simplified by using just it
for both cases. Thanks for the suggestion! I've made that
simplification and committed r261515.
Jakub/Richard, can you please review the commit and let me know
if you have any concerns with backporting it to the GCC 8 branch?
(I will proceed if I don't hear anything this week.)
I'm fine with backporting it.
I'm actually worried about the fold-const.c change and don't understand, why
it has been done at all in the context of this PR.
case SAVE_EXPR:
if (flags & OEP_LEXICOGRAPHIC)
return OP_SAME (0);
return 0;
means it does something different from the state before the patch:
default:
return 0;
only if OEP_LEXICOGRAPHIC is set, which AFAIK is only in the
-Wduplicated-branches warning code, so why it is needed for
-Wstringop-truncation warning is unclear to me.
It lets sizeof_pointer_memaccess_warning detect that sizeof b
refers to the size of the VLA b in cases like the one below
(the sizeof b expression is represented as SAVE_EXPR (n)) and
avoid false positives:
void f (void*, ...);
void g (unsigned n)
{
char a[n], b[n];
f (a, b);
strncpy (a, b, sizeof b); // bogus -Wsizeof-pointer-memaccess
f (a);
}
Short of moving the SAVE_EXPR logic out of operand_equal_p()
and into sizeof_pointer_memaccess_warning, is there a better
way to determine that? (I would expect the SAVE_EXPR logic
in operand_equal_p() to be useful in other contexts besides
this warning.)
Martin
More importantly,
especially -fsanitize=undefined has a tendency of creating trees
which contain two or more uses of the same SAVE_EXPR at each level, and very
deep nesting levels of such SAVE_EXPRs, so if we handle it without checking
for duplicates, it results in exponential compile time complexity.
So, if we really want to handle SAVE_EXPR here, we'd need to create some
cache where we'd remember if in the same toplevel operand_equal_p call we've
already compared two particular SAVE_EXPRs and what was the result.
Jakub
