RFR(XS): 8239856: [ntintel] asserts about copying unaligned array element
Doerr, Martin
martin.doerr at sap.com
Wed Feb 26 10:16:10 UTC 2020
Hi David,
thanks for you detailed input.
> The presence of the assertion to highlight the need for alignment is
> probably excessive in the case of these JNI APIs, but highly desirable
> for the low-level atomic copy routines themselves. I'm not concerned
> that these exceptions can "leak" up to the application code using these
> JNI API's simply because it only affects debug builds, and is easily
> remedied (either by changing the code or disabling this assertion). But
> if our own JDK code can encounter them, then we should modify that code.
This is an excellent explanation why I've proposed this change.
> Is this a windows only change because other compilers force 64-bit
> alignment of 64-bit types, even in 32-bit environments? I don't like
> seeing this be compiler specific when it is really processor specific
> and to be safe (and keep it simple) we should ensure 8-byte alignment in
> all cases it is needed.
It is a Windows 32 bit only problem.
"Without __declspec(align(#)), the compiler generally aligns data on natural boundaries based on the target processor and the size of the data, up to 4-byte boundaries on 32-bit processors, and 8-byte boundaries on 64-bit processors." [1]
GCC supports -malign-double for jlong / jdouble alignment on 32 bit processors [2].
Best regards,
Martin
[1] https://docs.microsoft.com/en-us/cpp/cpp/align-cpp?view=vs-2019
[2] https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Mittwoch, 26. Februar 2020 03:55
> To: Doerr, Martin <martin.doerr at sap.com>; Chris Plummer
> <chris.plummer at oracle.com>; OpenJDK Serviceability <serviceability-
> dev at openjdk.java.net>; hotspot-runtime-dev <hotspot-runtime-
> dev at openjdk.java.net>
> Subject: Re: RFR(XS): 8239856: [ntintel] asserts about copying unaligned array
> element
>
> Hi Martin,
>
> On 26/02/2020 4:20 am, Doerr, Martin wrote:
> > Hi Chris,
> >
> > I know how JNI is meant. However, C/C++ is (almost) never platform
> > independent. Especially when it comes to primitive types.
>
> There is potentially a question mark over how the JNI
> Get/Set<PrimitiveType>ArrayRegion methods are implemented, as the spec
> makes no mention of atomic updates or accesses. In the absence of any
> mention I would expect normal atomicity rules for Java datatypes to
> apply - which means long and double do not have to be atomic.
>
> If our implementation offers atomicity as an extra feature that is in
> itself okay, but if that feature imposes additional constraints on the
> programmer which are not evident in the specification, that is
> questionable IMO. If the lack of alignment simply results in potential
> non-atomic access that would be fine; but if it results in a runtime h/w
> fault then I would suggest we should not be attempting atomic accesses.
>
> IIUC you have to run in a special mode to enable memory alignment checks
> on x86, so it seems we would potentially just not get atomic accesses.
>
> The presence of the assertion to highlight the need for alignment is
> probably excessive in the case of these JNI APIs, but highly desirable
> for the low-level atomic copy routines themselves. I'm not concerned
> that these exceptions can "leak" up to the application code using these
> JNI API's simply because it only affects debug builds, and is easily
> remedied (either by changing the code or disabling this assertion). But
> if our own JDK code can encounter them, then we should modify that code.
>
> >
> > My change is not particularly beautiful, but I haven’t found a more
> > beautiful way to fix it.
> >
> > Note that SetLongArrayRegion seems to work without the alignment
> > requirement in the product build. However, word tearing could possibly
> > be observed.
> >
> > It's not possible to guarantee element-wise atomicity without alignment
> > because of processor architecture. That’s why I think the assertion
> > makes sense and violations at least in the code which is part of OpenJDK
> > should be fixed IMHO.
>
> Is this a windows only change because other compilers force 64-bit
> alignment of 64-bit types, even in 32-bit environments? I don't like
> seeing this be compiler specific when it is really processor specific
> and to be safe (and keep it simple) we should ensure 8-byte alignment in
> all cases it is needed.
>
> Cheers,
> David
> -----
>
> > I had already asked for alternative fixes when I was working on
> > JDK-8220348 (like force the compiler to 64-bit align 64-bit types on
> > stack), but nobody has found a way to do this.
> >
> > Best regards,
> >
> > Martin
> >
> > *From:*Chris Plummer <chris.plummer at oracle.com>
> > *Sent:* Dienstag, 25. Februar 2020 18:03
> > *To:* Doerr, Martin <martin.doerr at sap.com>; OpenJDK Serviceability
> > <serviceability-dev at openjdk.java.net>; hotspot-runtime-dev
> > <hotspot-runtime-dev at openjdk.java.net>
> > *Subject:* Re: RFR(XS): 8239856: [ntintel] asserts about copying
> > unaligned array element
> >
> > [Adding runtime-dev as this regards the JNI spec]
> >
> > Hi Martin,
> >
> > JNI is meant as a means to write code that interfaces with the JVM in a
> > platform independent way. Therefore the declaration of a jlong or a
> > jdouble should not require any extra platform dependent considerations.
> > This also means requirements of an internal JVM API should not impose
> > any extra requirements on the JNI code. IMHO this should be fixed in
> > hotspot. Maybe fixing it in jni_md.h (if there is a way to force 64-bit
> > alignment) or in the makefiles (force the compiler to 64-bit align)
> > would also be acceptable.
> >
> > thanks,
> >
> > Chris
> >
> > On 2/25/20 3:22 AM, Doerr, Martin wrote:
> >
> > Hi Chris,
> >
> > according to arraycopy.hpp,
> >
> > “arraycopy operations are implicitly atomic on each array element.”
> >
> > This requires 8 Byte alignment for jlong and jdouble.
> >
> > I don’t want to give up this property just because Windows 32 bit
> > doesn’t align them this way by default.
> >
> > All other supported platforms do it right by default.
> >
> > Best regards,
> >
> > Martin
> >
> > *From:*Chris Plummer <chris.plummer at oracle.com>
> > <mailto:chris.plummer at oracle.com>
> > *Sent:* Montag, 24. Februar 2020 21:52
> > *To:* Doerr, Martin <martin.doerr at sap.com>
> > <mailto:martin.doerr at sap.com>; OpenJDK Serviceability
> > <serviceability-dev at openjdk.java.net>
> > <mailto:serviceability-dev at openjdk.java.net>
> > *Subject:* Re: RFR(XS): 8239856: [ntintel] asserts about copying
> > unaligned array element
> >
> > Hi Martin,
> >
> > I'm not so sure I agree with the approach to this fix, nor for the
> > one already done for JDK-8220348. Shouldn't a user be expected to be
> > able to pass a jlong variable to SetLongArrayRegion() without the
> > need for any special platform dependent modifiers added to the
> > declaration of the variable?
> >
> > cheers,
> >
> > Chris
> >
> > On 2/24/20 5:51 AM, Doerr, Martin wrote:
> >
> > Hi,
> >
> > reposting on serviceability-dev (was core-libs-dev before).
> >
> > Bug:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8239856
> >
> > Webrev:
> >
> >
> http://cr.openjdk.java.net/~mdoerr/8239856_win32_long_double_align/we
> brev.00/
> >
> > Thanks for the review, Thomas!
> >
> > Best regards,
> >
> > Martin
> >
> > *From:*Thomas Stüfe <thomas.stuefe at gmail.com>
> > <mailto:thomas.stuefe at gmail.com>
> > *Sent:* Montag, 24. Februar 2020 14:41
> > *To:* Doerr, Martin <martin.doerr at sap.com>
> > <mailto:martin.doerr at sap.com>
> > *Cc:* core-libs-dev at openjdk.java.net
> > <mailto:core-libs-dev at openjdk.java.net>; Lindenmaier, Goetz
> > <goetz.lindenmaier at sap.com> <mailto:goetz.lindenmaier at sap.com>;
> > Langer, Christoph <christoph.langer at sap.com>
> > <mailto:christoph.langer at sap.com>
> > *Subject:* Re: RFR(XS): 8239856: [ntintel] asserts about copying
> > unaligned array element
> >
> > Oh okay. Then it looks okay to me.
> >
> > Cheers, Thomas
> >
> > On Mon, Feb 24, 2020 at 12:56 PM Doerr, Martin
> > <martin.doerr at sap.com <mailto:martin.doerr at sap.com>> wrote:
> >
> > Hi Thomas,
> >
> > thanks for the quick review.
> >
> > ATTRIBUTE_ALIGNED is defined in hotspot. I can’t use it for
> > src/jdk.jdwp.agent/share/native/libjdwp/ArrayReferenceImpl.c.
> >
> > Christoph had already suggested to make it available for
> > core libs, too, but I haven’t found a good place for it.
> >
> > Best regards,
> >
> > Martin
> >
> > *From:*Thomas Stüfe <thomas.stuefe at gmail.com
> > <mailto:thomas.stuefe at gmail.com>>
> > *Sent:* Montag, 24. Februar 2020 12:52
> > *To:* Doerr, Martin <martin.doerr at sap.com
> > <mailto:martin.doerr at sap.com>>
> > *Cc:* core-libs-dev at openjdk.java.net
> > <mailto:core-libs-dev at openjdk.java.net>; Lindenmaier, Goetz
> > <goetz.lindenmaier at sap.com
> > <mailto:goetz.lindenmaier at sap.com>>; Langer, Christoph
> > <christoph.langer at sap.com <mailto:christoph.langer at sap.com>>
> > *Subject:* Re: RFR(XS): 8239856: [ntintel] asserts about
> > copying unaligned array element
> >
> > Hi Martin,
> >
> > maybe use ATTRIBUTE_ALIGNED instead?
> >
> > Cheers, Thomas
> >
> > On Mon, Feb 24, 2020 at 12:44 PM Doerr, Martin
> > <martin.doerr at sap.com <mailto:martin.doerr at sap.com>> wrote:
> >
> > Hi,
> >
> > we had fixed stack array alignment for Windows 32 bit
> > with JDK-8220348.
> >
> > However, there are also stack allocated jlong and
> > jdouble used as source for SetLongArrayRegion and
> > SetDoubleArrayRegion with insufficient alignment for
> > this platform.
> >
> > Here’s my proposed fix:
> >
> >
> http://cr.openjdk.java.net/~mdoerr/8239856_win32_long_double_align/we
> brev.00/
> >
> > Please review.
> >
> > Best regards,
> >
> > Martin
> >
More information about the hotspot-runtime-dev
mailing list