games/gzdoom disable JIT on amd64

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

games/gzdoom disable JIT on amd64

Solene Rapenne
brynet@ spotted that gzdoom wasn't W^X compliant due to a JIT engine
getting enabled automatically on amd64.

This patch makes vm_jit knob at false by default instead of being true.
I did not see performance impact on my i7 when it's disabled, but this
could be interesting if someone could test it.

Without the patch, it's possible to disable JIT with the parameter
'+vm_jit false'. While now we can still enable it with '+vm_jit true'
(with W^X violation).

I don't know if we should let it as this and add WX_NEEDED or patch it
out so it doesn't WX by default?


Index: patches/patch-src_scripting_vm_vmframe_cpp
===================================================================
RCS file: patches/patch-src_scripting_vm_vmframe_cpp
diff -N patches/patch-src_scripting_vm_vmframe_cpp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_scripting_vm_vmframe_cpp 12 Feb 2019 06:04:57 -0000
@@ -0,0 +1,16 @@
+$OpenBSD$
+
+disable JIT so it's W^X compatible
+
+Index: src/scripting/vm/vmframe.cpp
+--- src/scripting/vm/vmframe.cpp.orig
++++ src/scripting/vm/vmframe.cpp
+@@ -49,7 +49,7 @@
+ #endif
+
+ #ifdef ARCH_X64
+-CUSTOM_CVAR(Bool, vm_jit, true, CVAR_NOINITCALL)
++CUSTOM_CVAR(Bool, vm_jit, false, CVAR_NOINITCALL)
+ {
+ Printf("You must restart " GAMENAME " for this change to take effect.\n");
+ Printf("This cvar is currently not saved. You must specify it on the command line.");

Reply | Threaded
Open this post in threaded view
|

Re: games/gzdoom disable JIT on amd64

Jeremie Courreges-Anglas-2
On Tue, Feb 12 2019, Solene Rapenne <[hidden email]> wrote:

> brynet@ spotted that gzdoom wasn't W^X compliant due to a JIT engine
> getting enabled automatically on amd64.
>
> This patch makes vm_jit knob at false by default instead of being true.
> I did not see performance impact on my i7 when it's disabled, but this
> could be interesting if someone could test it.
>
> Without the patch, it's possible to disable JIT with the parameter
> '+vm_jit false'. While now we can still enable it with '+vm_jit true'
> (with W^X violation).
>
> I don't know if we should let it as this and add WX_NEEDED or patch it
> out so it doesn't WX by default?

Note that there's a third approach for such ports: try to make the JIT
W^X compliant.  Here, it doesn't seem trivial (look for kVMExecutable).

So your proposal makes sense to me, ok jca@ with a REVISION bump.  If
runtime still works, ofc, but I'll leave tests to actual users. :)

> Index: patches/patch-src_scripting_vm_vmframe_cpp
> ===================================================================
> RCS file: patches/patch-src_scripting_vm_vmframe_cpp
> diff -N patches/patch-src_scripting_vm_vmframe_cpp
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_scripting_vm_vmframe_cpp 12 Feb 2019 06:04:57 -0000
> @@ -0,0 +1,16 @@
> +$OpenBSD$
> +
> +disable JIT so it's W^X compatible
> +
> +Index: src/scripting/vm/vmframe.cpp
> +--- src/scripting/vm/vmframe.cpp.orig
> ++++ src/scripting/vm/vmframe.cpp
> +@@ -49,7 +49,7 @@
> + #endif
> +
> + #ifdef ARCH_X64
> +-CUSTOM_CVAR(Bool, vm_jit, true, CVAR_NOINITCALL)
> ++CUSTOM_CVAR(Bool, vm_jit, false, CVAR_NOINITCALL)
> + {
> + Printf("You must restart " GAMENAME " for this change to take effect.\n");
> + Printf("This cvar is currently not saved. You must specify it on the command line.");
>

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: games/gzdoom disable JIT on amd64

Timo Myyrä-6
Jeremie Courreges-Anglas <[hidden email]> writes:

> On Tue, Feb 12 2019, Solene Rapenne <[hidden email]> wrote:
>
>> brynet@ spotted that gzdoom wasn't W^X compliant due to a JIT engine
>> getting enabled automatically on amd64.
>>
>> This patch makes vm_jit knob at false by default instead of being true.
>> I did not see performance impact on my i7 when it's disabled, but this
>> could be interesting if someone could test it.
>>
>> Without the patch, it's possible to disable JIT with the parameter
>> '+vm_jit false'. While now we can still enable it with '+vm_jit true'
>> (with W^X violation).
>>
>> I don't know if we should let it as this and add WX_NEEDED or patch it
>> out so it doesn't WX by default?
>
> Note that there's a third approach for such ports: try to make the JIT
> W^X compliant.  Here, it doesn't seem trivial (look for kVMExecutable).
>
> So your proposal makes sense to me, ok jca@ with a REVISION bump.  If
> runtime still works, ofc, but I'll leave tests to actual users. :)

ok by me.

Timo

>
>> Index: patches/patch-src_scripting_vm_vmframe_cpp
>> ===================================================================
>> RCS file: patches/patch-src_scripting_vm_vmframe_cpp
>> diff -N patches/patch-src_scripting_vm_vmframe_cpp
>> --- /dev/null 1 Jan 1970 00:00:00 -0000
>> +++ patches/patch-src_scripting_vm_vmframe_cpp 12 Feb 2019 06:04:57 -0000
>> @@ -0,0 +1,16 @@
>> +$OpenBSD$
>> +
>> +disable JIT so it's W^X compatible
>> +
>> +Index: src/scripting/vm/vmframe.cpp
>> +--- src/scripting/vm/vmframe.cpp.orig
>> ++++ src/scripting/vm/vmframe.cpp
>> +@@ -49,7 +49,7 @@
>> + #endif
>> +
>> + #ifdef ARCH_X64
>> +-CUSTOM_CVAR(Bool, vm_jit, true, CVAR_NOINITCALL)
>> ++CUSTOM_CVAR(Bool, vm_jit, false, CVAR_NOINITCALL)
>> + {
>> + Printf("You must restart " GAMENAME " for this change to take effect.\n");
>> + Printf("This cvar is currently not saved. You must specify it on the command line.");
>>