Re: Patches for wip-rtl

[Noah Lavine <noah.b.lavine@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2865 bytes --]

And here's another patch that fixes an off-by-one error in the bind-rest
instruction. This solves the problems I was having in my earlier email
about an abort in the VM.

Best,
Noah


On Sun, Apr 21, 2013 at 11:23 AM, Noah Lavine <noah.b.lavine@gmail.com>wrote:

> Hello,
>
> Please don't worry about the last part of that message. I realized I can
> test the nargs value by (ab)using the assert-nargs-ee instruction.
>
> On a related note, there's a certain piece of code in vm.c that is not
> robust in the face of a corrupt nargs. Specifically, if you call
> vm_error_wrong_num_args (vm.c:518 in wip-rtl) with nargs=-1, guile will
> segfault. We could fix this, but I'm inclined to leave it as it is, because
> this can only happen if nargs is corrupted by some earlier call, and I
> think the right approach is to test the VM enough that nargs doesn't get
> corrupted. Does that sound reasonable? If not, it should be simple to
> insert some check there the nargs is big enough not to corrupt anything.
>
> Noah
>
>
> On Sat, Apr 20, 2013 at 7:30 PM, Noah Lavine <noah.b.lavine@gmail.com>wrote:
>
>> Hello,
>>
>> I've attached three patches for wip-rtl. The first is somewhat different
>> than the other two: it fixes an error that occurred when moving the linker
>> to its own file. (system vm rtl) and (system vm linker) both contain a
>> function called link-string-table, and (system vm rtl) was calling the
>> wrong one, which made rtl.test fail. I solved this with an @-reference.
>>
>> The second two provide better VM errors when box-ref and box-set! are
>> called on something that is not a variable. In particular, they throw an
>> exception instead of aborting. This could occur in user code, if the user
>> wants to hand-write RTL code, but it's also very useful in trying a new
>> compiler implementation. The first patch handles box-ref, and the second
>> handles box-set!. Also note that I had to add a new type of exception to
>> (test-suite lib) to catch these errors.
>>
>> Lastly, I'd like to ask for ideas on how to test for errors in VM
>> instructions that don't cause immediate problems, but do put the VM in an
>> inconsistent state. I know of a collection of these errors, and while I
>> could fix them, I'd rather fix them and add a test for them. I can think of
>> two possibilities off the top of my head: 1) add Scheme accessors for VM
>> state (I don't think mutators are necessary for now) or 2) write the tests
>> in C instead of Scheme.
>>
>> I'd prefer option 1, mostly because I think that making the VM state more
>> explicit is a good direction to go in anyway - eventually it would be great
>> if the debugger could print the contents of registers, and that would
>> require VM introspection anyway, so I think starting now is good.
>>
>> What do other people think of the patches and the ideas?
>>
>> Best,
>> Noah
>>
>
>

[-- Attachment #1.2: Type: text/html, Size: 3802 bytes --]

[-- Attachment #2: 0001-Bugfix-in-vm-engine.c.patch --]
[-- Type: application/octet-stream, Size: 2231 bytes --]

From 2cf7700ea500ec1fa51307201f32e850235c8e10 Mon Sep 17 00:00:00 2001
From: Noah Lavine <noah.b.lavine@gmail.com>
Date: Sun, 21 Apr 2013 11:25:49 -0400
Subject: [PATCH] Bugfix in vm-engine.c

 * libguile/vm-engine.c: fix off-by-one error in bind-rest instruction.
 * test-suite/tests/rtl.test: test the fix.
---
 libguile/vm-engine.c      |    8 ++++++--
 test-suite/tests/rtl.test |   26 ++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/libguile/vm-engine.c b/libguile/vm-engine.c
index fa8c8fc..a8fede6 100644
--- a/libguile/vm-engine.c
+++ b/libguile/vm-engine.c
@@ -1585,12 +1585,16 @@ RTL_VM_NAME (SCM vm, SCM program, SCM *argv, size_t nargs_)
 
       SCM_UNPACK_RTL_24 (op, dst);
 
-      while (nargs-- > dst)
+      for (; nargs > dst; nargs--)
         {
-          rest = scm_cons (LOCAL_REF (nargs), rest);
+          rest = scm_cons (LOCAL_REF (nargs - 1), rest);
           LOCAL_SET (nargs, SCM_UNDEFINED);
         }
 
+      /* we need nargs = dst + 1 so that a call to reserve-locals after
+	 this won't overwrite the list we just consed up. */
+      nargs = dst + 1;
+
       LOCAL_SET (dst, rest);
 
       RESET_FRAME (dst + 1);
diff --git a/test-suite/tests/rtl.test b/test-suite/tests/rtl.test
index e97a801..04324fa 100644
--- a/test-suite/tests/rtl.test
+++ b/test-suite/tests/rtl.test
@@ -265,3 +265,29 @@
         (box-ref 0 1)
         (return 0)
         (end-program))))))
+
+(with-test-prefix "bind-rest"
+  ;; check that bind-rest leaves nargs set correctly
+  (pass-if-equal "nargs = 0"
+    '()
+    ((assemble-program
+      '((begin-program foo)
+        (assert-nargs-ee/locals 0 1)
+        (bind-rest 0)
+        ;; nonintuitive, but the output of bind-rest has to count as an
+        ;; argument for reserve-locals to work. therefore, even if we
+        ;; started with 0 arguments, we end up with one.
+        (assert-nargs-ee 1)
+        (return 0)
+        (end-program)))))
+
+  (pass-if-equal "nargs = 3"
+    '(b c)
+    ((assemble-program
+      '((begin-program foo)
+        (assert-nargs-ee/locals 3 0)
+        (bind-rest 1)
+        (assert-nargs-ee 2)
+        (return 1)
+        (end-program)))
+     'a 'b 'c)))
-- 
1.7.10.4