From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id E613D3858C83 for ; Wed, 11 Jan 2023 20:13:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E613D3858C83 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673467983; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wrJ0wCOQWRmsg7iplG6YAp4Wq/40lkUNk8w7w0pPnAg=; b=PpBJk9cW7n7mPEbEX51IU7vvjn3BZgC4PhpeLIHQSKhAl2HTVrJKgXcRGtlUZgihRlhpap EWrOUOKFpJ+lnwfCB65v5AT+WDCv5bV5dfrhSmQx3Ey0QxlNRSLlba50E4xdrsSZFvXBjW nbAriR3kLRD40qlj+a6dgHEUXCPHDns= Received: from mail-io1-f71.google.com (mail-io1-f71.google.com [209.85.166.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-653-X9skjl3KO0e_ztbAIZf_AA-1; Wed, 11 Jan 2023 15:13:01 -0500 X-MC-Unique: X9skjl3KO0e_ztbAIZf_AA-1 Received: by mail-io1-f71.google.com with SMTP id y24-20020a5ec818000000b006e2c0847835so9759491iol.12 for ; Wed, 11 Jan 2023 12:13:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding: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=wrJ0wCOQWRmsg7iplG6YAp4Wq/40lkUNk8w7w0pPnAg=; b=yfXBN+Xs/EahyzMBBcxaciF1ARAWQlLN63rJ4ZKqTum4sg1ctDWsmjpt3kSSJ6EDrh ITgWGDpHT0EofJHD1iYOijibgs+YB7MELzEPM3Cf5D3u+gh68kNa2AXeyw04wi50C8a5 uhpEHkLYBdZ4by5uYsGoNnkQ7ifE1yw1+bGy/AtiXEldbVkuqBPJ2XefFdUwZtJK8OFL ntprAzHlRkCdAAfz8rV/C9oS6vkuuOhLRxvgIC9Vsgczk+kmvbqjAQobZ0FRE8u1khhd 3ZYUErDlj0/kUsnqeO97ARXLDTxZzPI+kLuJqQqb+nOkx1jw5vRQOCAYd/FDWFlsEdYc J3tA== X-Gm-Message-State: AFqh2krXufp9Uxk1Rmzw3x5Hd7Mf7r+6w2dnY7qUYcLN/sjbrgj5Ov1N QFOBykqpOzpCkzkh7KI3wNrB/qsL1ZWZOHbA4n1UpWdCcrdLnu4qE13r7ZEPXPsyoDEDc9xDeOI 5p07Fucnwk2W1Ks7iu50g X-Received: by 2002:a92:d1c1:0:b0:30d:899f:aed4 with SMTP id u1-20020a92d1c1000000b0030d899faed4mr15882958ilg.21.1673467980341; Wed, 11 Jan 2023 12:13:00 -0800 (PST) X-Google-Smtp-Source: AMrXdXucAtE0N+QAzOdWsIKb9XZ/X/kJXE/SZSeBvP9xizrDKe6BkwnPyG2BN52FSGaRYIBUHf9RxQ== X-Received: by 2002:a92:d1c1:0:b0:30d:899f:aed4 with SMTP id u1-20020a92d1c1000000b0030d899faed4mr15882952ilg.21.1673467979993; Wed, 11 Jan 2023 12:12:59 -0800 (PST) Received: from [192.168.0.241] (192-0-145-146.cpe.teksavvy.com. [192.0.145.146]) by smtp.gmail.com with ESMTPSA id b18-20020a026f52000000b0039e8f997ad6sm2295508jae.32.2023.01.11.12.12.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 11 Jan 2023 12:12:58 -0800 (PST) Message-ID: <6ca466e3-57cf-c9ac-255d-cb2ecb186cd5@redhat.com> Date: Wed, 11 Jan 2023 15:12:57 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH 3/4] string: Suppress -Wmaybe-unitialized for wordcopy [BZ #19444] To: Adhemerval Zanella Netto , 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> <7c9799f2-868c-c954-a59e-36a31701ebb7@linaro.org> From: Carlos O'Donell Organization: Red Hat In-Reply-To: <7c9799f2-868c-c954-a59e-36a31701ebb7@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: On 1/11/23 08:14, Adhemerval Zanella Netto wrote: > 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. Downthread I've asked you to repost in a new thread, but I'll carry out a review here to reduce the turnaround time. >>> 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. We are talking about examples like those in string/memmove.c: 58 if (len >= OP_T_THRES) 59 { 60 /* Copy just a few bytes to make DSTP aligned. */ 61 len -= (-dstp) % OPSIZ; 62 BYTE_COPY_FWD (dstp, srcp, (-dstp) % OPSIZ); 63 64 /* Copy whole pages from SRCP to DSTP by virtual address 65 manipulation, as much as possible. */ 66 67 PAGE_COPY_FWD_MAYBE (dstp, srcp, len, len); 68 69 /* Copy from SRCP to DSTP taking advantage of the known 70 alignment of DSTP. Number of bytes remaining is put 71 in the third argument, i.e. in LEN. This number may 72 vary from machine to machine. */ 73 74 WORD_COPY_FWD (dstp, srcp, len, len); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Only called with len >= OP_T_THRES 75 76 /* Fall out and copy the tail. */ 77 } I agree completely with you here. My point earlier was that the macros themselves have no 'assert(len > OP_T_THRES);' or any indication that they should not be called with len < OP_T_THRES. > This is similar for WORD_COPY_BWD, which is only used for memmove.c. Agreed. >> >>> --- >>> 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: Right. > /* 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. */ OK. >> >>> + 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). > */ OK. > >> >>> 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. > From 86b83da8c7836484654fbacaf54f764df378a31e Mon Sep 17 00:00:00 2001 > From: Adhemerval Zanella > Date: Thu, 29 Dec 2022 09:15:49 -0300 > Subject: [PATCH 3/4] string: Suppress -Wmaybe-unitialized for wordcopy [BZ > #19444] > > The GCC 6+ when compiling for sparc warns that some variables might be > used uninitialized. However it does not seem the fact, since the > variables are really initialized (and also other targets that use the > same code, like powerpc, do not warn about it). Suggest: When compiling with GCC 6+ the sparc build warns that some variables might be used uninitialized. > > So suppress the warning for now. > --- > string/wordcopy.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/string/wordcopy.c b/string/wordcopy.c > index ae5ccd793c..d0cfc22082 100644 > --- a/string/wordcopy.c > +++ b/string/wordcopy.c > @@ -19,7 +19,12 @@ > /* BE VERY CAREFUL IF YOU CHANGE THIS CODE...! */ > > #include > +#include > +/* Check comment of WORDCOPY_FWD_DEST_ALIGNED. */ Please move the comment up to here. > +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 +99,14 @@ WORDCOPY_FWD_ALIGNED (long int dstp, long int srcp, size_t len) > { > do8: > a0 = ((op_t *) srcp)[0]; > + /* Compiling with -O1 may warn that 'a1' in 'do8' switch may be used > + uninitialized. However, 'do8' is only reachable through 'case 1', > + since all possible modulo values are handling in the initial > + switch). */ Please move the comment to just above the store of a1 into dstp. Suggest: /* Compiling with -O1 may warn that 'a1' may be used uninitialized. There are only two ways to arrive at label 'do8' and they are via a do-while loop iteration or directly via the earlier switch 'case 1:' case. The switch case always sets 'a1' and all previous loop iterations will also have set 'a1' before the use. */ > + DIAG_PUSH_NEEDS_COMMENT; > + DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized"); > ((op_t *) dstp)[0] = a1; > + DIAG_POP_NEEDS_COMMENT; > do7: > a1 = ((op_t *) srcp)[1]; > ((op_t *) dstp)[1] = a0; > @@ -190,6 +202,14 @@ WORDCOPY_FWD_DEST_ALIGNED (long int dstp, long int srcp, size_t len) > goto do4; /* No-op. */ > } > Please move this comment up to the point at which you have the DIAG* since it should apply to both FWD and BWD uses that use the MERGE macros. > + /* 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. */ Suggest: /* Compiling with -O1 might warn that 'a2' and 'a3' may be used uninitialized. There are only two ways to arrive at labels 'do4', 'do3' or 'do1', all of which use 'a2' or 'a3' in the MERGE macro: either from the earlier switch case statement or via a loop iteration. In all cases the switch statement or previous loop sets both 'a2' and 'a3'. Since the usage is within the MERGE macro we disable the warning in the definition, but only in this file. */ > do > { > do4: > @@ -291,7 +311,11 @@ WORDCOPY_BWD_ALIGNED (long int dstp, long int srcp, size_t len) > { > do8: > a0 = ((op_t *) srcp)[7]; > + /* Check the comment on WORDCOPY_FWD_ALIGNED. */ > + DIAG_PUSH_NEEDS_COMMENT; > + DIAG_IGNORE_NEEDS_COMMENT (6, "-Wmaybe-uninitialized"); > ((op_t *) dstp)[7] = a1; > + DIAG_POP_NEEDS_COMMENT; > do7: > a1 = ((op_t *) srcp)[6]; > ((op_t *) dstp)[6] = a0; > -- > 2.34.1 -- Cheers, Carlos.