Skip to content

8344802: Crash in StubRoutines::verify_mxcsr with -XX:+EnableX86ECoreOpts and -Xcheck:jni #22673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vpaprotsk wants to merge 8 commits into from
Closed

8344802: Crash in StubRoutines::verify_mxcsr with -XX:+EnableX86ECoreOpts and -Xcheck:jni #22673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aeced07
fix mxcsr JNI checks
vpaprotsk Dec 10, 2024
c106e35
whitespace
vpaprotsk Dec 11, 2024
6768b9d
cleanup
vpaprotsk Jan 23, 2025
2b15f99
whitespace
vpaprotsk Jan 23, 2025
b1a712b
fix crash in fxrstor
vpaprotsk Feb 3, 2025
2e372f2
typo
vpaprotsk Feb 3, 2025
b23764a
comments from Sandhya
vpaprotsk Feb 11, 2025
cbd3812
Update src/hotspot/cpu/x86/macroAssembler_x86.cpp
vpaprotsk Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/hotspot/cpu/x86/macroAssembler_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,9 +779,17 @@ void MacroAssembler::warn(const char* msg) {
andq(rsp, -16); // align stack as required by push_CPU_state and call
push_CPU_state(); // keeps alignment at 16 bytes

#ifdef _WIN64
// Windows always allocates space for its register args
subq(rsp, frame::arg_reg_save_area_bytes);
#endif
lea(c_rarg0, ExternalAddress((address) msg));
call(RuntimeAddress(CAST_FROM_FN_PTR(address, warning)));

#ifdef _WIN64
// restore stack pointer
addq(rsp, frame::arg_reg_save_area_bytes);
#endif
pop_CPU_state();
mov(rsp, rbp);
pop(rbp);
Expand Down Expand Up @@ -2376,6 +2384,22 @@ void MacroAssembler::jump_cc(Condition cc, AddressLiteral dst, Register rscratch
}
}

void MacroAssembler::cmp32_mxcsr_std(Address mxcsr_save, Register tmp, Register rscratch) {
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
assert(rscratch != noreg || always_reachable(mxcsr_std), "missing");

stmxcsr(mxcsr_save);
movl(tmp, mxcsr_save);
if (EnableX86ECoreOpts) {
// The mxcsr_std has status bits set for performance on ECore
orl(tmp, 0x003f);
} else {
// Mask out status bits (only check control and mask bits)
andl(tmp, 0xFFC0);
}
cmp32(tmp, mxcsr_std, rscratch);
}

void MacroAssembler::ldmxcsr(AddressLiteral src, Register rscratch) {
assert(rscratch != noreg || always_reachable(src), "missing");

Expand Down
1 change: 1 addition & 0 deletions src/hotspot/cpu/x86/macroAssembler_x86.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,7 @@ class MacroAssembler: public Assembler {
void fmul_s(AddressLiteral src) { Assembler::fmul_s(as_Address(src)); }
#endif // !_LP64

void cmp32_mxcsr_std(Address mxcsr_save, Register tmp, Register rscratch = noreg);
void ldmxcsr(Address src) { Assembler::ldmxcsr(src); }
void ldmxcsr(AddressLiteral src, Register rscratch = noreg);

Expand Down
14 changes: 3 additions & 11 deletions src/hotspot/cpu/x86/stubGenerator_x86_32.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@

#define BIND(label) bind(label); BLOCK_COMMENT(#label ":")

const int MXCSR_MASK = 0xFFC0; // Mask out any pending exceptions
const int FPU_CNTRL_WRD_MASK = 0xFFFF;

ATTRIBUTE_ALIGNED(16) static const uint32_t KEY_SHUFFLE_MASK[] = {
Expand Down Expand Up @@ -175,11 +174,7 @@ class StubGenerator: public StubCodeGenerator {
// save and initialize %mxcsr
if (sse_save) {
Label skip_ldmx;
__ stmxcsr(mxcsr_save);
__ movl(rax, mxcsr_save);
__ andl(rax, MXCSR_MASK); // Only check control and mask bits
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ cmp32(rax, mxcsr_std);
__ cmp32_mxcsr_std(mxcsr_save, rax);
__ jcc(Assembler::equal, skip_ldmx);
__ ldmxcsr(mxcsr_std);
__ bind(skip_ldmx);
Expand Down Expand Up @@ -462,17 +457,14 @@ class StubGenerator: public StubCodeGenerator {

if (CheckJNICalls && UseSSE > 0 ) {
Label ok_ret;
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ push(rax);
__ subptr(rsp, wordSize); // allocate a temp location
__ stmxcsr(mxcsr_save);
__ movl(rax, mxcsr_save);
__ andl(rax, MXCSR_MASK);
__ cmp32(rax, mxcsr_std);
__ cmp32_mxcsr_std(mxcsr_save, rax);
__ jcc(Assembler::equal, ok_ret);

__ warn("MXCSR changed by native JNI code.");

ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ ldmxcsr(mxcsr_std);

__ bind(ok_ret);
Expand Down
12 changes: 3 additions & 9 deletions src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,12 +248,9 @@ address StubGenerator::generate_call_stub(address& return_address) {
const Address mxcsr_save(rbp, mxcsr_off * wordSize);
{
Label skip_ldmx;
__ stmxcsr(mxcsr_save);
__ movl(rax, mxcsr_save);
__ andl(rax, 0xFFC0); // Mask out any pending exceptions (only check control and mask bits)
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ cmp32(rax, mxcsr_std, rscratch1);
__ cmp32_mxcsr_std(mxcsr_save, rax, rscratch1);
__ jcc(Assembler::equal, skip_ldmx);
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ ldmxcsr(mxcsr_std, rscratch1);
__ bind(skip_ldmx);
}
Expand Down Expand Up @@ -574,10 +571,7 @@ address StubGenerator::generate_verify_mxcsr() {
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ push(rax);
__ subptr(rsp, wordSize); // allocate a temp location
__ stmxcsr(mxcsr_save);
__ movl(rax, mxcsr_save);
__ andl(rax, 0xFFC0); // Mask out any pending exceptions (only check control and mask bits)
__ cmp32(rax, mxcsr_std, rscratch1);
__ cmp32_mxcsr_std(mxcsr_save, rax, rscratch1);
__ jcc(Assembler::equal, ok_ret);

__ warn("MXCSR changed by native JNI code, use -XX:+RestoreMXCSROnJNICall");
Expand Down
9 changes: 2 additions & 7 deletions src/hotspot/cpu/x86/upcallLinker_x86_64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ static int compute_reg_save_area_size(const ABIDescriptor& abi) {
return size;
}

constexpr int MXCSR_MASK = 0xFFC0; // Mask out any pending exceptions

static void preserve_callee_saved_registers(MacroAssembler* _masm, const ABIDescriptor& abi, int reg_save_area_offset) {
// 1. iterate all registers in the architecture
// - check if they are volatile or not for the given abi
Expand Down Expand Up @@ -115,12 +113,9 @@ static void preserve_callee_saved_registers(MacroAssembler* _masm, const ABIDesc
{
const Address mxcsr_save(rsp, offset);
Label skip_ldmx;
__ stmxcsr(mxcsr_save);
__ movl(rax, mxcsr_save);
__ andl(rax, MXCSR_MASK); // Only check control and mask bits
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ cmp32(rax, mxcsr_std, rscratch1);
__ cmp32_mxcsr_std(mxcsr_save, rax, rscratch1);
__ jcc(Assembler::equal, skip_ldmx);
ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std());
__ ldmxcsr(mxcsr_std, rscratch1);
__ bind(skip_ldmx);
}
Expand Down
33 changes: 1 addition & 32 deletions src/hotspot/os/windows/os_windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2570,38 +2570,6 @@ LONG Handle_IDiv_Exception(struct _EXCEPTION_POINTERS* exceptionInfo) {
return EXCEPTION_CONTINUE_EXECUTION;
}

#if defined(_M_AMD64)
//-----------------------------------------------------------------------------
static bool handle_FLT_exception(struct _EXCEPTION_POINTERS* exceptionInfo) {
// handle exception caused by native method modifying control word
DWORD exception_code = exceptionInfo->ExceptionRecord->ExceptionCode;

switch (exception_code) {
case EXCEPTION_FLT_DENORMAL_OPERAND:
case EXCEPTION_FLT_DIVIDE_BY_ZERO:
case EXCEPTION_FLT_INEXACT_RESULT:
case EXCEPTION_FLT_INVALID_OPERATION:
case EXCEPTION_FLT_OVERFLOW:
case EXCEPTION_FLT_STACK_CHECK:
case EXCEPTION_FLT_UNDERFLOW: {
PCONTEXT ctx = exceptionInfo->ContextRecord;
// On Windows, the mxcsr control bits are non-volatile across calls
// See also CR 6192333
//
jint MxCsr = INITIAL_MXCSR;
// we can't use StubRoutines::x86::addr_mxcsr_std()
// because in Win64 mxcsr is not saved there
if (MxCsr != ctx->MxCsr) {
ctx->MxCsr = MxCsr;
return true;
}
}
}

return false;
}
#endif

static inline void report_error(Thread* t, DWORD exception_code,
address addr, void* siginfo, void* context) {
VMError::report_and_die(t, exception_code, addr, siginfo, context);
Expand Down Expand Up @@ -2786,6 +2754,7 @@ LONG WINAPI topLevelExceptionFilter(struct _EXCEPTION_POINTERS* exceptionInfo) {
}

#if defined(_M_AMD64)
extern bool handle_FLT_exception(struct _EXCEPTION_POINTERS* exceptionInfo);
Copy link
Contributor

@TheShermanTanker TheShermanTanker Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange to declare inside another method like this, not a showstopper but it might be better to declare it where handle_FLT_exception used to be defined in this file

Copy link
Contributor Author

@vpaprotsk vpaprotsk Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.. I remember wondering where to put this. Meant to come back and find the right header.

I couldn't find the right header 'easily' and leaving it where the old function was looked even more out of place (i.e. "things like this belong in headers"?). So this was my alternative...

if ((in_java || in_native) && handle_FLT_exception(exceptionInfo)) {
return EXCEPTION_CONTINUE_EXECUTION;
}
Expand Down
37 changes: 37 additions & 0 deletions src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,43 @@ bool os::win32::register_code_area(char *low, char *high) {
return true;
}

#if defined(_M_AMD64)
//-----------------------------------------------------------------------------
bool handle_FLT_exception(struct _EXCEPTION_POINTERS* exceptionInfo) {
// handle exception caused by native method modifying control word
DWORD exception_code = exceptionInfo->ExceptionRecord->ExceptionCode;

switch (exception_code) {
case EXCEPTION_FLT_DENORMAL_OPERAND:
case EXCEPTION_FLT_DIVIDE_BY_ZERO:
case EXCEPTION_FLT_INEXACT_RESULT:
case EXCEPTION_FLT_INVALID_OPERATION:
case EXCEPTION_FLT_OVERFLOW:
case EXCEPTION_FLT_STACK_CHECK:
case EXCEPTION_FLT_UNDERFLOW: {
PCONTEXT ctx = exceptionInfo->ContextRecord;
// On Windows, the mxcsr control bits are non-volatile across calls
// See also CR 6192333
//
jint MxCsr = INITIAL_MXCSR; // set to 0x1f80` in winnt.h
if (EnableX86ECoreOpts) {
// On ECore restore with status bits enabled
MxCsr |= 0x3F;
}

// we can't use StubRoutines::x86::addr_mxcsr_std()
// because in Win64 mxcsr is not saved there
if (MxCsr != ctx->MxCsr) {
ctx->MxCsr = MxCsr;
return true;
}
}
}

return false;
}
#endif

#ifdef HAVE_PLATFORM_PRINT_NATIVE_STACK
/*
* Windows/x64 does not use stack frames the way expected by Java:
Expand Down
2 changes: 1 addition & 1 deletion test/jdk/java/lang/String/IndexOf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* @summary test String indexOf() intrinsic
* @requires vm.cpu.features ~= ".*avx2.*"
* @requires vm.compiler2.enabled
* @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions -XX:+EnableX86ECoreOpts -XX:-CheckJNICalls IndexOf
* @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions -XX:+EnableX86ECoreOpts IndexOf
*/

public class IndexOf {
Expand Down
2 changes: 1 addition & 1 deletion test/jdk/java/lang/StringBuffer/ECoreIndexOf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* @summary Test indexOf and lastIndexOf
* @requires vm.cpu.features ~= ".*avx2.*"
* @requires vm.compiler2.enabled
* @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+EnableX86ECoreOpts -XX:-CheckJNICalls -XX:UseAVX=2 -Xbatch -XX:-TieredCompilation -XX:CompileCommand=dontinline,ECoreIndexOf.indexOfKernel ECoreIndexOf
* @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+EnableX86ECoreOpts -XX:UseAVX=2 -Xbatch -XX:-TieredCompilation -XX:CompileCommand=dontinline,ECoreIndexOf.indexOfKernel ECoreIndexOf
* @key randomness
*/

Expand Down