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.hol...@oracle.com> > Sent: Mittwoch, 26. Februar 2020 03:55 > To: Doerr, Martin <martin.do...@sap.com>; Chris Plummer > <chris.plum...@oracle.com>; OpenJDK Serviceability <serviceability- > d...@openjdk.java.net>; hotspot-runtime-dev <hotspot-runtime- > d...@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.plum...@oracle.com> > > *Sent:* Dienstag, 25. Februar 2020 18:03 > > *To:* Doerr, Martin <martin.do...@sap.com>; OpenJDK Serviceability > > <serviceability-dev@openjdk.java.net>; hotspot-runtime-dev > > <hotspot-runtime-...@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.plum...@oracle.com> > > <mailto:chris.plum...@oracle.com> > > *Sent:* Montag, 24. Februar 2020 21:52 > > *To:* Doerr, Martin <martin.do...@sap.com> > > <mailto:martin.do...@sap.com>; OpenJDK Serviceability > > <serviceability-dev@openjdk.java.net> > > <mailto:serviceability-dev@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.stu...@gmail.com> > > <mailto:thomas.stu...@gmail.com> > > *Sent:* Montag, 24. Februar 2020 14:41 > > *To:* Doerr, Martin <martin.do...@sap.com> > > <mailto:martin.do...@sap.com> > > *Cc:* core-libs-...@openjdk.java.net > > <mailto:core-libs-...@openjdk.java.net>; Lindenmaier, Goetz > > <goetz.lindenma...@sap.com> <mailto:goetz.lindenma...@sap.com>; > > Langer, Christoph <christoph.lan...@sap.com> > > <mailto:christoph.lan...@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.do...@sap.com <mailto:martin.do...@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.stu...@gmail.com > > <mailto:thomas.stu...@gmail.com>> > > *Sent:* Montag, 24. Februar 2020 12:52 > > *To:* Doerr, Martin <martin.do...@sap.com > > <mailto:martin.do...@sap.com>> > > *Cc:* core-libs-...@openjdk.java.net > > <mailto:core-libs-...@openjdk.java.net>; Lindenmaier, Goetz > > <goetz.lindenma...@sap.com > > <mailto:goetz.lindenma...@sap.com>>; Langer, Christoph > > <christoph.lan...@sap.com <mailto:christoph.lan...@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.do...@sap.com <mailto:martin.do...@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 > >
