RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Zoltán Majó
zoltan.majo at oracle.com
Thu Apr 13 13:29:03 UTC 2017
Hi Lutz,
thank you for the investigation and the detailed description of the problem!
On 04/13/2017 12:10 PM, Schmidt, Lutz wrote:
> [...]
>
> The bug is in MacroAssembler::crc32c_ipl_alg2_alt2() or one of its callees. I have not yet performed an in-depth analysis on what’s going wrong, mainly because I am not a proven expert on x86 machine code.
>
> So how do we proceed from here? Shouldn’t we file a separate bug for this x86 issue? Should this webrev (8176580) be split up into the (ppc, s390) chunk and a separate test bug webrev? Can you push this webrev despite there is an (unrelated) JPRT failure?
I agree, it's a good idea file a separate bug for the issue your test
just uncovered -- I can do that. [Volker actually just did that -- let
me continue in my reply to his email.]
Best regards,
Zoltan
>
> Thanks and best regards,
> Lutz
>
>
>
> On 12.04.2017, 18:36, "hotspot-compiler-dev on behalf of Schmidt, Lutz" <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of lutz.schmidt at sap.com> wrote:
>
> Hi Zoltan,
>
> I’m not able to reproduce the problem as of now. I will try out other (older) x86 machines tomorrow. Could be a problem in fallback code when required vector instructions are not available.
>
> Can you please find out on what H/W (CPU in particular) the tests are failing?
>
> Thanks,
> Lutz
>
>
> On 12.04.2017, 14:38, "Zoltán Majó" <zoltan.majo at oracle.com> wrote:
>
> Hi Lutz,
>
>
> On 04/12/2017 02:14 PM, Schmidt, Lutz wrote:
> > Hi Zoltan,
> >
> > First of all: thanks for trying to push! Second: sorry for the problems you ran into.
>
> you are welcome and no problem, of course.
>
> > I do not have an immediate explanation for the failure – my dev machine is MacOS/x86_64. I will try to reproduce immediately. For the time being: is there any log output that could shed some light on the issue?
>
> Please find the output below (and sorry for not including it before).
>
> Please let me know if you were able to reproduce the problem (or not).
>
> Best regards,
>
>
> Zoltan
>
> command: main -Xbatch compiler.intrinsics.zip.TestCRC32C -m
> reason: User specified action: run main/othervm/timeout=600 -Xbatch compiler.intrinsics.zip.TestCRC32C -m
> Mode: othervm [/othervm specified]
> elapsed time (seconds): 1.915
> configuration:
> STDOUT:
> testing 1050 cases ...
> STDERR:
> ERROR: crc = 6f894393, crcReference = 2cdf6e8f
> java.lang.Exception: TestCRC32C Error
> at compiler.intrinsics.zip.TestCRC32C.check(TestCRC32C.java:218)
> at compiler.intrinsics.zip.TestCRC32C.test_multi(TestCRC32C.java:280)
> at compiler.intrinsics.zip.TestCRC32C.main(TestCRC32C.java:74)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:563)
> at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110)
> at java.base/java.lang.Thread.run(Thread.java:844)
>
> JavaTest Message: Test threw exception: java.lang.Exception: TestCRC32C Error
> JavaTest Message: shutting down test
>
> STATUS:Failed.`main' threw exception: java.lang.Exception: TestCRC32C Error
>
>
>
> >
> > Thanks,
> > Lutz
> >
> >
> > On 12.04.2017, 13:10, "hotspot-compiler-dev on behalf of Zoltán Majó" <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of zoltan.majo at oracle.com> wrote:
> >
> > P.S.: Forgot to mention: The problem does not appear on any other x86_64
> > platform.
> >
> > On 04/12/2017 01:07 PM, Zoltán Majó wrote:
> > > Hi Volker,
> > > Hi Lutz,
> > >
> > >
> > > yesterday I tried to push webrev.03 using JPRT. Unfortunately, the
> > > TestCRC32C.java test you've modified fails on Mac OS X on x86_64. Do
> > > you have an idea why that could be?
> > >
> > > Thank you! Best regards,
> > >
> > >
> > > Zoltan
> > >
> > > On 04/11/2017 06:03 PM, Volker Simonis wrote:
> > >> Thanks a lot Zoltan!
> > >>
> > >>
> > >> On Tue, Apr 11, 2017 at 4:35 PM, Zoltán Majó <zoltan.majo at oracle.com
> > >> <mailto:zoltan.majo at oracle.com>> wrote:
> > >>
> > >> Hi Volker,
> > >>
> > >>
> > >> On 04/11/2017 03:34 PM, Volker Simonis wrote:
> > >>
> > >>
> > >> Hi Zoltan,
> > >>
> > >> could you please be so kind to sponsor this reviewed change
> > >> for jdk10?
> > >>
> > >>
> > >> yes, of course. I'll push it today.
> > >>
> > >> Best regards,
> > >>
> > >>
> > >> Zoltan
> > >>
> > >> Initially we wanted to push it ourselves because it was s390x
> > >> only but now that we've also touched the tests we need a
> > >> sponsor.
> > >>
> > >> Thank you and best regards,
> > >> Volker
> > >>
> > >> ---------- Forwarded message ----------
> > >> From: *Volker Simonis* <volker.simonis at gmail.com
> > >> <mailto:volker.simonis at gmail.com>
> > >> <mailto:volker.simonis at gmail.com
> > >> <mailto:volker.simonis at gmail.com>>>
> > >> Date: Fri, Mar 31, 2017 at 10:53 AM
> > >> Subject: Re: RFR (S) 8176580: [ppc, s390] CRC32C: wrong
> > >> checksum result in some cases
> > >> To: "Schmidt, Lutz" <lutz.schmidt at sap.com
> > >> <mailto:lutz.schmidt at sap.com> <mailto:lutz.schmidt at sap.com
> > >> <mailto:lutz.schmidt at sap.com>>>
> > >> Cc: Andrew Haley <aph at redhat.com <mailto:aph at redhat.com>
> > >> <mailto:aph at redhat.com <mailto:aph at redhat.com>>>,
> > >> "hotspot-compiler-dev at openjdk.java.net
> > >> <mailto:hotspot-compiler-dev at openjdk.java.net>
> > >> <mailto:hotspot-compiler-dev at openjdk.java.net
> > >> <mailto:hotspot-compiler-dev at openjdk.java.net>>"
> > >> <hotspot-compiler-dev at openjdk.java.net
> > >> <mailto:hotspot-compiler-dev at openjdk.java.net>
> > >> <mailto:hotspot-compiler-dev at openjdk.java.net
> > >> <mailto:hotspot-compiler-dev at openjdk.java.net>>>
> > >>
> > >>
> > >> Ping...
> > >>
> > >> Can somebody please push this change?
> > >>
> > >> It's ppc64/s390x only but as a courtesy to the community it
> > >> also fixes
> > >> the CRC JTreg tests so unfortunately I still can't push it
> > >> myself :)
> > >>
> > >> Thank you and best regards,
> > >> Volker
> > >>
> > >>
> > >> On Tue, Mar 28, 2017 at 5:01 PM, Volker Simonis
> > >> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>
> > >> <mailto:volker.simonis at gmail.com
> > >> <mailto:volker.simonis at gmail.com>>> wrote:
> > >> > Hi Lutz,
> > >> >
> > >> > thanks a lot for fixing the test!
> > >> > Your change looks good now.
> > >> >
> > >> > Because this touches shared (i.e. test) files, we still need
> > >> a sponsor
> > >> > so can somebody please sponsor this change?
> > >> >
> > >> > Thank you and best regards,
> > >> > Volker
> > >> >
> > >> >
> > >> >
> > >> > On Fri, Mar 24, 2017 at 4:54 PM, Schmidt, Lutz
> > >> <lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>
> > >> <mailto:lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>>>
> > >> wrote:
> > >> >> Hi Volker,
> > >> >>
> > >> >> Sorry for letting you wait. Here is the final(?) webrev,
> > >> containing all your requests for cleanup and improvements:
> > >> >> http://cr.openjdk.java.net/~lucy/webrevs/8176580.03/
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/>
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/>>
> > >> >>
> > >> >> As before, the *.cpp files have not been modified.
> > >> >>
> > >> >> Best Regards,
> > >> >> Lutz
> > >> >>
> > >> >>
> > >> >>
> > >> >> On 21/03/2017, 17:55, "Volker Simonis"
> > >> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>
> > >> <mailto:volker.simonis at gmail.com
> > >> <mailto:volker.simonis at gmail.com>>> wrote:
> > >> >>
> > >> >> Hi Lutz,
> > >> >>
> > >> >> thanks a lot for updating the tests. I think they look
> > >> much better now.
> > >> >>
> > >> >> There's just one more cleanup I'd like to propose. Can
> > >> you please move
> > >> >> the throw right into the check() function. Just make
> > >> check() return
> > >> >> void and throw from it if there's a mismatch between
> > >> the computed and
> > >> >> the expected result. I leave it up to you if you want
> > >> to pass an extra
> > >> >> error string to check() which will be printed in the
> > >> case of an error.
> > >> >> I personally don't think that's necessary as it will be
> > >> evident from
> > >> >> the stack trace which computation failed.
> > >> >>
> > >> >> Also the try/catch and rethrow in test_multi() isn't
> > >> necessary. The
> > >> >> test can be simply terminated by the initial exception.
> > >> >>
> > >> >> Thank you and best regards,
> > >> >> Volker
> > >> >>
> > >> >>
> > >> >> On Fri, Mar 17, 2017 at 10:03 PM, Schmidt, Lutz
> > >> <lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>
> > >> <mailto:lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>>>
> > >> wrote:
> > >> >> > Hi Volker,
> > >> >> >
> > >> >> > Thanks a lot for your valuable hints.
> > >> >> >
> > >> >> > I have worked some time on the Java test files:
> > >> >> > TestCRC32.java and TestCRC32C.java are now
> > >> identical as far as possible.
> > >> >> > They now throw an exception, should any error be
> > >> detected.
> > >> >> > The “reference CRC value” is now used in
> > >> test_multi() as well.
> > >> >> > The extra test runs have been removed again.
> > >> >> > The test methodology is fixed: each result is
> > >> tested against its reference.
> > >> >> > The tests now detect the bug introduced with
> > >> 8175368 and 8175369.
> > >> >> > No issue is indicated when testing with 8176580.
> > >> >> > I ran jcheck, and to the best of my ability and
> > >> knowledge, there is no trailing whitespace.
> > >> >> > All *.cpp files were left untouched!
> > >> >> >
> > >> >> > The next iteration of the webrev:
> > >> http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/>
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/>>
> > >> >> >
> > >> >> > Best regards,
> > >> >> > Lutz
> > >> >> >
> > >> >> >
> > >> >> > Dr. Lutz Schmidt | SAP JVM | PI SAP CP Core | T: +49
> > >> (6227) 7-42834 <tel:%2B49%20%286227%29%207-42834>
> > >> <tel:%2B49%20%286227%29%207-42834>
> > >> >> >
> > >> >> >
> > >> >> >
> > >> >> > On 16.03.17, 11:28, "Volker Simonis"
> > >> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>
> > >> <mailto:volker.simonis at gmail.com
> > >> <mailto:volker.simonis at gmail.com>>> wrote:
> > >> >> >
> > >> >> > On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz
> > >> <lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>
> > >> <mailto:lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>>>
> > >> wrote:
> > >> >> > >
> > >> >> > > Hi Andrew, Volker,
> > >> >> > >
> > >> >> > > What do you think about these test enhancements?
> > >> >> > > Webrev:
> > >> http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/>
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/
> > >> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/>>
> > >>
> > >> >> > >
> > >> >> > > Please note: the cpp files in the webrev
> > >> remained unchanged.
> > >> >> > >
> > >> >> > > I added some improvements (as I believe) to the
> > >> TestCRC32(C).java files.
> > >> >> > >
> > >> >> > > In some more detail:
> > >> >> > > The test now calculates a “reference CRC
> > >> value”, based on a java implementation of the CRC32 algorithm.
> > >> This reference value is used to verify all other crc values,
> > >> in particular during initialization and warmup. Three
> > >> additional test runs check a non-zero offset with –Xint,
> > >> -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp
> > >> -XX:+TieredCompilation (C1 + C2).
> > >> >> > >
> > >> >> >
> > >> >> > Hi Lutz,
> > >> >> >
> > >> >> > thanks for updating the tests. I've had a closer
> > >> look at the tests and
> > >> >> > realized that they actually can never fail! The
> > >> check() routine just
> > >> >> > prints an error message but that will not let the
> > >> test fail. So I
> > >> >> > would suggest to throw a runtime exception in the
> > >> check() routine
> > >> >> > after the error message was printed.
> > >> >> >
> > >> >> > I also suggest to do the check during the normal
> > >> test execution (i.e.
> > >> >> > in test_multi()) so there's no need for extra
> > >> test runs.
> > >> >> >
> > >> >> > Finally, the current test methodology in
> > >> test_multi() is broken:
> > >> >> > - it sets the reference by calling CRC from the
> > >> interpreter which
> > >> >> > won't work if the intrinsic is also used in the
> > >> interpreter.
> > >> >> > - it only compares the reference against the
> > >> last computation of CRC
> > >> >> > in the loop which will be the result of the C2
> > >> generated code. This
> > >> >> > misses errors in C1.
> > >> >> >
> > >> >> > I suggest to use your new, pure Java
> > >> implementation for the
> > >> >> > computation of the reference result and compare
> > >> the reference with the
> > >> >> > result of calling CRC in every iteration of the
> > >> loop so we really
> > >> >> > check all possibilities from interpreter trough
> > >> C1 to C2.
> > >> >> >
> > >> >> > Finally, can you please pay attention to not
> > >> insert trailing
> > >> >> > whitespace (there was some at line 88 in
> > >> TestCRC32C.java). You can
> > >> >> > easily verify this by running jcheck before
> > >> creating the webrevs.
> > >> >> >
> > >> >> > Thanks,
> > >> >> > Volker
> > >> >> >
> > >> >> > >
> > >> >> > > Best regards,
> > >> >> > > Lutz
> > >> >> > >
> > >> >> > >
> > >> >> > > On 15.03.17, 11:50, "Volker Simonis"
> > >> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>
> > >> <mailto:volker.simonis at gmail.com
> > >> <mailto:volker.simonis at gmail.com>>> wrote:
> > >> >> > >
> > >> >> > > On Tue, Mar 14, 2017 at 7:05 PM, Andrew
> > >> Haley <aph at redhat.com <mailto:aph at redhat.com>
> > >> <mailto:aph at redhat.com <mailto:aph at redhat.com>>> wrote:
> > >> >> > > > On 14/03/17 13:12, Schmidt, Lutz wrote:
> > >> >> > > >
> > >> >> > > >> Yes, one might think of running a test
> > >> suite subset multiple times
> > >> >> > > >> with different parameters. In this case,
> > >> -Xint and/or –Xcomp were
> > >> >> > > >> helpful. Forcing tests to run fully
> > >> interpreted or fully compiled
> > >> >> > > >> helps in cases where a certain function,
> > >> e.g. an intrinsic, is
> > >> >> > > >> invoked via distinct code paths.
> > >> >> > > >
> > >> >> > > > Right, so your patch should include that
> > >> change to the test suite.
> > >> >> > > >
> > >> >> > >
> > >> >> > > Hi Lutz,
> > >> >> > >
> > >> >> > > I agree with Andrew. We should really fix
> > >> the tests such that they
> > >> >> > > check the correctness of the intrinsics.
> > >> >> > >
> > >> >> > > This may be tricky if all three, the
> > >> interpreter, the client and the
> > >> >> > > server compiler use the same intrinsic
> > >> implementation. You could
> > >> >> > > either copy the pure Java implementation
> > >> into the test so that you can
> > >> >> > > compare the results of the intrinsic
> > >> operation against it or you can
> > >> >> > > switch them off in the compilers with
> > >> >> > > "-XX:DisableIntrinsic=_updateBytesCRC32C
> > >> >> > >
> > >> -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and
> > >> compare the
> > >> >> > > results. Not sure which solution is more
> > >> practical, but I would be
> > >> >> > > really scared if we wouldn't have these test.
> > >> >> > >
> > >> >> > > Regards,
> > >> >> > > Volker
> > >> >> > >
> > >> >> > > > Andrew.
> > >> >> > > >
> > >> >> > >
> > >> >> > >
> > >> >> >
> > >> >> >
> > >> >>
> > >> >>
> > >>
> > >>
> > >>
> > >
> >
> >
> >
>
>
>
>
>
More information about the hotspot-compiler-dev
mailing list