JIT and Debugger/DebugTools

Hi,

the only regression/issue I have with the JIT is inside the DebugTools
and e.g. the DebuggerTest>>#testCurrentLine. It happens with aContext ip
pointing to an ip that is way beyond the bytecode of the block.

An instance of BlockContext
  parent: [] in Process>>onBlock:at:suspend: (Process.st:412)
  nativeIP: 69535906
  ip: 48
  sp: -1
  receiver: DebuggerTest>>#testCurrentLine
  method: [] in DebuggerTest>>testCurrentLine
  outerContext: DebuggerTest>>testCurrentLine (debugtests.st:91)
  args:
  temps:
  optimized temps:
  stack:

...

  byte codes: [
    [1] source code line number 7
    [3] push 5
    [5] store into Temporary[1] from outer context #1
    [7] source code line number 8
        pop stack top
    [9] push 6
   [11] store into Temporary[2] from outer context #1
   [13] source code line number 9
        pop stack top
   [15] push 7
   [17] store into Temporary[3] from outer context #1
        return


...


interestingly the 'ip = 48' is the same as the outercontext. So the
minimal re-producer with the jit is this:

([3+3] newProcess singleStep; suspend; suspendedContext) inspect.

it already steps into the block but somehow does not set the ip
correctly. any ideas?

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sun, Dec 22, 2013 at 02:34:56PM +0100, Holger Hans Peter Freyther wrote:
> Hi,

> the only regression/issue I have with the JIT is inside the DebugTools
> and e.g. the DebuggerTest>>#testCurrentLine. It happens with aContext ip
> pointing to an ip that is way beyond the bytecode of the block.

I looked a bit more, tried to remember how the jit and process changes
work and I do hit:

        if UNCOMMON (!IS_NIL (switch_to_process))
          {
            change_process_context (switch_to_process);
            if UNCOMMON (single_step_semaphore)
              {
                printf("SINGLE STEP...%p %d %d %d\n", thisContext,
                        TO_INT(thisContext->native_ip), ip,
                        TO_INT(thisContext->ipOffset));
                _gst_async_signal (single_step_semaphore);

I stepped through the emitted code for the interrupt check and I do
see that _gst_ip is not 48 at that time and the thisContext->ipOffset
is != FROM_INT(48).


> An instance of BlockContext
>   parent: [] in Process>>onBlock:at:suspend: (Process.st:412)
>   nativeIP: 69535906
>   ip: 48

I am puzzled of where it is set. So far I will have a look at the
interrupt checks and see if _gst_this_context_oop is set correctly.


anyone else got an idea of what I should look at/verify?

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sun, Dec 22, 2013 at 10:19:08PM +0100, Holger Hans Peter Freyther wrote:
>
> anyone else got an idea of what I should look at/verify?

thisContext is actually not yet the current context. I am not sure how
this happens and which context/process I actually look at (being able to
send >>#inspect would be nice). But it looks like whenever the interrupt
check triggers we will also need to export the _gst_ip.


#0  empty_context_stack () at interp.c:758
#1  0xb7f6728c in change_process_context (newProcess=0x40443c08) at interp.c:1315
#2  0xb7f73298 in _gst_interpret (processOOP=processOOP@entry=0x40443c08)
    at interp-jit.inl:428
#3  0xb7f744f0 in _gst_nvmsg_send (receiver=receiver@entry=0x4043d800,
    sendSelector=sendSelector@entry=0x40443b58, args=args@entry=0x0,
    sendArgs=sendArgs@entry=0) at interp.c:2326

We could also move the new IP into the signature of emit_interrupt_check
so we have the load+store only in the failure case. Do you have an idea
what would be right ip for a checked primitive? 0? 1?


diff --git a/libgst/xlat.c b/libgst/xlat.c
index 3f4a555..3122f97 100644
--- a/libgst/xlat.c
+++ b/libgst/xlat.c
@@ -2975,7 +2975,10 @@ emit_primitive (int primitive, int numArgs)
   if (attr & (PRIM_SUCCEED | PRIM_RELOAD_IP))
     {
       if (attr & PRIM_CHECK_INTERRUPT)
-       emit_interrupt_check (JIT_V2);
+        {
+         #warning "TODO.. write out the current IP"
+         emit_interrupt_check (JIT_V2);
+        }
 
       jit_jmpr (JIT_V2);
     }
@@ -3218,6 +3221,8 @@ emit_method_prolog (OOP methodOOP,
   emit_context_setup (header.numArgs, header.numTemps);
 
   define_ip_map_entry (0);
+  jit_movi_ul (JIT_R0, 0);
+  jit_sti_ul (&ip, JIT_R0);
   emit_interrupt_check (JIT_NOREG);
 
   /* For simplicity, we emit user-defined methods by creating a code_tree
@@ -3326,6 +3331,8 @@ emit_block_prolog (OOP blockOOP,
   emit_context_setup (header.numArgs, header.numTemps);
 
   define_ip_map_entry (0);
+  jit_movi_ul (JIT_R0, 0);
+  jit_sti_ul (&ip, JIT_R0);
   emit_interrupt_check (JIT_NOREG);
 
   return (false);


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

This has been found with the DebugTools tests and the most simple
re-producer is:

  ([3+3] newProcess singleStep; suspend; suspendedContext) inspect.

The check for _gst_except_flag is triggerred in the method prologue
and the JIT returns to the interpreter loop. When the process is
switched an old _gst_ip was written to context->ipOffset. Modify the
emit_interrupt_check to take an optional ipOffset parameter that will
be stored when the interrupt check triggers. For primitives it is
not clear which ipOffset should be saved.

2013-12-23  Holger Hans Peter Freyther  <[hidden email]>

        * xlat.c: Add ipOffset parameter to emit_interrupt_check.
---
 libgst/ChangeLog |  4 ++++
 libgst/xlat.c    | 24 +++++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/libgst/ChangeLog b/libgst/ChangeLog
index 6e48372..c7bfdec 100644
--- a/libgst/ChangeLog
+++ b/libgst/ChangeLog
@@ -1,3 +1,7 @@
+2013-12-23  Holger Hans Peter Freyther  <[hidden email]>
+
+ * xlat.c: Add ipOffset parameter to emit_interrupt_check.
+
 2013-12-22  Holger Hans Peter Freyther  <[hidden email]>
 
  * comp.c: Only flush the cache once when adding a new
diff --git a/libgst/xlat.c b/libgst/xlat.c
index 3f4a555..fbae8f7 100644
--- a/libgst/xlat.c
+++ b/libgst/xlat.c
@@ -332,7 +332,7 @@ static inline mst_Boolean emit_block_prolog (OOP blockOOP, gst_compiled_block bl
 static inline mst_Boolean emit_inlined_primitive (int primitive, int numArgs, int attr);
 static inline mst_Boolean emit_primitive (int primitive, int numArgs);
 
-static inline void emit_interrupt_check (int restartReg);
+static inline void emit_interrupt_check (int restartReg, int ipOffset);
 static inline void generate_run_time_code (void);
 static inline void translate_method (OOP methodOOP, OOP receiverClass, int size);
 static void emit_basic_size_in_r0 (OOP classOOP, mst_Boolean tagged, int objectReg);
@@ -2498,7 +2498,7 @@ emit_deferred_sends (deferred_send *ds)
 }
 
 void
-emit_interrupt_check (int restartReg)
+emit_interrupt_check (int restartReg, int ipOffset)
 {
   jit_insn *jmp, *begin;
 
@@ -2507,9 +2507,17 @@ emit_interrupt_check (int restartReg)
 
   jit_ldi_i (JIT_R2, &_gst_except_flag);
   jmp = jit_beqi_i (jit_forward (), JIT_R2, 0);
+
+  /* Save the global ip pointer */
+  if (ipOffset != -1)
+    {
+      jit_movi_ul (JIT_R2, ipOffset);
+      jit_sti_ul (&ip, JIT_R2);
+    }
+
+  /* Where to restart?*/
   if (restartReg == JIT_NOREG)
     jit_movi_p (JIT_RET, begin);
-
   else
     jit_movr_p (JIT_RET, restartReg);
 
@@ -2975,7 +2983,7 @@ emit_primitive (int primitive, int numArgs)
   if (attr & (PRIM_SUCCEED | PRIM_RELOAD_IP))
     {
       if (attr & PRIM_CHECK_INTERRUPT)
- emit_interrupt_check (JIT_V2);
+ emit_interrupt_check (JIT_V2, -1);
 
       jit_jmpr (JIT_V2);
     }
@@ -3218,7 +3226,7 @@ emit_method_prolog (OOP methodOOP,
   emit_context_setup (header.numArgs, header.numTemps);
 
   define_ip_map_entry (0);
-  emit_interrupt_check (JIT_NOREG);
+  emit_interrupt_check (JIT_NOREG, 0);
 
   /* For simplicity, we emit user-defined methods by creating a code_tree
      for the acrual send of #valueWithReceiver:withArguments: that they do.
@@ -3326,7 +3334,7 @@ emit_block_prolog (OOP blockOOP,
   emit_context_setup (header.numArgs, header.numTemps);
 
   define_ip_map_entry (0);
-  emit_interrupt_check (JIT_NOREG);
+  emit_interrupt_check (JIT_NOREG, 0);
 
   return (false);
 }
@@ -3675,9 +3683,7 @@ translate_method (OOP methodOOP, OOP receiverClass, int size)
   if (!lbl_define (*this_label))
     {
       define_ip_map_entry (bp - bc);
-      jit_movi_ul (JIT_V0, bp - bc);
-      jit_sti_ul (&ip, JIT_V0);
-      emit_interrupt_check (JIT_NOREG);
+      emit_interrupt_check (JIT_NOREG, bp - bc);
     }
  }
 
--
1.8.5.1


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

For single stepping through a method we should continue _after_ the
interrupt check or otherwise the interrupt check will _always_
trigger. This makes the DebugTools continue a bit.

2013-12-23  Holger Hans Peter Freyther  <[hidden email]>

        * xlat.c: Change emitted code in emit_interrupt_check.
---
 libgst/ChangeLog | 4 ++++
 libgst/xlat.c    | 7 ++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/libgst/ChangeLog b/libgst/ChangeLog
index c7bfdec..c4dc93c 100644
--- a/libgst/ChangeLog
+++ b/libgst/ChangeLog
@@ -1,5 +1,9 @@
 2013-12-23  Holger Hans Peter Freyther  <[hidden email]>
 
+ * xlat.c: Change emitted code in emit_interrupt_check.
+
+2013-12-23  Holger Hans Peter Freyther  <[hidden email]>
+
  * xlat.c: Add ipOffset parameter to emit_interrupt_check.
 
 2013-12-22  Holger Hans Peter Freyther  <[hidden email]>
diff --git a/libgst/xlat.c b/libgst/xlat.c
index fbae8f7..9f45b33 100644
--- a/libgst/xlat.c
+++ b/libgst/xlat.c
@@ -2500,10 +2500,9 @@ emit_deferred_sends (deferred_send *ds)
 void
 emit_interrupt_check (int restartReg, int ipOffset)
 {
-  jit_insn *jmp, *begin;
+  jit_insn *jmp, *restart = NULL;
 
   jit_align (2);
-  begin = jit_get_label ();
 
   jit_ldi_i (JIT_R2, &_gst_except_flag);
   jmp = jit_beqi_i (jit_forward (), JIT_R2, 0);
@@ -2517,11 +2516,13 @@ emit_interrupt_check (int restartReg, int ipOffset)
 
   /* Where to restart?*/
   if (restartReg == JIT_NOREG)
-    jit_movi_p (JIT_RET, begin);
+    restart = jit_movi_p (JIT_RET, jit_forward());
   else
     jit_movr_p (JIT_RET, restartReg);
 
   jit_ret ();
+  if (restart)
+    jit_patch_movi (restart, jit_get_label());
   jit_patch (jmp);
 }
 
--
1.8.5.1


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Dec 23, 2013 at 04:15:02PM +0100, Holger Hans Peter Freyther wrote:
> For single stepping through a method we should continue _after_ the
> interrupt check or otherwise the interrupt check will _always_
> trigger. This makes the DebugTools continue a bit.

After this single step is working a bit better but there is still another
interrupt check issue.

 st> a := 0. p := [a := 3] newProcess

The code does not get across Dictionary>>findIndex: (Dictionary.st:588).

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Dec 23, 2013 at 07:01:16PM +0100, Holger Hans Peter Freyther wrote:

>  st> a := 0. p := [a := 3] newProcess
>
> The code does not get across Dictionary>>findIndex: (Dictionary.st:588).


translate_method()
...
            if (!lbl_define (*this_label))
            {
              define_ip_map_entry (bp - bc);
              emit_interrupt_check (JIT_NOREG, bp - bc);
            }


Dictionary>>#findIndex:
        [((element := self primAt: index) isNil or: [element key = anObject])
            ifTrue: [^index].
        index == size ifTrue: [index := 1] ifFalse: [index := index + 1]]
                repeat

so it does appear to loop over this statement. index and sp never increase:

An instance of MethodContext
  parent: Dictionary(HashedCollection)>>findIndexOrNil: (HashedColl.st:359)
  nativeIP: 0
  ip: 22
  sp: 3
  receiver: Dictionary (
        #p->Process(nil at userSchedulingPriority, ready to run)
        #a->nil
)
  method: Dictionary>>findIndex:
  flags: 0
  args:
    anObject -> #a
  temps:
    index -> 11
    size -> 16
    element -> #a->3
  optimized temps:
  stack:



So I think we might need to export the SP when moving out?

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Dec 23, 2013 at 10:32:41PM +0100, Holger Hans Peter Freyther wrote:

I have a train trip ahead and poked it a bit more. Not inlining the primAt:
primitive allows me to step through the code.

My current guess is that by not inlining.. a send is generated which leads
to the temporaries being stored on the stack. I don't understand the stack
handling in the xlat yet so it might take a while. :}

holger


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Dec 23, 2013 at 10:32:41PM +0100, Holger Hans Peter Freyther wrote:

Good Evening,

>
> An instance of MethodContext
>   parent: Dictionary(HashedCollection)>>findIndexOrNil: (HashedColl.st:359)
>   nativeIP: 0


^^^^ this is the hint. the returned native_ip was not satisfying
the IS_INT test. This means I had to move the jit_align(2) _after_
the ip storing. I think this code can be quite fragile when porting
to other architectures?!


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 23/12/2013 16:15, Holger Hans Peter Freyther ha scritto:
I think primitives should only check the _gst_except_flag if the
primitive fails.  Then you know the ipOffset is zero.

Paolo


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 03/01/2014 20:15, Holger Hans Peter Freyther ha scritto:
More precisely, just before the jit_get_label().

> I think this code can be quite fragile when porting
> to other architectures?!

Most other architectures (all other architectures supported by
lightning, at least) do not have instructions that aren't naturally
aligned.  That is, 4-byte instructions are 4-aligned.

It can break only on x86, as you experienced, but jit_align() takes care
of it.

I guess Thumb would be a problem, since the length of the instruction is
encoded in the bottom bit of PC: bit 0 = 0 means the 4-byte ARM
instruction set, bit 0 = 1 means the 2-byte Thumb instruction set.
GET_CONTEXT_IP and GET_NATIVE_IP would not work in that case.

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk