Fix MinGW compilation errors in jerry-ext #4510

lygstate · 2021-01-19T00:42:31Z

mingw also need linkage to ws2_32, try define socket types for unix/win32 for compat reason
and fixes #4512

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

jerry-ext/CMakeLists.txt

jerry-ext/debugger/debugger-tcp.c

jerry-ext/handle-scope/handle-scope-allocator.c

akosthekiss

And, please, remove the build log from the commit message. That's yet another thing that should go into a comment only.

akosthekiss · 2021-01-20T12:49:58Z

jerry-ext/debugger/debugger-tcp.c

-typedef SOCKET jerryx_socket;
-
+/*
+ * On Windows, socket functions has the following signature:


(minor) ... functions have ...

(minor) ... functions have ...

why? it's socket related functions

fix with socket related functions

Because that's how the English grammar works. Plural goes with "have". "Functions" is plural. The proper sentence should read as "On Windows, socket functions have the following signature:" (or "signatures")

Thanks, I fixed it with signatures

akosthekiss · 2021-01-20T12:50:12Z

jerry-ext/debugger/debugger-tcp.c

-/* On *nix the sockets are integer identifiers */
-typedef int jerryx_socket;
+/*
+ * On *nix, socket functions has the following signature:


(same here)

akosthekiss · 2021-01-20T12:52:53Z

jerry-ext/debugger/debugger-tcp.c

@@ -167,7 +184,10 @@ jerryx_debugger_tcp_send (jerry_debugger_transport_header_t *header_p, /**< tcp
    }
 #endif /* __linux__ */

-    ssize_t sent_bytes = send (tcp_p->tcp_socket, message_p, message_length, 0);
+    jerryx_socket_return_t sent_bytes = send (tcp_p->tcp_socket,
+                                              (const jerryx_socket_buffer_t) message_p,


I'm not sure that the const qualifier is necessary here, please double check.

ssize_t send(int sockfd, const void *buf, size_t len, int flags);

I'm not sure that the const qualifier is necessary here, please double check.

indeed message_p should be const uint8_t*, so that user can use send string literal, even though in jerry there is no such usage

I'm not sure I get your argument. Did you try it (as requested, see "please double check" above)? I've just fetched this PR branch and dropped these consts from the type casts, and everything compiled fine. As it should IMO. That const modifier in the signature of send prevents send from altering the contents of the buffer -- but does not prevent the caller of send from passing in a non-const buffer.

I'm not sure I get your argument. Did you try it (as requested, see "please double check" above)? I've just fetched this PR branch and dropped these consts from the type casts, and everything compiled fine. As it should IMO. That const modifier in the signature of send prevents send from altering the contents of the buffer -- but does not prevent the caller of send from passing in a non-const buffer.

Yeap, but when doing type cast, it's better to match the calle site, not the caller site, that's would misleading.
We do cast meant to match the called function's signature,

akosthekiss · 2021-01-20T12:59:40Z

jerry-ext/debugger/debugger-tcp.c

@@ -252,7 +275,7 @@ jerryx_debugger_tcp_configure_socket (jerryx_socket server_socket, /** < socket

  int opt_value = 1;

-  if (setsockopt (server_socket, SOL_SOCKET, SO_REUSEADDR, &opt_value, sizeof (int)) != 0)
+  if (setsockopt (server_socket, SOL_SOCKET, SO_REUSEADDR, (const jerryx_socket_opt_t) &opt_value, sizeof (int)) != 0)


Not sure about const here either.

int setsockopt(SOCKET s, int level, int optname, const char *optval, int optlen);

it's set, and get, so const are necessary.

can we merge
typedef char *jerryx_socket_buffer_t;
typedef char *jerryx_socket_opt_t;

maybe
typedef char jerrx_socket_void_t?

then we use (const jerrx_socket_void_t *) instead.

How about

/* * On Windows, socket related functions has the following signature: * int send(SOCKET s, const char *buf, int len, int flags); * int recv(SOCKET s, char *buf, int len, int flags); * int setsockopt(SOCKET s, int level, int optname, const char *optval, int optlen); */ typedef int jerryx_socket_ssize_t; typedef SOCKET jerryx_socket_t; typedef char jerryx_socket_void_t; typedef int jerryx_socket_size_t; typedef int jerryx_socket_len_t;

and

/* * On *nix, socket related functions has the following signature: * ssize_t send(int sockfd, const void *buf, size_t len, int flags); * ssize_t recv(int sockfd, void *buf, size_t len, int flags); * int setsockopt(int sockfd, int level, int optname, const void *optval, socklen_t optlen); */ typedef ssize_t jerryx_socket_ssize_t; typedef int jerryx_socket_t; typedef void jerryx_socket_void_t; typedef size_t jerryx_socket_size_t; typedef socklen_t jerryx_socket_len_t;

akosthekiss · 2021-01-20T13:00:18Z

jerry-ext/debugger/debugger-tcp.c

+typedef char *jerryx_socket_buffer_t;
+typedef int jerryx_socket_buffer_length_t;
+typedef char *jerryx_socket_opt_t;
+typedef int jerryx_socket_len_t;


this type is unused, should be removed

this type is unused, should be removed

forgot refer to it

This type is only used to cast (jerryx_socket_len_t) sizeof (int). The sizeof (int) is a compile-time constant, and most probably / it seems is compatible both with Windows's int and *nix's socklen_t. If that's so -- it is -- then casting is unnecessary. Code should not be cluttered with unnecessary casts. Thus, the type is not needed. Just remove it.

akosthekiss · 2021-01-20T21:56:04Z

jerry-ext/debugger/debugger-tcp.c

+typedef char *jerryx_socket_buffer_t;
+typedef int jerryx_socket_buffer_length_t;
+typedef char *jerryx_socket_opt_t;
+typedef int jerryx_socket_len_t;


This type is only used to cast (jerryx_socket_len_t) sizeof (int). The sizeof (int) is a compile-time constant, and most probably / it seems is compatible both with Windows's int and *nix's socklen_t. If that's so -- it is -- then casting is unnecessary. Code should not be cluttered with unnecessary casts. Thus, the type is not needed. Just remove it.

akosthekiss · 2021-01-20T22:00:28Z

jerry-ext/debugger/debugger-tcp.c

@@ -167,7 +182,10 @@ jerryx_debugger_tcp_send (jerry_debugger_transport_header_t *header_p, /**< tcp
    }
 #endif /* __linux__ */

-    ssize_t sent_bytes = send (tcp_p->tcp_socket, message_p, message_length, 0);
+    jerryx_socket_ssize_t sent_bytes = send (tcp_p->tcp_socket,
+                                              (const jerryx_socket_void_t *) message_p,


Let me write it again, drop the const qualifier from the cast. The const qualifier in the signature of send is a guarantee that it will not modify the contents of the buffer, not a requirement to the caller to pass in an unmodifiable buffer.

as you insist, then i remove the const,
but consider this
if people modify the message_p parameter from uint8_t * to const uint8_t *,
then this cast will hidden the compiling error, doing this just hidding things

If you rewrite the signature of the function to take message_p as a pointer to const (and/or rewrite the opt value to be a const int) then I'll accept a const cast.

akosthekiss · 2021-01-20T23:02:13Z

jerry-ext/debugger/debugger-tcp.c

+    jerryx_socket_ssize_t sent_bytes = send (tcp_p->tcp_socket,
+                                              (jerryx_socket_void_t *) message_p,
+                                              (jerryx_socket_size_t) message_length,
+                                              0);


(minor style) indentation is off by one space (the last three lines should start where tcp_p->... starts on the first line)

akosthekiss · 2021-01-20T23:03:00Z

jerry-ext/debugger/debugger-tcp.c

+  jerryx_socket_ssize_t length = recv (tcp_p->tcp_socket,
+                                        (jerryx_socket_void_t *) buffer_p,
+                                        (jerryx_socket_size_t) buffer_size,
+                                        0);


(minor style) indentation is off by one space

akosthekiss · 2021-01-21T08:24:42Z

jerry-ext/debugger/debugger-tcp.c

@@ -167,7 +181,8 @@ jerryx_debugger_tcp_send (jerry_debugger_transport_header_t *header_p, /**< tcp
    }
 #endif /* __linux__ */

-    ssize_t sent_bytes = send (tcp_p->tcp_socket, message_p, message_length, 0);
+    const jerryx_socket_void_t *message_to_send_p = (const jerryx_socket_void_t *) message_p;
+    jerryx_socket_ssize_t sent_bytes = send (tcp_p->tcp_socket, message_to_send_p, message_to_send_length, 0);


This was not what the previous review was about. I've suggested to change the signature of jerryx_debugger_tcp_send to have its parameter as uint8_t *message_p. Then, send can be called as send (tcp_p->tcp_socket, (const jerry_socket_void_t *) message_p, message_to_send_length, 0);

This was not what the previous review was about. I've suggested to change the signature of jerryx_debugger_tcp_send to have its parameter as uint8_t *message_p. Then, send can be called as send (tcp_p->tcp_socket, (const jerry_socket_void_t *) message_p, message_to_send_length, 0);

Tried but failed, jerryx_debugger_tcp_send parameter can not changed to const uint8_t *message_p.
It's will cause new compiling error, because websocket modify the content of message_p

Use (jerryx_socket_void_t *) message_p, din't want modify here anymore

akosthekiss · 2021-01-21T08:25:47Z

jerry-ext/debugger/debugger-tcp.c

@@ -250,9 +267,10 @@ jerryx_debugger_tcp_configure_socket (jerryx_socket server_socket, /** < socket
  addr.sin_port = htons (port);
  addr.sin_addr.s_addr = INADDR_ANY;

-  int opt_value = 1;
+  const int opt_value = 1;
+  const jerryx_socket_void_t *opt_value_p = (const jerryx_socket_void_t *) &opt_value;


This variable is unnecessary. Do the cast in the call to setsockopt.

akosthekiss · 2021-01-21T08:28:51Z

jerry-ext/debugger/debugger-tcp.c

@@ -152,6 +165,7 @@ jerryx_debugger_tcp_send (jerry_debugger_transport_header_t *header_p, /**< tcp
  JERRYX_ASSERT (jerry_debugger_transport_is_connected ());

  jerryx_debugger_transport_tcp_t *tcp_p = (jerryx_debugger_transport_tcp_t *) header_p;
+  jerryx_socket_size_t message_to_send_length = (jerryx_socket_size_t) message_length;


(minor) This is a bit long variable name. Let's look for something simpler but still descriptive. Could be remaining_bytes, as there is already a variable sent_bytes.

mingw also need linkage to ws2_32, try define socket types for unix/win32 for compatibility reason and fixes jerryscript-project#4512 JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

akosthekiss

LGTM

akosthekiss requested changes Jan 19, 2021

View reviewed changes

jerry-ext/CMakeLists.txt Outdated Show resolved Hide resolved

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

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

jerry-ext/handle-scope/handle-scope-allocator.c Outdated Show resolved Hide resolved

akosthekiss requested changes Jan 19, 2021

View reviewed changes

akosthekiss added windows Windows specific jerry-ext Related to the jerry-ext library labels Jan 19, 2021

akosthekiss changed the title ~~fixes mingw compiling errors~~ Fix MinGW compilation errors in jerry-ext Jan 19, 2021

lygstate force-pushed the mingw-fix branch 3 times, most recently from 24b7fbc to 0157129 Compare January 19, 2021 18:30

lygstate requested a review from akosthekiss January 19, 2021 21:04

rerobika approved these changes Jan 20, 2021

View reviewed changes

lygstate force-pushed the mingw-fix branch from 0157129 to bd45992 Compare January 20, 2021 09:43

akosthekiss requested changes Jan 20, 2021

View reviewed changes

lygstate force-pushed the mingw-fix branch 3 times, most recently from cfc0560 to 0c17bd1 Compare January 20, 2021 13:47

lygstate requested a review from akosthekiss January 20, 2021 14:03

lygstate force-pushed the mingw-fix branch 2 times, most recently from 114411f to 0d9df24 Compare January 20, 2021 16:15

akosthekiss requested changes Jan 20, 2021

View reviewed changes

lygstate force-pushed the mingw-fix branch from 0d9df24 to 25a85a9 Compare January 20, 2021 22:11

akosthekiss requested changes Jan 20, 2021

View reviewed changes

lygstate force-pushed the mingw-fix branch 3 times, most recently from c15f5b2 to 0bf0973 Compare January 21, 2021 07:07

akosthekiss requested changes Jan 21, 2021

View reviewed changes

lygstate force-pushed the mingw-fix branch 2 times, most recently from 2434d90 to 2807b7c Compare January 21, 2021 08:45

Fix MinGW compilation errors in jerry-ext

9bac917

mingw also need linkage to ws2_32, try define socket types for unix/win32 for compatibility reason and fixes jerryscript-project#4512 JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

lygstate force-pushed the mingw-fix branch from 2807b7c to 9bac917 Compare January 21, 2021 08:47

akosthekiss approved these changes Jan 22, 2021

View reviewed changes

akosthekiss merged commit 1db0905 into jerryscript-project:master Jan 22, 2021

lygstate deleted the mingw-fix branch January 23, 2021 03:48

Fix MinGW compilation errors in jerry-ext #4510

Fix MinGW compilation errors in jerry-ext #4510

Uh oh!

Conversation

lygstate commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akosthekiss left a comment

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!

Choose a reason for hiding this comment

Uh oh!

lygstate Jan 20, 2021 • 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!

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!

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!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lygstate commented Jan 19, 2021 •

edited

Loading

lygstate Jan 20, 2021 •

edited

Loading