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

Adam Farley8 adam.farley at uk.ibm.com
Wed May 30 16:08:08 UTC 2018


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-
> 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/69713ca2/attachment-0001.html>


More information about the 2d-dev mailing list