Sounds OK to me. :) Best Regards
Adam Farley Philip Race <philip.r...@oracle.com> wrote on 01/06/2018 03:04:12: > From: Philip Race <philip.r...@oracle.com> > To: Adam Farley8 <adam.far...@uk.ibm.com> > Cc: 2d-dev <2d-...@openjdk.java.net>, build-dev <build- > d...@openjdk.java.net>, Andrew Leonard <andrew_m_leon...@uk.ibm.com>, > "Stuefe, Thomas" <thomas.stu...@sap.com> > Date: 01/06/2018 03:04 > Subject: Re: [OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix > compile warning in jchuff.c > > > It looks fine to me but I am a bit hazy about who to give > attribution for the fix .. > > I pondered this for a little while and decided it should be > joint between Adam who identified the issue and championed > it and Thomas who I think first suggested the code change, modified only > slightly at Guido's suggestion. > > I'll push it tomorrow if every one is OK with that. > > -phil. > > On 5/31/18, 10:33 AM, Phil Race wrote: > > I've grabbed the bug from Thomas and re-opened it > > https://bugs.openjdk.java.net/browse/ > > Your webrev was stripped so I've uploaded here : > > http://cr.openjdk.java.net/~prr/8200052/ > > It looks fine to me but I am a bit hazy about who to give > attribution for the fix .. > It is small enough to not require an OCA so there's no issue there > if we attribute > it to the IJG guy. > > -phil. > On 05/31/2018 06:31 AM, Adam Farley8 wrote: > An attachment in the email has been found to contain executable code > and has been removed. > > File removed : webrev.zip, zip,html,js > Hi Phil, > > As requested: > > > > FYI: I've also contacted Guido, confirmed that the fix worked, and asked > him to go ahead with merging the fix into his code base. > > Best Regards > > Adam Farley > > > Phil Race <philip.r...@oracle.com> wrote on 30/05/2018 18:06:19: > > > From: Phil Race <philip.r...@oracle.com> > > To: Adam Farley8 <adam.far...@uk.ibm.com> > > Cc: 2d-dev <2d-...@openjdk.java.net>, Andrew Leonard > > <andrew_m_leon...@uk.ibm.com>, build-dev <build- > > d...@openjdk.java.net>, Magnus Ihse Bursie > > <magnus.ihse.bur...@oracle.com>, "Thomas Stüfe" <thomas.stu...@gmail.com> > > Date: 30/05/2018 18:07 > > Subject: Re: [OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix > > compile warning in jchuff.c > > > > Thank you for persevering with this. Please submit a webrev with this > > change .. I think you can leave out the change to jerror.h in the > jpeg6b case. > > > > -phil. > > > On 05/30/2018 09:08 AM, Adam Farley8 wrote: > > Hi Phil, > > > > I spoke with the jpegclub rep "Guido", and he was very helpful. > > > > He agreed to accept a code change, but recommended an error instead > > of a while check. > > > > ------------------------------ Line 808: > > < while (bits[j] == 0) > > < j--; > > --- > > > while (bits[j] == 0) { > > > if (j == 0) > > > ERREXIT(cinfo, JERR_HUFF_CLEN_OVERFLOW); > > > j--; > > > } > > ------------------------------ > > > > This makes sense to me, and I verified that it prevents the error. > > > > He says: > > @@@@@@@@@@@@ > > For the release version I would replace the specific > > JERR_HUFF_CLEN_OVERFLOW by the more general > > JERR_HUFF_CLEN_OUTOFBOUNDS so that it suits both cases, this > > requires a change in jerror.h: > > > > -JMESSAGE(JERR_HUFF_CLEN_OVERFLOW, "Huffman code size table overflow") > > +JMESSAGE(JERR_HUFF_CLEN_OUTOFBOUNDS, "Huffman code size table out > of bounds") > > > > The next version (9d) is planned for release in January 2020. > > A pre-release package will be provided in 2019 on http:// > > jpegclub.org/reference/reference-sources/, I will send you a notification. > > @@@@@@@@@@@@ > > > > While we *could* make the jerror.h change, I suggest we don't for > > now. If we merge from upstream in January 2020, we'll get that > > change anyway when the time comes. > > > > So what do you say to including the dashed change referenced above > > to fix our problem now, safe in the knowledge that upstream will be > > similarly modified (except with the new error type). > > > > Best Regards > > > > Adam Farley > > > > P.S. I'm holding off on giving Guido the green light until after > > people say if they're happy with the error-enabled version of the fix. > > > > Adam Farley8/UK/IBM wrote on 14/05/2018 11:06:28: > > > > > From: Adam Farley8/UK/IBM > > > To: Phil Race <philip.r...@oracle.com> > > > Cc: 2d-dev <2d-...@openjdk.java.net>, Andrew Leonard > > > <andrew_m_leon...@uk.ibm.com>, build-dev <build- > > > d...@openjdk.java.net>, Magnus Ihse Bursie > > > <magnus.ihse.bur...@oracle.com>, "Thomas Stüfe" <thomas.stu...@gmail.com> > > > Date: 14/05/2018 11:06 > > > Subject: Re: [OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix > > > compile warning in jchuff.c > > > > > > Hi Phil, > > > > > > Would an acceptable compromise be to deliver the source code change > > > and send the code to the upstream community, allowing them to include > > > the fix if/when they are able? > > > > > > I believe Magnus was advocating this idea as well. See below. > > > > > > Best Regards > > > > > > Adam Farley > > > > > > > Same here. I would like to have this fix in, but do not want to go > > > > over Phils head. > > > > > > > > I think Phil was the main objector, maybe he could reconsider?` > > > > > > > > Thanks, Thomas > > > > > > > > On Thu, Apr 26, 2018 at 10:39 AM, Magnus Ihse Bursie > > > > <magnus.ihse.bur...@oracle.com> wrote: > > > > > I don't object, but it's not build code so I don't have the > final say. > > > > > > > > > > /Magnus > > > > > > > > > > > > > > > On 2018-04-25 17:43, Adam Farley8 wrote: > > > > > > > > > > Hi All, > > > > > > > > > > Does anyone have an objection to pushing this tiny change in? > > > > > > > > > > It doesn't break anything, it fixes a build break on two supported > > > > > platforms, and it seems > > > > > like we never refresh the code from upstream. > > > > > > > > > > - Adam > > > > > > > > > >> I also advocate the source code fix, as Make isn't meant to > > use the sort > > > > >> of logic required > > > > >> to properly analyse the toolchain version string. > > > > >> > > > > >> e.g. An "eq" match on 4.8.5 doesn't protect the user who is > > using 4.8.6, > > > > >> and Make doesn't > > > > >> seem to do substring stuff unless you mess around with shells. > > > > >> > > > > >> Plus, as people have said, it's better to solve problem x (or > > work around > > > > >> a specific > > > > >> instance of x) than to ignore the exception, even if the > > ignoring code is > > > > >> toolchain- > > > > >> specific. > > > > >> > > > > >> - Adam Farley > > > > >> > > > > >> > On 2018-03-27 18:44, Phil Race wrote: > > > > >> > > > > > >> >> As I said I prefer the make file change, since this is a > > change to an > > > > >> >> upstream library. > > > > >> > > > > > >> > Newtons fourth law: For every reviewer, there's an equal > and opposite > > > > >> > reviewer. :) > > > > >> > > > > > >> > Here I am, advocating a source code fix. As Thomas says, > the compiler > > > > >> > complaint seems reasonable, and disabling it might cause us > > > to miss other > > > > >> > future errors. > > > > >> > > > > > >> > Why can't we push the source code fix, and also submit itupstream? > > > > >> > > > > > >> > /Magnus > > > > >> > > > > > >> > > > > > >> > I've looked at jpeg-9c and it still looks identical to > whatwe have in > > > > >> > 6b, so this code > > > > >> > seems to have stood the test of time. I'm also unclear why > > the compiler > > > > >> > would > > > > >> > complain about that and not the one a few lines later > > > > >> > > > > > >> > > > > > >> > 819 while (bits[i] == 0) /* find largest > > > codelength still in > > > > >> > use */ > > > > >> > 820 i--; > > > > >> > > > > > >> > A push to jchuff.c will get blown away if/when we upgrade. > > > > >> > A tool-chain specific fix in the makefile with an > > appropriatecomment is > > > > >> > more targeted. > > > > >> > > > > >> Phil, > > > > >> > > > > >> Returning to this. > > > > >> > > > > >> While I understand your reluctance to patch upstream code, > let me point > > > > >> out that we have not updated libjpeg a single time since the > > JDK was open > > > > >> sourced. We're using 6b from 27-Mar-1998. I had a look at the > > > Wikipedia page > > > > >> on libjpeg, and this is the latest "uncontroversial" version of > > > the source. > > > > >> Versions 7 and up have proprietary extensions, which in turn > > > has resulted in > > > > >> multiple forks of libjpeg. As it stands, it seems unlikely that > > > we will ever > > > > >> replace libjpeg 6b with a simple upgrade from upstream. > > > > >> > > > > >> I therefore maintain my position that a source code fix would > > be the best > > > > >> way forward here. > > > > >> > > > > >> /Magnus > > > > >> > > > > >> > > > > > >> > > > > > >> > -phil. > > > > >> > > > > > >> > > > > > >> > On 03/27/2018 05:44 AM, Thomas Stüfe wrote: > > > > >> > > > > > >> > Hi all, > > > > >> > > > > > >> > > > > > >> > just a friendly reminder. I would like to push this tiny > fix because > > > > >> > tripping over this on our linux s390x builds is annoying > (yes, we can > > > > >> > maintain patch queues, but this is a constant error source). > > > > >> > > > > > >> > > > > > >> > I will wait for 24 more hours until a reaction. If no > > seriousobjections > > > > >> > are forcoming, I want to push it (tier1 tests ran thru, and > > > me and Christoph > > > > >> > langer are both Reviewers). > > > > >> > > > > > >> > > > > > >> > Thanks! Thomas > > > > >> > > > > > >> > > > > > >> > On Wed, Mar 21, 2018 at 6:20 PM, Thomas Stüfe > > <thomas.stu...@gmail.com> > > > > >> > wrote: > > > > >> > > > > > >> > Hi all, > > > > >> > > > > > >> > > > > > >> > may I please have another review for this really trivial > change. It > > > > >> > fixes a gcc warning on s390 and ppc. Also, it is probably a > > > good idea to fix > > > > >> > this. > > > > >> > > > > > >> > > > > > >> > bug: https://bugs.openjdk.java.net/browse/JDK-8200052 > > > > >> > webrev: > > > > >> > http://cr.openjdk.java.net/~stuefe/webrevs/8200052-fix- > > > warning-in-jchuff.c/webrev.00/webrev/ > > > > >> > > > > > >> > > > > > >> > This was contributed by Adam Farley at IBM. > > > > >> > > > > > >> > > > > > >> > I already reviewed this. I also test-built on zlinux and it works. > > > > >> > > > > > >> > > > > > >> > Thanks, Thomas > > > > >> > > > > > >> > > > > >> Unless stated otherwise above: > > > > >> IBM United Kingdom Limited - Registered in England and Wales > > with number > > > > >> 741598. > > > > >> Registered office: PO Box 41, North Harbour, Portsmouth, > > > Hampshire PO6 3AU > > > > >> > > > > >> > > > > > > > > > > Unless stated otherwise above: > > > > > IBM United Kingdom Limited - Registered in England and Wales > with number > > > > > 741598. > > > > > Registered office: PO Box 41, North Harbour, Portsmouth, > > Hampshire PO6 3AU > > > > > > > > > > > > > > > > Unless stated otherwise above: > > > IBM United Kingdom Limited - Registered in England and Wales with > > > number 741598. > > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > > > > Unless stated otherwise above: > > IBM United Kingdom Limited - Registered in England and Wales with > > number 741598. > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > Unless stated otherwise above: > IBM United Kingdom Limited - Registered in England and Wales with > number 741598. > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU