Request for approval to sync Feb 2013 CPU fixes into jdk6-open
Andrew Hughes
gnu.andrew at redhat.com
Thu Mar 7 07:19:00 PST 2013
----- Original Message -----
> Hi Andrew,
>
> thanks for the review. I am pushing the change.
>
> Sorry for the mess.
>
It's ok. I'm still baffled that javac misses it. I was able to build with OpenJDK6,
just not with IcedTea's bootstrapping setup, which uses ecj.
Thanks for the quick fix.
> 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
> >>>>>>>
> >>>>>>>
> >>
>
>
--
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