Update Cobalt to support and use Lua 5.2 #50
Conversation
Still needs a LOT of testing. It's probably pretty broken right now.
Also fixed how environments are injected (I think?). Still generates 5.1 bytecode, so that'll have to be switched to 5.2.
I had to revert the changes to the code generator back to pure 5.1, so _ENV is no longer special in code. However, the environment is still stored as an upvalue, even on 5.1 (which is the version the bytecode is generated for). It properly loads 5.2 bytecode for the most part, but there's still a few things broken with upvalues. (It's able to get CC to partially boot through the BIOS!)
Boots CraftOS! (when precompiled to 5.2) Now to write the compiler...
This is complete enough to fully boot CraftOS in Lua 5.2; no precompilation necessary.
This should theoretically be the last thing to implement. I think there's also something with weak tables to fix, as well as adding tail call hooks, but those are much more minor details. If you don't like goto, you can disable it with LuaC.blockGoto.
Also fixed some bugs that made the tests fail.
goto can now be used as a variable name again, for old programs that used it. (Don't do this anymore.)
(read: `goto = 2` works)
Also fixed a bunch of stuff to make CC tests pass.
There's still a couple more to finish before it'll pass.
There was a problem hiding this comment.
Thanks for this, and sorry it's taken me so long to get to review.
Environments for Lua 5.1 functions are still stored as the first upvalue
I think this is my biggest concern here. There's a lot of isLua52 ? 0 : 1 expressions in the codebase, and that's going to be hellish to maintain (especially as the 5.1 codepath will be hit very infrequently).
I know I'm going back on what I said before, but maybe we do just bite the bullet and drop support for loading 5.1 bytecode. Don't know - do you have any thoughts?
| @@ -337,6 +351,12 @@ protected Varargs invoke(LuaState state, DebugFrame di, Varargs args) throws Lua | |||
|
|
|||
| case 2: // "load", // ( func|str [,chunkname[, mode[, env]]] ) -> chunk | nil, msg | |||
| { | |||
| // We'd typically rely on the argument checks in LuaValue, but those don't give us argument numbers so we do arg checks ahead of time. | |||
There was a problem hiding this comment.
Just wondering why this is here? Agree that the current system is bad, but would rather handle it consistently everywhere (see #31).
There was a problem hiding this comment.
The CC:T tests fail here because they expect the error message to be bad argument #1 (expected function or string, got <type>), but without the AoT type checks it errors with bad argument (function expected, got <type>). I put this in to make the tests pass, but I suppose it could be removed if the tests are reworked.
I had to delete any lines that used varargs, as Cobalt provides Lua 5.0-style arg tables, which does not exist in any form under Lua 5.2. Maybe this should be refactored/ adjusted so vararg compilation can be properly tested?
|
I've marked this as ready for review since all tests are now passing.
I'm looking at this change as a somewhat minor update, even if it completely changes code generation: most things should still work just like they did in 5.1. My idea is that even under 5.2-land, it'll work similar enough to the current behavior in 5.1 (besides the extra functionality). Then at some point in the future, a major, backwards-incompatible update brings the VM up to 5.3, and that's when all the old 5.1 stuff can be dropped. It'll add some extra cruft during this (indefinite) "transitional" period, but all that can be dropped in a major 5.3 update. (That could also correlate with a CC 2.0?) Of course, this is assuming someone at some point takes the time to update it to 5.3. OTOH, I doubt many things (at least in the CC ecosystem) would break from lack of 5.1 bytecode support. I know potatOS uses precompiled bytecode (which is also encrypted/obfuscated), but I'm not sure what other things would use it. Considering most users aren't doing things that would necessitate precompiled bytecode (let alone knowing how it works), it shouldn't be too big of a deal to remove. Perhaps asking the community might be a good idea? If 5.1 bytecode is removed, I'm inclined to removing functions removed in 5.2 as well; especially since the functions removed either a) have equivalent alternatives ( |
|
Haven't had a proper look at your changes since Sunday. Sorry, will try to do so this evening when I have more time (and mental bandwidth). But just some thoughts on other things:
Yeah, that seems fair enough.
We all know that isn't happening :p. It'd require me to make far too many decisions about what to break.
I do have a very small pot of money from Curse rewards which I put towards CC things (largely server/domain costs). Happy to spend some of that on a bounty for 5.3 support, though it's hardly a living wage for the amount of time spent on these :/. Similarly, if you've a PayPal/OpenCollective or something, happy to chuck some money your way!
I'm on a one-squid campaign to destroy PotatOS, so absolutely fine with this. The only thing I really worry about is JVML-JIT (it's so darn cool!), but it's not been updated in years so really doesn't matter.
I think what it might be worth doing is copying the useful bits of Lua's test suite into CC's, then use that to spec out what quirks we expect from the Lua VM (or our pure-Lua reimplementations of functions like Probably need to go through @Lemmmy's dump of CC pastebins at some point too, and see what potentially problematic code can be found there. Hopefully might turn into something CraftOS-PC can use too. I know you've complained about how useless CC:T's test suite is in the past :p. Not suggesting you should do this bit! Just me jotting things down so I have a higher chance of remembering to do them. |
Consider: breaking PotatOS is a bug fix |
|
My two cents are worth less than y'all's due to exchange rate but I'd rather just bite the bullet and kill all the 5.1 stuff now—It's been deprecated for years, since last decade, even, and I don't think the very few programs that would break are worth the maintenance cruft in the VM. It's already bad enough as is, it really doesn't need to be
|
Now, when blockGoto is enabled, goto is always treated as a name instead of throwing an error. This is much more useful than the previous behavior.
|
At this point, master has deviated from my branch far enough that trying to update this patch is unfeasible. Closing this for now - if 5.2 becomes necessary again, it would be best to redo this over the current version of the code. |
This PR adds official support for Lua 5.2. New features include:
gotostatement, labels_ENVupvaluesFor compatibility with Lua 5.1 features:
setfenv/getfenvare still present and have been updated to work with the new environmentsgotocontinues to be a valid identifier in Lua 5.2: it only becomes agotostatement when in the formgoto <name>There should be very few compatibility issues created by this change. However, some less-used things, such as anything that relies on the format of dumped bytecode, or directly creates the
_ENVupvalue, may break. In addition, since the value of_VERSIONchanges, scripts that depend on it being set to"Lua 5.1"may function incorrectly. Otherwise, most things should work fine.I've tested this with the CC: Tweaked test suite and it passes all tests. In addition, it passes all Cobalt tests (with a few adjustments made for 5.2). It should be all ready to go at this point in time.