From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22a.google.com (mail-oi1-x22a.google.com [IPv6:2607:f8b0:4864:20::22a]) by sourceware.org (Postfix) with ESMTPS id 538603858D38 for ; Wed, 11 Jan 2023 13:14:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 538603858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oi1-x22a.google.com with SMTP id h185so12684159oif.5 for ; Wed, 11 Jan 2023 05:14:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:organization:from:references:to:content-language :subject:user-agent:mime-version:date:message-id:from:to:cc:subject :date:message-id:reply-to; bh=/Xk71iAGkquZSuPRZz4vR6USEcaf9fOeLRalXxoPZgE=; b=Maq3R55jplHogo21MYaizm6JR/WhTQ0T0FhxB/X7kUGsqDUJLg8M8NoItGg/ADKQp1 U/ZBUSlaXqqM96n/3frnxWHCchTcSgB6+RNW1u48GPJqOtV9+DPgFmNo9cfTortfyQxm tEgXrRVs8OBxYLw8Bw8QDMiIs1JVu6VX/0gpwn9bNs3HyPi2cQO8JzvdFzbz6/EcxeXk cb/2yjS328+AWL76YTVEXe+yMGg6DXin4x8+IQSNH4trKckeIkh4MM3B2eqSOvpTjJHW AGepSIuoeR4KnFAZV/POMGx50gQrOpB+8isSE2AuNVXVnkJa/18IFRZhVAFuuy56ZJyV DyOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:organization:from:references:to:content-language :subject:user-agent:mime-version:date:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=/Xk71iAGkquZSuPRZz4vR6USEcaf9fOeLRalXxoPZgE=; b=VF39375rCeZG8cZY8P5kwPjykiPNZF4kU8swc/0Wh9yuzY54wYLDHedKohp3g/1YgG CMGKLOfEfl8wtCw00tRoqLyME8dVAHJZxMQDyhSY5jaCWJkHn3ywZ+5TYrmy0AYxeO8e 3aFZASwZBNNM+JL4Hu0aXRC513p5FJ2wb5Fca8YMHS6VA9fxyNII2XMCo7i5V+CvcUsf DhqVgpB5yGtd1A718fWnvTAzDTrSQ5PSo9Kf35b1MK4LHCDpedI5A+1mXCgqZg6zAlJA VfuQmOCh1epjxBGZYkfGPeayQnmVwiwwDea5/DYeQpIK/FeRdCoUPTu1X2BPJG948lcO aT+A== X-Gm-Message-State: AFqh2kqVpXMlW6MygewUb0Qxq5+z0p8bdnpiUxlMSP0DBzUv82Aq3IsV AzvuCPD3O+urhelzwR1QJH74ww== X-Google-Smtp-Source: AMrXdXv9TcYl74atP3bqc0l7fc/LWTxoH5rl6xkHOm8uPnGTBzJS68Vx3BProOqRRd9CTIcMiS2q4Q== X-Received: by 2002:a05:6808:1496:b0:35e:b434:93e6 with SMTP id e22-20020a056808149600b0035eb43493e6mr1065235oiw.37.1673442863145; Wed, 11 Jan 2023 05:14:23 -0800 (PST) Received: from ?IPV6:2804:1b3:a7c0:a93a:9160:47f0:6805:e35d? ([2804:1b3:a7c0:a93a:9160:47f0:6805:e35d]) by smtp.gmail.com with ESMTPSA id g19-20020a544f93000000b00363ef79e2a1sm6604881oiy.31.2023.01.11.05.14.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 11 Jan 2023 05:14:22 -0800 (PST) Content-Type: multipart/mixed; boundary="------------lzwZ1k8tlGBDQb8Eg55pfCAh" Message-ID: <7c9799f2-868c-c954-a59e-36a31701ebb7@linaro.org> Date: Wed, 11 Jan 2023 10:14:19 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH 3/4] string: Suppress -Wmaybe-unitialized for wordcopy [BZ #19444] Content-Language: en-US To: Carlos O'Donell , libc-alpha@sourceware.org References: <20221229125802.2715435-1-adhemerval.zanella@linaro.org> <20221229125802.2715435-4-adhemerval.zanella@linaro.org> <773a0c5e-406b-c3c0-a19d-77985a2d54ac@redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <773a0c5e-406b-c3c0-a19d-77985a2d54ac@redhat.com> X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: This is a multi-part message in MIME format. --------------lzwZ1k8tlGBDQb8Eg55pfCAh Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 10/01/23 19:47, Carlos O'Donell wrote: > On 12/29/22 07:58, Adhemerval Zanella via Libc-alpha wrote: > > I think this particular change needs a v2 and some more detailed analysis. > >> With GCC 6+ when compiling with -O1 warns that some MERGE macro usage >> might be used uninitialized. The issue is calling the function with >> len equal to 0 is undefined since the first 'switch' will not trigger >> any case and then subsequent loop will potentially use uninitialized >> variables. > > The comment does not seem to match what the code does. > > Calling the function with len equal to 0 results in 'case 0' executing in all of these > cases. > >> However all usages on mem routines always called the function for >> sizes larger than OP_T_THRES. > > This isn't always the case, you could have a call to copy just 8 bytes, which is > below the 16-byt OP_T_THRES. The usage are in fact all done on generic mem functions. The _wordcopy_fwd_aligned and/or _wordcopy_fwd_dest_aligned are called by the WORD_COPY_FWD which, used by both generic memcpy and memmove. And both implementations only call the macro for length larger than OP_T_THRES. This is similar for WORD_COPY_BWD, which is only used for memmove.c. > >> --- >> string/wordcopy.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/string/wordcopy.c b/string/wordcopy.c >> index d05718322c..3b6344115d 100644 >> --- a/string/wordcopy.c >> +++ b/string/wordcopy.c >> @@ -18,8 +18,19 @@ >> >> /* BE VERY CAREFUL IF YOU CHANGE THIS CODE...! */ >> >> +#include >> #include >> +#include >> +/* With GCC 6 when compiling with -O1 warns that some MERGE macro usage might >> + be used uninitialized. The issue is calling the function with len equal to >> + 0 is undefined since the first 'switch' will not trigger any case and then >> + subsequent loop will potentially use uninitialized variables. However all >> + usages on mem routines always called the function for sizes larger than > > This comment does not match what the code does. The switch 'case 0:' case should execute. Right, reading again the code I think it might be indeed an issue with the sparc backend since it is the only arch that triggers it, even though powerpc also build the same code for both 64 and 32 bits. What about: /* Compiling with -O1 might warn that 'a2' and 'a3' may be used uninitialized. However, 'do3' (which uses 'a3') is only reachable either by 'case 0' or 'case 1', which initializes 'a3' (and it is also set at 'do1' for subsequent loop iterations). This is similar for 'do4' (which uses 'a2') that is only reachable by 'case 1' which initializes 'a2'). Since the usage is within the MERGE macro, we need to disable the warning on its definition. */ > >> + OP_T_THRES. */ >> +DIAG_PUSH_NEEDS_COMMENT; >> +DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized"); >> #include >> +DIAG_POP_NEEDS_COMMENT; >> >> /* _wordcopy_fwd_aligned -- Copy block beginning at SRCP to >> block beginning at DSTP with LEN `op_t' words (not LEN bytes!). >> @@ -94,7 +105,11 @@ WORDCOPY_FWD_ALIGNED (long int dstp, long int srcp, size_t len) >> { >> do8: >> a0 = ((op_t *) srcp)[0]; >> + /* Check the comment on memcopy.h inclusion. */ >> + DIAG_PUSH_NEEDS_COMMENT; >> + DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized"); >> ((op_t *) dstp)[0] = a1; >> + DIAG_POP_NEEDS_COMMENT; > > Why is a1 considered uninitialized? > > The switch has case statements for every possible value. > > In case 1 we unconditionally set a1. > > We can't enter case 1 unless len was exactly 1 or any value of 1+8*n for valid n. > > It seems like the compiler can't see that 'OP_T_THRES <= 3 * OPSIZ' is always a true. I think it is unlikely because both OP_T_THRES and OPSIZ are constant macros, although it might be case that the sparc backend is doing something fuzzy that is maybe confusing some other passes. I changed to: /* Compiling with -O1 might warn that 'a1' in 'do8' switch may be used uninitialized. However, 'do8' is only reachable through 'case 1', since all possible modulo value are handling in the initial switch). */ > >> do7: >> a1 = ((op_t *) srcp)[1]; >> ((op_t *) dstp)[1] = a0; >> @@ -291,7 +306,11 @@ WORDCOPY_BWD_ALIGNED (long int dstp, long int srcp, size_t len) >> { >> do8: >> a0 = ((op_t *) srcp)[7]; >> + /* Check the comment on memcopy.h inclusion. */ >> + DIAG_PUSH_NEEDS_COMMENT; >> + DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized"); >> ((op_t *) dstp)[7] = a1; >> + DIAG_POP_NEEDS_COMMENT; > > Likewise. > >> do7: >> a1 = ((op_t *) srcp)[6]; >> ((op_t *) dstp)[6] = a0; > I am attaching the fixed patch. --------------lzwZ1k8tlGBDQb8Eg55pfCAh Content-Type: text/plain; charset=UTF-8; name="0003-string-Suppress-Wmaybe-unitialized-for-wordcopy-BZ-1.patch" Content-Disposition: attachment; filename*0="0003-string-Suppress-Wmaybe-unitialized-for-wordcopy-BZ-1.pa"; filename*1="tch" Content-Transfer-Encoding: base64 RnJvbSA4NmI4M2RhOGM3ODM2NDg0NjU0ZmJhY2FmNTRmNzY0ZGYzNzhhMzFlIE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBBZGhlbWVydmFsIFphbmVsbGEgPGFkaGVtZXJ2YWwu emFuZWxsYUBsaW5hcm8ub3JnPgpEYXRlOiBUaHUsIDI5IERlYyAyMDIyIDA5OjE1OjQ5IC0w MzAwClN1YmplY3Q6IFtQQVRDSCAzLzRdIHN0cmluZzogU3VwcHJlc3MgLVdtYXliZS11bml0 aWFsaXplZCBmb3Igd29yZGNvcHkgW0JaCiAjMTk0NDRdCgpUaGUgR0NDIDYrIHdoZW4gY29t cGlsaW5nIGZvciBzcGFyYyB3YXJucyB0aGF0IHNvbWUgdmFyaWFibGVzIG1pZ2h0IGJlCnVz ZWQgdW5pbml0aWFsaXplZC4gIEhvd2V2ZXIgaXQgZG9lcyBub3Qgc2VlbSB0aGUgZmFjdCwg c2luY2UgdGhlCnZhcmlhYmxlcyBhcmUgcmVhbGx5IGluaXRpYWxpemVkIChhbmQgYWxzbyBv dGhlciB0YXJnZXRzIHRoYXQgdXNlIHRoZQpzYW1lIGNvZGUsIGxpa2UgcG93ZXJwYywgZG8g bm90IHdhcm4gYWJvdXQgaXQpLgoKU28gc3VwcHJlc3MgdGhlIHdhcm5pbmcgZm9yIG5vdy4K LS0tCiBzdHJpbmcvd29yZGNvcHkuYyB8IDI0ICsrKysrKysrKysrKysrKysrKysrKysrKwog MSBmaWxlIGNoYW5nZWQsIDI0IGluc2VydGlvbnMoKykKCmRpZmYgLS1naXQgYS9zdHJpbmcv d29yZGNvcHkuYyBiL3N0cmluZy93b3JkY29weS5jCmluZGV4IGFlNWNjZDc5M2MuLmQwY2Zj MjIwODIgMTAwNjQ0Ci0tLSBhL3N0cmluZy93b3JkY29weS5jCisrKyBiL3N0cmluZy93b3Jk Y29weS5jCkBAIC0xOSw3ICsxOSwxMiBAQAogLyogQkUgVkVSWSBDQVJFRlVMIElGIFlPVSBD SEFOR0UgVEhJUyBDT0RFLi4uISAgKi8KIAogI2luY2x1ZGUgPHN0ZGRlZi5oPgorI2luY2x1 ZGUgPGxpYmMtZGlhZy5oPgorLyogQ2hlY2sgY29tbWVudCBvZiBXT1JEQ09QWV9GV0RfREVT VF9BTElHTkVELiAgKi8KK0RJQUdfUFVTSF9ORUVEU19DT01NRU5UOworRElBR19JR05PUkVf TkVFRFNfQ09NTUVOVCAoNiwgIi1XbWF5YmUtdW5pbml0aWFsaXplZCIpOwogI2luY2x1ZGUg PG1lbWNvcHkuaD4KK0RJQUdfUE9QX05FRURTX0NPTU1FTlQ7CiAKIC8qIF93b3JkY29weV9m d2RfYWxpZ25lZCAtLSBDb3B5IGJsb2NrIGJlZ2lubmluZyBhdCBTUkNQIHRvCiAgICBibG9j ayBiZWdpbm5pbmcgYXQgRFNUUCB3aXRoIExFTiBgb3BfdCcgd29yZHMgKG5vdCBMRU4gYnl0 ZXMhKS4KQEAgLTk0LDcgKzk5LDE0IEBAIFdPUkRDT1BZX0ZXRF9BTElHTkVEIChsb25nIGlu dCBkc3RwLCBsb25nIGludCBzcmNwLCBzaXplX3QgbGVuKQogICAgIHsKICAgICBkbzg6CiAg ICAgICBhMCA9ICgob3BfdCAqKSBzcmNwKVswXTsKKyAgICAgIC8qIENvbXBpbGluZyB3aXRo IC1PMSBtYXkgd2FybiB0aGF0ICdhMScgaW4gJ2RvOCcgc3dpdGNoIG1heSBiZSB1c2VkCisJ IHVuaW5pdGlhbGl6ZWQuICBIb3dldmVyLCAnZG84JyBpcyBvbmx5IHJlYWNoYWJsZSB0aHJv dWdoICdjYXNlIDEnLAorCSBzaW5jZSBhbGwgcG9zc2libGUgbW9kdWxvIHZhbHVlcyBhcmUg aGFuZGxpbmcgaW4gdGhlIGluaXRpYWwKKwkgc3dpdGNoKS4gICovCisgICAgICBESUFHX1BV U0hfTkVFRFNfQ09NTUVOVDsKKyAgICAgIERJQUdfSUdOT1JFX05FRURTX0NPTU1FTlQgKDYs ICItV21heWJlLXVuaW5pdGlhbGl6ZWQiKTsKICAgICAgICgob3BfdCAqKSBkc3RwKVswXSA9 IGExOworICAgICAgRElBR19QT1BfTkVFRFNfQ09NTUVOVDsKICAgICBkbzc6CiAgICAgICBh MSA9ICgob3BfdCAqKSBzcmNwKVsxXTsKICAgICAgICgob3BfdCAqKSBkc3RwKVsxXSA9IGEw OwpAQCAtMTkwLDYgKzIwMiwxNCBAQCBXT1JEQ09QWV9GV0RfREVTVF9BTElHTkVEIChsb25n IGludCBkc3RwLCBsb25nIGludCBzcmNwLCBzaXplX3QgbGVuKQogICAgICAgZ290byBkbzQ7 CQkJLyogTm8tb3AuICAqLwogICAgIH0KIAorICAvKiBDb21waWxpbmcgd2l0aCAtTzEgbWln aHQgd2FybiB0aGF0ICdhMicgYW5kICdhMycgbWF5IGJlIHVzZWQKKyAgICAgdW5pbml0aWFs aXplZC4gIEhvd2V2ZXIsICdkbzMnICh3aGljaCB1c2VzICdhMycpIGlzIG9ubHkgcmVhY2hh YmxlIGVpdGhlcgorICAgICBieSAnY2FzZSAwJyBvciAnY2FzZSAxJywgd2hpY2ggaW5pdGlh bGl6ZXMgJ2EzJyAoYW5kIGl0IGlzIGFsc28gc2V0IGF0CisgICAgICdkbzEnIGZvciBzdWJz ZXF1ZW50IGxvb3AgaXRlcmF0aW9ucykuICBUaGlzIGlzIHNpbWlsYXIgZm9yICdkbzQnCisg ICAgICh3aGljaCB1c2VzICdhMicpIHRoYXQgaXMgb25seSByZWFjaGFibGUgYnkgJ2Nhc2Ug MScgd2hpY2ggaW5pdGlhbGl6ZXMKKyAgICAgJ2EyJykuCisgICAgIFNpbmNlIHRoZSB1c2Fn ZSBpcyB3aXRoaW4gdGhlIE1FUkdFIG1hY3JvLCB3ZSBuZWVkIHRvIGRpc2FibGUgdGhlIHdh cm5pbmcKKyAgICAgb24gaXRzIGRlZmluaXRpb24uICAqLwogICBkbwogICAgIHsKICAgICBk bzQ6CkBAIC0yOTEsNyArMzExLDExIEBAIFdPUkRDT1BZX0JXRF9BTElHTkVEIChsb25nIGlu dCBkc3RwLCBsb25nIGludCBzcmNwLCBzaXplX3QgbGVuKQogICAgIHsKICAgICBkbzg6CiAg ICAgICBhMCA9ICgob3BfdCAqKSBzcmNwKVs3XTsKKyAgICAgIC8qIENoZWNrIHRoZSBjb21t ZW50IG9uIFdPUkRDT1BZX0ZXRF9BTElHTkVELiAgKi8KKyAgICAgIERJQUdfUFVTSF9ORUVE U19DT01NRU5UOworICAgICAgRElBR19JR05PUkVfTkVFRFNfQ09NTUVOVCAoNiwgIi1XbWF5 YmUtdW5pbml0aWFsaXplZCIpOwogICAgICAgKChvcF90ICopIGRzdHApWzddID0gYTE7Cisg ICAgICBESUFHX1BPUF9ORUVEU19DT01NRU5UOwogICAgIGRvNzoKICAgICAgIGExID0gKChv cF90ICopIHNyY3ApWzZdOwogICAgICAgKChvcF90ICopIGRzdHApWzZdID0gYTA7Ci0tIAoy LjM0LjEKCg== --------------lzwZ1k8tlGBDQb8Eg55pfCAh--