RFR: 8344802: Crash in StubRoutines::verify_mxcsr with -XX:+EnableX86ECoreOpts and -Xcheck:jni [v5]

[Sandhya Viswanathan]

On Mon, 3 Feb 2025 21:43:56 GMT, Volodymyr Paprotski <vpaprotski at openjdk.org> wrote:

>> (Also see `8319429: Resetting MXCSR flags degrades ecore`)
>> 
>> This PR fixes two issues:
>> - the original issue is a crash caused by `__ warn` corrupting the stack on Windows only
>> - This issue also uncovered that -Xcheck:jni test cases were getting 65k lines of warning on HelloWorld (on both Linux _and_ windows):
>> 
>> OpenJDK 64-Bit Server VM warning: MXCSR changed by native JNI code, use -XX:+RestoreMXCSROnJNICall
>> 
>> 
>> First, the crash. Caused when FXRSTOR is attempting to write reserved bits into MXCSR. If those bits happen to be set, crash. (Hence the crash isn't deterministic. But frequent enough if `__ warn` is used). It is caused by the binding not reserving stack space for register parameters ()
>> ![image](https://github.com/user-attachments/assets/4ad63908-088b-4e9d-9e7d-a3509bee046a)
>> Prolog of the warn function then proceeds to store the for arg registers onto the stack, overriding the fxstore save area. (See https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170#calling-convention-defaults)
>> 
>> Fix uses `frame::arg_reg_save_area_bytes` to bump the stack pointer.
>> 
>> ---
>> 
>> I also kept the fix to `verify_mxcsr` since without it, `-Xcheck:jni` is practically unusable when `-XX:+EnableX86ECoreOpts` are set (65k+ lines of warnings)
>
> Volodymyr Paprotski has updated the pull request incrementally with one additional commit since the last revision:
> 
>   typo

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 2393:

> 2391:   stmxcsr(mxcsr_save);
> 2392:   movl(tmp, mxcsr_save);
> 2393:   // Mask out any pending exceptions (only check control and mask bits)

This comment should go on the else path and could be changed to "Mask out status bits (only check control and mask bits)"

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 2396:

> 2394:   if (EnableX86ECoreOpts) {
> 2395:     // On Ecore, status bits are set by default (for performance)
> 2396:     orl(tmp, 0x003f);  // On Ecore, exception bits are set by default

Duplication in comment.  Comment could be modified to reflect something like "The mxcsr_std has status bits set for performance on ECore"

src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp line 183:

> 181:     jint MxCsr = INITIAL_MXCSR; // set to 0x1f80` in winnt.h
> 182:     if (EnableX86ECoreOpts) {
> 183:       // On ECore, restore with signaling flags enabled

Change comment to // On ECore restore with status bits enabled.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22673#discussion_r1946911130
PR Review Comment: https://git.openjdk.org/jdk/pull/22673#discussion_r1946915061
PR Review Comment: https://git.openjdk.org/jdk/pull/22673#discussion_r1946934759