Multiple problems when using with Unity/MacOS #8

staszain · 2020-04-09T22:16:41Z

Hi, I'm evaluating this library for some time now in Unity. I managed to import it into the project with some issues (for some reason it doesn't load the first time you run the game), however it doesn't work out of the box for me. I've compiled the library from 1.1.0 and took the same version of the bindings. Here are the problems I've encountered so far:

client.DispatchCallback(status) is getting called with info structure containing garbage. It looks like there is a problem with marshalling. I was able to fix this with adding "ref" to the StatusCallback declaration:
public delegate void StatusCallback(ref StatusInfo info, IntPtr context);
I'm not an expert in c# marshalling and not sure this is a right fix and the system is now working as it supposed to, or is it going to break other platforms.
Once StatusCallback is changed, the structure still seems to be off. It appears that some of the field values are shifted to neighbour fields. I was able to fix this by adding StructLayoutAttribute.Pack = 4 to StatusInfo structure declaration:
[StructLayout(LayoutKind.Sequential, Pack = 4)]
public struct StatusInfo {
...
as it looks like "connection" variable takes whole 8 bytes instead of 4. Not sure why Unity/Mono has a different default Pack setting, but I think this should fix the problem for Mono and not break other platforms.
Just a suggestion to mention deinitialization procedure, especially if you set a debug callback like described in the "Usage":

Library.Initialize();
NetworkingUtils utils = new NetworkingUtils();
utils.SetDebugCallback(DebugType.Everything, debug);

you absolutely MUST set it to None when you're done:

utils.SetDebugCallback(DebugType.None, null);
Library.Deinitilize();

otherwise Unity will freeze on the next launch (probably because debug callback is now invalid and crashes silently somewhere inside).

nxrighthere · 2020-04-10T05:14:59Z

client.DispatchCallback(status) is getting called with info structure containing garbage. It looks like there is a problem with marshalling. I was able to fix this with adding "ref" to the StatusCallback declaration:

You are right, the signature was incorrect, it should be passed by reference.

Going to investigate other problems too.

nxrighthere · 2020-04-10T07:06:30Z

Just a suggestion to mention deinitialization procedure, especially if you set a debug callback like described in the "Usage":

Library.Initialize();
NetworkingUtils utils = new NetworkingUtils();
utils.SetDebugCallback(DebugType.Everything, debug);

you absolutely MUST set it to None when you're done:

utils.SetDebugCallback(DebugType.None, null);
Library.Deinitilize();

otherwise Unity will freeze on the next launch (probably because debug callback is now invalid and crashes silently somewhere inside).

I think this should be resolved in the native library, we can simply zero out/reset all global data at deinitialization with GameNetworkingSockets_Kill() since we done with it, like: SteamNetworkingSocketsLib::s_pfnDebugOutput, SteamNetworkingSocketsLib::g_eSteamDatagramDebugOutputDetailLevel.

Other game engines might be affected by this as well, since usually libraries there are loaded once, and global data/variables will persist in memory while the application remains alive.

@fletcherdvalve What do you think about this?

nxrighthere · 2020-04-10T09:40:31Z

Once StatusCallback is changed, the structure still seems to be off. It appears that some of the field values are shifted to neighbour fields. I was able to fix this by adding StructLayoutAttribute.Pack = 4 to StatusInfo structure declaration:
[StructLayout(LayoutKind.Sequential, Pack = 4)]
public struct StatusInfo {
...
as it looks like "connection" variable takes whole 8 bytes instead of 4. Not sure why Unity/Mono has a different default Pack setting, but I think this should fix the problem for Mono and not break other platforms.

That one is strange. I'm unable to reproduce this on my end.

Native size of structures on Ubuntu:

Managed size of structures on macOS Catalina 10.15.4 (Unity 2020.1.0b2):

Managed size of structures on Windows 10 (Unity 2019.3.2f1):

Managed size of structures on Windows 10 (.NET Core 3.1.200):

Can you please run the following script and confirm that on your end?

using System;
using System.Runtime.InteropServices;
using Unity.Entities;
using Unity.Jobs;
using UnityEngine;

namespace Valve.Sockets {
	public class Test : JobComponentSystem {
		protected override void OnCreate() {
			Debug.Log("Address size: " + Marshal.SizeOf(typeof(Address)) + " bytes");
			Debug.Log("Configuration size: " + Marshal.SizeOf(typeof(Configuration)) + " bytes");
			Debug.Log("StatusInfo size: " + Marshal.SizeOf(typeof(StatusInfo)) + " bytes");
			Debug.Log("ConnectionInfo size: " + Marshal.SizeOf(typeof(ConnectionInfo)) + " bytes");
			Debug.Log("ConnectionStatus size: " + Marshal.SizeOf(typeof(ConnectionStatus)) + " bytes");
			Debug.Log("NetworkingIdentity size: " + Marshal.SizeOf(typeof(NetworkingIdentity)) + " bytes");
			Debug.Log("NetworkingMessage size: " + Marshal.SizeOf(typeof(NetworkingMessage)) + " bytes");
		}

		protected override JobHandle OnUpdate(JobHandle inputDependencies) => inputDependencies;
	}
}

staszain · 2020-04-10T16:34:23Z

Can you please run the following script and confirm that on your end?

It appears the problem is on C++ end. I have 704 bytes for the StatusInfo instead of 712, which probably means that some C++ defaults on Mac are to blame. I obviously didn't make any changes to the library.

nxrighthere · 2020-04-10T16:36:26Z

What compiler are you using?

staszain · 2020-04-10T16:38:24Z

AppleClang 11.0.3.11030032

nxrighthere · 2020-04-11T09:54:33Z

I have a feeling that this is because not all integer types are strictly set to exact-width, and some of those are compiler-specific, but I'm not sure.

I'll move this issue to the native repository if @fletcherdvalve doesn't appear here next Monday-Tuesday.

zpostfacto · 2020-04-13T14:21:30Z

Can you figure out where the difference is coming in?

nxrighthere · 2020-04-13T15:43:13Z

I might be wrong, but, probably what is affecting the layout is: int m_eEndReason; in SteamNetConnectionInfo_t and int m_cbSize; in SteamNetworkingIdentity which encapsulated into SteamNetConnectionInfo_t. It seems that only those guys are not strictly set to exact-width.

zpostfacto · 2020-04-13T15:50:10Z

But his size is 8 bytes smaller, right? Given that those int fields are 4 bytes on all platforms I know of, this would imply that they are 0 bytes on Apple clang. :)

I understand that it is legal for the C spec for int to be a size other than 4 bytes. I am just not aware of any ABI we care about that decided to make what would be an absolutely disastrous decision.

It's got to be some alignment thing or some enum or something.

nxrighthere · 2020-04-13T15:53:03Z

Yea, hard to diagnose this without the compiler and OS (I don't have it). So @staszain it would be great if you can help us to determine what exactly is affecting the layout.

staszain · 2020-04-14T09:28:07Z

I will be happy to provide any assistance you need from my side.

As I mentioned I believe the first shift comes from StatusInfo.connection field, which is UInt32 as declared in Valve.Sockets. It takes either 8 or 4 bytes depending on Pack attribute parameter and the entire content of the ConnectionInfo structure is shifted 4 bytes depending on that. I will double check.

staszain · 2020-04-14T11:17:40Z

Yes, it looks like it is exactly the case. If I set StatusInfo StructLayout.Pack attribute to 8, then the content of StatusInfo.connectionInfo.identity.type, which comes right after StatusInfo.connection, is already shifted by four bytes. Thus I have the content of SteamNetworkingIdentity.m_cbSize in SteamNetworkingIdentity.m_eType field instead of its proper value.

nxrighthere · 2020-04-14T11:26:46Z

It appears the problem is on C++ end. I have 704 bytes for the StatusInfo instead of 712, which probably means that some C++ defaults on Mac are to blame. I obviously didn't make any changes to the library.

We are trying to determine why this happens? What you are explaining is about the managed side, but we have the problem with the native one since the layout is screwed there?

staszain · 2020-04-14T11:37:10Z

Well, my idea was that this happens because with this particular compiler the structure SteamNetConnectionStatusChangedCallback_t is packed in such a way that SteamNetConnectionInfo_t m_info is aligned to 4 bytes instead of 8 in case of other compilers

Sorry, I'm not 100% sure that this is what you asking. I'll try to research it more on my side.

nxrighthere · 2020-04-14T11:43:22Z

Can you please check side by side which fields are causing this? It seems that the managed side is correct in your case (it should be 712 bytes), but the native one is 704 bytes in size.

staszain · 2020-04-14T11:51:06Z

Well, the idea was that if Pack attribute fixes this on the managed side then it is SteamNetConnectionStatusChangedCallback_t.m_hConn that causes the problem. It is packed to 4 bytes instead of 8. I will try to find where the other 4 bytes diff comes from. But my guess it is m_eOldState.

staszain · 2020-04-14T12:00:20Z

SteamNetConnectionStatusChangedCallback_t *p = (SteamNetConnectionStatusChangedCallback_t*)nullptr;
    long offsetInfo = reinterpret_cast<long>(&p->m_info);
    long offsetOldState = reinterpret_cast<long>(&p->m_eOldState);
    std::cout << "offsetInfo: " << offsetInfo << ", offsetOldState: " << offsetOldState << std::endl;
    std::cout << "SteamNetConnectionStatusChangedCallback_t size: " << sizeof(SteamNetConnectionStatusChangedCallback_t) << std::endl;

Result:
offsetInfo: 4, offsetOldState: 700
SteamNetConnectionStatusChangedCallback_t size: 704

staszain · 2020-04-14T12:14:52Z

What confuses me is that SteamNetworkingIdentity is clearly aligned to 4 bytes. I think the cause of this might be that some compilers try to align inner structures to 8 bytes within a parent structure, while other compilers do not. In this case adding some padding manually should solve the problem and not break other platforms. Something like:

struct SteamNetConnectionStatusChangedCallback_t
{ 
	enum { k_iCallback = k_iSteamNetworkingSocketsCallbacks + 1 };
	HSteamNetConnection m_hConn;
        int32_t padding1;
	SteamNetConnectionInfo_t m_info;
	ESteamNetworkingConnectionState m_eOldState;
        int32_t padding2;
};

Unfortunately I cannot verify this on other platforms.

nxrighthere · 2020-04-14T12:28:00Z

Yea, I think this should work. In my case without any manual padding SteamNetConnectionStatusChangedCallback_t is correctly aligned. @fletcherdvalve

staszain · 2020-04-14T12:53:35Z

Surprisingly struct alignas(8) SteamNetConnectionStatusChangedCallback_t doesn't help.

zpostfacto · 2020-04-14T17:33:28Z

I am very afraid of changing this struct, since it has been shipped in a Steamworks SDK. That means I am bound to support that ABI essentially indefinitely. If I change this struct I will have to introduce a backward compatibility layer or else break the games that are using the current version of the struct.

Another complication is that we have to use the same struct packing on both 32-bit and 64-bit within the same platform, because these are serialized in some cases between 32-bit and 64-bit processes. So where things have ended up is that structs are packed the same on the same platform even if bitness is different. But structs are NOT necessarily packed the same on different platforms. That is sort of anti the whole concept of .NET, where you have some virtual machine bytecode that can run anywhere. But that is the unfortunate state of affairs on Steamworks. We have to have platform specific binaries, even .net binaries.


#if defined(__linux__) || defined(__APPLE__) 
// The 32-bit version of gcc has the alignment requirement for uint64 and double set to
// 4 meaning that even with #pragma pack(8) these types will only be four-byte aligned.
// The 64-bit version of gcc has the alignment requirement for these types set to
// 8 meaning that unless we use #pragma pack(4) our structures will get bigger.
// The 64-bit structure packing has to match the 32-bit structure packing for each platform.
#define VALVE_CALLBACK_PACK_SMALL
#else
#define VALVE_CALLBACK_PACK_LARGE
#endif

#if defined( VALVE_CALLBACK_PACK_SMALL )
#pragma pack( push, 4 )
#elif defined( VALVE_CALLBACK_PACK_LARGE )
#pragma pack( push, 8 )
#else
#error ???
#endif 

typedef struct ValvePackingSentinel_t
{
    uint32 m_u32;
    uint64 m_u64;
    uint16 m_u16;
    double m_d;
} ValvePackingSentinel_t;

#pragma pack( pop )


#if defined(VALVE_CALLBACK_PACK_SMALL)
VALVE_COMPILE_TIME_ASSERT( sizeof(ValvePackingSentinel_t) == 24 )
#elif defined(VALVE_CALLBACK_PACK_LARGE)
VALVE_COMPILE_TIME_ASSERT( sizeof(ValvePackingSentinel_t) == 32 )
#else
#error ???
#endif

nxrighthere · 2020-04-14T19:33:21Z

That makes me sad... Should we close this issue and open/plan it as an enhancement in the native repository?

zpostfacto · 2020-04-14T20:25:40Z

Yeah, it is pretty crappy.

But now that I think more about it, though, it's probably pretty common for structs to be packed differently on different platforms, right? Also having an ABI that you are stuck with and that you cannot change doesn't seem that unusual -- outside of opensource projects that is. Perhaps the only thing that is unusual about this code is that we change the packing to ensure that 32-bit and 64-bit behave the same, but I don't think that really changes anything relevant to this problem. So surely there are standard workarounds for this, right?

Is it possible to have the managed code use different packing on different platforms? Seems like the the C# Steamworks wrappers must have hit this issue before. Again, I can't imagine that what we're doing here is really unusual. This has to be a common problem.

zpostfacto · 2020-04-14T20:30:59Z

@fletcherdvalve Also, what about this one? #8 (comment)

I could clear that pointer on shutdown, seems fine. You can set the debug output callback before initialization, and actually this is really useful. So I don't want to clear it on init.

nxrighthere · 2020-04-14T20:38:59Z

To be honest, this is the first time when I see something like this. My customers are using a lot of native libraries across platforms (C and C#) on Windows/Linux/macOS, and this was never an issue with structures. Their sizes were always consistent across compilers with MSVC/GCC/Clang.

In .NET, we can't determine a platform at compilation time, only during the runtime, which doesn't help here, unfortunately. Managed assemblies are unified before JIT. Except asking a user to manually define symbols in accordance with a platform if it's not Unity where they are automatically defined.

zpostfacto · 2020-04-14T22:20:03Z

I'm gonna throw in some padding fields to force the alignment. We can fix this, at least in the opensource code.

@staszain , can you confirm the values you currently have for:

sizeof(SteamNetConnectionInfo_t)
sizeof(SteamNetworkingIdentity)

staszain · 2020-04-15T09:07:23Z

std::cout << "sizeof(SteamNetConnectionStatusChangedCallback_t): " << sizeof(SteamNetConnectionStatusChangedCallback_t) << std::endl;
std::cout << "sizeof(SteamNetConnectionInfo_t): " << sizeof(SteamNetConnectionInfo_t) << std::endl;
std::cout << "sizeof(SteamNetworkingIdentity): " << sizeof(SteamNetworkingIdentity) << std::endl;

output:

sizeof(SteamNetConnectionStatusChangedCallback_t): 704
sizeof(SteamNetConnectionInfo_t): 696
sizeof(SteamNetworkingIdentity): 136

zpostfacto · 2020-04-17T20:12:44Z

I was working on inserting the padding fields, and then it hit me. Why not just use #pragma pack (4)? This is honestly what I wish we could do in Steamworks, but that ship probably sailed in 2010 or so.

@nxrighthere, if I set #pragma pack(4), that will actually cause the Windows packing to change, because it is currently 8. But it'd easy to make the C# match this, right? In fact, I think for all the structures we care about, 4 results in no padding anywhere, and so it is equivalent to 1.

zpostfacto · 2020-04-17T20:28:25Z

Looks like no matter what I do, the structs on 32-bit and 64-bit cannot be the same size, because e.g. SteamNetworkingMessage_t has a pointer member. I don't know how that works with C#.

nxrighthere · 2020-04-17T20:38:34Z

@fletcherdvalve Yes, it's not a problem to match it in C#, the size of the pointer is depends on the architecture of the system.

zpostfacto · 2020-04-17T20:44:28Z

So C# assemblies will dynamically adjust the offsets of members in structs when they are loaded, depending on the architecture? Whoa.

nxrighthere · 2020-04-17T20:53:07Z

Yes, this is one of the advantages of the virtual machine with just-in-time compiler.

Steamworks SDK has found itself in a really bad place having to support ABIs that differ on different platforms. That didn't matter in the days when everybody compiled all of their binaries per platform, but it breaks the "write once, run anywhere" ideal. We don't have to support old ABIs so let's just pick a packing and use it everywhere. (But note that we do have stucts with pointers in them, so our structs will vary between 32-bit and 64-bit.) nxrighthere/ValveSockets-CSharp#8

nxrighthere · 2020-04-17T23:46:24Z

@fletcherdvalve So we are always #pragma pack(push, 8) now?

nxrighthere · 2020-04-17T23:48:33Z

@staszain Can you confirm that the latest version from the master branch works for you?

zpostfacto · 2020-04-18T00:44:59Z

Yeah, always 8 now. Since I am basically just arbitrarily picking, I decided to arbitrarily pick compatibility with Windows.

ArnaudValensi · 2020-04-20T09:27:53Z

Hi @nxrighthere, what is the proper way to use this library?
Should we use release 1.1.0-F1 with GameNetworkingSockets 1.1.0 or master?

staszain · 2020-04-20T12:43:34Z

I've built the lib from the latest commit - it looks like the problem is gone. Thank you very much everyone!

nxrighthere · 2020-04-20T14:39:16Z

@ArnaudValensi From the release section, you should always use the version of the native library that specified there (see below changes). The source file from master might or not be compatible with the latest changes in the master of the native library, right now it's compatible.

nxrighthere · 2020-04-20T14:47:58Z

@ArnaudValensi
Release (Managed) -> Release (Native) with the specified version.
Master (Managed) -> Master (Native) but not guaranteed.

ArnaudValensi · 2020-04-20T15:13:01Z

Thank you very much for your answer and for your work.

nxrighthere · 2020-04-20T15:44:18Z

@ArnaudValensi It's Fletcher doing the job, I'm just matching stuff to changes that he's making. 😸

zpostfacto · 2020-04-20T15:49:43Z

@ArnaudValensi From the release section, you should always use the version of the native library that specified there (see below changes). The source file from master might or not be compatible with the latest changes in the master of the native library, right now it's compatible.

I could probably cherry pick and make a release. Or, just release what it is at master HEAD right now. There's some P2P work in master right now that isn't really complete, but I think everything still works. Let me know if it would help to make an official release.

ArnaudValensi · 2020-04-20T18:53:41Z

@ArnaudValensi It's Fletcher doing the job, I'm just matching stuff to changes that he's making. 😸

So thanks to you both @fletcherdvalve and @nxrighthere 🙏🙂

Steamworks SDK has found itself in a really bad place having to support ABIs that differ on different platforms. That didn't matter in the days when everybody compiled all of their binaries per platform, but it breaks the "write once, run anywhere" ideal. We don't have to support old ABIs so let's just pick a packing and use it everywhere. (But note that we do have stucts with pointers in them, so our structs will vary between 32-bit and 64-bit.) nxrighthere/ValveSockets-CSharp#8

nxrighthere added bug Something isn't working unity labels Apr 10, 2020

nxrighthere added the native label Apr 10, 2020

nxrighthere closed this as completed Apr 19, 2020

nxrighthere mentioned this issue Apr 20, 2020

Reset debug callback at destruction of the system ValveSoftware/GameNetworkingSockets#109

Merged

Multiple problems when using with Unity/MacOS #8

Multiple problems when using with Unity/MacOS #8

Comments

staszain commented Apr 9, 2020

nxrighthere commented Apr 10, 2020

nxrighthere commented Apr 10, 2020 • edited Loading

nxrighthere commented Apr 10, 2020

staszain commented Apr 10, 2020 • edited Loading

nxrighthere commented Apr 10, 2020

staszain commented Apr 10, 2020

nxrighthere commented Apr 11, 2020

zpostfacto commented Apr 13, 2020

nxrighthere commented Apr 13, 2020

zpostfacto commented Apr 13, 2020

nxrighthere commented Apr 13, 2020

staszain commented Apr 14, 2020 • edited Loading

staszain commented Apr 14, 2020

nxrighthere commented Apr 14, 2020

staszain commented Apr 14, 2020 • edited Loading

nxrighthere commented Apr 14, 2020

staszain commented Apr 14, 2020 • edited Loading

staszain commented Apr 14, 2020 • edited Loading

staszain commented Apr 14, 2020 • edited Loading

nxrighthere commented Apr 14, 2020

staszain commented Apr 14, 2020

zpostfacto commented Apr 14, 2020 • edited Loading

nxrighthere commented Apr 14, 2020 • edited Loading

zpostfacto commented Apr 14, 2020

zpostfacto commented Apr 14, 2020

nxrighthere commented Apr 14, 2020

zpostfacto commented Apr 14, 2020

staszain commented Apr 15, 2020 • edited Loading

zpostfacto commented Apr 17, 2020

zpostfacto commented Apr 17, 2020

nxrighthere commented Apr 17, 2020

zpostfacto commented Apr 17, 2020

nxrighthere commented Apr 17, 2020

nxrighthere commented Apr 17, 2020

nxrighthere commented Apr 17, 2020

zpostfacto commented Apr 18, 2020

ArnaudValensi commented Apr 20, 2020

staszain commented Apr 20, 2020

nxrighthere commented Apr 20, 2020

nxrighthere commented Apr 20, 2020

ArnaudValensi commented Apr 20, 2020

nxrighthere commented Apr 20, 2020 • edited Loading

zpostfacto commented Apr 20, 2020

ArnaudValensi commented Apr 20, 2020

nxrighthere commented Apr 10, 2020 •

edited

Loading

staszain commented Apr 10, 2020 •

edited

Loading

staszain commented Apr 14, 2020 •

edited

Loading

staszain commented Apr 14, 2020 •

edited

Loading

staszain commented Apr 14, 2020 •

edited

Loading

staszain commented Apr 14, 2020 •

edited

Loading

staszain commented Apr 14, 2020 •

edited

Loading

zpostfacto commented Apr 14, 2020 •

edited

Loading

nxrighthere commented Apr 14, 2020 •

edited

Loading

staszain commented Apr 15, 2020 •

edited

Loading

nxrighthere commented Apr 20, 2020 •

edited

Loading