Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
yichoi merged 1 commit into from
Jul 16, 2018
Merged

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

yichoi merged 1 commit into from
Jul 16, 2018

Conversation

@LaszloLango
Copy link
Contributor

@LaszloLango LaszloLango commented Jul 12, 2018

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ó llango.u-szeged@partner.samsung.com

@LaszloLango LaszloLango added development Feature implementation snapshot Related to the snapshot feature labels Jul 12, 2018
zherczeg
zherczeg reviewed Jul 12, 2018
View reviewed changes
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];
Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 13, 2018

Choose a reason for hiding this comment

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

I've added the check. Please check.

zherczeg
zherczeg requested changes Jul 12, 2018
View reviewed changes
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)
Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

Can it be 0?

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

Do you mean something like this?

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

Copy link
Member

@zherczeg zherczeg Jul 13, 2018

Choose a reason for hiding this comment

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

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.

jerry-main/main-unix-snapshot.c Outdated
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);
Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

Shall we do validation here?

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

jerry_register_magic_strings does the verification

Copy link
Member

@zherczeg zherczeg Jul 13, 2018

Choose a reason for hiding this comment

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

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.

jerry-main/main-unix-snapshot.c
case OPT_IMPORT_LITERAL_LIST:
{
if (save_literals_file_name_p != NULL)
if (literals_file_name_p != NULL)
Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

Cannot we both load and save literals?

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

there is no make sense

Copy link
Member

@zherczeg zherczeg Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@LaszloLango LaszloLango Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@zherczeg zherczeg Jul 13, 2018

Choose a reason for hiding this comment

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

Ok keep it this way for now.

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

LaszloLango commented Jul 13, 2018

I've updated the PR.

@LaszloLango
Added literal list loading feature to the snapshot tool.
5d9f5da 
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ó llango.u-szeged@partner.samsung.com
@LaszloLango LaszloLango force-pushed the static-snapshot-dev branch from 890a226 to 5d9f5da Compare July 13, 2018 08:15
zherczeg
zherczeg approved these changes Jul 13, 2018
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

yichoi
yichoi approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@yichoi yichoi left a comment

Choose a reason for hiding this comment

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

LGTM

@yichoi yichoi merged commit 5f4a220 into jerryscript-project:master Jul 16, 2018
@LaszloLango LaszloLango deleted the static-snapshot-dev branch September 20, 2018 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zherczeg zherczeg zherczeg approved these changes

@yichoi yichoi yichoi approved these changes

Assignees

No one assigned

Labels

development Feature implementation snapshot Related to the snapshot feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LaszloLango @zherczeg @yichoi