RFR: 8265292: [macos_aarch64] java/foreign/TestDowncall.java crashes with SIGBUS

[Andrew Haley]

On Sat, 8 May 2021 09:16:11 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> Please review a fix for the intermittent crash. It is caused by a mistake in the ProgrammableInvoker::invoke_native, the wrong order of W^X and JavaThread state transition. We need WXExec since we are about to call a generated stub. But we need to switch to WXExec only after JavaThread state change. The thread state change may trigger a safepoint, that would need to do bookkeeping in the codecache (MarkActivationClosure::do_code_blob from the bug). So the fix is to change JavaThread state first, then change WX.
>> 
>> The fix was verified with the help of https://bugs.openjdk.java.net/browse/JDK-8266742. The new check catches all test failures reported by 8265292, 8265183, 8265182. I've verified tests pass after the fix with that new check enabled.
>
> src/hotspot/share/prims/universalNativeInvoker.cpp line 35:
> 
>> 33: void ProgrammableInvoker::invoke_native(Stub stub, address buff, JavaThread* thread) {
>> 34:   ThreadToNativeFromVM ttnfvm(thread);
>> 35:   MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXExec, thread));
> 
> I think we need a comment here, not just in the bug report. if this confused you, it'll surely be enough to confuse a maintainer. Some think like this, perhaps?
> 
> `"We need WXExec because we are about to call a generated stub. But we need to switch to WXExec only after JavaThread state change. The thread state change may trigger a safepoint, that would need to do bookkeeping in the codecache. See JDK-8265292."`
> 
> I'm also wondering if it would be better to enable writes in the methods that actually write to the nmethod, WDYT?

And incidentally, this seems to be rather error prone. Wouldn't it be simpler to check W^X status in all of the nmethod accessors, and change it when actually required, rather than hoping that it's already in the correct state?

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

PR: https://git.openjdk.java.net/jdk/pull/3921