Re: Patches for wip-rtl
Hello, On Tue, Apr 23, 2013 at 6:13 AM, Andy Wingo wi...@pobox.com wrote: Heya, On Tue 23 Apr 2013 04:38, Noah Lavine noah.b.lav...@gmail.com writes: Ah I see what you mean. There are two uses of variables in the VM: one for calls to variable-ref or variable-set, and the other for internal use. For the internal uses, we know the variable should indeed be a variable and so we should emit the checks. For calls to variable-ref / variable-set, I think it would be best if somehow the compiler could replace those calls with (if (call variable? x) (primcall box-ref x) (call error ...)). Dunno. WDYT? Do we usually omit checks for internal stuff? If so, I agree, we should do that here too. But right now we do the check every time anyway - the only difference my patch made is whether we failed with abort() or vm_error_not_a_variable. I agree that that doesn't make much sense as an error for internal stuff. Maybe change it to vm_error_bad_instruction or something like that for now? In general I agree that we should be able to do nice error checking only for user-generated box-refs, but that seems more complex than we need right now (unless you want to add it to the VM). One thing that would be interesting - and I don't know if we do this now - is using different VMs for different parts of the code. Specifically, if the REPL ran in a different VM than user code, then the REPL wouldn't die in these cases. That might also enable cool debugging things, like single-stepping the user VM and examining its registers. I noticed that we already have support for changing the active VM. What do you think? We can do that already to a degree... probably the best fallback that we have is the stack VM, actually. What happens if you install traps on the VM that call stack VM procedures and then you run an RTL function? I would think that you should be able to do something useful there. That does sound interesting, but I've never used traps. I've been meaning to learn them some time so I can try to remember tail calls for debugging, but that probably won't be for a little while. Noah
Re: Patches for wip-rtl
Heya, On Tue 23 Apr 2013 04:38, Noah Lavine noah.b.lav...@gmail.com writes: I assume you're talking about the box-ref and box-set! stuff with checking for non-variables, right? I agree in general, but those instructions could plausibly be emitted for any code that uses module introspection to get its own variable objects, right? Ah I see what you mean. There are two uses of variables in the VM: one for calls to variable-ref or variable-set, and the other for internal use. For the internal uses, we know the variable should indeed be a variable and so we should emit the checks. For calls to variable-ref / variable-set, I think it would be best if somehow the compiler could replace those calls with (if (call variable? x) (primcall box-ref x) (call error ...)). Dunno. WDYT? One thing that would be interesting - and I don't know if we do this now - is using different VMs for different parts of the code. Specifically, if the REPL ran in a different VM than user code, then the REPL wouldn't die in these cases. That might also enable cool debugging things, like single-stepping the user VM and examining its registers. I noticed that we already have support for changing the active VM. What do you think? We can do that already to a degree... probably the best fallback that we have is the stack VM, actually. What happens if you install traps on the VM that call stack VM procedures and then you run an RTL function? I would think that you should be able to do something useful there. Just a thought! Andy -- http://wingolog.org/
Re: Patches for wip-rtl
Hi Noah, On Sun 21 Apr 2013 17:50, Noah Lavine noah.b.lav...@gmail.com writes: +((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) Do I understand you correctly that you're just making a sequence of non-idiomatic instructions because you know that doing the assert-nargs-ee will check nargs? I would think in a normal case you'd want to do assert-nargs-ge 0, then bind-rest 0, then reserve-locals 0 (or not; in this case it would not be necessary). However the bug you have seen with bind-rest is probably also present with keyword arguments. We should probably just change the reserve-locals instruction to also take nargs as a value, given that we know it statically at compile-time. Does that make sense to you? Andy -- http://wingolog.org/
Re: Patches for wip-rtl
Hi :) Thanks for working on the RTL VM! First of all, sorry about that linker error. I'm trying to make RTL programs more debuggable by hacking on the linker, so that it emits proper debugging information. I should have caught that typo though! I think however that the strategy of making the VM somehow more resilient is not going to work. There are so many ways that invalid instructions can make the VM fail -- and you have no idea what you can rely on if those invariants fail. So it doesn't seem to me that it's possible to do that job well at all, and for that reason it seems to me that we shouldn't try very hard. So my answer for dealing with VM issues would be to try to narrow down the problem, then use instruction-level traps to print instructions as you go. Once you find the problem you probably won't be able to recover; oh well. At least you would know how you got there. And I don't know how you've been able to get as far as you have with the deplorable state of the debugging infrastructure. You can't even disassemble a function you just made! I'll be working on that this week; we'll see what happens. So in summary, dunno. Surely there is some way we can do things better, but I wanted to register my skepticism for this line of hacking. What do you think? Cheers, Andy -- http://wingolog.org/
Re: Patches for wip-rtl
Hello, On Mon, Apr 22, 2013 at 4:27 PM, Andy Wingo wi...@pobox.com wrote: Hi Noah, Do I understand you correctly that you're just making a sequence of non-idiomatic instructions because you know that doing the assert-nargs-ee will check nargs? I would think in a normal case you'd want to do assert-nargs-ge 0, then bind-rest 0, then reserve-locals 0 (or not; in this case it would not be necessary). Yes, that's exactly why I'm doing it. I just wanted to find a way to test that the bug was fixed and stayed fixed. In this case that's a bit weird, because we probably don't want to guarantee that this pattern of instructions will continue to work, but I don't have a great alternative. I think any sort of test for this is going to be dependent on the internals of the VM. (My earlier email about adding accessors for the VM was about this same bug, but then I realized that nargs isn't part of the vm structure so I couldn't solve it like that.) However the bug you have seen with bind-rest is probably also present with keyword arguments. We should probably just change the reserve-locals instruction to also take nargs as a value, given that we know it statically at compile-time. Does that make sense to you? I'm not sure. As long as nargs is going to be a local variable, I don't see a reason not to use it. It might make more sense to either keep it the way it is or change every instruction that uses nargs. But I haven't looked to see how many there are, or how important it is that they know nargs. Noah
Re: Patches for wip-rtl
Hello, On Mon, Apr 22, 2013 at 4:39 PM, Andy Wingo wi...@pobox.com wrote: Hi :) Thanks for working on the RTL VM! Thanks for doing most of the work! I'm happy to help. :-) First of all, sorry about that linker error. I'm trying to make RTL programs more debuggable by hacking on the linker, so that it emits proper debugging information. I should have caught that typo though! Debugging would be great. I have thought about stopping this and working on debugging stuff some, but that actually seems harder than what I'm doing now, and I would like to get the compiler working. I think however that the strategy of making the VM somehow more resilient is not going to work. There are so many ways that invalid instructions can make the VM fail -- and you have no idea what you can rely on if those invariants fail. So it doesn't seem to me that it's possible to do that job well at all, and for that reason it seems to me that we shouldn't try very hard. I assume you're talking about the box-ref and box-set! stuff with checking for non-variables, right? I agree in general, but those instructions could plausibly be emitted for any code that uses module introspection to get its own variable objects, right?. At that point the not-a-variable error would be a user type error, and so I think it's better to throw wrong-type-arg. (But of course, that's not my real motivation - I just happened to hit this while debugging the compiler.) And I don't know how you've been able to get as far as you have with the deplorable state of the debugging infrastructure. You can't even disassemble a function you just made! I'll be working on that this week; we'll see what happens. So in summary, dunno. Surely there is some way we can do things better, but I wanted to register my skepticism for this line of hacking. What do you think? First of all, I agree that debugging infrastructure would be great. I'm very glad you're working on it - that should help a lot. I'm not sure if I have much to add there right now, but I will keep it in mind. As for the line of hacking, as I said above, I think in the particular case of box-ref and box-set! it's justified; I don't know in general. I certainly agree that once the VM's internal state is corrupt, all bets are off, and we probably won't be able to continue. One thing that would be interesting - and I don't know if we do this now - is using different VMs for different parts of the code. Specifically, if the REPL ran in a different VM than user code, then the REPL wouldn't die in these cases. That might also enable cool debugging things, like single-stepping the user VM and examining its registers. I noticed that we already have support for changing the active VM. What do you think? Best, Noah
Re: Patches for wip-rtl
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.lav...@gmail.comwrote: 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
Re: Patches for wip-rtl
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.lav...@gmail.comwrote: 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.lav...@gmail.comwrote: 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 0001-Bugfix-in-vm-engine.c.patch Description: Binary data
