[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR: Bug Pending: Build fails to compile jchuff.c
From: Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date: 2018-03-27 12:49:07
Message-ID: CAA-vtUzg+p7an1kxWG4SfPZw-U4e-3EyF83U-rT5raeKTtsjMg () mail ! gmail ! com
[Download RAW message or body]
Hi all,
Last week I openend JDK-8200052 and posted it to 2d-dev for RFR.
Me included we have two reviewers and the tier1 tests ran through. Are
there really any serious objections against pushing this tiny fix? It would
make life for us (working on zLinux) easier.
I will wait for 24 more hours until a reaction. If no serious objections
are forthcoming, I will push it.
Thanks! Thomas
On Thu, Mar 22, 2018 at 10:57 AM, Adam Farley8 <adam.farley@uk.ibm.com>
wrote:
> Hi Guys,
>
> I've provided a gcc-specific fix in the makefile to prevent the warning.
>
> -- Awt2dLibraries.gmk:471 --
> DISABLED_WARNINGS_gcc := *array-bounds* clobbered implicit-fallthrough
> shift-negative-value, \
>
> I've also provided an underflow fix in the .c file to fix the problem
> *causing* the warning.
>
> -- jchuff.c:808 --
> while (*(*bits[j] == 0)* && (j > 0)*)
>
> Either will work fine.
>
> Note: After determining that it affects multiple gcc versions, and that
> the logic to make a makefile do a
> compare (the shell business) on the gcc version seemed hacky to me, I
> considered the best solution
> to be one of the two simple fixes outlined above. This seemed to be
> acceptable to people in the
> community, yet we're still having trouble getting this fix through.
>
> I'm not sure why.
>
> Best Regards
>
> Adam Farley
>
>
> > Hi Phil!
> >
> >
> > thanks for pointing out the history, I was not aware of that.
> >
> >
> > I looked at that huffman coding and tried to determine whether the
> underflow may happen in real life scenarios. I could at least not exclude
> that possibility. I looked thru the mailing list threads - did someone
> analyse and conclude for sure this was just a pointless compiler warning?
> >
> >
> > I would prefer the pragmatic solution (and IMHO also safer one) of
> fixing this underflow in the proposed fashion. I had opened a bug report
> earlier today. However, if someone already spent brain cycles on it and a
> patch - in whatever form - is forthcoming, I do not want to butt in. In
> that case I will close this bug again.
> >
> >
> > I would just like to see this fixed this because it affects us at SAP
> too.
> >
> >
> > Kind Regards, Thomas
> >
> >
> > On Wed, Mar 21, 2018 at 6:56 PM, Phil Race <philip.race@oracle.com>
> wrote:
> >
> > I prefer the makefile fix, since we don't by policy, make changes to the
> imported libraries.
> >
> > On Jan 23rd [1] I expressed such a tool-chain specific makefile fix
> would be fine by me.
> >
> > Toolchain specific means ideally it would look like what Magnus wrote [2]
> >
> > Although you said GC 5.4.0 would need to be included in the logic.
> >
> > If it can be shown to affect current / future versions of gcc then it
> could be unqualified.
> >
> > I think we've just been waiting for a webrev since then ..
> >
> > -phil.
> >
> > [1] http://mail.openjdk.java.net/pipermail/2d-dev/2018-January/
> 008855.html
> > [2] http://mail.openjdk.java.net/pipermail/build-dev/2018-
> January/020695.html
> >
> >
> >
> > On 03/21/2018 09:53 AM, Adam Farley8 wrote:
> >
> > :)
> >
> > > Hi Adam,
> > >
> > > no problem. I'll open a bug and if necessary find a second reviewer.
> Thanks for fixing, maybe I can stop building with warnings disabled on our
> s390 machines now.
> > >
> > > ..Thomas
> > >
> > > > On Wed, Mar 21, 2018 at 5:10 PM, Andrew Leonard <
> andrew_m_leonard@uk.ibm.com> wrote:
> > > > Hi Thomas,
> > > > I'm a "contributor", but not a "committer", so not on that list,
> didn't even know that
> > > > list existed! I was sort of assuming since it was a trivial change,
> and the request was
> > > > for a review, i'd chip in...!
> > > > Thanks
> > > > Andrew
> > >
> > > > Andrew Leonard
> > > > Java Runtimes Development
> > > > IBM Hursley
> > > > IBM United Kingdom Ltd
> > > > Phone internal: 245913, external: 01962 815913
> > > > internet email: andrew_m_leonard@uk.ibm.com
> > >
> >
>
> 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
>
[Attachment #3 (text/html)]
<div dir="ltr">
<span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-sty \
le:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;lette \
r-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:norm \
al;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Hi \
all,</span><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:smal \
l;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight \
:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white- \
space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">Last \
week I openend JDK-8200052 and posted it to 2d-dev for RFR. </div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;wor \
d-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">Me \
included we have two reviewers and the tier1 tests ran through. Are there really any \
serious objections against pushing this tiny fix? It would make life for us (working \
on zLinux) easier.</div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;wor \
d-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">I \
will wait for 24 more hours until a reaction. If no serious objections are \
forthcoming, I will push it.</div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;wor \
d-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div \
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:nor \
mal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spac \
ing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;wor \
d-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">Thanks! \
Thomas</div>
<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 22, 2018 \
at 10:57 AM, Adam Farley8 <span dir="ltr"><<a href="mailto:adam.farley@uk.ibm.com" \
target="_blank">adam.farley@uk.ibm.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span style="font-size:10pt;font-family:sans-serif">Hi \
Guys,</span> <br>
<br><span style="font-size:10pt;font-family:sans-serif">I've provided
a gcc-specific fix in the makefile to prevent the warning.</span>
<br>
<br><span style="font-size:10pt;font-family:sans-serif">-- Awt2dLibraries.gmk:471
--</span>
<br><tt><span style="font-size:12pt">DISABLED_WARNINGS_gcc := </span></tt><tt><span \
style="font-size:12pt;color:#ff00ff"><b>array-bounds</b></span></tt><tt><span \
style="font-size:12pt"> clobbered implicit-fallthrough shift-negative-value, \
\</span></tt> <br>
<br><span style="font-size:10pt;font-family:sans-serif">I've also provided
an underflow fix in the .c file to fix the problem *causing* the warning.</span>
<br>
<br><span style="font-size:10pt;font-family:sans-serif">-- jchuff.c:808
--</span>
<br><span class=""><tt><span style="font-size:12pt">while (</span></tt><tt><span \
style="font-size:12pt;color:#ff00ff"><b>(</b></span></tt><tt><span \
style="font-size:12pt">bits[j] == 0)</span></tt><tt><span \
style="font-size:12pt;color:#ff00ff"><b> && (j > \
0)</b></span></tt><tt><span style="font-size:12pt">)</span></tt> <br>
<br></span><span style="font-size:10pt;font-family:sans-serif">Either will work
fine.</span>
<br>
<br><span style="font-size:10pt;font-family:sans-serif">Note: After determining
that it affects multiple gcc versions, and that the logic to make a makefile
do a </span>
<br><span style="font-size:10pt;font-family:sans-serif">compare (the shell
business) on the gcc version seemed hacky to me, I considered the best
solution</span>
<br><span style="font-size:10pt;font-family:sans-serif">to be one of the
two simple fixes outlined above. This seemed to be acceptable to people
in the</span>
<br><span style="font-size:10pt;font-family:sans-serif">community, yet
we're still having trouble getting this fix through. </span>
<br>
<br><span style="font-size:10pt;font-family:sans-serif">I'm not sure why.</span>
<br>
<br><span style="font-size:10pt;font-family:sans-serif">Best Regards </span>
<br>
<br><span style="font-size:10pt;font-family:sans-serif">Adam Farley</span>
<br><div class="HOEnZb"><div class="h5">
<br>
<br><span style="font-size:10pt;font-family:sans-serif">> Hi Phil!</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> thanks for
pointing out the history, I was not aware of that.</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> I looked
at that huffman coding and tried to determine whether the underflow may
happen in real life scenarios. I could at least not exclude that possibility.
I looked thru the mailing list threads - did someone analyse and conclude
for sure this was just a pointless compiler warning?</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> I would prefer
the pragmatic solution (and IMHO also safer one) of fixing this underflow
in the proposed fashion. I had opened a bug report earlier today. However,
if someone already spent brain cycles on it and a patch - in whatever form
- is forthcoming, I do not want to butt in. In that case I will close this
bug again. </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> I would just
like to see this fixed this because it affects us at SAP too.</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> Kind Regards,
Thomas</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> On Wed, Mar
21, 2018 at 6:56 PM, Phil Race <<a href="mailto:philip.race@oracle.com" \
target="_blank">philip.race@oracle.com</a>> wrote:</span> <br><span \
style="font-size:10pt;font-family:sans-serif">> </span> <br><span \
style="font-size:10pt;font-family:sans-serif">> I prefer the makefile fix, since \
we don't by policy, make changes to the imported libraries.</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> On Jan 23rd
[1] I expressed such a tool-chain specific makefile fix would be fine by
me.</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> Toolchain
specific means ideally it would look like what Magnus wrote [2]</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> Although
you said GC 5.4.0 would need to be included in the logic.</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> If it can
be shown to affect current / future versions of gcc then it could be \
unqualified.</span> <br><span style="font-size:10pt;font-family:sans-serif">> \
</span> <br><span style="font-size:10pt;font-family:sans-serif">> I think \
we've just been waiting for a webrev since then ..</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> -phil.</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> [1] </span><a \
href="http://mail.openjdk.java.net/pipermail/2d-dev/2018-January/008855.html" \
target="_blank"><span \
style="font-size:10pt;color:blue;font-family:sans-serif">http://mail.openjdk.java.net/<wbr>pipermail/2d-dev/2018-January/<wbr>008855.html</span></a>
<br><span style="font-size:10pt;font-family:sans-serif">> [2] </span><a \
href="http://mail.openjdk.java.net/pipermail/build-dev/2018-January/020695.html" \
target="_blank"><span \
style="font-size:10pt;color:blue;font-family:sans-serif">http://mail.openjdk.java.net/<wbr>pipermail/build-dev/2018-<wbr>January/020695.html</span></a>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> On 03/21/2018
09:53 AM, Adam Farley8 wrote:</span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> :) </span>
<br><span style="font-size:10pt;font-family:sans-serif">> </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > Hi Adam,
</span>
<br><span style="font-size:10pt;font-family:sans-serif">> > </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > no problem.
I'll open a bug and if necessary find a second reviewer. Thanks for fixing,
maybe I can stop building with warnings disabled on our s390 machines now.
</span>
<br><span style="font-size:10pt;font-family:sans-serif">> > </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > ..Thomas
</span>
<br><span style="font-size:10pt;font-family:sans-serif">> > </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
On Wed, Mar 21, 2018 at 5:10 PM, Andrew Leonard <<a \
href="mailto:andrew_m_leonard@uk.ibm.com" \
target="_blank">andrew_m_leonard@uk.ibm.com</a>>
wrote: </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
Hi Thomas, </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
I'm a "contributor", but not a "committer", so not
on that list, didn't even know that </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
list existed! I was sort of assuming since it was a trivial change, and
the request was </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
for a review, i'd chip in...! </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
Thanks </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
Andrew </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
Andrew Leonard </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
Java Runtimes Development </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
IBM Hursley </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
IBM United Kingdom Ltd </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
Phone internal: 245913, external: 01962 815913 </span>
<br><span style="font-size:10pt;font-family:sans-serif">> > >
internet email: <a href="mailto:andrew_m_leonard@uk.ibm.com" \
target="_blank">andrew_m_leonard@uk.ibm.com</a> </span> <br><span \
style="font-size:10pt;font-family:sans-serif">> > </span> <br><span \
style="font-size:10pt;font-family:sans-serif">> </span> <br><span \
style="font-size:10pt;font-family:sans-serif"><br> Unless stated otherwise above:<br>
IBM United Kingdom Limited - Registered in England and Wales with number
741598. <br>
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
3AU<br>
</span></div></div></blockquote></div><br></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic