Move pallenelib to inside the Lua interpreter #424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hugomg
force-pushed
the
remove-pallenelib
branch
2 times, most recently
from
5b06a5b to
302dd20
Compare
|
I think this might be one of those PRs where it is easier to understand if it is read one commit at a time. |
srijan-paul
requested changes
Jul 23, 2021
vm/src/lua.c
Outdated
| ** used by the executable itself. This is a problem for the Pallene runtime library, because those | ||
| ** symbols are only used by extension modules, which we expect to dynamically link. One hacky | ||
| ** workaround I found is to force the "lua" executable to reference one of the symbols from the | ||
| ** pallene_code, as done below. |
There was a problem hiding this comment.
This was supposed to say pallene_core perhaps?
vm/src/pallene_core.c
Outdated
| @@ -159,6 +161,7 @@ void pallene_grow_array(lua_State *L, const char* file, int line, Table *arr, un | |||
|
|
|||
| void pallene_io_write(lua_State *L, TString *str) | |||
| { | |||
| (void) L; | |||
There was a problem hiding this comment.
I wonder what this does? Would a comment help?
pallene/coder.lua
Outdated
| @@ -1265,7 +1265,7 @@ gen_cmd["GetTable"] = function(self, cmd, _func) | |||
|
|
|||
| return util.render([[ | |||
| { | |||
| static size_t cache = UINT_MAX; | |||
| static int cache = UINT_MAX; | |||
There was a problem hiding this comment.
should this be INT_MAX now?
hugomg
force-pushed
the
remove-pallenelib
branch
2 times, most recently
from
11acf88 to
d81c0e6
Compare
srijan-paul
requested changes
Jul 25, 2021
README.md
Outdated
|
|
||
| These two components can be built through the Makefile we provide. The command | ||
| to be used depends on your operating system: | ||
| To compile the custom version of Lua, follow the instructions found the vm/doc/readme.html. |
There was a problem hiding this comment.
instructions found the
"Instructions found in the [path]" perhaps?
There was a problem hiding this comment.
Apparently, Github displays the raw HTML if we link to that. I think I'm going to link to the lua.org website instead.
This set of commits gets rid of the separate pallenelib library. This is part of the ongoing effort to simplify the build and installation process for Pallene (see #16, #153). The pallene_core files are now going to be part of our the custom installation of Lua. Unfortunately, for some reason it is not working. When I compile the custom Lua, the `pallene_symbols` are not being exported by the custom Lua executable. We'll need to figure out what is the problem. TODO: - [] Find out why the symbols are not being exported - [] Fix compilation warnings that appeared because of -Wextra - [] Update the ".patch" file we use when updating to a new Lua version - [] Delete the "runtime" folder, and - [] Update the installation instructions in the README.
This workaround ensures that the pallene_core. symbols are included in the compiled executable. By default, these symbols were not being included because they are not used anywhere else in the Lua executable. They are only intended for dynamically linked modules. One possible way to avoid this would be to pass some special flags to the linker, such as -Wl,--whole-archive. However, I'm worried that this might only work with a GCC toolchain on Linux. The proposed hack is something that I hope might be more portable. However, I would certainly be very happy if we could find an alternative less hacky solution.
Now that pallenelib is compiled with the rest of the Lua interpreter we are compiling with -Wextra, which has revealed a couple of warnings. This commit fixes them. 1. The inline cache now has type int instead of size_t. This reflects how Lua itself uses int for this purpose. (For example, see the various places that use the sizenode macro) 2. In the implementation of io.write, the L parameter should be marked as unused.
Previously, all we had to do was a small change in the luaconf. Now there are more things to do: add the pallene_core files, update the makefile, add the linker hack. Because these changes broke the "patch" file that we had, I deleted that patch file and replaced it with manual instructions. When we update to the next release (5.4.4, or maybe 5.5?) we could consider creating a new "patch" file. It's easier to create that patch file after we apply the changes on a fresh version of Lua.
I think this version of the linker hack has a better change of working on more compilers. If the symbols from lapi.o are being exported, then including Pallene symbols there should probably do the trick! As a bonus, we can get away with not modifying the makefile.
hugomg
force-pushed
the
remove-pallenelib
branch
from
d81c0e6 to
541e96a
Compare
|
Because the Lua Makefile cds into the "src" directory do to it's thing, I think it's clearer if we put all our files there instead of on a "pallene" directory. But I'm glad you asked, because I found a separate bug: I had forgotten to modify the parent Makefile to tell it to install the additional header files. |
srijan-paul
approved these changes
Jul 25, 2021
hugomg
force-pushed
the
remove-pallenelib
branch
from
541e96a to
2f85fc4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This set of commits gets rid of the separate pallenelib library. This is part of the ongoing effort to simplify the build and installation process for Pallene (see #16, #153). The pallene_core files are now going to be part of our the custom installation of Lua.
Unfortunately, for some reason it is not working. When I compile the custom Lua, the
pallene_*symbols are not being exported by the custom Lua executable. We'll need to figure out what is the problem.TODO: