RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

ok, you are the author – so I won’t insist. ��

Best regards
Christoph

From: Doerr, Martin <martin.doerr at sap.com>
Sent: Freitag, 6. Dezember 2019 12:22
To: Langer, Christoph <christoph.langer at sap.com>
Cc: core-libs-dev at openjdk.java.net; security-dev <security-dev at openjdk.java.net>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Christoph,

that would work, but I don’t want to pollute this file with compiler specific defines. In addition, I don’t like introducing a macro which works on some platforms and does nothing on other ones (which is the case for hotspot’s ATTRIBUTE_ALIGNED).

Because Windows 32 bit is the only affected platform, I prefer not to touch other ones. Are you ok with webrev.00 as it is?

Best regards,
Martin



From: Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>>
Sent: Donnerstag, 5. Dezember 2019 12:16
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>; security-dev <security-dev at openjdk.java.net<mailto:security-dev at openjdk.java.net>>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>; Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

I can see that both places already include headers from src/java.base/share/native/libjava/. I suggest adding the define in jni_util.h. WindowsPreferences.c already includes it.

Best regards
Christoph

From: Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
Sent: Donnerstag, 5. Dezember 2019 12:00
To: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>; Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>>
Cc: core-libs-dev at openjdk.java.net<mailto:core-libs-dev at openjdk.java.net>; security-dev <security-dev at openjdk.java.net<mailto:security-dev at openjdk.java.net>>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Thomas and Christoph,

thanks for the reviews.

Other code in java.security.jgss also uses these #defined checks:
src/java.security.jgss/share/native/libj2gss/gssapi.h:#if defined (_WIN32) && defined (_MSC_VER)

I’d like to have it consistent with that.

@Christoph: I think having ATTRIBUTE_ALIGNED(x) would be nice. It could get defined easily for Visual Studio and GCC, but some other compilers may be more difficult. Note that this macro is only defined for a selected set of compilers in hotspot. If we wanted to add it, where should we define it?

Windows 32 bit seems to be the only platform which is affected by the problem that jlongs on stack are not 8 byte aligned.
(AFAIK, GCC uses -malign-double by default on 32 bit which should do the job for jlongs, too:
https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/i386-and-x86_002d64-Options.html)

Best regards,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Sent: Mittwoch, 4. Dezember 2019 17:56
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>; security-dev <security-dev at openjdk.java.net<mailto:security-dev at openjdk.java.net>>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>
Subject: Re: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

this makes sense. This is the right way to force alignment. I do not like the platform code in the shared file but do not think this is a big deal.

+#if defined (_WIN32) && defined (_MSC_VER)

Why do you think we need _MSC_VER too? Is OpenJDK on Windows even buildable with anything other than VC++?

Cheers, Thomas

On Mon, Dec 2, 2019 at 4:14 PM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi,

I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u backport):
https://bugs.openjdk.java.net/browse/JDK-8220348

Some jdk native methods use jni_SetLongArrayRegion with a stack allocated buffer.
jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires jlongs to be 8 byte aligned (asserted).
However, Windows 32 bit only uses 4 byte alignment for jlong arrays by default.
I found such issues in the following files:
src/java.prefs/windows/native/libprefs/WindowsPreferences.c
src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
I suggest to use __declspec(align(8)) there.

Webrev:
http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.00/
Please review.

I think using 8 byte alignment is not a disadvantage for 64 bit.

I guess there are still people interested in this platform with jdk14. Otherwise I could contribute it as 11u only fix.

Is there a better way to force 8 byte alignment for jlongs or jlong arrays on stack?
Best regards,
Martin