RFR(XS): 8239856: [ntintel] asserts about copying unaligned array element
David Holmes
david.holmes at oracle.com
Wed Feb 26 13:32:15 UTC 2020
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 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