[OpenJDK 2D-Dev] RFR(xxxs): 8200052: libjavajpeg: Fix compile warning in jchuff.c
Philip Race
philip.race at oracle.com
Fri Jun 1 02:04:12 UTC 2018
> 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 it
>> upstream?
>> > > > >> >
>> > > > >> > /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-
>> <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
>> 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/20180531/d0b7826f/attachment-0001.html>
More information about the 2d-dev
mailing list