<span style=" font-size:10pt;font-family:sans-serif">Hi,</span>
<br><span style=" font-size:10pt;font-family:sans-serif">So there seems
to be varying opinion here, taking the 2D view point since it is going
to be maintained, the opinion seems to be more with the fix (</span><a href="http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/"><span style=" font-size:10pt;color:blue;font-family:sans-serif">http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/</span></a><span style=" font-size:10pt;font-family:sans-serif">).
This would be my personal preference also, but previous comments seemed
to prefer compiler options.</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">Looking at each
error:</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- </span><span style=" font-size:12pt"><b>/libfdlibm/k_rem_pio2.c</b>
: This is a simple "for" loop bound check, which with good programming
practice should really have been there to prevent ArrayOutOfBounds.. or
native overwrites.. Simple fix.</span>
<br><span style=" font-size:12pt">- <b>libmlib_image/mlib_ImageLookUp_Bit.c</b>
: This is -ve bit shifting which is spec undefined, which in theory could
mean it breaks in the future if we upgrade/change compiler... However,
this is complex code, we need to be very sure the new fix is "correct",
myself and my colleague here have examined it closely and are happy, but
i'd appreciate your in-depth analysis please. </span>
<br>
<br><span style=" font-size:12pt">Magnus, Philip, Brian, Goetz, can we
have a vote? => "Fix" or "DisableWarnings" ?</span>
<br>
<br><span style=" font-size:12pt">Thanks</span>
<br><span style=" font-size:12pt">Andrew</span>
<br>
<br><span style=" font-size:12pt">Andrew Leonard<br>
Java Runtimes Development<br>
IBM Hursley<br>
IBM United Kingdom Ltd<br>
Phone internal: 245913, external: 01962 815913<br>
internet email: andrew_m_leonard@uk.ibm.com </span>
<br>
<br>
<br>
<br>
<br><span style=" font-size:9pt;color:#5f5f5f;font-family:sans-serif">From:
       </span><span style=" font-size:9pt;font-family:sans-serif">Magnus
Ihse Bursie <magnus.ihse.bursie@oracle.com></span>
<br><span style=" font-size:9pt;color:#5f5f5f;font-family:sans-serif">To:
       </span><span style=" font-size:9pt;font-family:sans-serif">Philip
Race <philip.race@oracle.com>, Brian Burkhalter <brian.burkhalter@oracle.com></span>
<br><span style=" font-size:9pt;color:#5f5f5f;font-family:sans-serif">Cc:
       </span><span style=" font-size:9pt;font-family:sans-serif">2d-dev
<2d-dev@openjdk.java.net>, build-dev <build-dev@openjdk.java.net>,
Andrew Leonard <andrew_m_leonard@uk.ibm.com>, core-libs-dev <core-libs-dev@openjdk.java.net></span>
<br><span style=" font-size:9pt;color:#5f5f5f;font-family:sans-serif">Date:
       </span><span style=" font-size:9pt;font-family:sans-serif">31/08/2018
09:27</span>
<br><span style=" font-size:9pt;color:#5f5f5f;font-family:sans-serif">Subject:
       </span><span style=" font-size:9pt;font-family:sans-serif">Re:
[OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux</span>
<br>
<hr noshade>
<br>
<br>
<br><tt><span style=" font-size:10pt">On 2018-08-31 01:28, Philip Race
wrote:<br>
> Some day, I'd like to replace a lot of medialib functionality with
<br>
> something<br>
> like the proposed Vector API. But that is far enough away that <br>
> medialib needs<br>
> to be maintained, and unlike a previous discussion about a similar
<br>
> issue in<br>
> the JPEG library, we are on the hook for maintaining medialib.<br>
> So if there is an actual logic error in medialib, I'd prefer to fix
it.<br>
> If the issue is a false positive, I'd be OK to disable the warning,
<br>
> but wrapped<br>
> in a platform-specific manner. So is it a real error here ?<br>
<br>
Gcc is emitting a warning due to shifting of negative values, which is
<br>
deemed undefined behavior by the spec.<br>
<br>
Is this a real error? Well. Undefined behavior is no good. It can work
<br>
for now and then suddenly start breaking.<br>
<br>
The originally suggested code change <br>
(</span></tt><a href="http://cr.openjdk.java.net/~aleonard/8209786/webrev.00"><tt><span style=" font-size:10pt">http://cr.openjdk.java.net/~aleonard/8209786/webrev.00</span></tt></a><tt><span style=" font-size:10pt">)
seems <br>
reasonable to me. It is at the very least much clearer what the <br>
intention is. And it's conformant with the spec.<br>
<br>
So I'd advocate using that fix, if you want to continue supporting the
<br>
library.<br>
<br>
/Magnus<br>
<br>
<br>
<br>
<br>
><br>
> -phil.<br>
><br>
> On 8/30/18, 3:37 PM, Brian Burkhalter wrote:<br>
>> Hi Andrew,<br>
>><br>
>> As noted in the issue comments, the fdlibm C code is obsolescent
and <br>
>> eventually to be superseded by equivalent Java implementations.
As <br>
>> for the mediaLib code being moribund I cannot speak but am copying
<br>
>> 2d-dev which owns java.desktop.<br>
>><br>
>> Thanks,<br>
>><br>
>> Brian<br>
>><br>
>> On Aug 30, 2018, at 6:03 AM, Andrew Leonard <br>
>> <andrew_m_leonard@uk.ibm.com <</span></tt><a href="mailto:andrew_m_leonard@uk.ibm.com"><tt><span style=" font-size:10pt">mailto:andrew_m_leonard@uk.ibm.com</span></tt></a><tt><span style=" font-size:10pt">>>
<br>
>> wrote:<br>
>><br>
>>> Thanks Magnus,<br>
>>> Yes, these libraries are moribound it seems, hence their preference
<br>
>>> for compile options..<br>
>>> Cheers<br>
>>> Andrew<br>
>>><br>
>>> Andrew Leonard<br>
>>> Java Runtimes Development<br>
>>> IBM Hursley<br>
>>> IBM United Kingdom Ltd<br>
>>> Phone internal: 245913, external: 01962 815913<br>
>>> internet email: andrew_m_leonard@uk.ibm.com <br>
>>> <</span></tt><a href="mailto:andrew_m_leonard@uk.ibm.com"><tt><span style=" font-size:10pt">mailto:andrew_m_leonard@uk.ibm.com</span></tt></a><tt><span style=" font-size:10pt">><br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> From: Magnus Ihse Bursie <magnus.ihse.bursie@oracle.com
<br>
>>> <</span></tt><a href="mailto:magnus.ihse.bursie@oracle.com"><tt><span style=" font-size:10pt">mailto:magnus.ihse.bursie@oracle.com</span></tt></a><tt><span style=" font-size:10pt">>><br>
>>> To: Andrew Leonard <andrew_m_leonard@uk.ibm.com <br>
>>> <</span></tt><a href="mailto:andrew_m_leonard@uk.ibm.com"><tt><span style=" font-size:10pt">mailto:andrew_m_leonard@uk.ibm.com</span></tt></a><tt><span style=" font-size:10pt">>>,
Brian Burkhalter <br>
>>> <brian.burkhalter@oracle.com <</span></tt><a href="mailto:brian.burkhalter@oracle.com"><tt><span style=" font-size:10pt">mailto:brian.burkhalter@oracle.com</span></tt></a><tt><span style=" font-size:10pt">>><br>
>>> Cc: core-libs-dev@openjdk.java.net <br>
>>> <</span></tt><a href="mailto:core-libs-dev@openjdk.java.net"><tt><span style=" font-size:10pt">mailto:core-libs-dev@openjdk.java.net</span></tt></a><tt><span style=" font-size:10pt">>,
build-dev <br>
>>> <build-dev@openjdk.java.net <</span></tt><a href="mailto:build-dev@openjdk.java.net"><tt><span style=" font-size:10pt">mailto:build-dev@openjdk.java.net</span></tt></a><tt><span style=" font-size:10pt">>><br>
>>> Date: 30/08/2018 13:49<br>
>>> Subject: Re: RFR JDK-8209786: gcc 7.3 compiler errors on zLinux<br>
>>> ------------------------------------------------------------------------
<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> Andrew,<br>
>>><br>
>>> If you want to make changes to the build system (files in
make/*),<br>
>>> please always include build-dev in the reviews.<br>
>>><br>
>>> From a build point, this fix looks okay. My general preference
is to<br>
>>> fix shady code instead of disabling warnings, but in this
case it's in<br>
>>> two libraries that are either external or moribound, so if
the<br>
>>> maintainer's of the respective libraries want to avoid code
changes, I<br>
>>> accept that.<br>
>>><br>
>>> /Magnus<br>
>>><br>
>>><br>
>>> On 2018-08-30 14:18, Andrew Leonard wrote:<br>
>>> > Hi Brian,<br>
>>> > Thanks for taking a look at this, I have just done a
rebuild with <br>
>>> a new<br>
>>> > patch with appropriate gcc disable warnings for these
libraries:<br>
>>> > </span></tt><a href="http://cr.openjdk.java.net/~aleonard/8209786/webrev.01/"><tt><span style=" font-size:10pt">http://cr.openjdk.java.net/~aleonard/8209786/webrev.01/</span></tt></a><tt><span style=" font-size:10pt">
<br>
>>> <</span></tt><a href="http://cr.openjdk.java.net/%7Ealeonard/8209786/webrev.01/"><tt><span style=" font-size:10pt">http://cr.openjdk.java.net/%7Ealeonard/8209786/webrev.01/</span></tt></a><tt><span style=" font-size:10pt">><br>
>>> > This works fine, so if you think this is a more favourable
<br>
>>> approach for<br>
>>> > these libraries? i'd like to get this merged please.<br>
>>> > Thanks<br>
>>> > Andrew<br>
>>> ><br>
>>> > Andrew Leonard<br>
>>> > Java Runtimes Development<br>
>>> > IBM Hursley<br>
>>> > IBM United Kingdom Ltd<br>
>>> > Phone internal: 245913, external: 01962 815913<br>
>>> > internet email: andrew_m_leonard@uk.ibm.com <br>
>>> <</span></tt><a href="mailto:andrew_m_leonard@uk.ibm.com"><tt><span style=" font-size:10pt">mailto:andrew_m_leonard@uk.ibm.com</span></tt></a><tt><span style=" font-size:10pt">><br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > From:   Brian Burkhalter <brian.burkhalter@oracle.com
<br>
>>> <</span></tt><a href="mailto:brian.burkhalter@oracle.com"><tt><span style=" font-size:10pt">mailto:brian.burkhalter@oracle.com</span></tt></a><tt><span style=" font-size:10pt">>><br>
>>> > To:     Andrew Leonard <andrew_m_leonard@uk.ibm.com
<br>
>>> <</span></tt><a href="mailto:andrew_m_leonard@uk.ibm.com"><tt><span style=" font-size:10pt">mailto:andrew_m_leonard@uk.ibm.com</span></tt></a><tt><span style=" font-size:10pt">>><br>
>>> > Cc: core-libs-dev@openjdk.java.net <br>
>>> <</span></tt><a href="mailto:core-libs-dev@openjdk.java.net"><tt><span style=" font-size:10pt">mailto:core-libs-dev@openjdk.java.net</span></tt></a><tt><span style=" font-size:10pt">><br>
>>> > Date:   28/08/2018 15:52<br>
>>> > Subject:        Re:
RFR JDK-8209786: gcc 7.3 compiler errors on <br>
>>> zLinux<br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > Hi Andrew,<br>
>>> ><br>
>>> > It was suggested that it would be preferable to dial
down the <br>
>>> compilation<br>
>>> > settings for the fdlibm code rather than make a source
code <br>
>>> change. Was<br>
>>> > this investigated?<br>
>>> ><br>
>>> > Thanks,<br>
>>> ><br>
>>> > Brian<br>
>>> ><br>
>>> > On Aug 28, 2018, at 7:18 AM, Andrew Leonard <br>
>>> <andrew_m_leonard@uk.ibm.com <</span></tt><a href="mailto:andrew_m_leonard@uk.ibm.com"><tt><span style=" font-size:10pt">mailto:andrew_m_leonard@uk.ibm.com</span></tt></a><tt><span style=" font-size:10pt">>><br>
>>> > wrote:<br>
>>> ><br>
>>> > We have discovered issues with gcc 7.3 on zLinux, combined
with <br>
>>> OpenJDK's<br>
>>> > default compiler options has highlighted a couple of
native code <br>
>>> issues,<br>
>>> > with undefined behaviours:<br>
>>> >   - validating loop test array bounds<br>
>>> >   - left shifts of negative values<br>
>>> > I have created bug </span></tt><a href="https://bugs.openjdk.java.net/browse/JDK-8209786"><tt><span style=" font-size:10pt">https://bugs.openjdk.java.net/browse/JDK-8209786</span></tt></a><tt><span style=" font-size:10pt"><br>
>>> > and attached the webrev fix here:<br>
>>> > </span></tt><a href="http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/"><tt><span style=" font-size:10pt">http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/</span></tt></a><tt><span style=" font-size:10pt">
<br>
>>> <</span></tt><a href="http://cr.openjdk.java.net/%7Ealeonard/8209786/webrev.00/"><tt><span style=" font-size:10pt">http://cr.openjdk.java.net/%7Ealeonard/8209786/webrev.00/</span></tt></a><tt><span style=" font-size:10pt">><br>
>>> ><br>
>>> > This has already been discussed and refined on the "s390x-port-dev"<br>
>>> > maillist<br>
>>> > and as it was pointed out, it should have been posted
here...<br>
>>> ><br>
>>> > I'd like to request a sponsor for this fix please?<br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > Unless stated otherwise above:<br>
>>> > IBM United Kingdom Limited - Registered in England and
Wales with <br>
>>> number<br>
>>> > 741598.<br>
>>> > Registered office: PO Box 41, North Harbour, Portsmouth,
Hampshire <br>
>>> PO6 3AU<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> Unless stated otherwise above:<br>
>>> IBM United Kingdom Limited - Registered in England and Wales
with <br>
>>> number 741598.<br>
>>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
<br>
>>> PO6 3AU<br>
>><br>
<br>
<br>
</span></tt>
<br>
<br>
<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>