| Message ID | 20200407155118.20139-9-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/13] .github: Enable repo-lockdown bot to refuse GitHub pull requests | expand |
17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла: > > From: Richard Henderson <richard.henderson@linaro.org> > > All other calls to normalize*Subnormal detect zero input before > the call -- this is the only outlier. This case can happen with > +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of > the correct sign. > > Reported-by: Coverity (CID 1421991) > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org> > Message-Id: <20200403191150.863-8-alex.bennee@linaro.org> > > diff --git a/fpu/softfloat.c b/fpu/softfloat.c > index 301ce3b537b..ae6ba718540 100644 > --- a/fpu/softfloat.c > +++ b/fpu/softfloat.c > @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, flag zSign, > zSig1 = 0; > zSig0 = aSig + bSig; > if ( aExp == 0 ) { > + if (zSig0 == 0) { > + return packFloatx80(zSign, 0, 0); > + } > normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); > goto roundAndPack; > } > -- > 2.20.1 > > We in MIPS have extensive FP tests, that certainly include many cases of operations with +0 and -0. And they are all correct even before this patch. Unfortunately, because of my current remote work, I don't havecthese tests with me, and can't confirm if they work correctly, or perhaps are unaffected at all. Alex, from the commit message, it not clear if this is a fix of a bug (in which case a test example would be useful to have, and the assesment on what scenarios could be affected), or just a correction for some rare condition that practically for all intents and purposes was never triggered, or perhaps something third. Alex, please explain this in more detail to me. Secondly, and not related to this patch only, I see more and more patches integrated into the main tree without "Reviewed-by:" tag. I don't think this is the best way an open source community works. In my personal opinion, this must stop. Regards, Aleksandar
On 4/10/20 2:38 AM, Aleksandar Markovic wrote: > 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org > <mailto:alex.bennee@linaro.org>> је написао/ла: >> >> From: Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> >> >> All other calls to normalize*Subnormal detect zero input before >> the call -- this is the only outlier. This case can happen with >> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of >> the correct sign. >> >> Reported-by: Coverity (CID 1421991) >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org > <mailto:alex.bennee@linaro.org>> >> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org > <mailto:20200327232042.10008-1-richard.henderson@linaro.org>> >> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org > <mailto:20200403191150.863-8-alex.bennee@linaro.org>> >> >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c >> index 301ce3b537b..ae6ba718540 100644 >> --- a/fpu/softfloat.c >> +++ b/fpu/softfloat.c >> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, > flag zSign, >> zSig1 = 0; >> zSig0 = aSig + bSig; >> if ( aExp == 0 ) { >> + if (zSig0 == 0) { >> + return packFloatx80(zSign, 0, 0); >> + } >> normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); >> goto roundAndPack; >> } >> -- >> 2.20.1 >> >> > > We in MIPS have extensive FP tests, that certainly include many cases of > operations with +0 and -0. And they are all correct even before this patch. This is for the 80-bit extended-double type, only used on x86 and m68k. You will not execute this path using MIPS. > Alex, from the commit message, it not clear if this is a fix of a bug (in which > case a test example would be useful to have, and the assesment on what > scenarios could be affected), or just a correction for some rare condition that > practically for all intents and purposes was never triggered, or perhaps > something third. This only avoids a Coverity out-of-range shift warning. Beforehand, we executed 0 << 64, got 0 as the result (regardless of whether or not the host truncates the shift count), and constructed the correctly signed fp zero in the end. There was more discussion about this in an earlier thread, associated with a different patch for this same problem: https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html > Secondly, and not related to this patch only, I see more and more patches > integrated into the main tree without "Reviewed-by:" tag. I don't think this is > the best way an open source community works. In my personal opinion, this must > stop. The only way to avoid this is to have more developers review code outside their own bailiwick. The patch has been on the list for two weeks and was pinged twice. Although why Alex didn't add his own R-b to my patch when merging it to his branch, I don't know. r~
17:17 Pet, 10.04.2020. Richard Henderson <richard.henderson@linaro.org> је написао/ла: > > On 4/10/20 2:38 AM, Aleksandar Markovic wrote: > > 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org > > <mailto:alex.bennee@linaro.org>> је написао/ла: > >> > >> From: Richard Henderson <richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> > >> > >> All other calls to normalize*Subnormal detect zero input before > >> the call -- this is the only outlier. This case can happen with > >> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of > >> the correct sign. > >> > >> Reported-by: Coverity (CID 1421991) > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org > > <mailto:alex.bennee@linaro.org>> > >> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org > > <mailto:20200327232042.10008-1-richard.henderson@linaro.org>> > >> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org > > <mailto:20200403191150.863-8-alex.bennee@linaro.org>> > >> > >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c > >> index 301ce3b537b..ae6ba718540 100644 > >> --- a/fpu/softfloat.c > >> +++ b/fpu/softfloat.c > >> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, > > flag zSign, > >> zSig1 = 0; > >> zSig0 = aSig + bSig; > >> if ( aExp == 0 ) { > >> + if (zSig0 == 0) { > >> + return packFloatx80(zSign, 0, 0); > >> + } > >> normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); > >> goto roundAndPack; > >> } > >> -- > >> 2.20.1 > >> > >> > > > > We in MIPS have extensive FP tests, that certainly include many cases of > > operations with +0 and -0. And they are all correct even before this patch. > > This is for the 80-bit extended-double type, only used on x86 and m68k. You > will not execute this path using MIPS. > Thanks, Richard! First and foremost, may health be with you and all people of United States and all Americas!! Yes, the fact that m68k also uses 80-bit FP arithmetic was known to me. Though probably many people think 80-bit FP is limited to x86. I was just afraid of some strange way that other targets may end up using the function in question. But again, thanks for reassurances! > > Alex, from the commit message, it not clear if this is a fix of a bug (in which > > case a test example would be useful to have, and the assesment on what > > scenarios could be affected), or just a correction for some rare condition that > > practically for all intents and purposes was never triggered, or perhaps > > something third. > > This only avoids a Coverity out-of-range shift warning. > > Beforehand, we executed 0 << 64, got 0 as the result (regardless of whether or > not the host truncates the shift count), and constructed the correctly signed > fp zero in the end. > > There was more discussion about this in an earlier thread, associated with a > different patch for this same problem: > > https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html > OK. I didn't have a chance to read this pattivular thread. Thanks for the pointer! > > > Secondly, and not related to this patch only, I see more and more patches > > integrated into the main tree without "Reviewed-by:" tag. I don't think this is > > the best way an open source community works. In my personal opinion, this must > > stop. > > The only way to avoid this is to have more developers review code outside their > own bailiwick. The patch has been on the list for two weeks and was pinged > twice. > > Although why Alex didn't add his own R-b to my patch when merging it to his > branch, I don't know. > I also have a very similar impression as yours, that Alex in fact reviewed the patch (as if he implicitely gave R-B, but forgot to insert it in a hurry, given these hectic days around 5.0 final release). Best regards, and best wishes to all Sietlans, or wherever you happen to live! Aleksandar > > r~
On Fri, 10 Apr 2020 at 16:17, Richard Henderson <richard.henderson@linaro.org> wrote: > Although why Alex didn't add his own R-b to my patch when merging it to his > branch, I don't know. I think this is one of those areas where different submaintainers have different work practices. Personally I distinguish "did I actually review this" from "did I just put this into my tree and rely on others doing the review" and use r-by for the former and not on the latter (although obviously everything I put in my tree I will have at least very very briefly looked over). But I think some submaintainers don't bother to add r-by tags for things they review in the process of assembling their tree because they see it as implicit in the process. thanks -- PMM
18:14 Pet, 10.04.2020. Peter Maydell <peter.maydell@linaro.org> је написао/ла: > But I think some submaintainers don't bother to add r-by tags > for things they review in the process of assembling their > tree because they see it as implicit in the process. > I think that was precisely the case in this patch. May I wish you, Peter, the best health, and thanks UK for giving the world Dr. John Campbell from northern England, whose videos I watch every day with the closest possible attention, and the highest admiration. Aleksandar > thanks > -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 10 Apr 2020 at 16:17, Richard Henderson > <richard.henderson@linaro.org> wrote: >> Although why Alex didn't add his own R-b to my patch when merging it to his >> branch, I don't know. > > I think this is one of those areas where different submaintainers > have different work practices. Personally I distinguish "did I > actually review this" from "did I just put this into my tree and > rely on others doing the review" and use r-by for the former > and not on the latter (although obviously everything I put in > my tree I will have at least very very briefly looked over). > But I think some submaintainers don't bother to add r-by tags > for things they review in the process of assembling their > tree because they see it as implicit in the process. It was exactly this - pulling in via my tree and adding my own s-o-b implies I'm happy enough with it. Typically for longer series that gestate on the list the total number of r-b tags grows with each re-roll until the series gets pulled into a maintainer branch. This PR is atypical in that regard because it's a fairly random collection of fixes. I think the only patches we should be wary of are those with only a single s-o-b tag from the author. I have to admit there was one such patch in this PR: Subject: [PULL 09/13] linux-user: factor out reading of /proc/self/maps Date: Tue, 7 Apr 2020 16:51:14 +0100 Message-Id: <20200407155118.20139-10-alex.bennee@linaro.org> I made an executive decision to include it as it was part of the bug fix for patch 10 and as we approach RC releases I wanted to get it merged. If you follow the msg-id in the patch you will see the changes in the patch are purely in response to review comments so while missing a r-b tag it's not like it's not been on the list and had some scrutiny. However we should certainly aim for most patches to be fully reviewed even if we never achieve that level of perfection. > > thanks > -- PMM
diff --git a/fpu/softfloat.c b/fpu/softfloat.c index 301ce3b537b..ae6ba718540 100644 --- a/fpu/softfloat.c +++ b/fpu/softfloat.c @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, flag zSign, zSig1 = 0; zSig0 = aSig + bSig; if ( aExp == 0 ) { + if (zSig0 == 0) { + return packFloatx80(zSign, 0, 0); + } normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); goto roundAndPack; }