On 10/12/18 9:34 PM, Bernd Edlinger wrote: > On 10/12/18 16:55, Jeff Law wrote: >> On 9/15/18 2:43 AM, Bernd Edlinger wrote: >>> Hi, >>> >>> this is an update on my strlen range patch (V7). Again re-based and >>> retested to current trunk. >>> >>> I am aware that Martin wants to re-factor the interface of get_range_strlen >>> and have no objections against, but I'd suggest that to be a follow-up >>> patch. >>> >>> I might suggest to rename one of the two get_range_strlen functions at the >>> same time as it is rather confusing to have to count the parameters in order >>> to tell which function is meant. >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> >>> changelog-range-strlen-v7.txt >>> >>> gcc: >>> 2018-08-26 Bernd Edlinger <bernd.edlin...@hotmail.de> >>> >>> * gimple-fold.c (looks_like_a_char_array_without_typecast_p): New >>> helper function for strlen range estimations. >>> (get_range_strlen): Use looks_like_a_char_array_without_typecast_p >>> for warnings, but use GIMPLE semantics otherwise. >>> * tree-ssa-strlen.c (maybe_set_strlen_range): Use GIMPLE semantics. >>> (get_min_string_length): Avoid not NUL terminated string literals. >> The introduction of looks_like_a_char_array_without_typecast_p is >> probably a good thing. Too much code is already implemented inline >> within get_range_strlen. >> >> It looks like you added handling of ARRAY_RANGE_REF. I don't know how >> often they come up in practice, but handling it seems like a reasonable >> extension to what we're doing. Bonus points if it's triggering with any >> kind of consistency. >> > > I did only want to be consistent with get_inner_reference here, > but did not have encountered these, probably only an Ada thing? Trying to be consistent with get_inner_reference is fine :-) GCC supports case ranges as an extension for C/C++. No clue if they're natively supported by Ada or any other langauge.
> >> I actually prefer Martin's unification of type/fuzzy into a single >> enumeration to describe the desired behavior. Doing it with two args >> where some values are mutually exclusive is just asking for trouble. >> Though I like that you called out the values that are mutually exclusive. >> >> I definitely want to look at how your patch and Martin's differ on the >> handling of flexible array members -- clearly we must avoid setting a >> range in that case. I'm surprised this didn't trigger a failure in the >> testsuite though. Martin's work in this space did. >> >> The bugfix in get_min_string_length looks like it probably stands on its >> own. >> >> I'm still evaluating the two approaches... >> > > One thing I should mention is, that there is still one place where > opportunistic > range info influence conde gen. I mean at least with my patch. ACK. That's soemthing Martin's patch does address. AT least it's supposed to. > > That is the return value from sprintf is using the range info from the > warning, and uses that to set the range info of the result. > In try_substitute_return_value, which uses the range info that was > from the warnings and feeds that into set_range_info. Right. In Martin's work we have enough range info to distinguish between the range info for warnings and the true range info and only use the latter in the call to set_range_info. jeff
