On 05/10/2018 01:26 PM, Martin Sebor wrote:
> GCC 8.1 warns for unbounded (and some bounded) string comparisons
> involving arrays declared attribute nonstring (i.e., char arrays
> that need not be nul-terminated). For instance:
>
> extern __attribute__((nonstring)) char a[4];
>
> int f (void)
> {
> return strncmp (a, "123", sizeof a);
> }
>
> warning: ‘strcmp’ argument 1 declared attribute ‘nonstring’
>
> Note that the warning refers to strcmp even though the call in
> the source is to strncmp, because prior passes transform one to
> the other.
>
> The warning above is unnecessary (for strcmp) and incorrect for
> strncmp because the call reads exactly four bytes from the non-
> string array a regardless of the bound and so there is no risk
> that it will read past the end of the array.
>
> The attached change enhances the warning to use the length of
> the string argument to suppress some of these needless warnings
> for both bounded and unbounded string comparison functions.
> When the length of the string is unknown, the warning uses its
> size (when possible) as the upper bound on the number of accessed
> bytes. The change adds no new warnings.
>
> I'm looking for approval to commit it to both trunk and 8-branch.
>
> Martin
>
> gcc-85623.diff
>
>
> PR c/85623 - strncmp() warns about attribute 'nonstring' incorrectly in
> -Wstringop-overflow
>
> gcc/ChangeLog:
>
> PR c/85623
> * calls.c (maybe_warn_nonstring_arg): Use string length to set
> or ajust the presumed bound on an operation to avoid unnecessary
> warnings.
s/ajust/adjust/
>
> gcc/testsuite/ChangeLog:
>
> PR c/85623
> * c-c++-common/attr-nonstring-3.c: Adjust.
> * c-c++-common/attr-nonstring-4.c: Adjust.
> * c-c++-common/attr-nonstring-6.c: New test.
>
> diff --git a/gcc/calls.c b/gcc/calls.c
> index 9eb0467..f5c8ad4 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -55,6 +55,7 @@ along with GCC; see the file COPYING3. If not see
> #include "stringpool.h"
> #include "attribs.h"
> #include "builtins.h"
> +#include "gimple-fold.h"
>
> /* Like PREFERRED_STACK_BOUNDARY but in units of bytes, not bits. */
> #define STACK_BYTES (PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT)
> @@ -1612,15 +1613,36 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
> /* The bound argument to a bounded string function like strncpy. */
> tree bound = NULL_TREE;
>
> + /* The range of lengths of a string argument to one of the comparison
> + functions. If the length is less than the bound it is used instead. */
> + tree lenrng[2] = { NULL_TREE, NULL_TREE };
> +
> /* It's safe to call "bounded" string functions with a non-string
> argument since the functions provide an explicit bound for this
> purpose. */
> switch (DECL_FUNCTION_CODE (fndecl))
> {
> - case BUILT_IN_STPNCPY:
> - case BUILT_IN_STPNCPY_CHK:
> + case BUILT_IN_STRCMP:
> case BUILT_IN_STRNCMP:
> case BUILT_IN_STRNCASECMP:
> + {
> + /* For these, if one argument refers to one or more of a set
> + of string constants or arrays of known size, determine
> + the range of their known or possible lengths and use it
> + conservatively as the bound for the unbounded function,
> + and to adjust the range of the bound of the bounded ones. */
> + unsigned stride = with_bounds ? 2 : 1;
> + for (unsigned argno = 0; argno < nargs && !*lenrng; argno += stride)
> + {
> + tree arg = CALL_EXPR_ARG (exp, argno);
> + if (!get_attr_nonstring_decl (arg))
> + get_range_strlen (arg, lenrng);
> + }
> + }
> + /* Fall through. */
> +
> + case BUILT_IN_STPNCPY:
> + case BUILT_IN_STPNCPY_CHK:
> case BUILT_IN_STRNCPY:
> case BUILT_IN_STRNCPY_CHK:
> {
> @@ -1647,6 +1669,33 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
> + {
> + /* Replace the bound on the oparation with the upper bound
s/oparation/operation/
OK for the trunk with the nits fixed.
Also note that I've acked a patch from Martin L (I believe) that removes
the chkp/bounds checking bits that were deprecated in gcc-8. So there's
some chance the bounds-related bits will need to be updated depending on
whether or not Martin's L's patch has been committed.
This isn't strictly a regression. So unless this is affecting some
critical code (ie glibc, kernel or something similar) this probably
would require an explicit OK from Jakub or Richi to be eligible for the
gcc-8 branch.
jeff
