Request for approval to sync Feb 2013 CPU fixes into jdk6-open
Andrew Brygin
andrew.brygin at oracle.com
Thu Mar 7 07:14:56 PST 2013
Hi Andrew,
thanks for the review. I am pushing the change.
Sorry for the mess.
Thanks,
Andrew
On 3/7/2013 7:12 PM, Andrew Hughes wrote:
> ----- Original Message -----
>> Hello,
>>
>> please review a fix for the build failure:
>>
>> http://cr.openjdk.java.net/~bae/8009641/webrev.00/
>>
> Looks good to me and better than what I had as the synchronized blocks were still outside the try
> (so the variables could end up being null).
>
> Ok to commit.
>
>> Thanks,
>> Andrew
>>
>> On 3/7/2013 5:55 PM, Seán Coffey wrote:
>>> Even better if you want to push the change ? I haven't heard from
>>> Andrew B. yet.
>>>
>>> I've logged 8009641 to track this. You could use that ID if you
>>> want.
>>>
>>> 8009641: OpenJDK 6 build broken via 8007675 fix
>>>
>>> regards,
>>> Sean.
>>>
>>> On 07/03/2013 13:51, Andrew Hughes wrote:
>>>> ----- Original Message -----
>>>>> Yes - you're right. That does look like an issue. Andrew Brygin
>>>>> ran
>>>>> pre
>>>>> integration tests before pushing the changes internally and they
>>>>> were
>>>>> successful. However - I've traced back over the sources and what
>>>>> was
>>>>> run
>>>>> in his test build and what he pushed to internal repo differs. (
>>>>> in 2
>>>>> areas) - No doubt, it's the curse of the last minute change.
>>>>>
>>>>> Andrew Brygin, can you log a bug and get this corrected asap ?
>>>>>
>>>>> Thanks for the notice Andrew!
>>>>>
>>>> I have a patch I can post if that would help. With that, this
>>>> builds.
>>>> I just need to clean up my workspace and post a webrev.
>>>>
>>>>> Thanks,
>>>>> Sean.
>>>>>
>>>>>
>>>>>> public short[] colorConvert(short[] src, short[] dst) {
>>>>>>
>>>>>> if (dst == null) {
>>>>>> dst = new short
>>>>>> [(src.length/getNumInComponents())*getNumOutComponents()];
>>>>>> }
>>>>>>
>>>>>> try {
>>>>>> LCMSImageLayout srcIL = new LCMSImageLayout(
>>>>>> src, src.length/getNumInComponents(),
>>>>>> LCMSImageLayout.CHANNELS_SH(getNumInComponents()) |
>>>>>> LCMSImageLayout.BYTES_SH(2),
>>>>>> getNumInComponents()*2);
>>>>>>
>>>>>> LCMSImageLayout dstIL = new LCMSImageLayout(
>>>>>> dst, dst.length/getNumOutComponents(),
>>>>>> LCMSImageLayout.CHANNELS_SH(getNumOutComponents())
>>>>>> |
>>>>>> LCMSImageLayout.BYTES_SH(2),
>>>>>> getNumOutComponents()*2);
>>>>>>
>>>>>> synchronized(this) {
>>>>>> LCMS.colorConvert(this, srcIL, dstIL);
>>>>>> }
>>>>>> } catch (ImageLayoutException e) {
>>>>>> throw new CMMException("Unable to convert data");
>>>>>> }
>>>>>>
>>>>>> return dst;
>>>>>> }
>>>>> On 07/03/2013 12:39, Andrew Hughes wrote:
>>>>>> ----- Original Message -----
>>>>>>> ----- Original Message -----
>>>>>>>> I'm only the proxy here but I created the webrev from the
>>>>>>>> changesets
>>>>>>>> that I pushed. I don't see any difference.
>>>>>>>>
>>>>>>>> t4 $diff LCMSTransform.java.webrev
>>>>>>>> jdk/src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java
>>>>>>>> t4 $
>>>>>>>>
>>>>>>> Indeed. I'm still seeing the same failure using the changesets
>>>>>>> pushed
>>>>>>> to OpenJDK6, yet the repository itself builds. Very odd.
>>>>>>>
>>>>>> The copy of OpenJDK6 I was looking at wasn't up-to-date, hence
>>>>>> the
>>>>>> diff.
>>>>>> The patched version and upstream repo are identical when
>>>>>> compared
>>>>>> correctly.
>>>>>>
>>>>>> What's baffling is how this code compiles with the javac
>>>>>> compiler
>>>>>> as it looks
>>>>>> clearly wrong to me, which is why ecj falls over on it.
>>>>>>
>>>>>> try {
>>>>>> LCMSImageLayout srcIL = new LCMSImageLayout(
>>>>>> src, src.length/getNumInComponents(),
>>>>>> LCMSImageLayout.CHANNELS_SH(getNumInComponents())
>>>>>> |
>>>>>> LCMSImageLayout.BYTES_SH(2),
>>>>>> getNumInComponents()*2);
>>>>>>
>>>>>> LCMSImageLayout dstIL = new LCMSImageLayout(
>>>>>> dst, dst.length/getNumOutComponents(),
>>>>>> LCMSImageLayout.CHANNELS_SH(getNumOutComponents())
>>>>>> |
>>>>>> LCMSImageLayout.BYTES_SH(2),
>>>>>> getNumOutComponents()*2);
>>>>>> } catch (ImageLayoutException e) {
>>>>>> throw new CMMException("Unable to convert data");
>>>>>> }
>>>>>>
>>>>>> synchronized(this) {
>>>>>> LCMS.colorConvert(this, srcIL, dstIL);
>>>>>> }
>>>>>>
>>>>>> srcIL and dstIL are declared inside the try block so aren't
>>>>>> visible
>>>>>> outside it.
>>>>>>
>>>>>> In the other colorConvert methods, these are declared at the top
>>>>>> of
>>>>>> the method.
>>>>>>
>>>>>>>> regards,
>>>>>>>> Sean.
>>>>>>>>
>>>>>>>> On 07/03/2013 10:59, Andrew Hughes wrote:
>>>>>>>>> ----- Original Message -----
>>>>>>>>>> February CPU pushes completed as reviewed in last round of
>>>>>>>>>> webrevs.
>>>>>>>>>>
>>>>>>>>>> I'd like to push 2 extra fixes now for issues addressed in
>>>>>>>>>> yesterday's
>>>>>>>>>> JDK releases.
>>>>>>>>>>
>>>>>>>>>> webrev :
>>>>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.6open.mar5/
>>>>>>>>>>
>>>>>>>>>> Good to push ?
>>>>>>>>>>
>>>>>>>>>> regards,
>>>>>>>>>> Sean.
>>>>>>>>>>
>>>>>>>>>> On 05/03/2013 18:44, Omair Majid wrote:
>>>>>>>>>>> On 03/05/2013 10:52 AM, Edvard Wendelin wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> The change in JAXP can be viewed here:
>>>>>>>>>>>> http://cr.openjdk.java.net/~joehw/jdk8/8001235/webrev/
>>>>>>>>>>>> While
>>>>>>>>>>>> it's
>>>>>>>>>>>> generated against JDK 8, the change in 6 is identical.
>>>>>>>>>>> The JAXP changes look identical to what was pushed to
>>>>>>>>>>> jdk7u.
>>>>>>>>>>> Looks
>>>>>>>>>>> all
>>>>>>>>>>> good to me!
>>>>>>>>>>>
>>>>>>>>>>>> I plan to push the changes today.
>>>>>>>>>>> Thank you. It will be great to have all the security fixes
>>>>>>>>>>> in!
>>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> Omair
>>>>>>>>>>>
>>>>>>>>> I notice that what was finally committed differs from this
>>>>>>>>> webrev
>>>>>>>>> (in a positive
>>>>>>>>> way). The version posted from the webrev failed to compile:
>>>>>>>>>
>>>>>>>>> 1. ERROR in
>>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java
>>>>>>>>> (at line 580)
>>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL);
>>>>>>>>> ^^^^^
>>>>>>>>> srcIL cannot be resolved to a variable
>>>>>>>>> ----------
>>>>>>>>> 2. ERROR in
>>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java
>>>>>>>>> (at line 580)
>>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL);
>>>>>>>>> ^^^^^
>>>>>>>>> dstIL cannot be resolved to a variable
>>>>>>>>> ----------
>>>>>>>>> 3. ERROR in
>>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java
>>>>>>>>> (at line 606)
>>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL);
>>>>>>>>> ^^^^^
>>>>>>>>> srcIL cannot be resolved to a variable
>>>>>>>>> ----------
>>>>>>>>> 4. ERROR in
>>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java
>>>>>>>>> (at line 606)
>>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL);
>>>>>>>>> ^^^^^
>>>>>>>>> dstIL cannot be resolved to a variable
>>>>>>>>>
>>>>>>>>> but this seems to have been fixed in the committed version,
>>>>>>>>> presumably due to:
>>>>>>>>>
>>>>>>>>> - try {
>>>>>>>>> - LCMSImageLayout srcIL = new LCMSImageLayout(
>>>>>>>>> - src, src.length/getNumInComponents(),
>>>>>>>>> -
>>>>>>>>> LCMSImageLayout.CHANNELS_SH(getNumInComponents())
>>>>>>>>> |
>>>>>>>>> - LCMSImageLayout.BYTES_SH(1),
>>>>>>>>> getNumInComponents());
>>>>>>>>> -
>>>>>>>>> - LCMSImageLayout dstIL = new LCMSImageLayout(
>>>>>>>>> - dst, dst.length/getNumOutComponents(),
>>>>>>>>> -
>>>>>>>>> LCMSImageLayout.CHANNELS_SH(getNumOutComponents())
>>>>>>>>> |
>>>>>>>>> - LCMSImageLayout.BYTES_SH(1),
>>>>>>>>> getNumOutComponents());
>>>>>>>>> - } catch (ImageLayoutException e) {
>>>>>>>>> - throw new CMMException("Unable to convert
>>>>>>>>> data");
>>>>>>>>> - }
>>>>>>>>> + LCMSImageLayout srcIL = new LCMSImageLayout(
>>>>>>>>> + src, src.length/getNumInComponents(),
>>>>>>>>> + LCMSImageLayout.CHANNELS_SH(getNumInComponents()) |
>>>>>>>>> + LCMSImageLayout.BYTES_SH(1),
>>>>>>>>> getNumInComponents());
>>>>>>>>> +
>>>>>>>>> + LCMSImageLayout dstIL = new LCMSImageLayout(
>>>>>>>>> + dst, dst.length/getNumOutComponents(),
>>>>>>>>> + LCMSImageLayout.CHANNELS_SH(getNumOutComponents())
>>>>>>>>> |
>>>>>>>>> + LCMSImageLayout.BYTES_SH(1),
>>>>>>>>> getNumOutComponents());
>>>>>>>>>
>>>>>>>>> so srcIL and dstIL are now declared at a wider scope. I'll
>>>>>>>>> use
>>>>>>>>> the
>>>>>>>>> committed versions
>>>>>>>>> in IcedTea6 HEAD.
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>> --
>>>>>>> Andrew :)
>>>>>>>
>>>>>>> Free Java Software Engineer
>>>>>>> Red Hat, Inc. (http://www.redhat.com)
>>>>>>>
>>>>>>> PGP Key: 248BDC07 (https://keys.indymedia.org/)
>>>>>>> Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B
>>>>>>> DC07
>>>>>>>
>>>>>>>
>>
More information about the jdk6-dev
mailing list