Added literal list loading feature to the snapshot tool. #2422

LaszloLango · 2018-07-12T12:39:14Z

It is needed to load literals and register them as magic strings
to be able to generate static snapshots. Also modified the list
format saving feature to save all of the literals not only the
identifiers.

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

zherczeg · 2018-07-12T12:45:16Z

jerry-core/lit/lit-magic-strings.c

-    lit_utf8_size_t string_size = lit_zt_utf8_string_size (lit_get_magic_string_ex_utf8 (id));
-    JERRY_ASSERT (JERRY_CONTEXT (lit_magic_string_ex_sizes)[id] == string_size);
-    JERRY_ASSERT (JERRY_CONTEXT (lit_magic_string_ex_sizes)[id] <= LIT_MAGIC_STRING_LENGTH_LIMIT);
+    lit_utf8_size_t string_size = JERRY_CONTEXT (lit_magic_string_ex_sizes)[id];


While the change is ok, I would keep the asserts in some form. They provide useful checks (maxlimit, cesu8 format).

What should be the limit? I don't know why was it constrained to 32. Do you know the reason?
lit_zt_utf8_string_size cannot be used here, because strings can contain zeros.

Only 32? that is new to me. Btw it would be good to remove the whole macro then.
No, you can trust the size provided by the user, but you should check that the string is valid cesu8.

I've added the check. Please check.

zherczeg · 2018-07-12T12:48:33Z

jerry-main/main-unix-snapshot.c

+      {
+        char* sp_buffer_end_p = NULL;
+        jerry_length_t mstr_size = (jerry_length_t) strtol (sp_buffer_p, &sp_buffer_end_p, 10);
+        if (mstr_size > 0)


Can it be 0?

Sanity check, but it can when the input file contains empty lines at the end.

Shouldn't we detect that before calling strtol? What happens for invalid numbers?

Do you mean something like this?

if ( '0' > *sp_buffer_p || *sp_buffer_p > '9') { break; }

Yes. The format must be:
[0-9]+\s and a valid cesu8 string which length is the number followed by a newline

If the newline is not present, the file must end.
If the file ends right before the number, that is valid as well.

zherczeg · 2018-07-12T12:48:51Z

jerry-main/main-unix-snapshot.c

+          num_of_lit++;
+        }
+        sp_buffer_p = sp_buffer_end_p + mstr_size + 1;
+      } while ((size_t)(sp_buffer_p - (char*) sp_buffer_start_p) < sp_buffer_size);


Shall we do validation here?

e.g: JERRY_ASSERT (sp_buffer_p > (char*) sp_buffer_start_p) ?

I mean the items are correct and correctly ordered. This is a tool, so even expensive validation is justified here.

jerry_register_magic_strings does the verification

Only in debug mode. This is not a hard requirement, but I wouldn't mind doing a real check here, since it is not a difficult thing. Users may accidentally pass random files to this option.

zherczeg · 2018-07-12T12:49:35Z

jerry-main/main-unix-snapshot.c

      {
-        if (save_literals_file_name_p != NULL)
+        if (literals_file_name_p != NULL)


Cannot we both load and save literals?

there is no make sense

Why? The loaded and saved list can be different. Although loading probably used only with static snapshots.

I can change this, but it is not a valid use case. It will never happen to use them in the same command.

Ok keep it this way for now.

LaszloLango · 2018-07-13T07:42:30Z

I've updated the PR.

It is needed to load literals and register them as magic strings to be able to generate static snapshots. Also modified the list format saving feature to save all of the literals not only the identifiers. JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

zherczeg

LGTM

yichoi

LGTM

LaszloLango added development Feature implementation snapshot Related to the snapshot feature labels Jul 12, 2018

zherczeg reviewed Jul 12, 2018

View reviewed changes

zherczeg requested changes Jul 12, 2018

View reviewed changes

LaszloLango force-pushed the static-snapshot-dev branch from b9d238f to 890a226 Compare July 13, 2018 07:05

LaszloLango force-pushed the static-snapshot-dev branch from 890a226 to 5d9f5da Compare July 13, 2018 08:15

zherczeg approved these changes Jul 13, 2018

View reviewed changes

yichoi approved these changes Jul 16, 2018

View reviewed changes

yichoi merged commit 5f4a220 into jerryscript-project:master Jul 16, 2018

LaszloLango deleted the static-snapshot-dev branch September 20, 2018 14:56

Added literal list loading feature to the snapshot tool. #2422

Added literal list loading feature to the snapshot tool. #2422

Uh oh!

Conversation

LaszloLango commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango commented Jul 13, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

yichoi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!