[PATCH] jit: Using BYTECODE_SIZE is not always the right thing

Use the difference between bp and IP0 to figure out how many bytes
were used for the bytecode of the send message. Add a small test
case and try to make it as stable as possible.

For message sends we remember the bytecode length as calculated
by the decoding of the bytecodes.
---
 libgst/xlat.c      | 24 +++++++-------
 tests/Makefile.am  |  3 +-
 tests/testsuite.at |  1 +
 tests/xlat.ok      | 41 ++++++++++++++++++++++++
 tests/xlat.st      | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 148 insertions(+), 12 deletions(-)
 create mode 100644 tests/xlat.ok
 create mode 100644 tests/xlat.st

diff --git a/libgst/xlat.c b/libgst/xlat.c
index 8a0dc93..67eff29 100644
--- a/libgst/xlat.c
+++ b/libgst/xlat.c
@@ -125,6 +125,7 @@ typedef struct code_tree
   PTR data;
   label *jumpDest;
   gst_uchar *bp;
+  unsigned char bc_len; /* only used for sends */
 } code_tree, *code_stack_element, **code_stack_pointer;
 
 /* This structure represents a message send.  A sequence of
@@ -318,7 +319,7 @@ static inline method_entry *finish_method_entry (void);
 static inline code_tree *push_tree_node (gst_uchar *bp, code_tree *firstChild, int operation, PTR data);
 static inline code_tree *push_tree_node_oop (gst_uchar *bp, code_tree *firstChild, int operation, OOP literal);
 static inline code_tree *pop_tree_node (code_tree *linkedChild);
-static inline code_tree *push_send_node (gst_uchar *bp, OOP selector, int numArgs, mst_Boolean super, int operation, int imm);
+static inline code_tree *push_send_node (gst_uchar *bp, unsigned char bc_len, OOP selector, int numArgs, mst_Boolean super, int operation, int imm);
 static inline void set_top_node_extra (int extra, int jumpOffset);
 static inline gst_uchar *decode_bytecode (gst_uchar *bp);
 
@@ -795,7 +796,7 @@ set_inline_cache (OOP selector, int numArgs, mst_Boolean super, int operation, i
 }
 
 code_tree *
-push_send_node (gst_uchar *bp, OOP selector, int numArgs, mst_Boolean super, int operation, int imm)
+push_send_node (gst_uchar *bp, unsigned char bc_len, OOP selector, int numArgs, mst_Boolean super, int operation, int imm)
 {
   code_tree *args, *node;
   int tot_args;
@@ -807,6 +808,7 @@ push_send_node (gst_uchar *bp, OOP selector, int numArgs, mst_Boolean super, int
     args = pop_tree_node (args);
 
   node = push_tree_node (bp, args, operation, (PTR) ic);
+  node->bc_len = bc_len;
   return (node);
 }
 
@@ -1419,7 +1421,7 @@ gen_send (code_tree *tree)
   else
     EXPORT_SP (JIT_V0);
 
-  jit_movi_ul (JIT_R0, tree->bp - bc + BYTECODE_SIZE);
+  jit_movi_ul (JIT_R0, tree->bp - bc + tree->bc_len);
   jit_ldxi_p (JIT_R1, JIT_V1, jit_field (inline_cache, cachedIP));
   jit_sti_ul (&ip, JIT_R0);
 
@@ -1427,7 +1429,7 @@ gen_send (code_tree *tree)
   jit_align (2);
 
   ic->native_ip = jit_get_label ();
-  define_ip_map_entry (tree->bp - bc + BYTECODE_SIZE);
+  define_ip_map_entry (tree->bp - bc + tree->bc_len);
 
   IMPORT_SP;
   CACHE_NOTHING;
@@ -3094,7 +3096,7 @@ emit_user_defined_method_call (OOP methodOOP, int numArgs,
   /* TODO: use instantiate_oop_with instead.  */
   push_tree_node_oop (bp, NULL, TREE_PUSH | TREE_LIT_VAR, arrayAssociation);
   push_tree_node_oop (bp, NULL, TREE_PUSH | TREE_LIT_CONST, FROM_INT (numArgs));
-  push_send_node (bp, _gst_intern_string ("new:"), 1, false,
+  push_send_node (bp, BYTECODE_SIZE, _gst_intern_string ("new:"), 1, false,
   TREE_SEND | TREE_NORMAL, NEW_COLON_SPECIAL);
 
   for (i = 0; i < numArgs; i++)
@@ -3105,7 +3107,7 @@ emit_user_defined_method_call (OOP methodOOP, int numArgs,
       (PTR) (uintptr_t) i);
     }
 
-  push_send_node (bp, _gst_value_with_rec_with_args_symbol, 2,
+  push_send_node (bp, BYTECODE_SIZE, _gst_value_with_rec_with_args_symbol, 2,
   false, TREE_SEND | TREE_NORMAL, 0);
 
   set_top_node_extra (TREE_EXTRA_RETURN, 0);
@@ -3366,7 +3368,7 @@ decode_bytecode (gst_uchar *bp)
  {
           push_tree_node_oop (IP0, NULL, TREE_PUSH | TREE_LIT_CONST,
                               literals[n]);
-          push_send_node (IP0, _gst_builtin_selectors[VALUE_SPECIAL].symbol,
+          push_send_node (IP0, bp - IP0, _gst_builtin_selectors[VALUE_SPECIAL].symbol,
   0, false, TREE_SEND, 0);
  }
     }
@@ -3427,7 +3429,7 @@ decode_bytecode (gst_uchar *bp)
     }
 
     SEND {
-      push_send_node (IP0, literals[n], num_args, super, TREE_SEND, 0);
+      push_send_node (IP0, bp - IP0, literals[n], num_args, super, TREE_SEND, 0);
     }
 
     POP_INTO_NEW_STACKTOP {
@@ -3479,16 +3481,16 @@ decode_bytecode (gst_uchar *bp)
     SEND_ARITH {
       int op = special_send_bytecodes[n];
       const struct builtin_selector *bs = &_gst_builtin_selectors[n];
-      push_send_node (IP0, bs->symbol, bs->numArgs, false, op, n);
+      push_send_node (IP0, bp - IP0, bs->symbol, bs->numArgs, false, op, n);
     }
     SEND_SPECIAL {
       int op = special_send_bytecodes[n + 16];
       const struct builtin_selector *bs = &_gst_builtin_selectors[n + 16];
-      push_send_node (IP0, bs->symbol, bs->numArgs, false, op, n + 16);
+      push_send_node (IP0, bp - IP0, bs->symbol, bs->numArgs, false, op, n + 16);
     }
     SEND_IMMEDIATE {
       const struct builtin_selector *bs = &_gst_builtin_selectors[n];
-      push_send_node (IP0, bs->symbol, bs->numArgs, super,
+      push_send_node (IP0, bp - IP0, bs->symbol, bs->numArgs, super,
                       TREE_SEND | TREE_NORMAL, n);
     }
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5f934e6..8e9cf31 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -18,7 +18,8 @@ objinst.st processes.ok processes.st prodcons.ok prodcons.st quit.ok \
 quit.st random-bench.ok random-bench.st sets.ok \
 sets.st sieve.ok sieve.st strcat.ok strcat.st strings.ok strings.st \
 pools.ok pools.st Ansi.st AnsiDB.st AnsiInit.st AnsiLoad.st AnsiRun.st \
-stcompiler.st stcompiler.ok shape.st shape.ok streams.st streams.ok
+stcompiler.st stcompiler.ok shape.st shape.ok streams.st streams.ok \
+xlat.st xlat.ok
 
 CLEANFILES = gst.im
 DISTCLEANFILES = atconfig
diff --git a/tests/testsuite.at b/tests/testsuite.at
index b95e1bf..696bef2 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -51,6 +51,7 @@ AT_DIFF_TEST([quit.st])
 AT_DIFF_TEST([pools.st])
 AT_DIFF_TEST([shape.st])
 AT_DIFF_TEST([streams.st])
+AT_DIFF_TEST([xlat.st])
 
 AT_BANNER([Other simple tests.])
 AT_DIFF_TEST([ackermann.st])
diff --git a/tests/xlat.ok b/tests/xlat.ok
new file mode 100644
index 0000000..637fa77
--- /dev/null
+++ b/tests/xlat.ok
@@ -0,0 +1,41 @@
+
+Execution begins...
+An instance of CompiledMethod
+  header: 96
+  Header Flags:
+    flags: 0
+    primitive index: 0
+    number of arguments: 0
+    number of temporaries: 0
+    number of literals: 2
+    needed stack slots: 12
+  descriptor: a MethodInfo
+  literals: [
+    [1] #bytecodeIndex
+    [2] #dispatchByte:with:at:to:with:
+  ]
+  byte codes: [
+    [1] source code line number 36
+    [3] source code line number 27
+ push self
+    [5] dup stack top
+    [7] send 0 args message #bytecodeIndex
+    [9] source code line number 29
+ pop stack top
+   [11] push 0
+   [13] source code line number 30
+   [15] push 0
+   [17] source code line number 31
+   [19] push 0
+   [21] source code line number 32
+   [23] push 0
+   [25] source code line number 33
+   [27] push 0
+   [29] send 5 args message #dispatchByte:with:at:to:with:
+   [33] push self
+ return stack top
+  ]
+ByteCodeHoles
+ByteCodeHoles>>dispatchTo
+33
+returned value is 33
diff --git a/tests/xlat.st b/tests/xlat.st
new file mode 100644
index 0000000..2bd3650
--- /dev/null
+++ b/tests/xlat.st
@@ -0,0 +1,91 @@
+
+"======================================================================
+|
+|   Regression tests for the XLAT code
+|
+|
+ ======================================================================"
+
+
+"======================================================================
+|
+| Copyright (C) 2014 Free Software Foundation.
+| Written by Holger Hans Peter Freyther.
+|
+| This file is part of GNU Smalltalk.
+|
+| GNU Smalltalk is free software; you can redistribute it and/or modify it
+| under the terms of the GNU General Public License as published by the Free
+| Software Foundation; either version 2, or (at your option) any later version.
+|
+| GNU Smalltalk is distributed in the hope that it will be useful, but WITHOUT
+| ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+| FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
+| details.
+|
+| You should have received a copy of the GNU General Public License along with
+| GNU Smalltalk; see the file COPYING.  If not, write to the Free Software
+| Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  
+|
+ ======================================================================"
+
+Object subclass: ByteCodeHoles [
+    bytecodeIndex [
+    ]
+
+    dispatchTo [
+        "This method should compile to the below bytecode. The
+        sending of the dispatchByte:with:at:to:with: will not take
+        two bytecodes but four. This broke the assumption inside the
+        xlat code.
+byte codes: [
+    [1] source code line number 5
+    [3] source code line number 7
+    push self
+    [5] dup stack top
+    [7] send 0 args message #bytecodeIndex
+    [9] source code line number 9
+    pop stack top
+   [11] push 0
+   [13] source code line number 10
+   [15] push 0
+   [17] source code line number 11
+   [19] push 0
+   [21] source code line number 12
+   [23] push 0
+   [25] source code line number 13
+   [27] push 0
+   [29] send 5 args message #dispatchByte:with:at:to:with:
+   [33] push self
+    return stack top"
+        <category: 'decoding bytecodes'>
+            self
+                bytecodeIndex;
+                dispatchByte: 0
+                with: 0
+                at: 0
+                to: 0
+                with: 0
+    ]
+]
+
+Eval [
+    "This should generate a DNU for the dispatchTo and not end up with
+    issues inside the exception handling code. We want to print the entire
+    stack handling code as a call to >>#currentLine will error when we
+    have saved a wrong IP."
+    (ByteCodeHoles >> #dispatchTo) inspect.
+
+    [
+        ByteCodeHoles new dispatchTo.
+    ] on: Exception do: [:e |
+        | context |
+        context := thisContext parentContext.
+        [context isInternalExceptionHandlingContext]
+            whileTrue: [context := context parentContext].
+        context := context parentContext parentContext.
+        context receiver class printNl.
+        context method printNl.
+        "The below should trigger the infinite loop"
+        context currentLine printNl].
+]
--
1.8.5.2


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

Il 06/02/2014 21:02, Holger Hans Peter Freyther ha scritto:
> Use the difference between bp and IP0 to figure out how many bytes
> were used for the bytecode of the send message. Add a small test
> case and try to make it as stable as possible.
>
> For message sends we remember the bytecode length as calculated
> by the decoding of the bytecodes.

IIRC "IP - IP0" works as well, and it already appears in byte.def.  Can
you use that if it works?

Paolo

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

On Fri, Feb 07, 2014 at 04:47:47PM +0100, Paolo Bonzini wrote:

> IIRC "IP - IP0" works as well, and it already appears in byte.def.
> Can you use that if it works?

Yes. it seems to work. May I push that change? In terms of AMD64
and JIT. The bundled version of GNU lightning doesn't compile for
AMD64 and after using the latest 1.x version it compiles but fails
quite quickly.

 current = (method_entry *) xmalloc (MAX_BYTES_PER_BYTECODE * (size + 2));

is not enough for inlined primitives on AMD64. But even then I had
crashes on the travis-ci system. Do you remember if xlat.c ever
worked on AMD64?

holger

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

Il 07/02/2014 19:00, Holger Hans Peter Freyther ha scritto:
I never tried it (I was PPC-based at the time).  Gwen at some point
played with it.

Paolo

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