On 01/16/2018 01:36 PM, Martin Sebor wrote:
> PR 83698 - bogus offset in -Wrestrict messages for strcat of
> unknown strings, points out that the offsets printed by
> -Wrestrict for possibly overlapping strcat calls with unknown
> strings don't look meaningful in some cases. The root cause
> of the bogus values is wrapping during the conversion from
> offset_int in which the pass tracks numerical values to
> HOST_WIDE_INT for printing. (The problem will go away once
> GCC's pretty-printer supports wide int formatting.) For
> instance, the following:
>
> extern char *d;
> strcat (d + 3, d + 5);
>
> results in
>
> warning: ‘strcat’ accessing 0 or more bytes at offsets 3 and 5 may
> overlap 1 byte at offset [3, -9223372036854775806]
>
> which, besides printing the bogus negative offset on LP64
> targets, isn't correct because strcat always accesses at least
> one byte (the nul) and there can be no overlap at offset 3.
> To be more accurate, the warning should say something like:
>
> warning: ‘strcat’ accessing 3 or more bytes at offsets 3 and 5 may
> overlap 1 byte at offset 5 [-Wrestrict]
>
> because the function must access at least 3 bytes in order to
> cause an overlap, and when it does, the overlap starts at the
> higher of the two offsets, i.e., 5. (Though it's virtually
> impossible to have a single sentence and a singled set of
> numbers cover all the cases with perfect accuracy.)
>
> The attached patch fixes these issues to make the printed values
> make more sense. (It doesn't affect when diagnostics are printed.)
>
> Although this isn't strictly a regression, it has an impact on
> the readability of the warnings. If left unchanged, the original
> messages are likely to confuse users and lead to bug reports.
>
> Martin
>
> gcc-83698.diff
>
>
> PR tree-optimization/83698 - bogus offset in -Wrestrict messages for strcat
> of unknown strings
>
> gcc/ChangeLog:
>
> PR tree-optimization/83698
> * gimple-ssa-warn-restrict.c (builtin_memref::builtin_memref): For
> arrays constrain the offset range to their bounds.
> (builtin_access::strcat_overlap): Adjust the bounds of overlap offset.
> (builtin_access::overlap): Avoid setting the size of overlap if it's
> already been set.
> (maybe_diag_overlap): Also consider arrays when deciding what values
> of offsets to include in diagnostics.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/83698
> * gcc.dg/Wrestrict-7.c: New test.
> * c-c++-common/Wrestrict.c: Adjust expected values for strcat.
> * gcc.target/i386/chkp-stropt-17.c: Same.
>
> Index: gcc/gimple-ssa-warn-restrict.c
> ===================================================================
> --- gcc/gimple-ssa-warn-restrict.c (revision 256752)
> +++ gcc/gimple-ssa-warn-restrict.c (working copy)
> @@ -384,6 +384,12 @@ builtin_memref::builtin_memref (tree expr, tree si
> base = SSA_NAME_VAR (base);
> }
>
> + if (DECL_P (base) && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE)
> + {
> + if (offrange[0] < 0 && offrange[1] > 0)
> + offrange[0] = 0;
> + }
> +
Comment before this code so that it's more obvious to the reader that
you're narrowing the range of the low bound because it's an array
(presumably Ada's virtual origins aren't a concern here).
Note some of the preceding context has changed, but I don't think it
materially affects your patch, only the ability to apply it cleanly.
Jakub had a nit with that hunk (unnecessarily nested conditional). I'm
going to assume you'll address that.
I concur that the changes to strcat_overlap Jakub was concerned about
merely affect the diagnostic output, not whether or not we emit a
diagnostic. I'm going to assume that even though we may drop bits that
the net is better with this patch rather than without.
OK for the trunk with the comment and nit fixed.
jeff
