Message ID | 871316fb87a99a59c31e6d3fbd4d35bff2ecc3c4.1567097252.git.alistair.francis@wdc.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 126590 invoked by alias); 29 Aug 2019 16:53:49 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 126581 invoked by uid 89); 29 Aug 2019 16:53:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=HContent-Transfer-Encoding:8bit X-HELO: esa2.hgst.iphmx.com DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1567097688; x=1598633688; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=E9qDogQvHM1P5LB1qJtwb9EyeCUCE4M4P6sFf3Wc5kY=; b=gkVlZPYwJ3ldSeQc+IOzn0aVuK5nNnG4UJUcOii0cf7435KlMnKF8+1j lbMOrA08BI3hBwbounKsg+9dUdaWaFqzxlv7IZRoa22vKqWgDvmJeSfef CKWN1J5iinIJPzFYidjsPNXXsJOPsqTag7Hh8Q9PYIcGnMpPn2o56okdL eQHOJ3qM9Wczw5n09krchldnk1+BSz9m86XPGu2GbkvcEPdapN9TdGfdu OWcJP5bdDmxswrL0Qmp3s85EgMKiD/ys3pB0Si8HuvUZPQK+1BFTp8nzW B0heLmfdVaunaVzytHwK7fLyruPVCic/axlXrEZrOK458kkHJXmKF5lVe g==; IronPort-SDR: ORmoXeyhbWIqsJmmWETU2z3x4vIE7H4pUo7J/gcvsgkYuzJt86Xj9mA7RVuhLJME90dXlibE6o kH4kS2aKaI/bR22uggt6FLGUQVPva35kcmgxTwYwwprFCz/kMgk8O45haYBVQtvaHSqMSWGgqw oqSH03BkE4EhZnW8A4HIpXjMqq80AFOxe4rCbZXN7VZFjQMu+RjALn2aD2bvcjcpAQDUj9+rHu UpSejlpni0W1zlYeiqDVfnoOAoFOM28b1ESI0ieZ4tuBvfC5rap6ineVKD7dqvJnOxKAkkH9ZR 9io= IronPort-SDR: nmxDVBjfe1oKJX0I2xVT5U0v7Xr0zsTcoNdKn2SwVuEpaj2S0b2SSiuOj9sH8Zvk92yNJ7o1sh UuOf2HbJq6qlnDsT0oDPimOz18XRp2cvX6zbChpFdBRIIxjapiJ94/Jik/X6dNaNTLji4MHE/n HuPeglvqYpgN3mQNugAktyD7M6m2QOhVJm9porsSmOi9JeAK6zS/ZhGbaKjERf2k+AkaIXocvh jyI6xibBG/HxKhRjc80oE0ZOURtH8zDiHjfnVgGOcotkZYNJUCW2/6gy97XoNNprWtV8DAF4lz uTxbODFlv2Dbk+HudBYS0EaD IronPort-SDR: olcd3RMskOl61dMMPZhPayY45pP5hkSLHO+pwUDLxC370qf0e7lj/iOdlIpez++XD9xlvXjeaE z8VusdFJcJR/p3m2di2mdq0oCGyNHeLdCKHySGSGd4EbvMXl6z8lVo2OYAfqQ+tWoV7WzXPyJr D0MFoxs3XMOFVIfwx0WzHPEHBsRcH9L/PfHNup8dRHBaEoXfj1LNOKWEdC3oRI7hC8ERKKwugv LG2lqHYjDzSJtDYVRA72JRKhXulRcyeJzvGa409FA+r5fCcxi8gg1Fk+ZOS8KZn/V3fGPrmFao xgU= WDCIronportException: Internal From: Alistair Francis <alistair.francis@wdc.com> To: libc-alpha@sourceware.org Cc: arnd@arndb.de, adhemerval.zanella@linaro.org, fweimer@redhat.com, palmer@sifive.com, macro@wdc.com, zongbox@gmail.com, alistair.francis@wdc.com, alistair23@gmail.com Subject: [RFC v5 01/21] sunrpc/clnt_udp: Ensure total_deadline is initalised Date: Thu, 29 Aug 2019 09:50:03 -0700 Message-Id: <871316fb87a99a59c31e6d3fbd4d35bff2ecc3c4.1567097252.git.alistair.francis@wdc.com> In-Reply-To: <cover.1567097252.git.alistair.francis@wdc.com> References: <cover.1567097252.git.alistair.francis@wdc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Alistair Francis
Aug. 29, 2019, 4:50 p.m. UTC
Even though total_deadline won't be accessed uninitalised GCC can still
complain that it is accessed unitalised, to avod those errors let's make
sure we initalise it to 0.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
* sunrpc/clnt_udp.c: Ensure total_deadline is initalised.
---
sunrpc/clnt_udp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On Thu, 29 Aug 2019, Alistair Francis wrote: > Even though total_deadline won't be accessed uninitalised GCC can still > complain that it is accessed unitalised, to avod those errors let's make > sure we initalise it to 0. It's glibc practice (although missing from <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* add initializations like that to avoid warnings. If necessary we may use the DIAG_* macros with appropriate comments about why the warning is a false positive (sometimes other approaches work to avoid the warnings, e.g. use of __builtin_unreachable () calls).
On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > On Thu, 29 Aug 2019, Alistair Francis wrote: >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu > > Even though total_deadline won't be accessed uninitalised GCC can still > > complain that it is accessed unitalised, to avod those errors let's make > > sure we initalise it to 0. > > It's glibc practice (although missing from > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > add initializations like that to avoid warnings. Although this has historically been glibc practice, I think it is unwisely incautious, and we should change the policy to be that we *do* add initializations whenever the compiler thinks a variable even _might_ be used uninitialized. zw
On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 29 Aug 2019, Alistair Francis wrote: > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu > > > Even though total_deadline won't be accessed uninitalised GCC can still > > > complain that it is accessed unitalised, to avod those errors let's make > > > sure we initalise it to 0. > > > > It's glibc practice (although missing from > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > > add initializations like that to avoid warnings. > > Although this has historically been glibc practice, I think it is > unwisely incautious, and we should change the policy to be that we > *do* add initializations whenever the compiler thinks a variable even > _might_ be used uninitialized. Does that mean this patch is ok? Alistair > > zw
On Thu, 5 Sep 2019, Alistair Francis wrote: > On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: > > > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > On Thu, 29 Aug 2019, Alistair Francis wrote: > > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu > > > > Even though total_deadline won't be accessed uninitalised GCC can still > > > > complain that it is accessed unitalised, to avod those errors let's make > > > > sure we initalise it to 0. > > > > > > It's glibc practice (although missing from > > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > > > add initializations like that to avoid warnings. > > > > Although this has historically been glibc practice, I think it is > > unwisely incautious, and we should change the policy to be that we > > *do* add initializations whenever the compiler thinks a variable even > > _might_ be used uninitialized. > > Does that mean this patch is ok? No. You can't deduce consensus like that from two different views on a patch or a convention. Even if we were to change the convention regarding how to silence such warnings, I see reason to have any less requirement for comments explaining why the warning is a false positive and that the initializer is only there to silence a warning than there is for the DIAG_* macros.
On 9/5/19 9:02 AM, Joseph Myers wrote: > On Thu, 5 Sep 2019, Alistair Francis wrote: > >> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: >>> >>> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: >>>> On Thu, 29 Aug 2019, Alistair Francis wrote: >>>> The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu >>>>> Even though total_deadline won't be accessed uninitalised GCC can still >>>>> complain that it is accessed unitalised, to avod those errors let's make >>>>> sure we initalise it to 0. >>>> >>>> It's glibc practice (although missing from >>>> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* >>>> add initializations like that to avoid warnings. >>> >>> Although this has historically been glibc practice, I think it is >>> unwisely incautious, and we should change the policy to be that we >>> *do* add initializations whenever the compiler thinks a variable even >>> _might_ be used uninitialized. >> >> Does that mean this patch is ok? > > No. You can't deduce consensus like that from two different views on a > patch or a convention. Even if we were to change the convention regarding > how to silence such warnings, I see reason to have any less requirement > for comments explaining why the warning is a false positive and that the > initializer is only there to silence a warning than there is for the > DIAG_* macros. > BTW, has a bug been filed against GCC for the bogus warning? Jeff
On Thu, Sep 5, 2019 at 10:53 AM Alistair Francis <alistair23@gmail.com> wrote: > On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > On Thu, 29 Aug 2019, Alistair Francis wrote: > > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu > > > > Even though total_deadline won't be accessed uninitalised GCC can still > > > > complain that it is accessed unitalised, to avod those errors let's make > > > > sure we initalise it to 0. > > > > > > It's glibc practice (although missing from > > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > > > add initializations like that to avoid warnings. > > > > Although this has historically been glibc practice, I think it is > > unwisely incautious, and we should change the policy to be that we > > *do* add initializations whenever the compiler thinks a variable even > > _might_ be used uninitialized. > > Does that mean this patch is ok? I haven't actually looked at your patch, so no, that was not a review. zw
On Thu, Sep 5, 2019 at 11:03 AM Joseph Myers <joseph@codesourcery.com> wrote: > On Thu, 5 Sep 2019, Alistair Francis wrote: > > On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > On Thu, 29 Aug 2019, Alistair Francis wrote: > > > > > Even though total_deadline won't be accessed uninitalised GCC can still > > > > > complain that it is accessed unitalised, to avod those errors let's make > > > > > sure we initalise it to 0. > > > > > > > > It's glibc practice (although missing from > > > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > > > > add initializations like that to avoid warnings. > > > > > > Although this has historically been glibc practice, I think it is > > > unwisely incautious, and we should change the policy to be that we > > > *do* add initializations whenever the compiler thinks a variable even > > > _might_ be used uninitialized. > > > > Does that mean this patch is ok? > > No. You can't deduce consensus like that from two different views on a > patch or a convention. Joseph, you have a tendency to throw up procedural objections to _everything_, and I feel I need to ask you to tone it down a little. This project has historically been perceived as unwelcoming to new contributors, and I think a major remaining reason for that is all the procedure -- some of which is genuinely necessary, but not all. > Even if we were to change the convention regarding > how to silence such warnings, I see reason to have any less requirement > for comments explaining why the warning is a false positive and that the > initializer is only there to silence a warning than there is for the > DIAG_* macros. In general I would agree, but this is the obsolete sunrpc directory we're talking about; I don't think it's worth putting more than the bare minimum effort into code that we plan to delete eventually. zw
On Thu, 5 Sep 2019, Jeff Law wrote:
> BTW, has a bug been filed against GCC for the bogus warning?
In this case, it looks like the code is
if (xargs != NULL)
{
total_deadline = ...
}
...
if (xargs != NULL)
{
use total_deadline
}
(and xargs doesn't get modified in the function and the various gotos in
this function are all after that setting of total_deadline). So that
should be a known issue (and we could consider if the existing comment
/* Choose the timeout value. For non-sending usage (xargs == NULL),
the total deadline does not matter, only cu->cu_wait is used
below. */
is sufficient or should be extended to say explicitly the warning is
bogus).
On 9/5/19 9:21 AM, Joseph Myers wrote: > On Thu, 5 Sep 2019, Jeff Law wrote: > >> BTW, has a bug been filed against GCC for the bogus warning? > > In this case, it looks like the code is > > if (xargs != NULL) > { > total_deadline = ... > } > > ... > > if (xargs != NULL) > { > use total_deadline > } > > (and xargs doesn't get modified in the function and the various gotos in > this function are all after that setting of total_deadline). So that > should be a known issue (and we could consider if the existing comment > > /* Choose the timeout value. For non-sending usage (xargs == NULL), > the total deadline does not matter, only cu->cu_wait is used > below. */ > > is sufficient or should be extended to say explicitly the warning is > bogus). Right. My concern is we should be analyzing this on the GCC side. I can hazard a guess that there's too much code between the two tests and as a result jump threading isn't deemed profitable enough. That's not terribly unusual. But even so, the uninit pass has predicate analysis which should see the use as properly guarded. So ISTM that something isn't behaving properly on the GCC side so we should get bug report opened so that we can analyze why GCC isn't doing the right thing here. jeff
On Thu, Sep 5, 2019 at 8:02 AM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 5 Sep 2019, Alistair Francis wrote: > > > On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > On Thu, 29 Aug 2019, Alistair Francis wrote: > > > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu > > > > > Even though total_deadline won't be accessed uninitalised GCC can still > > > > > complain that it is accessed unitalised, to avod those errors let's make > > > > > sure we initalise it to 0. > > > > > > > > It's glibc practice (although missing from > > > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > > > > add initializations like that to avoid warnings. > > > > > > Although this has historically been glibc practice, I think it is > > > unwisely incautious, and we should change the policy to be that we > > > *do* add initializations whenever the compiler thinks a variable even > > > _might_ be used uninitialized. > > > > Does that mean this patch is ok? > > No. You can't deduce consensus like that from two different views on a > patch or a convention. Even if we were to change the convention regarding > how to silence such warnings, I see reason to have any less requirement > for comments explaining why the warning is a false positive and that the > initializer is only there to silence a warning than there is for the > DIAG_* macros. No worries, I'll happily change the patch, I just want to make sure I change it to the right thing. I'll: - Investigate filing a GCC bug for this false positive - Add comments to the init explaining why I am setting it - Use the DIAG_* macros Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Thu, Sep 5, 2019 at 8:22 AM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 5 Sep 2019, Jeff Law wrote: > > > BTW, has a bug been filed against GCC for the bogus warning? > > In this case, it looks like the code is > > if (xargs != NULL) > { > total_deadline = ... > } > > ... > > if (xargs != NULL) > { > use total_deadline > } > > (and xargs doesn't get modified in the function and the various gotos in > this function are all after that setting of total_deadline). So that > should be a known issue (and we could consider if the existing comment That is my understanding as well. > > /* Choose the timeout value. For non-sending usage (xargs == NULL), > the total deadline does not matter, only cu->cu_wait is used > below. */ > > is sufficient or should be extended to say explicitly the warning is > bogus). I was planning on adding a comment in the next version, let me know otherwise. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On 9/5/19 6:46 PM, Alistair Francis wrote: > On Thu, Sep 5, 2019 at 8:02 AM Joseph Myers <joseph@codesourcery.com> wrote: >> >> On Thu, 5 Sep 2019, Alistair Francis wrote: >> >>> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: >>>> >>>> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: >>>>> On Thu, 29 Aug 2019, Alistair Francis wrote: >>>>> The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu >>>>>> Even though total_deadline won't be accessed uninitalised GCC can still >>>>>> complain that it is accessed unitalised, to avod those errors let's make >>>>>> sure we initalise it to 0. >>>>> >>>>> It's glibc practice (although missing from >>>>> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* >>>>> add initializations like that to avoid warnings. >>>> >>>> Although this has historically been glibc practice, I think it is >>>> unwisely incautious, and we should change the policy to be that we >>>> *do* add initializations whenever the compiler thinks a variable even >>>> _might_ be used uninitialized. >>> >>> Does that mean this patch is ok? >> >> No. You can't deduce consensus like that from two different views on a >> patch or a convention. Even if we were to change the convention regarding >> how to silence such warnings, I see reason to have any less requirement >> for comments explaining why the warning is a false positive and that the >> initializer is only there to silence a warning than there is for the >> DIAG_* macros. > > No worries, I'll happily change the patch, I just want to make sure I > change it to the right thing. > > I'll: > - Investigate filing a GCC bug for this false positive All we need is the .i file (Add -save-temps to the compilation line), a copy of the full compilation line and the target triplet. No need to try and create a minimal testcase or anything like that. Jeff
On Thu, Sep 5, 2019 at 6:31 PM Jeff Law <law@redhat.com> wrote: > > On 9/5/19 6:46 PM, Alistair Francis wrote: > > On Thu, Sep 5, 2019 at 8:02 AM Joseph Myers <joseph@codesourcery.com> wrote: > >> > >> On Thu, 5 Sep 2019, Alistair Francis wrote: > >> > >>> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote: > >>>> > >>>> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > >>>>> On Thu, 29 Aug 2019, Alistair Francis wrote: > >>>>> The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know. I'm assu > >>>>>> Even though total_deadline won't be accessed uninitalised GCC can still > >>>>>> complain that it is accessed unitalised, to avod those errors let's make > >>>>>> sure we initalise it to 0. > >>>>> > >>>>> It's glibc practice (although missing from > >>>>> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* > >>>>> add initializations like that to avoid warnings. > >>>> > >>>> Although this has historically been glibc practice, I think it is > >>>> unwisely incautious, and we should change the policy to be that we > >>>> *do* add initializations whenever the compiler thinks a variable even > >>>> _might_ be used uninitialized. > >>> > >>> Does that mean this patch is ok? > >> > >> No. You can't deduce consensus like that from two different views on a > >> patch or a convention. Even if we were to change the convention regarding > >> how to silence such warnings, I see reason to have any less requirement > >> for comments explaining why the warning is a false positive and that the > >> initializer is only there to silence a warning than there is for the > >> DIAG_* macros. > > > > No worries, I'll happily change the patch, I just want to make sure I > > change it to the right thing. > > > > I'll: > > - Investigate filing a GCC bug for this false positive > All we need is the .i file (Add -save-temps to the compilation line), a > copy of the full compilation line and the target triplet. No need to > try and create a minimal testcase or anything like that. Thanks for that info. I have filed the bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691 I'll send this patch out separately and we can go from there. Alistair > > Jeff
diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c index c2436e3ebcc..311b1734733 100644 --- a/sunrpc/clnt_udp.c +++ b/sunrpc/clnt_udp.c @@ -290,7 +290,7 @@ clntudp_call (/* client handle */ int anyup; /* any network interface up */ struct deadline_current_time current_time = __deadline_current_time (); - struct deadline total_deadline; /* Determined once by overall timeout. */ + struct deadline total_deadline = { 0 }; /* Determined once by overall timeout. */ struct deadline response_deadline; /* Determined anew for each query. */ /* Choose the timeout value. For non-sending usage (xargs == NULL),