Devilutionx: 64-bit multiplayer

[namtsui]

I wanted to get a discussion going on the state of 64-bit multiplayer. I am targeting 64-bit on OpenBSD, but this should affect other operating systems on 64-bit platforms. I managed to get OpenBSD amd64 (64-bit) TCP multiplayer to work with these two diffs.

I briefly tried building against a recent commit (https://github.com/diasurgical/devilutionX/commit/10ebca4efd422bbf46bad6d12ea4cdade9038b01). I have not had time to investigate player 1 not being able to join its own game because of a segfault (when trying to apply my diffs) and a popup saying "unable to create game" (without the diffs). Perhaps a regression was introduced between 0.4.0 and this commit? I can at least confirm that 0.4.0 can be patched to have TCP multiplayer working.

Also, I am not sure how to detect i386 vs amd64 and insert macros for the frame_queue diff and DiabloAllocPtr(32012). You can decide how to best do so for portability and maintainability.

I wrote these two diffs to attempt to get multiplayer to work. It is built on top of the porting work of @ibara @rfht and @brynet.
https://github.com/jasperla/openbsd-wip/commit/4f3d22edd93d4f96a38ac1929b9bd6c29dae1d69

0.4.0 i386 (32-bit) OpenBSD:
TCP: Player 2 successfully connects to Player 1.
UDP: socket in use error

0.4.0 amd64 (64-bit) FreeBSD:
TCP: Player 1 can't join its own game
UDP: socket in use error

0.4.0 amd64 (64-bit) OpenBSD:
TCP: Player 1 can join, but Player 2 crashes. Fixed with these diffs.
UDP: socket in use error (terminating with uncaught exception of type std::__1::system_error: send_to: Socket is already connected)

$OpenBSD$

This fixes a padding bug.

(char *)&packeta[1] - packeta->dwSpaceLeft doesn't work because it assumes there
is no padding at the end of TMegaPkt. With padding it started writing a few
bytes after packeta->data, so the first few bytes were garbage.

The goal is to write to packeta->data at the first available byte, based on
packeta->dwSpaceLeft. It is safer to calculate starting from packeta->data.

Also, pointers like pNext are 8 bytes long on 64-bit platforms. Should allocate
8 + 4 + 32000 = 32012.

typedef struct TMegaPkt {^M
    struct TMegaPkt *pNext;^M
    DWORD dwSpaceLeft;^M
    BYTE data[32000];^M
} TMegaPkt;

Index: Source/msg.cpp
--- Source/msg.cpp.orig
+++ Source/msg.cpp
@@ -47,7 +47,7 @@ void msg_send_packet(int pnum, const void *packet, DWO
        msg_get_next_packet();
        packeta = sgpCurrPkt;
    }
-   memcpy((char *)&packeta[1] - packeta->dwSpaceLeft, packet, dwSize);
+   memcpy((char *)(packeta->data + 32000 - packeta->dwSpaceLeft), packet, dwSize);
    sgpCurrPkt->dwSpaceLeft -= dwSize;
 }
 // 65AB24: using guessed type int sgnCurrMegaPlayer;
@@ -56,7 +56,7 @@ TMegaPkt *msg_get_next_packet()
 {
    TMegaPkt *result;

-   sgpCurrPkt = (TMegaPkt *)DiabloAllocPtr(32008);
+   sgpCurrPkt = (TMegaPkt *)DiabloAllocPtr(32012);
    sgpCurrPkt->pNext = NULL;
    sgpCurrPkt->dwSpaceLeft = 32000;

$OpenBSD$

nextsize is 8 bytes. framesize_t is 4 bytes. Copy framesize_t to
nextsize then shift 32 bits to zero out the most significant 32
bits. This fixes packet_ready().

Remove this patch when building for i386 (32-bit).

Index: SourceX/dvlnet/frame_queue.cpp
--- SourceX/dvlnet/frame_queue.cpp.orig
+++ SourceX/dvlnet/frame_queue.cpp
@@ -48,7 +48,8 @@ bool frame_queue::packet_ready()
        if(size() < sizeof(framesize_t))
            return false;
        auto szbuf = read(sizeof(framesize_t));
-       std::memcpy(&nextsize, &szbuf[0], sizeof(nextsize));
+       std::memcpy(&nextsize, &szbuf[0], sizeof(framesize_t));
+       nextsize >> 32;
        if(!nextsize)
            throw frame_queue_exception();
    }

Also, I had to change a cast in towners.cpp for 0.4.0, but it doesn't apply to more recent commits.

$OpenBSD$

Index: Source/towners.cpp
--- Source/towners.cpp.orig
+++ Source/towners.cpp
@@ -198,7 +198,7 @@ void InitQstSnds(int i)
        v3 += 24;
        v4 = (QuestTalkData *)((char *)v4 + 4);
        v2 += 3;
-   } while ((signed int)v3 < (signed int)&quests[16]._qtype);
+   } while (v3 < &quests[16]._qtype);
 }
 // 69BE90: using guessed type int qline;
 // 6AAC2C: using guessed type int boyloadflag;

[AJenbo]

I believe MP has regressed since 0.4.0 but haven't had much time to dig in to it, it's probably related to this commit: https://github.com/diasurgical/devilutionX/commit/1d7a548cd72910340131f2c2de9ba7f2fd120b4e

Also, it's very likely that 64bit multiplayer doesn't work it's against something I haven't had time to look in to. Often the solution is to not use memcpy for structures or pack the structures.

[namtsui]

Thank you for pointing out that commit.

I managed to get multiplayer to work against a recent commit. https://github.com/diasurgical/devilutionX/commit/72f65d577124d24ab9f459ef164e31c9ab225b3e

The only multiplayer regression from 0.4.0 to this recent commit was that glpMsgTbl holds 64-bit pointers to turn_t variables (see turn_last in SourceX/dvlnet/base.{h,cpp}). This won't fit inside an int.

New diffs:

$OpenBSD$

glpMsgTbl stores 64-bit pointers to turn_t variables. This won't fit inside int.

Index: Source/nthread.cpp
--- Source/nthread.cpp.orig
+++ Source/nthread.cpp
@@ -11,7 +11,7 @@ static CCritSect sgMemCrit;
 DWORD gdwDeltaBytesSec;
 BOOLEAN nthread_should_run;
 DWORD gdwTurnsInTransit;
-int glpMsgTbl[MAX_PLRS];
+uintptr_t glpMsgTbl[MAX_PLRS];
 unsigned int glpNThreadId;
 char sgbSyncCountdown;
 int turn_upper_bit;

$OpenBSD$

glpMsgTbl stores 64-bit pointers to turn_t variables. This won't fit inside int.

Index: Source/nthread.h
--- Source/nthread.h.orig
+++ Source/nthread.h
@@ -7,7 +7,7 @@ extern DWORD gdwMsgLenTbl[MAX_PLRS];
 extern DWORD gdwDeltaBytesSec;
 extern BOOLEAN nthread_should_run;
 extern DWORD gdwTurnsInTransit;
-extern int glpMsgTbl[MAX_PLRS];
+extern uintptr_t glpMsgTbl[MAX_PLRS];
 extern unsigned int glpNThreadId;
 extern int turn_upper_bit;
 extern BOOLEAN sgbThreadIsRunning;



md5-5e552f0e307227024912eb12fad7612f



$OpenBSD$

Index: Source/list.h
--- Source/list.h.orig
+++ Source/list.h
@@ -154,7 +154,7 @@ void TList<T>::UnlinkAll()
 {
    for (;;) {
        T *node = m_link.Next();
-       if ((int)node <= 0)
+       if ((uintptr_t)node <= 0)
            break;
        node->m_Link.Unlink();
    }



md5-1a9395fe549fc34d9577c4662683395c



$OpenBSD$

nextsize is 8 bytes. framesize_t is 4 bytes. Copy framesize_t to
nextsize then shift 32 bits to zero out the most significant 32
bits. This fixes packet_ready().

Index: SourceX/dvlnet/frame_queue.cpp
--- SourceX/dvlnet/frame_queue.cpp.orig
+++ SourceX/dvlnet/frame_queue.cpp
@@ -48,7 +48,12 @@ bool frame_queue::packet_ready()
        if(size() < sizeof(framesize_t))
            return false;
        auto szbuf = read(sizeof(framesize_t));
+       #ifdef __LP64__
+       std::memcpy(&nextsize, &szbuf[0], sizeof(framesize_t));
+       nextsize >> 32;
+       #else
        std::memcpy(&nextsize, &szbuf[0], sizeof(nextsize));
+       #endif
        if(!nextsize)
            throw frame_queue_exception();
    }



md5-70f3cd84d26193e1d1270980836dcef9



$OpenBSD$

This fixes a padding bug.

(char *)&packeta[1] - packeta->dwSpaceLeft doesn't work because it assumes there
is no padding at the end of TMegaPkt. With padding it started writing a few
bytes after packeta->data, so the first few bytes were garbage.

The goal is to write to packeta->data at the first available byte, based on
packeta->dwSpaceLeft. It is safer to calculate starting from packeta->data.

Also, pointers like pNext are 8 bytes long on 64-bit platforms. Should allocate
8 + 4 + 32000 = 32012.

typedef struct TMegaPkt {^M
    struct TMegaPkt *pNext;^M
    DWORD dwSpaceLeft;^M
    BYTE data[32000];^M
} TMegaPkt;

Index: Source/msg.cpp
--- Source/msg.cpp.orig
+++ Source/msg.cpp
@@ -47,7 +47,7 @@ void msg_send_packet(int pnum, const void *packet, DWO
        msg_get_next_packet();
        packeta = sgpCurrPkt;
    }
-   memcpy((char *)&packeta[1] - packeta->dwSpaceLeft, packet, dwSize);
+   memcpy((char *)(packeta->data + 32000 - packeta->dwSpaceLeft), packet, dwSize);
    sgpCurrPkt->dwSpaceLeft -= dwSize;
 }

@@ -55,7 +55,11 @@ TMegaPkt *msg_get_next_packet()
 {
    TMegaPkt *result;

+   #ifdef __LP64__
+   sgpCurrPkt = (TMegaPkt *)DiabloAllocPtr(32012);
+   #else
    sgpCurrPkt = (TMegaPkt *)DiabloAllocPtr(32008);
+   #endif
    sgpCurrPkt->pNext = NULL;
    sgpCurrPkt->dwSpaceLeft = 32000;

[mewmew]

Wonderful work @namtsui! Thanks for working on this. Now the 64-bit version is much more stable.