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/webrev.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/webrev.00/
Please review.
Best regards,
Martin
