On 9/23/19 4:14 PM, Martin Sebor wrote: > > Yes, it looks redundant. I never remember which of these functions > ICE when their argument is not a constant (e.g., tree_int_cst_lt) > and which ones handle it gracefully (e.g., tree_int_cst_equal) so > I often check even when it isn't necessary. It would be nice if > these closely related APIs had consistent preconditions. Can't argue with that!
> > MAXBOUND is only non-constant when set that way by client code to > have the function set it to the longest PHI argument, otherwise > it's either an INTEGER_CST or null. The inner test may be dead > code, a leftover from something earlier. Either way, MAXBOUND > is only used for diagnostics so it probably doesn't matter. > >>>> @@ -1653,8 +1661,11 @@ get_range_strlen (tree arg, bitmap *visited, >>> >>> /* Try to obtain the range of the lengths of the string(s) referenced >>> by ARG, or the size of the largest array ARG refers to if the range >>> - of lengths cannot be determined, and store all in *PDATA. ELTSIZE >>> - is the expected size of the string element in bytes: 1 for char and >>> + of lengths cannot be determined, and store all in *PDATA which must >>> + be zero-initialized on input except PDATA->MAXBOUND may be set to >>> + a non-null tree node other than INTEGER_CST to request to have it >>> + set to the length of the longest string in a PHI. ELTSIZE is >>> + the expected size of the string element in bytes: 1 for char and >> Is there any reason we can't just make a clean distinction between input >> and output objects in this routine? As an API this seems awkward at best. >> >> Any thoughts on the API question raised? > > I didn't add a new argument because in GCC 9 we got rid of a bunch > of them to make the function less confusing. The final signature > (before the simplification) had 8 arguments: > > get_range_strlen (tree arg, tree length[2], bitmap *visited, > int type, int fuzzy, bool *flexp, > unsigned eltsize, tree *nonstr) > > Some of them were being tested inconsistently and their effects > were pretty subtle (especially TYPE and FUZZY). The MAXBOUND > setting is also subtle and used only for warnings so I'd rather > not expose it as an argument that every caller has to worry about > if it isn't necessary. > > Longer term, I think a better design than directly accessing > the data members is for c_strlen_data to become a proper C++ class > with accessor functions to hide this stuff behind so these kinds > of "warts" could be hidden out of sight. Since it will touch all > callers it should be made in a change independent of this one. > > So for now I've removed the redundant test and fixed the typos below > (clearly, I need a spell check for code comments). I also had to > make a few other minor tweaks to adjust to the recent changes on > trunk. Attached is an updated patch. Sounds reasonable. And yes, I'm certainly a fan of moving towards proper classes. > gcc-90879.diff > > 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 > changes. > * 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. > (get_range_strlen_dynamic): Overwrite initial maxbound when non-null. Parts of this look like bits which I've already approved (some of the get_range_strlen_dynamic bits). But regardless, this is OK. Jeff
