[OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix compile warning in jchuff.c

Adam Farley8 adam.farley at uk.ibm.com
Fri Jun 1 08:10:09 UTC 2018


Sounds OK to me. :)

Best Regards

Adam Farley 

Philip Race <philip.race at oracle.com> wrote on 01/06/2018 03:04:12:

> From: Philip Race <philip.race at oracle.com>
> To: Adam Farley8 <adam.farley at uk.ibm.com>
> Cc: 2d-dev <2d-dev at openjdk.java.net>, build-dev <build-
> dev at openjdk.java.net>, Andrew Leonard <andrew_m_leonard at uk.ibm.com>,
> "Stuefe, Thomas" <thomas.stuefe at 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.race at oracle.com> wrote on 30/05/2018 18:06:19:
> 
> > From: Phil Race <philip.race at oracle.com> 
> > To: Adam Farley8 <adam.farley at uk.ibm.com> 
> > Cc: 2d-dev <2d-dev at openjdk.java.net>, Andrew Leonard 
> > <andrew_m_leonard at uk.ibm.com>, build-dev <build-
> > dev at openjdk.java.net>, Magnus Ihse Bursie 
> > <magnus.ihse.bursie at oracle.com>, "Thomas Stüfe" 
<thomas.stuefe at 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.race at oracle.com> 
> > > Cc: 2d-dev <2d-dev at openjdk.java.net>, Andrew Leonard 
> > > <andrew_m_leonard at uk.ibm.com>, build-dev <build-
> > > dev at openjdk.java.net>, Magnus Ihse Bursie 
> > > <magnus.ihse.bursie at oracle.com>, "Thomas Stüfe" 
<thomas.stuefe at 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.bursie at 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.stuefe at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180601/3d988c11/attachment-0001.html>


More information about the 2d-dev mailing list