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.
> +
> + // FIXME: handle anti-ranges?
> + return false;
Please don't if we can avoid them. anti-ranges are on the chopping
block, so I'd prefer not to add cases where we're trying to handle them
if we can reasonably avoid it.
No objections elsewhere. So I think we just need to figure out what's
going on with the ranges when you've got an INTEGER_CST on the RHS of an
assignment.
jeff
