Rework snapshot generation API. #2259

zherczeg · 2018-03-28T09:48:37Z

No description provided.

martijnthe · 2018-03-28T10:05:18Z

docs/02.API-REFERENCE.md

+[jerry_register_magic_strings](#jerry_register_magic_strings) must be the
+same when the snapshot is generated and executed. Furthermore the
+`copy_bytecode` flag of [jerry_exec_snapshot_at](#jerry_exec_snapshot_at)
+must be set to `false`.


I have not yet looked at the changes to implementation, but:
Is there any detection/protection "up front" to prevent a snapshot from executing when the magic strings in the run time don't match? I'm thinking of something like a field in the snapshot header with a hash of all the magic strings, or something along those lines.

No. Static snapshots should be part of the application core binary, not something comes externally. Normal snapshots are better for that.

Yeah, that makes sense.
So what will happen now if there is a mismatch? Will it fail gracefully? (at least not crash?)

May crash if there is a magic string with higher index the current maximum index (magic_string[idx] cause buffer overflow). Other than that, the identifiers / strings will be wrong.

Maybe it makes sense to add the type of snapshot (static vs non-static vs ...) to the header of the snapshot? That way one can at least check the header and not call jerry_exec_snapshot in case it's a static snapshot and it's uncertain whether the snapshot was generated with the same set of magic strings.

Later is fine.

I don't see the snapshot header version being bumped in this PR.
I would do that at the very least?

There is no reason to do it (this is strictly an API change) unless people agree to remove the eval thing.

Anyway I noticed one difference: in strict mode, global eval does not create new variables in the global scope unless you use the this.var form (creates a new local context below the global context). This could be a reason to keep the eval.

What shall we do?

this is strictly an API change

Didn't snapshots before this change always include string literals even if they were magic strings? If so, I would argue it's also a snapshot format change in a subtle way.

in strict mode, global eval does not create new variables in the global scope unless you use the this.var form (creates a new local context below the global context). This could be a reason to keep the eval.

Hmm, but the user controls "strictness" when creating the snapshot, no? Maybe I'm misunderstanding, but isn't this more a property of strictness rather than global context vs top context?

Didn't snapshots before this change always include string literals even if they
were magic strings?

Static snapshort support was landed in another change: #2239

Would be too much for this patch.

👍 ignore me!

zherczeg · 2018-03-28T10:35:18Z

Question: shall we remove JERRY_SNAPSHOT_SAVE_EVAL ?

Reason: it does not work.

Consider the following function:
function f(a) { eval("something"); }
The engine is aware that the eval might access a, so it creates a lexical context with a binding called a.

Consider its native variant:
function f(a) { native_eval("something"); }
Here the engine does not aware of a presence of an eval, places a into a register and forgets its name. Accessing a from native_eval will result an undefined reference.

I am also not aware of a use case where it is useful.

martijnthe · 2018-03-28T14:04:35Z

Re. native_eval scope example: doesn't that apply to any use of jerry_eval in general?

zherczeg · 2018-03-29T06:47:58Z

@martijnthe yes, good point. I checked the code and realized that is_direct is set to false in both cases, which means they are evaluated globally now. Therefore there is no executing difference between calling jerry_run or jerry_eval. Furthermore the snapshot eval flag has no effect, and its description is wrong.

martijnthe · 2018-03-29T07:37:22Z

Ah, OK, it's already an indirect eval.
I'm fine with removing JERRY_SNAPSHOT_SAVE_EVAL.

LaszloLango

LGTM in general. My only concern is the too many parameters of jerry_generate_snapshot and jerry_generate_function_snapshot. They have 7 and 9 parameters respectively. IMHO we should consider to use a jerry_snapshot_ctx_t instead.

zherczeg · 2018-04-04T09:45:15Z

A context is always inconvenient to use :( The sanpshot saving is used on desktop anyway.

Also remove eval context support. It provides no practical advantage. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

martijnthe · 2018-04-04T10:53:47Z

The sanpshot saving is used on desktop anyway.

JFYI, in our case it's not (and I suspect in Fitbit's case it's also used on-device).

zherczeg · 2018-04-04T11:11:36Z

Does the device has 64K RAM?

martijnthe · 2018-04-04T11:46:10Z

I can't give any specific numbers, but in our case there's enough to do it on-device for our use-case, yes.

zherczeg · 2018-04-04T11:48:34Z

Well if you have several mbytes of ram 9 arguments should not be a problem.

martijnthe · 2018-04-04T12:08:47Z

No, it's not a problem. I'm fine with it.
Just wanted to inform you about that it's not only used on a desktop.

robertsipka

LGTM

* Add the ability to throw an error to python debugger (jerryscript-project#2188) This patch makes it possible to throw an error from the python debugger client using the `throw` command. JerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu * Fix typo and redundant text in the documentation. (jerryscript-project#2260) JerryScript-DCO-1.0-Signed-off-by: Daniel Vince vinced@inf.u-szeged.hu * Fix accessing the contents of a direct string (jerryscript-project#2261) In the `ecma_string_get_chars` method the contents of a direct string is accessed incorrectly. It tries to extract the magic string id from the string pointer but the direct string does not need this step. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com * Remove websocket message macros in debugger (jerryscript-project#2262) JERRY_DEBUGGER_INIT_SEND_MESSAGE JERRY_DEBUGGER_SET_SEND_MESSAGE_SIZE JERRY_DEBUGGER_SET_SEND_MESSAGE_SIZE_FROM_TYPE JerryScript-DCO-1.0-Signed-off-by: Jimmy Huang jimmy.huang@intel.com * Remove TARGET_HOST macros (jerryscript-project#2264) Remove TARGET_HOST defines from the jerry-libc module and replace with compiler provided macros. JerryScript-DCO-1.0-Signed-off-by: Istvan Miklos imiklos2@inf.u-szeged.hu * Fix bug in stringToCesu8 conversion for 0x7ff (jerryscript-project#2267) JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com * Add ecma_free_all_enqueued_jobs function (jerryscript-project#2265) This function releases any remaining promise job that wasn't completed. Also added this function to `jerry_cleanup ()`, therefore it will be automatically run. JerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu * Improve jerry_is_feature_enabled with object availability information (jerryscript-project#2250) JerryScript-DCO-1.0-Signed-off-by: Zsolt Raduska rzsolt@inf.u-szeged.hu * Add json parse and stringify function to jerryscript c api (jerryscript-project#2243) JerryScript-DCO-1.0-Signed-off-by: Zsolt Raduska rzsolt@inf.u-szeged.hu * Simplify debugger-ws.h (jerryscript-project#2266) Remove several macros and types from it. This change simplifies the external debugger interface. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com * Add finalize_cb to jerry_context_data_manager_t (jerryscript-project#2269) This patch adds a new finalize_cb callback to jerry_context_data_manager_t. The callback is run as the very last thing in jerry_cleanup, after the VM has been torn down entirely. There was already the deinit_cb, which is run while the VM is still in the process of being torn down. The reason the deinit_cb is not always sufficient is that there may still be objects alive (because they still being referenced) that have native pointers associated with the context manager that is being deinit'ed. As a result, the free_cb's for those objects can get called *after* the associated context manager's deinit_cb is run. This makes cleanup of manager state that is depended on by the live objects impossible to do in the deinit_cb. That type of cleanup can only be done when all values have been torn down completely. JerryScript-DCO-1.0-Signed-off-by: Martijn The martijn.the@intel.com * Rework snapshot generation API. (jerryscript-project#2259) Also remove eval context support. It provides no practical advantage. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com * Implement the ES2015 version of Object.getPrototypeOf and add a test file for it (jerryscript-project#2256) JerryScript-DCO-1.0-Signed-off-by: Peter Marki marpeter@inf.u-szeged.hu * Move DevTools integration code into jerry-client-ts subdir JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com * Fix some things to match the new directory for TS code JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com

Also remove eval context support. It provides no practical advantage. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

zherczeg mentioned this pull request Mar 28, 2018

JerryScript 2.0 #2213

Closed

LaszloLango added snapshot Related to the snapshot feature api Related to the public API labels Mar 28, 2018

LaszloLango added this to the Release 2.0 milestone Mar 28, 2018

martijnthe reviewed Mar 28, 2018

View reviewed changes

zherczeg force-pushed the snapshot_api branch from 3819c17 to 47e1ae6 Compare March 28, 2018 14:10

zherczeg force-pushed the snapshot_api branch from 47e1ae6 to d377a4b Compare March 29, 2018 07:00

zherczeg force-pushed the snapshot_api branch 6 times, most recently from ac98c37 to 9568c3e Compare April 4, 2018 08:05

LaszloLango approved these changes Apr 4, 2018

View reviewed changes

Rework snapshot generation API.

a1b7c3a

Also remove eval context support. It provides no practical advantage. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

zherczeg force-pushed the snapshot_api branch from 9568c3e to a1b7c3a Compare April 4, 2018 09:54

robertsipka approved these changes Apr 5, 2018

View reviewed changes

zherczeg merged commit 7b226f5 into jerryscript-project:master Apr 5, 2018

zherczeg deleted the snapshot_api branch April 5, 2018 11:46

Rework snapshot generation API. #2259

Rework snapshot generation API. #2259

Uh oh!

Conversation

zherczeg commented Mar 28, 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!

zherczeg Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnthe commented Mar 28, 2018

Uh oh!

zherczeg commented Mar 29, 2018

Uh oh!

martijnthe commented Mar 29, 2018

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

zherczeg commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

zherczeg commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

robertsipka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zherczeg Mar 29, 2018 •

edited

Loading

zherczeg commented Mar 28, 2018 •

edited

Loading