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

Phil Race philip.race at oracle.com
Wed May 30 17:06:19 UTC 2018 

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 it upstream?
> > > >> >
> > > >> > /Magnus
> > > >> >
> > > >> >
> > > >> > I've looked at jpeg-9c and it still looks identical to what we 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- 
> <http://cr.openjdk.java.net/%7Estuefe/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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180530/d4bfc1d0/attachment-0001.html>

More information about the 2d-dev mailing list