Skip to content

pkg/lua: Make the module searchers conform to the API.#9726

Merged
leandrolanzieri merged 3 commits intoRIOT-OS:masterfrom
jcarrano:lua-loader-fix
Jun 3, 2019
Merged

pkg/lua: Make the module searchers conform to the API.#9726
leandrolanzieri merged 3 commits intoRIOT-OS:masterfrom
jcarrano:lua-loader-fix

Conversation

@jcarrano
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano commented Aug 7, 2018 •
edited
Loading

Contribution description

Bug 1

I messed up the specification for the Lua loader module.

The module searchers in the require package should return a string
if the module is not found, and not raise an error.

See: https://www.lua.org/manual/5.3/manual.html#pdf-package.searchers

If the searchers raised an error, then the search would stop after the first loader fails to find (which is the current- buggy- behaviour.) The result is that only the pure-lua module table get searched, and it is impossible to load a C extension module.

Bug 2

I violated my own specification, resulting in a crash where a module was requested but no modules were supplied by the user.

This used to work, but then some time ago I remove the definition of the weak symbols used for the tables, leaving them pointing to NULL by default.

Testing procedure

I included a test script in the third commit.

@jcarrano jcarrano requested a review from danpetry August 7, 2018 12:04
@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports labels Oct 17, 2018
@jcarrano jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Oct 18, 2018
@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Oct 18, 2018

I labeled it as a bug because I was not respecting the API and that qualifies as a bug.

@danpetry Do you mind giving me a hand with this one?

@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented Nov 19, 2018

@danpetry Ping.

danpetry
danpetry previously requested changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@danpetry danpetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reasoning makes sense - could you please include a test (code or instructions)?

@tcschmidt
Copy link
Copy Markdown
Member

tcschmidt commented Dec 23, 2018

@jcarrano ping.

@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented May 29, 2019

Well, sorry for the delay. Testing this required creating some lua C-extension modules, which I was lazy to write and also did not have time to. I finally got around to doing it, and uncovered another bug in the process.

I added a test application.

@jcarrano jcarrano force-pushed the lua-loader-fix branch 2 times, most recently from 5b19166 to 6587c75 Compare May 29, 2019 13:08
@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 29, 2019
@jcarrano jcarrano requested a review from danpetry May 29, 2019 13:21
@leandrolanzieri leandrolanzieri self-requested a review May 29, 2019 14:44
leandrolanzieri
leandrolanzieri requested changes May 31, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense. I noticed a change of behaviour compared to master, when trying to require a module that is not defined. I guess this is due to the fix of Bug 1?:

On current master

main(): This is RIOT! (Version: 2019.07-devel-527-gd1f06)
Using memory range for Lua heap: 0x56614090 - 0x5661dcd0, 4 bytes
This is Lua: starting interactive session

Welcome to the interactive interpreter
L> require "something"
Runtime error   Module 'something' not found in Lua-builtins
L> 
L>

With this PR

main(): This is RIOT! (Version: 2019.07-devel-519-g18a38-lua-loader-fix)
Using memory range for Lua heap: 0x5668e090 - 0x56697cd0, 4 bytes
This is Lua: starting interactive session

Welcome to the interactive interpreter
L> require "something"
/home/leandro/Work/RIOT/examples/lua_REPL/../../Makefile.include:557: recipe for target 'term' failed
make: *** [term] Segmentation fault (core dumped)

Also, some minor comments.

@jcarrano jcarrano added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 31, 2019
@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented May 31, 2019

@leandrolanzieri

I noticed a change of behaviour compared to master, when trying to require a module that is not defined.

D'oh! good catch. I made a copypasta-error (it already felt like copypasta when I was writing the code, I should't have ignored that hunch).

I removed the ready-for build. I'll add it back once I have squashed.

@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented May 31, 2019

@jcarrano that seems to fix it. Maybe you could add '\n' to the error strings, currently the output looks like:

Runtime error   [string "return require "something""]:1: module 'something' not found:
        no field package.preload['something']Module not found in Lua-builtinsModule not found in C-builtins

@jcarrano
Copy link
Copy Markdown
Contributor Author

jcarrano commented May 31, 2019

I added some newline and tabs, it should be prettier now.

leandrolanzieri
leandrolanzieri reviewed May 31, 2019
View reviewed changes
leandrolanzieri
leandrolanzieri approved these changes May 31, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, ACK. Please squash!

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels May 31, 2019
@jcarrano jcarrano removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 31, 2019
@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 31, 2019
jcarrano added 3 commits May 31, 2019 17:15
@jcarrano
pkg/lua: Make the module searchers conform to the API.
fe07bff 
The module searchers in the `require` package should return a string
if the module is not found, and not raise an error.

See: https://www.lua.org/manual/5.3/manual.html#pdf-package.searchers

Also make error strings contain newlines and tabs just like the original
ones.
@jcarrano
pkg/lua: detect if weak symbol has been overriden.
56eb7d9 
After removing the default definitions of lua module tables and table
lengths (see lua_builtin.h) the symbols have been left undefined, which
results in them getting an address of NULL and a crash if there are no
user symbols and the user attempts a "require".

This patch checks the address of the table length variable and fails the
module search function of the table is not set (i.e. it behaves as if the
table was empty.)
@jcarrano
tests/lua_loader: test the RIOT-customized lua loader.
913f74b 
For the RIOT port of Lua, the module loader has been more or less
rewritten to allow for easily integrating source modules defined in static
arrays and C modules embedded in the application.

So  far the loader had not been tested (and a bug was found). This test
should give a bit more certainty that the RIOT integration works as it
should.
@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented May 31, 2019 •
edited
Loading

@danpetry any other comments? Please ACK, review or dismiss.

@leandrolanzieri leandrolanzieri merged commit f56709e into RIOT-OS:master Jun 3, 2019
@MrKevinWeiss MrKevinWeiss added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jun 13, 2019
@cladmi cladmi mentioned this pull request Aug 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@danpetry danpetry Awaiting requested review from danpetry

Assignees

No one assigned

Labels

Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jcarrano @tcschmidt @leandrolanzieri @danpetry @miri64 @MrKevinWeiss