On 8/13/19 2:07 PM, Jeff Law wrote:
On 8/9/19 10:51 AM, Martin Sebor wrote:
PR tree-optimization/90879 - fold zero-equality of strcmp between a longer
string and a smaller array
gcc/c-family/ChangeLog:
PR tree-optimization/90879
* c.opt (-Wstring-compare): New option.
gcc/testsuite/ChangeLog:
PR tree-optimization/90879
* gcc.dg/Wstring-compare-2.c: New test.
* gcc.dg/Wstring-compare.c: New test.
* gcc.dg/strcmpopt_3.c: Scan the optmized dump instead of strlen.
* gcc.dg/strcmpopt_6.c: New test.
* gcc.dg/strlenopt-65.c: Remove uinnecessary declarations, add
test cases.
* gcc.dg/strlenopt-66.c: Run it.
* gcc.dg/strlenopt-68.c: New test.
gcc/ChangeLog:
PR tree-optimization/90879
* builtins.c (check_access): Avoid using maxbound when null.
* calls.c (maybe_warn_nonstring_arg): Adjust to get_range_strlen change.
* doc/invoke.texi (-Wstring-compare): Document new warning option.
* gimple-fold.c (get_range_strlen_tree): Make setting maxbound
conditional.
(get_range_strlen): Overwrite initial maxbound when non-null.
* gimple-ssa-sprintf.c (get_string_length): Adjust to get_range_strlen
change.
* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Same.
(used_only_for_zero_equality): New function.
(handle_builtin_memcmp): Call it.
(determine_min_objsize): Return an integer instead of tree.
(get_len_or_size, strxcmp_eqz_result): New functions.
(maybe_warn_pointless_strcmp): New function.
(handle_builtin_string_cmp): Call it. Fold zero-equality of strcmp
between a longer string and a smaller array.
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 4af47855e7c..31e012b741b 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -3079,196 +3042,388 @@ determine_min_objsize (tree dest)
type = TYPE_MAIN_VARIANT (type);
- /* We cannot determine the size of the array if it's a flexible array,
- which is declared at the end of a structure. */
- if (TREE_CODE (type) == ARRAY_TYPE
- && !array_at_struct_end_p (dest))
+ /* The size of a flexible array cannot be determined. Otherwise,
+ for arrays with more than one element, return the size of its
+ type. GCC itself misuses arrays of both zero and one elements
+ as flexible array members so they are excluded as well. */
+ if (TREE_CODE (type) != ARRAY_TYPE
+ || !array_at_struct_end_p (dest))
{
- tree size_t = TYPE_SIZE_UNIT (type);
- if (size_t && TREE_CODE (size_t) == INTEGER_CST
- && !integer_zerop (size_t))
- return size_t;
+ tree type_size = TYPE_SIZE_UNIT (type);
+ if (type_size && TREE_CODE (type_size) == INTEGER_CST
+ && !integer_onep (type_size)
+ && !integer_zerop (type_size))
+ return tree_to_uhwi (type_size);
So I nearly commented on this when looking at the original patch. Can
we really depend on the size when we've got an array at the end of a
struct with a declared size other than 0/1? While 0/1 are by far the
most common way to declare them, couldn't someone have used other sizes?
I think we pondered doing that at one time to cut down on the noise
from Coverity for RTL and TREE operand accessors.
Your code makes us safer, so I'm not saying you've done anything wrong,
just trying to decide if we need to tighten this up even further.
This patch issues a warning in these cases, i.e., when it sees
a call like, say, strcmp("foobar", A) with an A that's smaller
than the string, because it seems they are likely (rare) bugs.
I haven't seen the warning in any of the projects I tested it
with (Binutils/GDB, GCC, Glibc, the Linux kernel, and LLVM).
The warning uses strcmp to detect these mistakes (or misuses)
but I'd like to add similar warnings for other string functions
as well and have code out there that does this on purpose use
true flexible array members (or the zero-length extension)
instead. That makes the intent clear.
It's a judgment call whether to also fold (or do something else
like insert a trap) in addition to issuing a warning. In this
case (reading) I don't think it matters as much as it does for
writes. Either way, it would be nice to set a policy and
document it in the manual so users know what to expect and
so we don't have to revisit this question for each patch that
touches on this subject.
Martin
