eclipse-cyclonedds / cyclonedds

Eclipse Cyclone DDS project
https://projects.eclipse.org/projects/iot.cyclonedds
Other
798 stars 349 forks source link

HelloWorld example broken on master with MinGW/ucrt64 #2027

Open tonichedgehog opened 3 weeks ago

tonichedgehog commented 3 weeks ago

The HelloWorld example (and likely, more than just the example) seems to be broken on master, when compiling with gcc (14.1) under MSYS2/MinGW/ucrt64. It does work as expected using the same code with a different compiler (VS2022).

It looks like 83b81f54d75893a004598bc35cdff5fef26fb919 is the offending commit, if that may help to locate the issue.

tonichedgehog commented 2 weeks ago

The output from publisher is:

$ ./bin/HelloworldPublisher.exe
=== 1718287565.183223 [0]       recv: UDP recvmsg sock 544: ret 0 retcode -58
[Publisher]  Waiting for a reader to be discovered ...
1718287565.297631 [0]       recv: UDP recvmsg sock 544: ret 0 retcode -58

and the subscriber:

$ ./bin/HelloworldSubscriber.exe

==1718287894.386247 [0]       recv: UDP recvmsg sock 548: ret 0 retcode -58
= [Subscriber] Waiting for a sample ...
1718287894.487633 [0]       recv: UDP recvmsg sock 548: ret 0 retcode -58
1718287898.788869 [0]       recv: UDP recvmsg sock 548: ret 0 retcode -58
1718287898.887715 [0]       recv: UDP recvmsg sock 548: ret 0 retcode -58
1718287902.501479 [0]       recv: UDP recvmsg sock 548: ret 0 retcode -58
tonichedgehog commented 2 weeks ago

The below patch is a not so pretty workaround for building CycloneDDS (master) with mingw-w64-gcc on Windows:

diff --git "a/src/core/ddsi/src/ddsi_udp.c" "b/src/core/ddsi/src/ddsi_udp.c"
index 5efd60b4..e3e71a5b 100644
--- "a/src/core/ddsi/src/ddsi_udp.c"
+++ "b/src/core/ddsi/src/ddsi_udp.c"
@@ -49,6 +49,13 @@ DDSRT_STATIC_ASSERT (DDSI_LOCATOR_UDPv4MCGEN_INDEX_MASK_BITS <= 32 - UDP_MC_ADDR
 #  endif
 #endif
 #ifndef PACKET_DESTINATION_INFO
+#  if defined (__MINGW32__) && !defined (CMSG_SPACE)
+#    include <mswsock.h>
+#    define CMSG_SPACE WSA_CMSG_SPACE
+#    define CMSG_FIRSTHDR WSA_CMSG_FIRSTHDR
+#    define CMSG_NXTHDR WSA_CMSG_NXTHDR
+#    define cmsghdr _WSACMSGHDR
+#  endif
 #  if defined CMSG_SPACE && (defined IP_PKTINFO || (DDSRT_HAVE_IPV6 && defined IPV6_PKTINFO))
 #    define PACKET_DESTINATION_INFO 1
 #  else

The CMSG_* macros used in CycloneDDS are provided by Windows SDK, but not (yet?) by mingw-w64-gcc.

I still do not fully understand the intended effect of PACKET_DESTINATION_INFO beeing undefined, but feel free to close this issue.

eboasson commented 2 weeks ago

Hi @tonichedgehog, if that workaround is all it takes to make it work on MinGW again ... then a PR with that would be most welcome. (I can do it myself it for some reason doing a PR is not an option for you, but I prefer contributions to be properly attributed.)

I still do not fully understand the intended effect of PACKET_DESTINATION_INFO beeing undefined, but feel free to close this issue.

I do not expect all platforms to provide this and so it will never be fully dependent on it.

Right now it is only used in filtering out some unwanted discovery. That's nice, especially with the agreed-upon discovery behaviour for ROS 2, but it is not like you suddenly have to have this information or Cyclone becomes useless.

One way in which I want to use it in the future is by being a bit smarter in deciding whether a multicast is likely to reach a remote node. The assumption would be receiving a multicast from a remote node is a very good indication that I can actually reach that node via a multicast.

With that in mind,

+#  if defined (__MINGW32__) && !defined (CMSG_SPACE)
+#    define PACKET_DESTINATION_INFO 0
+#  endif

would also work fine. Whichever you prefer 🙂

tonichedgehog commented 2 weeks ago

Hi, and thanks for the explanation. I got the impression that it was supposed to work with PACKET_DESTINATION_INFO=0, but got confused when it didn't. On Windows, the following seems to make the example work together with PACKET_DESTINATION_INFO=0.

diff --git "a/src/ddsrt/src/sockets/windows/socket.c" "b/src/ddsrt/src/sockets/windows/socket.c"
index a875d18f..cf802135 100644
--- "a/src/ddsrt/src/sockets/windows/socket.c"
+++ "b/src/ddsrt/src/sockets/windows/socket.c"
@@ -570,6 +570,7 @@ ddsrt_recvmsg(
     if (sockext->wsarecvmsg (sockext->sock, &wsamsg, &n, NULL, 0) != 0)
     {
       int err = WSAGetLastError();
+      *rcvd = (ssize_t) n;
       return recv_error_to_retcode(err);
     }
     else

For my needs, PACKET_DESTINATION_INFO is not a necessary feature, and I'm happy to provide a PR. I think your suggestion of setting a single feature flag is much better than trying to guess what macros that may or may not be defined by various versions and flavors of MinGW.