dschmenk / PLASMA

Proto Language AsSeMbler for All (formerly Apple)
MIT License
189 stars 26 forks source link

PLASM compiler crashes if invoked with a space between "+" and PLASM #53

Open ZornsLemma opened 5 years ago

ZornsLemma commented 5 years ago

If I invoke the PLASM self-hosted compiler as "+ PLASM TEST.PLA", it crashes; here's a screenshot of this in an Apple II emulator: Screenshot at 2019-05-10 21-37-31 I think at least part of the issue is that the core VM strips away the space between the "+" and "PLASM", so it's able to successfully execute the PLASM module, but the argument parsing code in the ARGS module returns "+" and then "PLASM" as the first and second arguments.

I think the actual crash is caused by PLASM trying to load itself as source code, as if we'd typed "+PLASM PLASM", and crashing because the file is not text; I seem to be able to get a similar crash by typing that. I don't know if this is worth fixing; perhaps just changing the parsing so the core VM and ARGS are consistent in handling a space between the leading "+" and the module name would be adequate.

ETA: The commit in my repo which references this just notes that I found the problem; I am playing around with alternative implementation details on the Acorn port, that commit is not any kind of fix or suggestion for this issue and you should just ignore github being helpful and mentioning it...

ZornsLemma commented 5 years ago

Actually I've been unfair in calling this a crash, it seems to generate an odd message but maybe that's fair enough given the invalid input. But it does seem a bit odd to have this parsing inconsistency.

dschmenk commented 5 years ago

Hi Steve-

Yep, the command line parsing is pathetic at best. But, there are literally no bytes available to improve anything. It really wasn’t meant to be more than a way to automatically lunch modules at startup or run a better shell. There is no reason you have to suffer with it if you want to fix it for the Beeb.

As for looking at the JIT compiler, I’m going to have to re-populate all the neurons that either died or got repurposed ;-) I should get around to sometime in the near future - it will give me an opportunity to look at it with somewhat fresh eyes.

Dave…

P.S. Isn’t Will’s emulator incredible?

On May 10, 2019, at 1:42 PM, ZornsLemma notifications@github.com wrote:

If I invoke the PLASM self-hosted compiler as "+ PLASM TEST.PLA", it crashes; here's a screenshot of this in an Apple II emulator: https://user-images.githubusercontent.com/7988240/57555333-e8722380-736b-11e9-83a3-30e951ea3f1e.png I think at least part of the issue is that the core VM strips away the space between the "+" and "PLASM", so it's able to successfully execute the PLASM module, but the argument parsing code in the ARGS module returns "+" and then "PLASM" as the first and second arguments.

It think the actual crash is caused by PLASM trying to load itself as source code, as if we'd typed "+PLASM PLASM", and crashing because the file is not text; I seem to be able to get a similar crash by typing that. I don't know if this is worth fixing; perhaps just changing the parsing so the core VM and ARGS are consistent in handling a space between the leading "+" and the module name would be adequate.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dschmenk/PLASMA/issues/53, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBOGJWGTJZE63VAZXYCYY3PUXM43ANCNFSM4HMGC66Q.

ZornsLemma commented 5 years ago

Hi Dave,

Thanks for the response, I didn't really expect one so quickly!

I may be missing something, but one fix (untested, of course) would simply be to remove the last call to stripspaces() in parsecmd() in cmd.pla. That would shrink the core VM, not grow it, and I think it would make args.pla consistent with the core VM and get rid of this inconsistency - "+ PLASM" would then simply be invalid at the PLASMA prompt.

As for the compiler itself, I overreacted by calling it a crash. On the Acorn VM it spews out some random text which happens to include the "turn printer on" control code, and then the output hangs because there's no printer connected and the printer buffer fills up. I misinterpreted this as the VM/compiler actually having crashed the machine and that primed my brain to misinterpret the even better behaviour of it on the Apple. The behaviour of the compiler is a little weird but it's being given weird input, and I think it actually copes pretty well, so there's probably no need to go fiddling with it.

Not that I want to discourage you from having another look at it, I'm sure you'll come up with some cool stuff if you do look at it with fresh eyes!

Cheers.

Steve

PS Yes, the emulator is a very nice piece of work - perfect too for someone like me who doesn't necessarily want the faff of installing an emulator on my machine but wants to play with something like PLASMA.

ZornsLemma commented 5 years ago

PPS In terms of freeing up space in the core VM, one thing I've just done in the Acorn VM is that I don't copy the command line into a separate buffer for use later by args.pla before parsing it in cmd.pla. I have a single copy of it and the command parsing in cmd.pla has been tweaked to avoid modifying the command line, so it's left intact for args.pla to parse later. This avoids the need to set memory aside for a second copy of the command line. It may be you could do something similar on the Apple. (Or it may be there's a hidden flaw in this approach I simply haven't noticed yet. :-)) You can see - though it's perhaps a bit noisy - this change on my rather misnamed jit2 branch: https://github.com/ZornsLemma/PLASMA/commit/245d5065002d7525a5299477352aa21757319753

dschmenk commented 5 years ago

The Apple II will blow away the input buffer as it loads files. It’s why I had to move it in the first place. 64K is such a hassle, it’s amazing we ever got anything done.

On May 11, 2019, at 7:14 AM, ZornsLemma notifications@github.com wrote:

PPS In terms of freeing up space in the core VM, one thing I've just done in the Acorn VM is that I don't copy the command line into a separate buffer for use later by args.pla before parsing it in cmd.pla. I have a single copy of it and the command parsing in cmd.pla has been tweaked to avoid modifying the command line, so it's left intact for args.pla to parse later. This avoids the need to set memory aside for a second copy of the command line. It may be you could do something similar on the Apple. (Or it may be there's a hidden flaw in this approach I simply haven't noticed yet. :-)) You can see - though it's perhaps a bit noisy - this change

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/dschmenk/PLASMA/issues/53#issuecomment-491514476, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBOGJR5T5WI4MPWUOQBCW3PU3IEZANCNFSM4HMGC66Q.

ZornsLemma commented 5 years ago

Yeah, I guess that would be a problem... :-)

On Sat, 11 May 2019, 15:27 David Schmenk, notifications@github.com wrote:

The Apple II will blow away the input buffer as it loads files. It’s why I had to move it in the first place. 64K is such a hassle, it’s amazing we ever got anything done.

On May 11, 2019, at 7:14 AM, ZornsLemma notifications@github.com wrote:

PPS In terms of freeing up space in the core VM, one thing I've just done in the Acorn VM is that I don't copy the command line into a separate buffer for use later by args.pla before parsing it in cmd.pla. I have a single copy of it and the command parsing in cmd.pla has been tweaked to avoid modifying the command line, so it's left intact for args.pla to parse later. This avoids the need to set memory aside for a second copy of the command line. It may be you could do something similar on the Apple. (Or it may be there's a hidden flaw in this approach I simply haven't noticed yet. :-)) You can see - though it's perhaps a bit noisy - this change

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/dschmenk/PLASMA/issues/53#issuecomment-491514476>, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABBOGJR5T5WI4MPWUOQBCW3PU3IEZANCNFSM4HMGC66Q .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dschmenk/PLASMA/issues/53#issuecomment-491515451, or mute the thread https://github.com/notifications/unsubscribe-auth/AB46IEB22N7ZHE4BVI5OJXDPU3JUTANCNFSM4HMGC66Q .