First cut of new LFS for initial review #3272

TerryE · 2020-09-06T01:17:28Z

Fixes #3206 (part)

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/*.

Notes

This is a major upgrade to LFS for implement on-ESP building of LFS regions. I have had to do some major rework of the dump / undump architecture to achieve this, and we can discuss this later.
This in a initial tranche for review and comment. I have done basic testing, but still have a lot more use cases to enumerate, so this should be considered alpha code at this stage. I also have documentation changes and some peripheral API stuff (such as Addition of file.savedump(filename, func [, striplevel]) #3145) to implement.
I have tagged Johny and Nathaniel as reviews, as they are most familiar with the NodeMCU Lua internals. However, any constructive input is welcome.
The basic functionality also works in the luac.cross -e environment. Create a script containing debug.debug() and execute this to make a basic interpreter available. os and io are available and there are (HOST only) base functions lfsreload and lfsindex.

nwf

Some initial nits; I haven't pulled the code to play with it.

nwf · 2020-09-06T02:32:24Z

app/lua53/host/luac.c

    L = luaL_newstate();
-    if (L == NULL) fatal("not enough memory for state");
+    if (L == NULL)
+      fatal("not enough memory for state");


There are a great many "stylistic" changes like the above three lines of diff in these commits. Do they increase or decrease the diff between "upstream Lua" and "nodemcu Lua"? Most of them appear to my mind to be improvements in the code readability; if they increase the gap between "upstream" and "nodemcu", can we send them upstream? :)

luac.c needs a lot more work largely because the unification of the LFS and LC formats. Given the volume of change and the previous nasty non-standard formatting of lauc.c which is nothing like the rest of the lua core codebase, ~~I have been considering running it through a pretty print style formatter to make it more readable anyway.~~

PS. Mindfart. Already done this, but the code still needs a tidy.

nwf · 2020-09-06T02:33:58Z

app/lua53/host/luac.c

@@ -359,256 +269,286 @@ int main(int argc, char *argv[]) {
        lua_close(L);
        continue; /* and loop around once more simulating restart */
      }
-      char *err = strdup(lua_tostring(L, -1));
+      char *


This appears to be some violence. What happened to the rest of the line?

nwf · 2020-09-06T02:35:25Z

app/lua53/host/luac.c

+    case '\t': printf("\\t");  break;
+    case '\v': printf("\\v");  break;
+    default:
+      printf(isprint(c) ? "%c" : "\\%03d", c);


While "obviously" equivalent, this still takes a moment to check, so I'd advocate either for reverting to match mainline or proposing this change to mainline for their inclusion. Many of the following changes also have the same feel.

I will post a general discussion in #3206. Have read and review in that light.

nwf · 2020-09-06T02:37:19Z

app/lua53/lauxlib.c

  LoadF lf;
+  (void)(multi);  ////  TODO


Yea, I think I'm content with multi waiting for a later PR. :)

nwf · 2020-09-06T02:38:21Z

app/lua53/lauxlib.c

@@ -1153,6 +1154,8 @@ static int errhandler_aux (lua_State *L) {
 ** to convert into an error message which it handles in a separate posted task.
 */
 static int errhandler (lua_State *L) {
+  if (!strncmp(lua_tostring(L, -1), "!LFS reload!", sizeof("!LFS reload!") - 1))


It's not really worth using strncmp when one of the parameters is a constant; go ahead and use strcmp without the repetition. The compiler's going to inline it anyway, one imagines.

Yup memcmp is better. We can't use strcmp because the various reload messages include a qualification.

"include a qualification"?

TerryE · 2020-09-06T20:09:38Z

As a codicil to my #3206 comments, I do suggest that reviewers print off and go through the three main rewrites: ldump.c, lundump.c, lnodemcu.c

TerryE · 2020-09-07T13:18:39Z

I will be adding #3145 to this PR as this is needed for on-ESP LFS maintenance.

jmattsson

Phew! Big stuff!
Overall looks good. See review comments for some qs and comments.
I haven't reviewed the dump/undump logic in depth - I'm not in a headspace to manage that right now, and besides I'm figuring you'll have caught any nasty surprises in your testing anyway :)

jmattsson · 2020-09-08T10:36:27Z

app/lua53/host/luac.c

 #include "lnodemcu.h"
 #include "lobject.h"
 #include "lstate.h"
-#include "lstring.h"
+#include "lstring.h"      // ???


I guess these three can/should be reviewed before commit?

Johny, in terms of this and all of your luac.c comments, I get the impression that the original luac.c was a "throw-in" not developed by the original Lua developers. The coding style and layout is quite different to the core Lua code. We are getting to the stage where this is heavily modified because of our LFS, Absolute LFS and execution environment support. I've pretty much decided to give this a complete hose down starting with a pretty print reformat, and getting rid of the some of the coding nasties. I'll add this to the next push. I will also reflect your general comments.

jmattsson · 2020-09-08T10:37:56Z

app/lua53/host/luac.c

 #define IROM0_SEG    0x40200000ul
 #define IROM0_SEGMAX 0x00100000ul
-#define IROM_OFFSET(a) (cast(lu_int32, (a)) - IROM0_SEG)
-
+#define IROM_OFFSET(a)(((lu_int32) (a)) - IROM0_SEG)


A space after the first close-bracket would be nice. Weren't you the proponent of using a cast macro for easier greppability in the first place btw?

I personally prefer cast() for readability. I am not sure of the rationale here, but it is defined in limits.h and this is typically pathed in via lobject.h which is included in most of the LUA_CORE modules but not the LUA_LIB so

the Lua library modules (l*lib.c) -- i.e.the ones that interface to the core through the lua.h API -- don't use it, but

the Lua core itself which access the VM internals, internal data structures, etc., do.

Standard luac.c (even though it includes object.h for the code listing function) treats itself as non-core and therefore doesn't use cast(). Confused?

Confused?

Yup! 😄

jmattsson · 2020-09-08T10:38:59Z

app/lua53/host/luac.c

    "  -v       show version information\n"
    "  --       stop handling options\n"
    "  -        stop handling options and process stdin\n", progname, Output);
  exit(EXIT_FAILURE);
 }

-#define IS(s)	(strcmp(argv[i],s)==0)
+#define IS(s)(strcmp(argv[i], s) == 0)


I don't think the space after the macro name & param should be removed. Personal view, of course.

Sorry pretty print reformatting did this. I'll go through these.

jmattsson · 2020-09-08T10:39:59Z

app/lua53/host/luac.c

-    } else if (IS("-m")) {                    /* specify a maximum image size */
-      flash = lookup = 1;
+    } else if (IS("-m")) {    /* specify a maximum image size */
+//      flash = lookup = 1;


Hmm? Looks suspicious! :D

jmattsson · 2020-09-08T10:40:28Z

app/lua53/host/luac.c

      maxSize = strtol(argv[++i], NULL, 0);
-      if (maxSize & 0xFFF)
+      if (maxSize &0xFFF)


I can't help but feel some auto-spacer has gone a bit trigger happy in places

jmattsson · 2020-09-08T10:57:24Z

app/lua53/lapi.c

+    luaH_setint(L, t, 1, L->top - 2);
+    luaC_barrierback(L, t, L->top - 2);
+    status = luaU_dump(L, t, writer, data, strip);
+    L->top--;    /* Drop able and ceck GC */


Here, have a t and an h ;)

jmattsson · 2020-09-08T11:12:39Z

app/lua53/lnodemcu.c

+  lu_int32 protoHead;     /* offset of linked list of Protos in LFS */
+  lu_int32 shortTShead;   /* offset of linked list of short TStrings in LFS */
+  lu_int32 longTShead;    /* offset of linked list of long TStrings in LFS */
+  short    nFiles;


How come short rather than an explicit 16bit type?

I will make these a lu_int16 type. The reason for the 16-bit was that I wanted to keep the header the same size if possible and need to pass and additional two integer parameters from pass 1 to pass 2 but only had 4 bytes reserved. Both are small integers so 2 × lu_int16 is fine.

jmattsson · 2020-09-08T11:13:56Z

app/lua53/lnodemcu.c

+typedef struct LFSflashState {
+  lua_State  *L;
+  LoadState  *S;
+  int         pass;               /* 1 or 2; also 0 implies 1 */


Should we be explicit about integer width here too (and in nSave below), for consistency if nothing else?

Not really. This an internal state structure (which is private to nodemcu.c. This pattern just follows Lua coding style which uses int in its structures for generic integer. The reason for the lu_int32 declarations is that these must be 32bit.

jmattsson · 2020-09-08T11:22:09Z

app/lua53/lundump.c

+** The following compression maps are used in Store() ONLY in the case of
+** 64-bit hosts to repack structs to adjust for 64-bit->32-bit points in
+** absolute mode LFS creation.  These must match the definitions in lobject.h
+*/


Thanks Terry! :)

jmattsson · 2020-09-08T11:23:44Z

app/lua53/lundump.c

+  } else if (S->mode == MODE_LFSA && pack) {
+   /*
+    * 64-bit hosts have two LFS modes. The default LFS mode is for writing to a
+    * when writing to native LFS where the address pointers are 64 bit and any


Is there a line missing here? Or is it just a mangled comment?

mangled will rewrite

galjonsfigur · 2020-09-09T19:40:59Z

As I'm not that knowledgeable in Lua internals I can only test this PR. My observations:

My GCC (10.1.1) returned some warnings when compiling this code - I don't want to paste the whole log here so I uploaded the important parts to this Gist
The -f option from luac.cross could be depreciated first instead of being removed
Old way of using LFS with _init.luawon't work even with changes from Makes lfs/_init.lua compatible with Lua 5.3 #3168 - but it's not really needed with new LFS API.

TerryE · 2020-09-09T22:06:14Z

@galjonsfigur, thanks I will add these to the TODO list. In terms of flash output, there isn't any difference between an LC file and a Flash Image file. Hence the -f option is redundant. Are you suggesting that this is retained as a deprecated synonym for -o?

galjonsfigur · 2020-09-16T20:38:15Z

Are you suggesting that this is retained as a deprecated synonym for -o?

Sorry for late reply - didn't notice it. As now -f option is redundant it could just print deprecation notice - that way it won't interfere with any existing scripts that use luac.cross

stale · 2022-04-16T06:11:09Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

First cut of new LFS for initial reviwe

7d8b13e

TerryE requested review from nwf and jmattsson September 6, 2020 01:17

TerryE self-assigned this Sep 6, 2020

TerryE changed the title ~~First cut of new LFS for initial reviwe~~ First cut of new LFS for initial review Sep 6, 2020

nwf requested changes Sep 6, 2020

View reviewed changes

This was referenced Sep 6, 2020

Support for two LFS regions and on-ESP LFS building #3206

Open

node.flashreload() Not Working #3251

Closed

jmattsson approved these changes Sep 8, 2020

View reviewed changes

TerryE mentioned this pull request Sep 11, 2020

Add modules struct, bit, color_utils and sjson to luac.cross #3230

Merged

4 tasks

nwf added this to the Next release milestone Sep 29, 2020

marcelstoer removed this from the Next release milestone Nov 3, 2020

stale bot added the stale label Apr 16, 2022

stale bot closed this Apr 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First cut of new LFS for initial review #3272

First cut of new LFS for initial review #3272

TerryE commented Sep 6, 2020

nwf left a comment

nwf Sep 6, 2020

TerryE Sep 7, 2020 •

edited

Loading

nwf Sep 6, 2020

nwf Sep 6, 2020

TerryE Sep 6, 2020

nwf Sep 6, 2020

nwf Sep 6, 2020

TerryE Sep 6, 2020

nwf Sep 12, 2020

TerryE commented Sep 6, 2020

TerryE commented Sep 7, 2020

jmattsson left a comment

jmattsson Sep 8, 2020

TerryE Sep 9, 2020

jmattsson Sep 8, 2020

TerryE Sep 10, 2020

jmattsson Sep 10, 2020

jmattsson Sep 8, 2020

TerryE Sep 10, 2020

jmattsson Sep 8, 2020

jmattsson Sep 8, 2020

jmattsson Sep 8, 2020

jmattsson Sep 8, 2020

TerryE Sep 9, 2020

jmattsson Sep 8, 2020

TerryE Sep 9, 2020

jmattsson Sep 8, 2020

jmattsson Sep 8, 2020

TerryE Sep 9, 2020

galjonsfigur commented Sep 9, 2020

TerryE commented Sep 9, 2020 •

edited

Loading

galjonsfigur commented Sep 16, 2020

stale bot commented Apr 16, 2022

First cut of new LFS for initial review #3272

First cut of new LFS for initial review #3272

Conversation

TerryE commented Sep 6, 2020

Notes

nwf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryE Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryE commented Sep 6, 2020

TerryE commented Sep 7, 2020

jmattsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galjonsfigur commented Sep 9, 2020

TerryE commented Sep 9, 2020 • edited Loading

galjonsfigur commented Sep 16, 2020

stale bot commented Apr 16, 2022

TerryE Sep 7, 2020 •

edited

Loading

TerryE commented Sep 9, 2020 •

edited

Loading