Update the debugger to work on Windows #2684

galpeter · 2019-01-08T17:17:10Z

Changed the debugger-tcp.c file to include Windows specific
socket handling code.

Added appveyor configuration to build with and without debugger.

galpeter · 2019-01-08T17:29:30Z

Currently on newer gcc there is a bit of problem:

jerry-ext/debugger/debugger-tcp.c: In function ‘jerryx_debugger_tcp_log_error’:
jerry-ext/debugger/debugger-tcp.c:103:3: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
   JERRYX_ERROR_MSG (format, strerror (err_val));

Currently checking on how this can be fixed.

appveyor.yml

jerry-ext/debugger/debugger-tcp.c

galpeter · 2019-01-10T12:34:53Z

I've updated the PR.

rerobika

Good patch, only minor things.

jerry-ext/debugger/debugger-tcp.c

galpeter · 2019-01-14T10:19:42Z

@rerobika I've updated the PR.

zherczeg

How much % of the file changed? Is it worth to keep this in 1 file?

jerry-ext/debugger/debugger-tcp.c

rerobika · 2019-01-14T10:25:46Z

jerry-ext/debugger/debugger-tcp.c

-#include <arpa/inet.h>
 #include <errno.h>
+
+#if defined (WIN32)


Nitpicking: All over the PR this is used like #ifdef WIN32. Could we unify it?

galpeter · 2019-01-14T10:37:33Z

@zherczeg the biggest change is the creation of the jerryx_debugger_tcp_configure_socket method which was extracted from the jerryx_debugger_tcp_create method and this new method is the same on all platform. Almost all of the original methods only have one or two minor modifications (aside from the previously mentioned one).

akosthekiss · 2019-01-14T19:28:48Z

jerry-ext/CMakeLists.txt

 target_link_libraries(${JERRY_EXT_NAME} jerry-core)

+if(USING_MSVC AND FEATURE_DEBUGGER)
+  target_link_libraries(${JERRY_EXT_NAME} ws2_32.lib)


Let's state it upfront, I'm not completely familiar with cmake on windows (although I had to play with it a bit): but is the .lib suffix/extension needed for target_link_libraries? Isn't that function working with "library names" and appends/prepends whatever is mandated by the platform/os? (E.g., name "xxx" gets converted to libxxx.a, libxxx.so, xxx.lib, xxx.dll, etc. as needed?)

True, there is no need for the .lib extension.

akosthekiss · 2019-01-14T19:36:19Z

jerry-ext/debugger/debugger-tcp.c

 */
 static void
-jerryx_debugger_tcp_log_error (int err_val)
+jerryx_debugger_tcp_log_error (const char *prefix, /**< message prefix */


minor: is the prefix really necessary? Are not all errors TCP-related so that it shouldn't be hardcoded? This is the TCP transport implementation after all.

Initially I wanted to use the simple TCP error hard-coded prefix, but in this variant I've followed the original Error/TCP Error prefixes. I'm happy to change this to the "TCP Error" prefix.

I see. Well, then, it's no big deal. I vote for a single prefix but whatever majority votes for should prevail.

akosthekiss · 2019-01-15T00:34:30Z

jerry-ext/debugger/debugger-tcp.c

  }

-  int opt_value = 1;
+  if (bind (server_socket, (struct sockaddr *)&addr, sizeof (struct sockaddr)) != 0)


According to the docs of bind the 3rd arg should be the size of the 2nd arg. But addr is of type struct sockaddr_in, not struct sockaddr.

Ahh yeah. Currently it works as the sockaddr_in can be considered as a "subclass" of the sockaddr and they have the same size (at least on linux). But will change it to sizeof (struct sockaddr_in)

akosthekiss · 2019-01-15T00:45:19Z

jerry-ext/debugger/debugger-tcp.c

  {
-    JERRYX_ERROR_MSG ("Error: %s\n", strerror (errno));
+    int error = jerryx_debugger_tcp_get_errno ();
+    jerryx_debugger_tcp_log_error ("Error", error);


These two lines can be merged. There is no need for a temp var.

zherczeg

LGTM

Changed the debugger-tcp.c file to include Windows specific socket handling code and mode user all components are compilable for Windows. Added appveyor configuration to build with and without debugger. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com

rerobika

LGTM (informal)

akosthekiss

LGTM

galpeter added the debugger Related to the debugger label Jan 8, 2019

galpeter force-pushed the debugger_win branch 4 times, most recently from b797774 to 52dc2e6 Compare January 9, 2019 16:18

akosthekiss reviewed Jan 9, 2019

View reviewed changes

appveyor.yml Outdated Show resolved Hide resolved

jerry-ext/debugger/debugger-tcp.c Outdated Show resolved Hide resolved

galpeter force-pushed the debugger_win branch 2 times, most recently from 5a067a2 to f5921cd Compare January 10, 2019 12:34

rerobika requested changes Jan 11, 2019

View reviewed changes

jerry-ext/debugger/debugger-tcp.c Outdated Show resolved Hide resolved

jerry-ext/debugger/debugger-tcp.c Outdated Show resolved Hide resolved

jerry-ext/debugger/debugger-tcp.c Show resolved Hide resolved

galpeter force-pushed the debugger_win branch from f5921cd to e2d380a Compare January 11, 2019 13:20

zherczeg reviewed Jan 14, 2019

View reviewed changes

jerry-ext/debugger/debugger-tcp.c Show resolved Hide resolved

rerobika reviewed Jan 14, 2019

View reviewed changes

akosthekiss reviewed Jan 14, 2019

View reviewed changes

akosthekiss requested changes Jan 15, 2019

View reviewed changes

zherczeg approved these changes Jan 15, 2019

View reviewed changes

galpeter force-pushed the debugger_win branch from e2d380a to 827c78a Compare January 16, 2019 15:12

galpeter force-pushed the debugger_win branch from 827c78a to 1a0ac06 Compare January 16, 2019 15:24

rerobika approved these changes Jan 17, 2019

View reviewed changes

akosthekiss approved these changes Jan 17, 2019

View reviewed changes

akosthekiss added jerry-port Related to the port API or the default port implementation jerry-ext Related to the jerry-ext library labels Jan 17, 2019

akosthekiss merged commit 545c6c7 into jerryscript-project:master Jan 17, 2019

akosthekiss mentioned this pull request Jan 17, 2019

Add pkg-config metadata files for libraries #2680

Merged

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

Update the debugger to work on Windows #2684

Update the debugger to work on Windows #2684

Uh oh!

Conversation

galpeter commented Jan 8, 2019

Uh oh!

galpeter commented Jan 8, 2019

Uh oh!

Uh oh!

Uh oh!

galpeter commented Jan 10, 2019

Uh oh!

rerobika left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

galpeter commented Jan 14, 2019

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter commented Jan 14, 2019

Uh oh!

akosthekiss Jan 14, 2019 • 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

rerobika left a comment

Choose a reason for hiding this comment

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akosthekiss Jan 14, 2019 •

edited

Loading