Skip to content

Simplify debugger-ws.h #2266

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.

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

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

zherczeg
Copy link
Member

@zherczeg zherczeg commented Apr 3, 2018

Remove several macros and types from it. This change simplify the required debugger interface.

@martijnthe
Copy link
Contributor

cc @jimmy-huang

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

uint8_t max_send_size = (JERRY_DEBUGGER_MAX_BUFFER_SIZE - JERRY_DEBUGGER_WEBSOCKET_HEADER_SIZE);
if (max_send_size > 125)
{
max_send_size = 125;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add #define for these magics + a comment why it is 125?

@@ -453,6 +488,7 @@ bool
jerry_debugger_receive (jerry_debugger_uint8_data_t **message_data_p) /**< [out] data received from client */
{
JERRY_ASSERT (JERRY_CONTEXT (debugger_flags) & JERRY_DEBUGGER_CONNECTED);
JERRY_ASSERT (JERRY_CONTEXT (debugger_max_receive_size) <= 125);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same magic again.

uint8_t type; /**< type of the message */
jerry_debugger_frame_t frames[JERRY_DEBUGGER_SEND_MAX (jerry_debugger_frame_t)]; /**< frames */
jerry_debugger_frame_t frames[1]; /**< frames */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why [1] and not [] (also in the other structs)?
Aren't these variable sized arrays?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the latter is not supported by all C compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that part of C99 (the README claims that the project uses this standard)?

https://en.wikipedia.org/wiki/Flexible_array_member

In previous standards of the C language, it was common to declare a zero-sized array member instead of a flexible array member. The GCC compiler explicitly accepts zero-sized arrays for such purposes.

Maybe use that instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the problem with 1?

Copy link
Contributor

@martijnthe martijnthe Apr 4, 2018

Choose a reason for hiding this comment

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

Static analyzers that perform bounds checking on array member access may fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which one? Coverity has no problem with it:
https://travis-ci.org/jerryscript-project/jerryscript/jobs/361615057

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized that the definition is wrong (non-packed form), and it only works because it is never used. Perhaps we could replace the second member with a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I finally chose []. I hope it is really part of C99.

{
string_length += 1;
string_length -= max_byte_count;
string_p += max_byte_count;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nice catch

Remove several macros and types from it. This change simplify
the required debugger interface.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
@zherczeg zherczeg force-pushed the simplify_debugger_ws branch from bc46a84 to 04cd233 Compare April 4, 2018 11:58
@martijnthe
Copy link
Contributor

@zherczeg thanks for helping with this.

Now that the websocket related stuff is almost separated out as a separate layer, I'm starting to wonder whether it would make sense to not regard WS to be part of the JrS debug protocol. That way we can save some of the overhead for bandwidth-constraint transports like BLE.

The websocket transport would on its turn rely on a "lower transport" that has a platform specific implementation (yet another "port" interface).

Thoughts?

cc @jimmy-huang

@zherczeg
Copy link
Member Author

zherczeg commented Apr 5, 2018

Exactly my plan.

Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

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

LGTM

@zherczeg zherczeg merged commit 27939f0 into jerryscript-project:master Apr 5, 2018
@zherczeg zherczeg deleted the simplify_debugger_ws branch April 5, 2018 09:22
martijnthe pushed a commit to martijnthe/jerryscript that referenced this pull request Apr 6, 2018
* 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 [email protected]

* Fix typo and redundant text in the documentation. (jerryscript-project#2260)

JerryScript-DCO-1.0-Signed-off-by: Daniel Vince [email protected]

* 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 [email protected]

* 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 [email protected]

* 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 [email protected]

* Fix bug in stringToCesu8 conversion for 0x7ff (jerryscript-project#2267)

JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson [email protected]

* 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 [email protected]

* Improve jerry_is_feature_enabled with object availability information (jerryscript-project#2250)

JerryScript-DCO-1.0-Signed-off-by: Zsolt Raduska [email protected]

* Add json parse and stringify function to jerryscript c api (jerryscript-project#2243)

JerryScript-DCO-1.0-Signed-off-by: Zsolt Raduska [email protected]

* 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 [email protected]

* 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 [email protected]

* 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 [email protected]

* 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 [email protected]

* Move DevTools integration code into jerry-client-ts subdir

JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson [email protected]

* Fix some things to match the new directory for TS code

JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson [email protected]
jimmy-huang pushed a commit to jimmy-huang/jerryscript that referenced this pull request May 8, 2018
Remove several macros and types from it. This change simplifies
the external debugger interface.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
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.

4 participants