On 11/6/19 2:06 PM, Martin Sebor wrote: > On 11/6/19 1:39 PM, Jeff Law wrote: >> On 11/6/19 1:27 PM, Martin Sebor wrote: >>> On 11/6/19 11:55 AM, Jeff Law wrote: >>>> On 11/6/19 11:00 AM, Martin Sebor wrote: >>>>> The -Wstringop-overflow warnings for single-byte and multi-byte >>>>> stores mention the amount of data being stored and the amount of >>>>> space remaining in the destination, such as: >>>>> >>>>> warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] >>>>> >>>>> 123 | *p = 0; >>>>> | ~~~^~~ >>>>> note: destination object declared here >>>>> 45 | char b[N]; >>>>> | ^ >>>>> >>>>> A warning like this can take some time to analyze. First, the size >>>>> of the destination isn't mentioned and may not be easy to tell from >>>>> the sources. In the note above, when N's value is the result of >>>>> some non-trivial computation, chasing it down may be a small project >>>>> in and of itself. Second, it's also not clear why the region size >>>>> is zero. It could be because the offset is exactly N, or because >>>>> it's negative, or because it's in some range greater than N. >>>>> >>>>> Mentioning both the size of the destination object and the offset >>>>> makes the existing messages clearer, are will become essential when >>>>> GCC starts diagnosing overflow into allocated buffers (as my >>>>> follow-on patch does). >>>>> >>>>> The attached patch enhances -Wstringop-overflow to do this by >>>>> letting compute_objsize return the offset to its caller, doing >>>>> something similar in get_stridx, and adding a new function to >>>>> the strlen pass to issue this enhanced warning (eventually, I'd >>>>> like the function to replace the -Wstringop-overflow handler in >>>>> builtins.c). With the change, the note above might read something >>>>> like: >>>>> >>>>> note: at offset 11 to object ‘b’ with size 8 declared here >>>>> 45 | char b[N]; >>>>> | ^ >>>>> >>>>> Tested on x86_64-linux. >>>>> >>>>> Martin >>>>> >>>>> gcc-store-offset.diff >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> * builtins.c (compute_objsize): Add an argument and set it to >>>>> offset >>>>> into destination. >>>>> * builtins.h (compute_objsize): Add an argument. >>>>> * tree-object-size.c (addr_object_size): Add an argument and >>>>> set it >>>>> to offset into destination. >>>>> (compute_builtin_object_size): Same. >>>>> * tree-object-size.h (compute_builtin_object_size): Add an >>>>> argument. >>>>> * tree-ssa-strlen.c (get_addr_stridx): Add an argument and set it >>>>> to offset into destination. >>>>> (maybe_warn_overflow): New function. >>>>> (handle_store): Call maybe_warn_overflow to issue warnings. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * c-c++-common/Wstringop-overflow-2.c: Adjust text of expected >>>>> messages. >>>>> * g++.dg/warn/Wstringop-overflow-3.C: Same. >>>>> * gcc.dg/Wstringop-overflow-17.c: Same. >>>>> >>>> >>>>> Index: gcc/tree-ssa-strlen.c >>>>> =================================================================== >>>>> --- gcc/tree-ssa-strlen.c (revision 277886) >>>>> +++ gcc/tree-ssa-strlen.c (working copy) >>>>> @@ -189,6 +189,52 @@ struct laststmt_struct >>>>> static int get_stridx_plus_constant (strinfo *, unsigned >>>>> HOST_WIDE_INT, tree); >>>>> static void handle_builtin_stxncpy (built_in_function, >>>>> gimple_stmt_iterator *); >>>>> +/* Sets MINMAX to either the constant value or the range VAL is in >>>>> + and returns true on success. */ >>>>> + >>>>> +static bool >>>>> +get_range (tree val, wide_int minmax[2], const vr_values *rvals = >>>>> NULL) >>>>> +{ >>>>> + if (tree_fits_uhwi_p (val)) >>>>> + { >>>>> + minmax[0] = minmax[1] = wi::to_wide (val); >>>>> + return true; >>>>> + } >>>>> + >>>>> + if (TREE_CODE (val) != SSA_NAME) >>>>> + return false; >>>>> + >>>>> + if (rvals) >>>>> + { >>>>> + gimple *def = SSA_NAME_DEF_STMT (val); >>>>> + if (gimple_assign_single_p (def) >>>>> + && gimple_assign_rhs_code (def) == INTEGER_CST) >>>>> + { >>>>> + /* get_value_range returns [0, N] for constant assignments. */ >>>>> + val = gimple_assign_rhs1 (def); >>>>> + minmax[0] = minmax[1] = wi::to_wide (val); >>>>> + return true; >>>>> + } >>>> Umm, something seems really off with this hunk. If the SSA_NAME is set >>>> via a simple constant assignment, then the range ought to be a >>>> singleton >>>> ie [CONST,CONST]. Is there are particular test were this is not true? >>>> >>>> The only way offhand I could see this happening is if originally the >>>> RHS >>>> wasn't a constant, but due to optimizations it either simplified into a >>>> constant or a constant was propagated into an SSA_NAME appearing on the >>>> RHS. This would have to happen between the last range analysis and the >>>> point where you're making this query. >>> >>> Yes, I think that's right. Here's an example where it happens: >>> >>> void f (void) >>> { >>> char s[] = "1234"; >>> unsigned n = strlen (s); >>> char vla[n]; // or malloc (n) >>> vla[n] = 0; // n = [4, 4] >>> ... >>> } >>> >>> The strlen call is folded to 4 but that's not propagated to >>> the access until sometime after the strlen pass is done. >> Hmm. Are we calling set_range_info in that case? That goes behind the >> back of pass instance of vr_values. If so, that might argue we want to >> be setting it in vr_values too. > > No, set_range_info is only called for ranges. In this case, > handle_builtin_strlen replaces the strlen() call with 4: > > s = "1234"; > _1 = __builtin_strlen (&s); > n_2 = (unsigned int) _1; > a.1_8 = __builtin_alloca_with_align (_1, 8); > (*a.1_8)[n_2] = 0; Right. But at the point where we make the substitution for the call on the RHS the range is a singleton and we could set the range of _1 to [4, 4]. We could also set its SSA_NAME_VALUE to 4. Hell, we could even forward propagate the constant to the uses. Any/all of those would seem better than the hack in question.
> > When the access is made, the __builtin_alloca_with_align call > is found as the destination and the _1 SSA_NAME is used to > get its size. We get back the range [4, 4]. Now I'm confused. If we're getting [4, 4], then that's exactly what we want. Jeff