[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR(XS): 8239856: [ntintel] asserts about copying unaligned array element
From: "Doerr, Martin" <martin.doerr () sap ! com>
Date: 2020-02-26 16:39:03
Message-ID: AM0PR0202MB32975E2A9205AD6640DCA7389AEA0 () AM0PR0202MB3297 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
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.holmes@oracle.com>
> Sent: Mittwoch, 26. Februar 2020 14:32
> To: Doerr, Martin <martin.doerr@sap.com>; Chris Plummer
> <chris.plummer@oracle.com>; OpenJDK Serviceability <serviceability-
> dev@openjdk.java.net>; hotspot-runtime-dev <hotspot-runtime-
> dev@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.holmes@oracle.com>
> >> Sent: Mittwoch, 26. Februar 2020 03:55
> >> To: Doerr, Martin <martin.doerr@sap.com>; Chris Plummer
> >> <chris.plummer@oracle.com>; OpenJDK Serviceability <serviceability-
> >> dev@openjdk.java.net>; hotspot-runtime-dev <hotspot-runtime-
> >> dev@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@oracle.com>
> >>> *Sent:* Dienstag, 25. Februar 2020 18:03
> >>> *To:* Doerr, Martin <martin.doerr@sap.com>; OpenJDK Serviceability
> >>> <serviceability-dev@openjdk.java.net>; hotspot-runtime-dev
> >>> <hotspot-runtime-dev@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@oracle.com>
> >>> <mailto:chris.plummer@oracle.com>
> >>> *Sent:* Montag, 24. Februar 2020 21:52
> >>> *To:* Doerr, Martin <martin.doerr@sap.com>
> >>> <mailto:martin.doerr@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.stuefe@gmail.com>
> >>> <mailto:thomas.stuefe@gmail.com>
> >>> *Sent:* Montag, 24. Februar 2020 14:41
> >>> *To:* Doerr, Martin <martin.doerr@sap.com>
> >>> <mailto:martin.doerr@sap.com>
> >>> *Cc:* core-libs-dev@openjdk.java.net
> >>> <mailto:core-libs-dev@openjdk.java.net>; Lindenmaier, Goetz
> >>> <goetz.lindenmaier@sap.com>
> <mailto:goetz.lindenmaier@sap.com>;
> >>> Langer, Christoph <christoph.langer@sap.com>
> >>> <mailto:christoph.langer@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@sap.com <mailto:martin.doerr@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@gmail.com
> >>> <mailto:thomas.stuefe@gmail.com>>
> >>> *Sent:* Montag, 24. Februar 2020 12:52
> >>> *To:* Doerr, Martin <martin.doerr@sap.com
> >>> <mailto:martin.doerr@sap.com>>
> >>> *Cc:* core-libs-dev@openjdk.java.net
> >>> <mailto:core-libs-dev@openjdk.java.net>; Lindenmaier, Goetz
> >>> <goetz.lindenmaier@sap.com
> >>> <mailto:goetz.lindenmaier@sap.com>>; Langer, Christoph
> >>> <christoph.langer@sap.com <mailto:christoph.langer@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@sap.com <mailto:martin.doerr@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
> >>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic