[pypy-commit] pypy default: Improve the fix for shadowstack. The previous fix was sometimes

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: 
Changeset: r73021:632c832bd32f
Date: 2014-08-24 09:56 +0200
http://bitbucket.org/pypy/pypy/changeset/632c832bd32f/

Log:	Improve the fix for shadowstack. The previous fix was sometimes
	overwriting the return value from the call_release_gil.

diff --git a/rpython/jit/backend/x86/callbuilder.py b/rpython/jit/backend/x86/callbuilder.py
--- a/rpython/jit/backend/x86/callbuilder.py
+++ b/rpython/jit/backend/x86/callbuilder.py
@@ -1,7 +1,6 @@
 import sys
 from rpython.rlib.clibffi import FFI_DEFAULT_ABI
 from rpython.rlib.objectmodel import we_are_translated
-from rpython.rtyper.lltypesystem import lltype, rffi
 from rpython.jit.metainterp.history import INT, FLOAT
 from rpython.jit.backend.x86.arch import (WORD, IS_X86_64, IS_X86_32,
                                           PASS_ON_MY_FRAME, FRAME_FIXED_SIZE)
@@ -22,8 +21,6 @@
 def align_stack_words(words):
     return (words + CALL_ALIGN - 1) & ~(CALL_ALIGN-1)
 
-NO_ARG_FUNC_PTR = lltype.Ptr(lltype.FuncType([], lltype.Void))
-
 
 class CallBuilderX86(AbstractCallBuilder):
 
@@ -94,30 +91,9 @@
         gcrootmap = self.asm.cpu.gc_ll_descr.gcrootmap
         if gcrootmap:
             if gcrootmap.is_shadow_stack and self.is_call_release_gil:
-                from rpython.jit.backend.x86.assembler import heap
-                from rpython.jit.backend.x86 import rx86
-                from rpython.rtyper.lltypesystem.lloperation import llop
-                #
-                # When doing a call_release_gil with shadowstack, there
-                # is the risk that the 'rpy_fastgil' was free but the
-                # current shadowstack can be the one of a different
-                # thread.  So here we check if the shadowstack pointer
-                # is still the same as before we released the GIL (saved
-                # in 'ebx'), and if not, we call 'thread_run'.
-                rst = gcrootmap.get_root_stack_top_addr()
-                mc = self.mc
-                mc.CMP(ebx, heap(rst))
-                mc.J_il8(rx86.Conditions['E'], 0)
-                je_location = mc.get_relative_pos()
-                # call 'thread_run'
-                t_run = llop.gc_thread_run_ptr(NO_ARG_FUNC_PTR)
-                mc.CALL(imm(rffi.cast(lltype.Signed, t_run)))
-                # patch the JE above
-                offset = mc.get_relative_pos() - je_location
-                assert 0 < offset <= 127
-                mc.overwrite(je_location-1, chr(offset))
+                # in this mode, 'ebx' happens to contain the shadowstack
+                # top at this point, so reuse it instead of loading it again
                 ssreg = ebx
-        #
         self.asm._reload_frame_if_necessary(self.mc, shadowstack_reg=ssreg)
         if self.change_extra_stack_depth:
             self.asm.set_extra_stack_depth(self.mc, 0)
@@ -206,8 +182,35 @@
             mc.MOV_ri(X86_64_SCRATCH_REG.value, fastgil)
             mc.XCHG_rm(old_value.value, (X86_64_SCRATCH_REG.value, 0))
         mc.CMP(old_value, css_value)
-        mc.J_il8(rx86.Conditions['E'], 0)
-        je_location = mc.get_relative_pos()
+        #
+        gcrootmap = self.asm.cpu.gc_ll_descr.gcrootmap
+        if bool(gcrootmap) and gcrootmap.is_shadow_stack:
+            from rpython.jit.backend.x86.assembler import heap
+            #
+            # When doing a call_release_gil with shadowstack, there
+            # is the risk that the 'rpy_fastgil' was free but the
+            # current shadowstack can be the one of a different
+            # thread.  So here we check if the shadowstack pointer
+            # is still the same as before we released the GIL (saved
+            # in 'ebx'), and if not, we fall back to 'reacqgil_addr'.
+            mc.J_il8(rx86.Conditions['NE'], 0)
+            jne_location = mc.get_relative_pos()
+            # here, ecx is zero (so rpy_fastgil was not acquired)
+            rst = gcrootmap.get_root_stack_top_addr()
+            mc = self.mc
+            mc.CMP(ebx, heap(rst))
+            mc.J_il8(rx86.Conditions['E'], 0)
+            je_location = mc.get_relative_pos()
+            # revert the rpy_fastgil acquired above, so that the
+            # general 'reacqgil_addr' below can acquire it again...
+            mc.MOV(heap(fastgil), ecx)
+            # patch the JNE above
+            offset = mc.get_relative_pos() - jne_location
+            assert 0 < offset <= 127
+            mc.overwrite(jne_location-1, chr(offset))
+        else:
+            mc.J_il8(rx86.Conditions['E'], 0)
+            je_location = mc.get_relative_pos()
         #
         # Yes, we need to call the reacqgil() function
         self.save_result_value_reacq()
diff --git a/rpython/memory/gctransform/framework.py b/rpython/memory/gctransform/framework.py
--- a/rpython/memory/gctransform/framework.py
+++ b/rpython/memory/gctransform/framework.py
@@ -977,12 +977,6 @@
             hop.genop("direct_call", [self.root_walker.thread_run_ptr])
             self.pop_roots(hop, livevars)
 
-    def gct_gc_thread_run_ptr(self, hop):
-        assert self.translator.config.translation.thread
-        assert hasattr(self.root_walker, 'thread_run_ptr')
-        hop.genop("same_as", [self.root_walker.thread_run_ptr],
-                  resultvar=hop.spaceop.result)
-
     def gct_gc_thread_start(self, hop):
         assert self.translator.config.translation.thread
         if hasattr(self.root_walker, 'thread_start_ptr'):
diff --git a/rpython/rtyper/lltypesystem/lloperation.py b/rpython/rtyper/lltypesystem/lloperation.py
--- a/rpython/rtyper/lltypesystem/lloperation.py
+++ b/rpython/rtyper/lltypesystem/lloperation.py
@@ -473,7 +473,6 @@
     'gc_set_max_heap_size': LLOp(),
     'gc_can_move'         : LLOp(sideeffects=False),
     'gc_thread_run'       : LLOp(),
-    'gc_thread_run_ptr'   : LLOp(sideeffects=False),
     'gc_thread_start'     : LLOp(),
     'gc_thread_die'       : LLOp(),
     'gc_thread_before_fork':LLOp(),   # returns an opaque address