Hi David, > > GCC supports -malign-double for jlong / jdouble alignment on 32 bit > processors [2]. > > But we aren't using that in the build AFAICS and it isn't the default on > x86_32.
We have stopped building linux 32 bit since JDK8, so I can't tell. At least -malign-double should be a valid workaround which we don't have for Windows 32 bit. Best regards, Martin > -----Original Message----- > From: David Holmes <david.hol...@oracle.com> > Sent: Mittwoch, 26. Februar 2020 14:32 > 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 > > On 26/02/2020 8:16 pm, Doerr, Martin wrote: > > 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]. > > But we aren't using that in the build AFAICS and it isn't the default on > x86_32. > > David > ----- > > > 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 > >>>
