On 9/24/18 12:18 PM, Bernd Edlinger wrote: > On 09/24/18 19:48, Jeff Law wrote: >> On 9/16/18 1:58 PM, Bernd Edlinger wrote: >>> Hi, >>> >>> this is a cleanup of the recently added strlen/strcpy/stpcpy >>> no nul warning code. >>> >>> Most importantly it moves the SSA_NAME handling from >>> unterminated_array to string_constant, thereby fixing >>> another round of xfails in the strlen and stpcpy test cases. >>> >>> I need to say that the fix for bug 86622 is relying in >>> type info on the pointer which is probably not safe in >>> GIMPLE in the light of the recent discussion. >>> >>> I had to add two further exceptions, which should >>> be safe in general: that is a pointer arithmentic on a string >>> literal is okay, and arithmetic on a string constant >>> that is exactly the size of the whole DECL, cannot >>> access an adjacent member. >>> >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> >>> patch-cleanup-no-nul.diff >>> >>> gcc: >>> 2018-09-16 Bernd Edlinger <bernd.edlin...@hotmail.de> >>> >>> * builtins.h (unterminated_array): Remove prototype. >>> * builtins.c (unterminated_array): Simplify. Make static. >>> (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here. >>> (expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect. >>> * expr.c (string_constant): Handle SSA_NAME. Add more exceptions >>> where pointer arithmetic is safe. >>> * gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen. >>> (get_max_strlen): Remove the unnecessary mynonstr handling. >>> (gimple_fold_builtin_strcpy): Simplify. >>> (gimple_fold_builtin_stpcpy): Simplify. >>> (gimple_fold_builtin_sprintf): Remove NO_WARNING propagation >>> without effect. >>> (gimple_fold_builtin_strlen): Simplify. >> So my thinking right now is to go forward with the API change to allow >> c_strlen to fill in a structure with relevant tidbits about the string. >> >> That in turn allows us to use simplify unterminated_array in a manner >> similar to what you've done in your patch -- while carrying forward the >> capabilities we need for Martin's nul terminator warnings. This would >> be combined with the expr.c chunks from your patch. >> > > Do you want me to elaborate that idea? No need. I've already got those bits here.
> >> However, most of the changes to drop NO_WARNING stuff should be handled >> separately. I don't think they're safe as-is. I'm also pretty sure the >> stpcpy changes in builtins.c aren't correct as-is. >> >> > > Well, I think you must be referring to this: > > @@ -3984,14 +3964,10 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, mac > compile-time, not an expression containing a string. This is > because the latter will potentially produce pessimized code > when used to produce the return value. */ > - tree nonstr = NULL_TREE; > if (!c_getstr (src, NULL) > - || !(len = c_strlen (src, 0, &nonstr, 1))) > + || !(len = c_strlen (src, 0))) > return expand_movstr (dst, src, target, /*endp=*/2); > > - if (nonstr && !TREE_NO_WARNING (exp)) > - warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, nonstr); > - > lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1)); > ret = expand_builtin_mempcpy_args (dst, src, lenp1, > target, exp, /*endp=*/2); > > > My observation is: If that one is necessary and does not only emit some > duplicated warnings, then the test case must be incomplete, at least it did > not > regress when this code is removed. I'm pretty sure the test is incomplete. > > Maybe there could a better way than TREE_NO_WARNING to get rid > of the duplicated warnings. > > Maybe it will be best to concentrate the warnings on a single pass, > which means expand will it not be, right? COnceptually getting all the warnings out of the folder is a good start, but insufficient. You'd need to look at the thread for the duplicated warnings due to how the expanders call into each other. jeff
