Skip to content

Conversation

@knightburton
Copy link
Member

With this feature the use can restart the actual debug session
(similar to the multiple source context reset) within a client.

JerryScript-DCO-1.0-Signed-off-by: Imre Kiss kissi.szeged@partner.samsung.com

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.

What happens if client source is enabled and a r353t comes?

register_js_function ("assert", jerryx_handler_assert);
register_js_function ("gc", jerryx_handler_gc);
register_js_function ("print", jerryx_handler_print);
if (jerry_is_feature_enabled (JERRY_FEATURE_DEBUGGER))
Copy link
Member

Choose a reason for hiding this comment

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

if (jerry_is_feature_enabled (JERRY_FEATURE_DEBUGGER))
    && jerry_value_is_abort (ret_value))

break;
}

jerry_cleanup ();
Copy link
Member

Choose a reason for hiding this comment

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

What happens with ret_value?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ret_value is released in case of "r353t" match, otherwise that value is needed in the global error checking, around line 840.


register_js_function ("assert", jerryx_handler_assert);
register_js_function ("gc", jerryx_handler_gc);
register_js_function ("print", jerryx_handler_print);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a code duplication (init engine). Would be good to move it into a static function.

@knightburton
Copy link
Member Author

@zherczeg I've updated the PR.
If the --debugger-wait-source is enabled and the r353t abort is received (or any other abort) the engine closes the connection with No more client source message. Is this good for us or not?

@zherczeg
Copy link
Member

Closes the connection and restart jerry and wait for sources again? Sounds ok.

@knightburton
Copy link
Member Author

@zherczeg I've updated the PR, now the restart works properly in wait mode, too.

jerry_value_t str_val = jerry_value_to_string (abort_value);
jerry_size_t str_size = jerry_get_string_size (str_val);
jerry_size_t str_end = jerry_string_to_char_buffer (str_val, str_buf, str_size);
assert (str_end == str_size);
Copy link
Member

@zherczeg zherczeg Jun 21, 2018

Choose a reason for hiding this comment

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

No assert please!
Abort can contain any string. Just check if str size is 5, and compare it.

assert (str_end == str_size);
str_buf[str_end] = 0;

if (strcmp ("r353t", (const char *) (str_buf)) == 0)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer memcmp here. It would make str_buf[str_end] = 0; unnecessary.

@knightburton
Copy link
Member Author

@zherczeg done, removed the assert and used memcmp instead of strcmp.

jerry_char_t str_buf[6];
jerry_value_t str_val = jerry_value_to_string (abort_value);
jerry_size_t str_size = jerry_get_string_size (str_val);
jerry_size_t str_end = jerry_string_to_char_buffer (str_val, str_buf, str_size);
Copy link
Member

@zherczeg zherczeg Jun 21, 2018

Choose a reason for hiding this comment

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

Better but this is still not 100%

+        jerry_size_t str_size = jerry_get_string_size (str_val);
+        jerry_size_t str_end = jerry_string_to_char_buffer (str_val, str_buf, str_size);

Please check the str_size before jerry_string_to_char_buffer because this might be a memory overwrite.

I also think that jerry_size_t str_end is unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right.

With this feature the use can restart the actual debug session
(similar to the multiple source context reset) within a client.

JerryScript-DCO-1.0-Signed-off-by: Imre Kiss kissi.szeged@partner.samsung.com
@knightburton
Copy link
Member Author

@zherczeg I've updated the PR.

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

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 e326588 into jerryscript-project:master Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants