From patchwork Fri Jan 8 00:53:28 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 1423593 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=none dis=none) header.from=gcc.gnu.org Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=WaM3LZAh; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4DBl381MCcz9sVy for ; Fri, 8 Jan 2021 11:53:40 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 365693971C0F; Fri, 8 Jan 2021 00:53:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 365693971C0F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1610067217; bh=y9WbRJ5t71b/1lKLNTHVhi/e9vpvA+XAHERvBglbHFI=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=WaM3LZAhMC46yMmqbtOzKW1X8h7G7xOtuuc04CJHqHqAPV0D/m9tsIAkLPEbhvgnw InbimPAxGvDI99k0rtnCvPP/fdLOMXTGULmL85LtGS38rDXBpVPQJ7mDcFIdSfHxBA VOgRK3PdcmbxzTBtfuanyoc3eOt+a/B472ZOXd9s= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pf1-x431.google.com (mail-pf1-x431.google.com [IPv6:2607:f8b0:4864:20::431]) by sourceware.org (Postfix) with ESMTPS id C56353971C0C for ; Fri, 8 Jan 2021 00:53:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C56353971C0C Received: by mail-pf1-x431.google.com with SMTP id h10so5152966pfo.9 for ; Thu, 07 Jan 2021 16:53:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:from:subject:message-id:date:user-agent :mime-version:content-language; bh=y9WbRJ5t71b/1lKLNTHVhi/e9vpvA+XAHERvBglbHFI=; b=Z17hR1YIOx3P9IxP7rBuiSegWEEXOuse+LY3a52qZmI/12dpBIPKQDtrR34I9Nc1R7 M+pfrI4I/wsuiPJ3bfRcgg8rHoXxxGRIsFsY75tqBeuJyI7fuJqHspVqim4NxVUpBNic GfuxZ6uT25zQAr53XydO68adxN2vTSbrBqw/7pdDnK8J+079kslZUw1+J61fHSqt4F5o ogop1bE2ffHi0cGPZHQStWu/zvBNqPYteVgKvV/DFJEjyeLF8O6M6b5vpP8vHfxtqTPx aRsIWL14JHk54aNKIzNyMpzFhVuD5Tz+YQnEhT+YiyB/l3duNXofkxQ/f0yjjc0k0Ygq Sx9g== X-Gm-Message-State: AOAM530CCkFAH9MKwi3maDMYf+um1iW0ROk6A4iDLthAHYQ7i23QsU4Q pVIM0MQex69tIPEwfcofRQivqrzccLY= X-Google-Smtp-Source: ABdhPJz4ndMiYCBifaatGzfWwdik7WMKV+atjRk9VXhbGBJKH8/ENXDwRYjpPuHzq28xL8OXLdeZxA== X-Received: by 2002:a65:48c9:: with SMTP id o9mr4419945pgs.156.1610067210616; Thu, 07 Jan 2021 16:53:30 -0800 (PST) Received: from [192.168.0.41] (75-166-96-128.hlrn.qwest.net. [75.166.96.128]) by smtp.gmail.com with ESMTPSA id y3sm6670663pjb.18.2021.01.07.16.53.29 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 07 Jan 2021 16:53:29 -0800 (PST) To: gcc-patches Subject: [PATCH] issue -Wstring-compare for member arrays (PR 98097) Message-ID: Date: Thu, 7 Jan 2021 17:53:28 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 Content-Language: en-US X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Martin Sebor via Gcc-patches From: Martin Sebor Reply-To: Martin Sebor Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" In PR 98097 Richard expects -Wstring-compare for a call to strcmp() with a member array and a string literal of larger size, used in an equality test. In virtually all cases the test will indicate the two are unequal because the string stored in the member must be shorter (to fit the terminating nul), but GCC doesn't fold the result because there's wicked code out there that treats whole aggregates as if they were strings, up their full size. Because the warning is based on the same conservative assumptions as the optimization, it doesn't trigger, letting the almost certain bug go unnoticed. The attached patch allows -Wstring-compare to trigger for these bugs by partly decoupling the warning from the underlying strcmp optimization. Making this possible requires adding a new member to the c_strlen_data struct, which in turn called for changing the meaning of the existing decl member to nonstr. That led to changes elsewhere, simply to adjust to the name change. For the purposes of review, the meat of the warning changes is in tree-ssa-strlen.c. All the rest of changes simply adjust code to the new name. Tested on x86_64-linux (None of Binutils, GDB, Glibc, or Valgrind triggers any instances of the warning with this change.) Martin PR middle-end/98097 - missing -Wstring-compare with a member array gcc/ChangeLog: PR middle-end/98097 * builtins.c (unterminated_array): Adjust to a name change. Adjust indentation. (c_strlen): Use a member instead of a local variable. (expand_builtin_stpcpy_1): Adjust to a name change. (fold_builtin_strlen): Same. * builtins.h (struct c_strlen_data::nonstr): New data member to use instead of decl. (struct c_strlen_data::decl): Adjust comment. * gimple-fold.c (get_range_strlen_tree): Set c_strlen_data::nonstr in addition to c_strlen_data::decl. (get_maxval_strlen): Adjust to a name change. (gimple_fold_builtin_stpcpy): Same. (gimple_fold_builtin_strlen): Same. * gimple-ssa-sprintf.c (get_string_length): Same. * tree-ssa-strlen.c (get_range_strlen_dynamic): Same. Also set struct c_strlen_data::decl. (get_len_or_size): Use c_strlen_data::decl. Succeed even for nonconstant member arrays. (strxcmp_eqz_result): Handle member arrays. (handle_builtin_string_cmp): Issue warnings for member arrays. gcc/testsuite/ChangeLog: PR middle-end/98097 * gcc.dg/Wstring-compare.c: * gcc.dg/strcmpopt_10.c: * gcc.dg/Wstring-compare-4.c: New test. * gcc.dg/Wstring-compare-5.c: New test. diff --git a/gcc/builtins.c b/gcc/builtins.c index ffbb9b7f5f1..9b7a82153c8 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -1253,42 +1253,41 @@ check_nul_terminated_array (tree expr, tree src, tree unterminated_array (tree exp, tree *size /* = NULL */, bool *exact /* = NULL */) { - /* C_STRLEN will return NULL and set DECL in the info - structure if EXP references a unterminated array. */ + /* C_STRLEN will return NULL and set LENDATA.NONSTR to the DECL + of the unterminated array if EXP references one. */ c_strlen_data lendata = { }; tree len = c_strlen (exp, 1, &lendata); - if (len == NULL_TREE && lendata.minlen && lendata.decl) - { - if (size) + if (len || !lendata.minlen || !lendata.nonstr) + return NULL_TREE; + + if (size) + { + len = lendata.minlen; + if (lendata.off) { - len = lendata.minlen; - if (lendata.off) + /* Constant offsets are already accounted for in LENDATA.MINLEN, + but not in a SSA_NAME + CST expression. */ + if (TREE_CODE (lendata.off) == INTEGER_CST) + *exact = true; + else if (TREE_CODE (lendata.off) == PLUS_EXPR + && TREE_CODE (TREE_OPERAND (lendata.off, 1)) == INTEGER_CST) { - /* Constant offsets are already accounted for in LENDATA.MINLEN, - but not in a SSA_NAME + CST expression. */ - if (TREE_CODE (lendata.off) == INTEGER_CST) - *exact = true; - else if (TREE_CODE (lendata.off) == PLUS_EXPR - && TREE_CODE (TREE_OPERAND (lendata.off, 1)) == INTEGER_CST) - { - /* Subtract the offset from the size of the array. */ - *exact = false; - tree temp = TREE_OPERAND (lendata.off, 1); - temp = fold_convert (ssizetype, temp); - len = fold_build2 (MINUS_EXPR, ssizetype, len, temp); - } - else - *exact = false; + /* Subtract the offset from the size of the array. */ + *exact = false; + tree temp = TREE_OPERAND (lendata.off, 1); + temp = fold_convert (ssizetype, temp); + len = fold_build2 (MINUS_EXPR, ssizetype, len, temp); } else - *exact = true; - - *size = len; + *exact = false; } - return lendata.decl; - } + else + *exact = true; - return NULL_TREE; + *size = len; + } + + return lendata.nonstr; } /* Compute the length of a null-terminated character string or wide @@ -1353,8 +1352,7 @@ c_strlen (tree arg, int only_value, c_strlen_data *data, unsigned eltsize) /* Offset from the beginning of the string in bytes. */ tree byteoff; tree memsize; - tree decl; - src = string_constant (src, &byteoff, &memsize, &decl); + src = string_constant (src, &byteoff, &memsize, &data->decl); if (src == 0) return NULL_TREE; @@ -1399,7 +1397,7 @@ c_strlen (tree arg, int only_value, c_strlen_data *data, unsigned eltsize) return NULL_TREE; else if (len >= maxelts) { - data->decl = decl; + data->nonstr = data->decl; data->off = byteoff; data->minlen = ssize_int (len); return NULL_TREE; @@ -1449,8 +1447,9 @@ c_strlen (tree arg, int only_value, c_strlen_data *data, unsigned eltsize) "offset %qwi outside bounds of constant string", eltoff)) { - if (decl) - inform (DECL_SOURCE_LOCATION (decl), "%qE declared here", decl); + if (data->decl) + inform (DECL_SOURCE_LOCATION (data->decl), "%qE declared here", + data->decl); TREE_NO_WARNING (arg) = 1; } return NULL_TREE; @@ -1470,14 +1469,15 @@ c_strlen (tree arg, int only_value, c_strlen_data *data, unsigned eltsize) unsigned len = string_length (ptr + eltoff * eltsize, eltsize, strelts - eltoff); - /* Don't know what to return if there was no zero termination. - Ideally this would turn into a gcc_checking_assert over time. - Set DECL/LEN so callers can examine them. */ if (len >= maxelts - eltoff) { - data->decl = decl; + /* The array is not nul-termimated. Set NONSTR/LEN so callers can + examine them. + FIXME: Return a failure other than null (e.g., error_mark_node) + instead. */ data->off = byteoff; data->minlen = ssize_int (len); + data->nonstr = data->decl; return NULL_TREE; } @@ -6231,8 +6231,8 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, machine_mode mode) return expand_movstr (dst, src, target, /*retmode=*/ RETURN_END_MINUS_ONE); - if (lendata.decl) - warn_string_no_nul (EXPR_LOCATION (exp), exp, NULL, src, lendata.decl); + if (lendata.nonstr) + warn_string_no_nul (EXPR_LOCATION (exp), exp, NULL, src, lendata.nonstr); lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1)); ret = expand_builtin_mempcpy_args (dst, src, lenp1, @@ -10907,16 +10907,16 @@ fold_builtin_strlen (location_t loc, tree expr, tree type, tree arg) if (len) return fold_convert_loc (loc, type, len); - if (!lendata.decl) + if (!lendata.nonstr) c_strlen (arg, 1, &lendata); - if (lendata.decl) + if (lendata.nonstr) { if (EXPR_HAS_LOCATION (arg)) loc = EXPR_LOCATION (arg); else if (loc == UNKNOWN_LOCATION) loc = input_location; - warn_string_no_nul (loc, expr, "strlen", arg, lendata.decl); + warn_string_no_nul (loc, expr, "strlen", arg, lendata.nonstr); } return NULL_TREE; diff --git a/gcc/builtins.h b/gcc/builtins.h index 642923281c1..e021adbe7d7 100644 --- a/gcc/builtins.h +++ b/gcc/builtins.h @@ -91,15 +91,16 @@ struct c_strlen_data tree minlen; tree maxlen; tree maxbound; - /* When non-null, DECL refers to the declaration known to store - an unterminated constant character array, as in: - const char s[] = { 'a', 'b', 'c' }; - It is used to diagnose uses of such arrays in functions such as - strlen() that expect a nul-terminated string as an argument. */ + /* When non-null, stores the DECL or expression based on which + the data was obtained. */ tree decl; /* Non-constant offset from the beginning of a string not accounted for in the length range. Used to improve diagnostics. */ tree off; + /* Set to the DECL of an initialized but unterminated const array. + Used to diagnose uses of such arrays in functions such as strlen() + that expect a nul-terminated string as an argument. */ + tree nonstr; }; extern tree c_strlen (tree, int, c_strlen_data * = NULL, unsigned = 1); diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c index 3148c6b84d9..235be08d638 100644 --- a/gcc/gimple-fold.c +++ b/gcc/gimple-fold.c @@ -1367,6 +1367,10 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, } } + /* The DECL, COMPONENT_REF, or MEM_REF referencing the string used + to set either MAXBOUND or MAXLEN. */ + tree decl = arg; + if (rkind == SRK_INT_VALUE) { /* We are computing the maximum value (not string length). */ @@ -1379,13 +1383,13 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, { c_strlen_data lendata = { }; val = c_strlen (arg, 1, &lendata, eltsize); - - if (!val && lendata.decl) + decl = lendata.decl; + if (!val && lendata.nonstr) { /* ARG refers to an unterminated const character array. DATA.DECL with size DATA.LEN. */ val = lendata.minlen; - pdata->decl = lendata.decl; + pdata->nonstr = lendata.nonstr; } } @@ -1401,7 +1405,8 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, if (TREE_CODE (arg) == ARRAY_REF) { - tree optype = TREE_TYPE (TREE_OPERAND (arg, 0)); + decl = TREE_OPERAND (arg, 0); + tree optype = TREE_TYPE (decl); /* Determine the "innermost" array type. */ while (TREE_CODE (optype) == ARRAY_TYPE @@ -1547,6 +1552,8 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, if (pdata->maxbound && TREE_CODE (pdata->maxbound) == INTEGER_CST) { + pdata->decl = decl; + /* Adjust the tighter (more optimistic) string length bound if necessary and proceed to adjust the more conservative bound. */ @@ -1559,13 +1566,18 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, pdata->maxbound = val; } else if (pdata->maxbound || maxbound) - /* Set PDATA->MAXBOUND only if it either isn't INTEGER_CST or - if VAL corresponds to the maximum length determined based - on the type of the object. */ - pdata->maxbound = val; + { + /* Set PDATA->MAXBOUND only if it either isn't INTEGER_CST or + if VAL corresponds to the maximum length determined based + on the type of the object. */ + pdata->maxbound = val; + pdata->decl = decl; + } if (tight_bound) { + decl = NULL_TREE; + /* VAL computed above represents an optimistically tight bound on the length of the string based on the referenced object's or subobject's type. Determine the conservative upper bound @@ -1593,6 +1605,7 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, val = build_all_ones_cst (size_type_node); else { + decl = base; val = DECL_SIZE_UNIT (base); val = fold_build2 (MINUS_EXPR, TREE_TYPE (val), val, size_int (offset + 1)); @@ -1613,7 +1626,11 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, return false; if (tree_int_cst_lt (pdata->maxlen, val)) - pdata->maxlen = val; + { + if (decl) + pdata->decl = decl; + pdata->maxlen = val; + } return true; } else if (simple_cst_equal (val, pdata->maxlen) != 1) @@ -1625,6 +1642,9 @@ get_range_strlen_tree (tree arg, bitmap *visited, strlen_range_kind rkind, } pdata->maxlen = val; + if (decl) + pdata->decl = decl; + return rkind == SRK_LENRANGE || !integer_all_onesp (val); } @@ -1816,15 +1836,18 @@ get_maxval_strlen (tree arg, strlen_range_kind rkind, tree *nonstr = NULL) if (nonstr) { - /* For callers prepared to handle unterminated arrays set - *NONSTR to point to the declaration of the array and return - the maximum length/size. */ - *nonstr = lendata.decl; + /* For callers prepared to handle unterminated arrays set *NONSTR + to point to the declaration of the array and return the maximum + length/size. */ + *nonstr = lendata.nonstr; return lendata.maxlen; } - /* Fail if the constant array isn't nul-terminated. */ - return lendata.decl ? NULL_TREE : lendata.maxlen; + if (lendata.nonstr) + /* Fail if the constant array isn't nul-terminated. */ + return NULL_TREE; + + return lendata.maxlen; } @@ -3083,16 +3106,16 @@ gimple_fold_builtin_stpcpy (gimple_stmt_iterator *gsi) if (!len || TREE_CODE (len) != INTEGER_CST) { - data.decl = unterminated_array (src, &size, &exact); - if (!data.decl) + data.nonstr = unterminated_array (src, &size, &exact); + if (!data.nonstr) return false; } - if (data.decl) + if (data.nonstr) { /* Avoid folding calls with unterminated arrays. */ if (!gimple_no_warning_p (stmt)) - warn_string_no_nul (loc, NULL_TREE, "stpcpy", src, data.decl, size, + warn_string_no_nul (loc, NULL_TREE, "stpcpy", src, data.nonstr, size, exact); gimple_set_no_warning (stmt, true); return false; @@ -3846,7 +3869,7 @@ gimple_fold_builtin_strlen (gimple_stmt_iterator *gsi) c_strlen_data lendata = { }; if (get_range_strlen (arg, &lendata, /* eltsize = */ 1) - && !lendata.decl + && !lendata.nonstr && lendata.minlen && TREE_CODE (lendata.minlen) == INTEGER_CST && lendata.maxlen && TREE_CODE (lendata.maxlen) == INTEGER_CST) { diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index ca0572f53d3..dd3000bebb0 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -2053,7 +2053,7 @@ get_string_length (tree str, gimple *stmt, unsigned eltsize, || !tree_fits_uhwi_p (lendata.maxlen)) { fmtresult res; - res.nonstr = lendata.decl; + res.nonstr = lendata.nonstr; return res; } @@ -2063,7 +2063,7 @@ get_string_length (tree str, gimple *stmt, unsigned eltsize, && lenmax <= tree_to_uhwi (lendata.maxlen)) { fmtresult res; - res.nonstr = lendata.decl; + res.nonstr = lendata.nonstr; return res; } @@ -2097,7 +2097,7 @@ get_string_length (tree str, gimple *stmt, unsigned eltsize, max = HOST_WIDE_INT_M1U; fmtresult res (min, max); - res.nonstr = lendata.decl; + res.nonstr = lendata.nonstr; /* Set RES.KNOWNRANGE to true if and only if all strings referenced by STR are known to be bounded (though not necessarily by their diff --git a/gcc/testsuite/gcc.dg/Wstring-compare-4.c b/gcc/testsuite/gcc.dg/Wstring-compare-4.c new file mode 100644 index 00000000000..dee8a316048 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstring-compare-4.c @@ -0,0 +1,189 @@ +/* PR middle-end/98097 - Missing warning about strcmp (char[4], "BARZ") + { dg-do compile } + { dg-options "-O2 -Wall -Wstring-compare" } */ + +extern int strcmp (const char*, const char*); + +struct SAx { char n, a[]; }; +struct SA0 { char n, a[0]; }; +struct SA1 { char n, a[1]; }; +struct SA2 { char n, a[2]; }; +struct SA5 { char n, a[5]; }; + +void sink (void*, ...); +#define T(x) sink (0, (x)) + + +void test_ax (int i) +{ + extern char ax[]; + const char *ps = i ? "" : "123"; + + T (!strcmp (ax, "1")); + T (!strcmp (ax, "12")); + T (!strcmp (ax, "123")); + T (!strcmp (ax, ps)); +} + + +void test_SAx (int i, const struct SAx *px, const struct SA5 *p5) +{ + struct SAx ax; + sink (&ax); + + T (!strcmp (ax.a, "1")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (ax.a, "12")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (ax.a, "123")); // { dg-warning "\\\[-Wstringop-overread" } + + const char *ps = i ? "" : "123"; + T (!strcmp (ax.a, ps)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strcmp (ax.a, px->a)); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (ax.a, p5->a)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strcmp (px->a, "1")); + T (!strcmp (px->a, "12")); + T (!strcmp (px->a, "123")); + + T (!strcmp (px->a, ps)); + T (!strcmp (px->a, p5->a)); +} + + +void test_a0 (int i) +{ + extern char a0[0]; + const char *ps = i ? "" : "123"; + + T (!strcmp (a0, "1")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (a0, "12")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (a0, "123")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (a0, ps)); // { dg-warning "\\\[-Wstringop-overread" } +} + + +void test_SA0 (int i, const struct SA0 *p0, const struct SA5 *p5) +{ + struct SA0 a0; + sink (&a0); + + T (!strcmp (a0.a, "1")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (a0.a, "12")); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (a0.a, "123")); // { dg-warning "\\\[-Wstringop-overread" } + + const char *ps = i ? "" : "123"; + T (!strcmp (a0.a, ps)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strcmp (a0.a, p0->a)); // { dg-warning "\\\[-Wstringop-overread" } + T (!strcmp (a0.a, p5->a)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strcmp (p0->a, "1")); + T (!strcmp (p0->a, "12")); + T (!strcmp (p0->a, "123")); + + T (!strcmp (p0->a, ps)); + T (!strcmp (p0->a, p5->a)); +} + + +void test_a1 (int i) +{ + extern char a1[1]; + const char *ps = i ? "" : "123"; + + T (!strcmp (a1, "1")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a1, "12")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a1, "123")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a1, ps)); +} + + +void test_SA1 (int i, const struct SA1 *p1, const struct SA5 *p5) +{ + struct SA1 a1; + sink (&a1); + + T (!strcmp (a1.a, "1")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a1.a, "12")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a1.a, "123")); // { dg-warning "\\\[-Wstring-compare" } + + /* This should arguably be diagnosed but probably only at a higher + warning level (if one were to be added). */ + const char *ps = i ? "" : "123"; + T (!strcmp (a1.a, ps)); + + T (!strcmp (a1.a, p1->a)); + T (!strcmp (a1.a, p5->a)); + + T (!strcmp (p1->a, "1")); + T (!strcmp (p1->a, "12")); + T (!strcmp (p1->a, "123")); + + T (!strcmp (p1->a, ps)); + T (!strcmp (p1->a, p5->a)); +} + + +void test_a2 (int i) +{ + extern char a2[2]; + const char *ps = i ? "" : "123"; + + T (!strcmp (a2, "1")); + T (!strcmp (a2, "12")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a2, "123")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a2, ps)); +} + + +void test_SA2 (const struct SA2 *p2, const struct SA5 *p5) +{ + struct SA2 a2; + sink (&a2); + + T (!strcmp (a2.a, "1")); + T (!strcmp (a2.a, "12")); // { dg-warning "\\\[-Wstring-compare" } + + T (!strcmp (a2.a, p2->a)); + T (!strcmp (a2.a, p5->a)); + + T (!strcmp (p2->a, "1")); + T (!strcmp (p2->a, "12")); // { dg-warning "\\\[-Wstring-compare" } + + T (!strcmp (p2->a, p5->a)); +} + + +void test_a5 (int i) +{ + extern char a5[5]; + const char *ps = i ? "" : "123"; + + T (!strcmp (a5, "1")); + T (!strcmp (a5, "12")); + T (!strcmp (a5, "123")); + T (!strcmp (a5, "1234")); + T (!strcmp (a5, "12345")); // { dg-warning "\\\[-Wstring-compare" } + T (!strcmp (a5, ps)); +} + + +void test_SA5 (const struct SA5 *p5) +{ + struct SA5 a5; + sink (&a5); + + T (!strcmp (a5.a, "1")); + T (!strcmp (a5.a, "12")); + T (!strcmp (a5.a, "123")); + T (!strcmp (a5.a, "1234")); + T (!strcmp (a5.a, "12345")); // { dg-warning "\\\[-Wstring-compare" } + + T (!strcmp (a5.a, p5->a)); + + T (!strcmp (p5->a, "1")); + T (!strcmp (p5->a, "12")); + T (!strcmp (p5->a, "123")); + T (!strcmp (p5->a, "1234")); + T (!strcmp (p5->a, "12345")); // { dg-warning "\\\[-Wstring-compare" } +} diff --git a/gcc/testsuite/gcc.dg/Wstring-compare-5.c b/gcc/testsuite/gcc.dg/Wstring-compare-5.c new file mode 100644 index 00000000000..14c0c02f93f --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstring-compare-5.c @@ -0,0 +1,213 @@ +/* PR middle-end/98097 - Missing warning about strcmp (char[4], "BARZ") + { dg-do compile } + { dg-options "-O2 -Wall -Wstring-compare" } */ + +typedef __SIZE_TYPE__ size_t; + +extern int strncmp (const char*, const char*, size_t); + +struct SAx { char n, a[]; }; +struct SA0 { char n, a[0]; }; +struct SA1 { char n, a[1]; }; +struct SA2 { char n, a[2]; }; +struct SA5 { char n, a[5]; }; + +void sink (void*, ...); +#define T(x) sink (0, (x)) + + +void test_ax (int i) +{ + extern char ax[]; + const char *ps = i ? "" : "123"; + + T (!strncmp (ax, "1", 1)); + T (!strncmp (ax, "12", 2)); + T (!strncmp (ax, "123", 3)); + T (!strncmp (ax, "123", 9)); + T (!strncmp (ax, ps, 9)); +} + + +void test_SAx (int i, const struct SAx *px, const struct SA5 *p5) +{ + struct SAx ax; + sink (&ax); + + T (!strncmp (ax.a, "12", 2)); // { dg-warning "\\\[-Wstringop-overread" "pr98553" { xfail *-*-* } } + T (!strncmp (ax.a, "123", 3)); // { dg-warning "\\\[-Wstringop-overread" "pr98553" { xfail *-*-* } } + + const char *ps = i ? "" : "123"; + T (!strncmp (ax.a, ps, 2)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strncmp (ax.a, px->a, 2)); // { dg-warning "\\\[-Wstringop-overread" } + T (!strncmp (ax.a, p5->a, 3)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strncmp (px->a, "1", 1)); + T (!strncmp (px->a, "12", 2)); + T (!strncmp (px->a, "123", 3)); + T (!strncmp (px->a, "123", 9)); + + T (!strncmp (px->a, ps, 4)); + T (!strncmp (px->a, p5->a, 5)); + T (!strncmp (px->a, p5->a, 6)); +} + + +void test_a0 (int i) +{ + extern char a0[0]; + const char *ps = i ? "" : "123"; + + T (!strncmp (a0, "12", 2)); // { dg-warning "\\\[-Wstringop-overread" "pr98553" { xfail *-*-* } } + T (!strncmp (a0, "123", 3)); // { dg-warning "\\\[-Wstringop-overread" "pr98553" { xfail *-*-* } } + T (!strncmp (a0, ps, 3)); // { dg-warning "\\\[-Wstringop-overread" } +} + + +void test_SA0 (int i, const struct SA0 *p0, const struct SA5 *p5) +{ + struct SA0 a0; + sink (&a0); + + T (!strncmp (a0.a, "12", 2)); // { dg-warning "\\\[-Wstringop-overread" "pr98553" { xfail *-*-* } } + T (!strncmp (a0.a, "123", 3)); // { dg-warning "\\\[-Wstringop-overread" "pr98553" { xfail *-*-* } } + + const char *ps = i ? "" : "123"; + T (!strncmp (a0.a, ps, 3)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strncmp (a0.a, p0->a, 3)); // { dg-warning "\\\[-Wstringop-overread" } + T (!strncmp (a0.a, p5->a, 4)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strncmp (p0->a, "1", 1)); + T (!strncmp (p0->a, "12", 2)); + T (!strncmp (p0->a, "123", 3)); + T (!strncmp (p0->a, "123", 9)); + + T (!strncmp (p0->a, ps, 4)); + T (!strncmp (p0->a, p5->a, 4)); + // This should probably be diagnosed. + T (!strncmp (p0->a, p5->a, 9)); +} + + +void test_a1 (int i) +{ + extern char a1[1]; + const char *ps = i ? "" : "123"; + + T (!strncmp (a1, "12", 2)); // { dg-warning "\\\[-Wstring-compare" } + T (!strncmp (a1, "123", 3)); // { dg-warning "\\\[-Wstring-compare" } + T (!strncmp (a1, ps, 4)); +} + + +void test_SA1 (int i, const struct SA1 *p1, const struct SA5 *p5) +{ + struct SA1 a1; + sink (&a1); + + T (!strncmp (a1.a, "12", 2)); // { dg-warning "\\\[-Wstring-compare" } + T (!strncmp (a1.a, "123", 3)); // { dg-warning "\\\[-Wstring-compare" } + + /* This should arguably be diagnosed but probably only at a higher + warning level (if one were to be added). */ + const char *ps = i ? "" : "123"; + T (!strncmp (a1.a, ps, 3)); + + T (!strncmp (a1.a, p1->a, 3)); + T (!strncmp (a1.a, p5->a, 3)); + T (!strncmp (a1.a, p5->a, 7)); // { dg-warning "\\\[-Wstringop-overread" } + + T (!strncmp (p1->a, "1", 1)); + T (!strncmp (p1->a, "12", 2)); + T (!strncmp (p1->a, "123", 3)); + + T (!strncmp (p1->a, ps, 3)); + T (!strncmp (p1->a, p5->a, 4)); + /* This should probably be diagnosed for a similar reason as is + strncmp (p1->a, p5->a, 7), even though p1->a's length is unknown + and unbounded. */ + T (!strncmp (p1->a, p5->a, 7)); +} + + +void test_a2 (int i) +{ + extern char a2[2]; + const char *ps = i ? "" : "123"; + + T (!strncmp (a2, "12", 2)); + T (!strncmp (a2, "123", 3)); // { dg-warning "\\\[-Wstring-compare" } + T (!strncmp (a2, ps, 2)); + T (!strncmp (a2, ps, 3)); +} + + +void test_SA2 (const struct SA2 *p2, const struct SA5 *p5) +{ + struct SA2 a2; + sink (&a2); + + T (!strncmp (a2.a, "12", 2)); + T (!strncmp (a2.a, "123", 3)); // { dg-warning "\\\[-Wstring-compare" } + + T (!strncmp (a2.a, p2->a, 2)); + T (!strncmp (a2.a, p5->a, 4)); + + T (!strncmp (p2->a, "12", 2)); + T (!strncmp (p2->a, "123", 3)); // { dg-warning "\\\[-Wstring-compare" } + + T (!strncmp (p2->a, p5->a, 4)); + T (!strncmp (p2->a, p5->a, 7)); // { dg-warning "\\\[-Wstringop-overread" } +} + + +void test_a5 (int i, const char *s) +{ + extern char a5[5]; + + T (!strncmp (a5, "12", 2)); + T (!strncmp (a5, "12345", 5)); + T (!strncmp (a5, "123456", 6)); // { dg-warning "\\\[-Wstring-compare" } + + { + const char *ps = i ? "" : "123"; + T (!strncmp (a5, ps, 3)); + T (!strncmp (a5, ps, 4)); + T (!strncmp (a5, ps, 5)); + } + + { + extern char ax[]; + const char *ps = i ? ax : "123"; + + T (!strncmp (a5, ps, 3)); + T (!strncmp (a5, ps, 4)); + T (!strncmp (a5, ps, 5)); + } + + { + const char a[] = "12345"; + const char *ps = i ? "abc" : s; + T (!strncmp (ps, a, sizeof a - 1)); + } +} + + +void test_SA5 (const struct SA5 *p5) +{ + struct SA5 a5; + sink (&a5); + + T (!strncmp (a5.a, "12", 2)); + T (!strncmp (a5.a, "12345", 5)); + T (!strncmp (a5.a, "123456", 6)); // { dg-warning "\\\[-Wstring-compare" } + + T (!strncmp (a5.a, p5->a, 4)); + T (!strncmp (a5.a, p5->a, 5)); + + T (!strncmp (p5->a, "12", 2)); + T (!strncmp (p5->a, "12345", 5)); + T (!strncmp (p5->a, "123456", 6));// { dg-warning "\\\[-Wstring-compare" } +} diff --git a/gcc/testsuite/gcc.dg/Wstring-compare.c b/gcc/testsuite/gcc.dg/Wstring-compare.c index d1534bf7555..44a7c97b516 100644 --- a/gcc/testsuite/gcc.dg/Wstring-compare.c +++ b/gcc/testsuite/gcc.dg/Wstring-compare.c @@ -120,8 +120,7 @@ void strcmp_array_copy (void) void strcmp_member_array_lit (const struct S *p) { - // Not handled due to the fix for PR 92756. - T (p->a4, "1234"); // { dg-warning "length 4 and an array of size 4 " "pr92765" { xfail *-*-* } } + T (p->a4, "1234"); // { dg-warning "length 4 and an array of size 4 " "pr92765" } } diff --git a/gcc/testsuite/gcc.dg/strcmpopt_10.c b/gcc/testsuite/gcc.dg/strcmpopt_10.c index d7f94ac4d52..f094aed3930 100644 --- a/gcc/testsuite/gcc.dg/strcmpopt_10.c +++ b/gcc/testsuite/gcc.dg/strcmpopt_10.c @@ -117,7 +117,9 @@ void f2_memptr (void) struct A2 *p = (struct A2*)b.p; - if (__builtin_strncmp (p->a, "0123456789", 10) == 0) + /* The call triggers a warning based p->a's size but should not be + folded because p->a is a trailing array. */ + if (__builtin_strncmp (p->a, "0123456789", 10) == 0) // { dg-warning "\\\[-Wstring-compare" } { extern void memptr_test (void); memptr_test (); diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c index 522b2d45b3a..b6b5b0af304 100644 --- a/gcc/tree-ssa-strlen.c +++ b/gcc/tree-ssa-strlen.c @@ -1053,10 +1053,10 @@ get_range_strlen_dynamic (tree src, gimple *stmt, if (get_range_strlen_dynamic (arg, phi, &argdata, visited, rvals, pssa_def_max)) { - /* Set the DECL of an unterminated array this argument + /* Set NONSTR of an unterminated array this argument refers to if one hasn't been found yet. */ - if (!pdata->decl && argdata.decl) - pdata->decl = argdata.decl; + if (!pdata->nonstr && argdata.nonstr) + pdata->nonstr = argdata.nonstr; if (!argdata.minlen || (integer_zerop (argdata.minlen) @@ -1090,6 +1090,9 @@ get_range_strlen_dynamic (tree src, gimple *stmt, pdata->maxlen = build_all_ones_cst (size_type_node); } + /* Store the whole PHI as the DECL, overwriting any more + "specific" DECL set above for any of the arguments. */ + pdata->decl = src; return true; } } @@ -3880,20 +3883,21 @@ get_len_or_size (gimple *stmt, tree arg, int idx, unsigned HOST_WIDE_INT *size, bool *nulterm, range_query *rvals) { - /* Invalidate. */ - *size = HOST_WIDE_INT_M1U; - if (idx < 0) { /* IDX is the inverted constant string length. */ lenrng[0] = ~idx; lenrng[1] = lenrng[0]; + *size = lenrng[0] + 1; *nulterm = true; return true; } - /* Set so that both LEN and ~LEN are invalid lengths, i.e., maximum - possible length + 1. */ + /* Invalidate. */ + *size = HOST_WIDE_INT_M1U; + + /* Set so that both LEN and ~LEN are invalid lengths, i.e., larger + than maximum possible length. */ lenrng[0] = lenrng[1] = HOST_WIDE_INT_MAX; if (strinfo *si = idx ? get_strinfo (idx) : NULL) @@ -3944,23 +3948,41 @@ get_len_or_size (gimple *stmt, tree arg, int idx, unsigned HOST_WIDE_INT minlen = tree_to_uhwi (lendata.minlen); unsigned HOST_WIDE_INT maxlen = tree_to_uhwi (lendata.maxlen); - /* The longest string in this data model. */ - const unsigned HOST_WIDE_INT lenmax - = tree_to_uhwi (max_object_size ()) - 2; - if (maxbound == HOST_WIDE_INT_M1U) { lenrng[0] = minlen; lenrng[1] = maxlen; *nulterm = minlen == maxlen; } - else if (maxlen < lenmax) + else { + /* The longest string in this data model. */ + const unsigned HOST_WIDE_INT lenmax + = tree_to_uhwi (max_object_size ()) - 2; + + if (lenmax <= maxlen) + { + if (!maxbound) + /* Bail for member arrays of size 1. */ + return false; + + if (lendata.decl && TREE_CODE (lendata.decl) == SSA_NAME) + /* Bail if ARG is or involves a PHI with at least one + string of an unknown length. */ + return false; + + /* As a special case, make the range both invalid and inverted + to let the caller avoid folding on the basis of array size + while still enabling warnings. */ + lenrng[1] = 0; + } + + /* MAXBOUND is the length of the longest string in a PHI or + the maximum possible length determined based on the size + of the largest array. Bump it up by 1 to reflect the size. */ *size = maxbound + 1; *nulterm = false; } - else - return false; return true; } @@ -3987,7 +4009,10 @@ get_len_or_size (gimple *stmt, tree arg, int idx, the array pointer to by the argument, set *PLEN and *PSIZE to the corresponding length (or its complement when the string is known to be at least as long and need not be nul-terminated) and size. - Otherwise return null. */ + If the result were to be zero based on the string lengths constrained + to the sizes of the member character arrays they are stored in return + void_node instead to let the caller issue a warning but avoid folding + the result of the call. Otherwise return null. */ static tree strxcmp_eqz_result (gimple *stmt, tree arg1, int idx1, tree arg2, int idx2, @@ -4003,7 +4028,24 @@ strxcmp_eqz_result (gimple *stmt, tree arg1, int idx1, tree arg2, int idx2, || !get_len_or_size (stmt, arg2, idx2, len2rng, &siz2, &nul2, rvals)) return NULL_TREE; - /* BOUND is set to HWI_M1U for strcmp and less to strncmp, and LENiRNG + /* LENiRNG is set to the inverted maximum range below for trailing + arrays where folding on the basis of member array size is disabled + but warning is enabled the same as for non-member arrays. */ + tree mos_def_zero = integer_zero_node; + if (len1rng[0] == HOST_WIDE_INT_MAX && len1rng[1] == 0) + { + /* Restore the upper bound to make the range valid. */ + len1rng[1] = HOST_WIDE_INT_MAX; + mos_def_zero = void_node; + } + + if (len2rng[0] == HOST_WIDE_INT_MAX && len2rng[1] == 0) + { + len2rng[1] = HOST_WIDE_INT_MAX; + mos_def_zero = void_node; + } + + /* BOUND is set to HWI_M1U for strcmp and less for strncmp, and LENiRNG to HWI_MAX when invalid. Adjust the length of each string to consider to be no more than BOUND. */ if (len1rng[0] < HOST_WIDE_INT_MAX && len1rng[0] > bound) @@ -4033,7 +4075,7 @@ strxcmp_eqz_result (gimple *stmt, tree arg1, int idx1, tree arg2, int idx2, nul-terminated or to the complement of its minimum length otherwise, */ len[1] = nul2 ? len2rng[0] : ~len2rng[0]; - return integer_zero_node; + return mos_def_zero; } if (len2rng[0] == HOST_WIDE_INT_MAX @@ -4044,7 +4086,7 @@ strxcmp_eqz_result (gimple *stmt, tree arg1, int idx1, tree arg2, int idx2, *psize = siz2; len[0] = nul1 ? len1rng[0] : ~len1rng[0]; len[1] = len2rng[0]; - return integer_zero_node; + return mos_def_zero; } /* The strings are also definitely unequal when their lengths are unequal @@ -4061,7 +4103,7 @@ strxcmp_eqz_result (gimple *stmt, tree arg1, int idx1, tree arg2, int idx2, len[0] = len1rng[0]; len[1] = len2rng[0]; - return integer_zero_node; + return mos_def_zero; } /* The string lengths may be equal or unequal. Even when equal and @@ -4197,6 +4239,12 @@ handle_builtin_string_cmp (gimple_stmt_iterator *gsi, range_query *rvals) if (tree eqz = strxcmp_eqz_result (stmt, arg1, idx1, arg2, idx2, bound, len, &siz, rvals)) { + if (eqz == void_node) + { + maybe_warn_pointless_strcmp (stmt, bound, len, siz); + return false; + } + if (integer_zerop (eqz)) { maybe_warn_pointless_strcmp (stmt, bound, len, siz);