Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MacOS Support #27

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

MacOS Support #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9584076
ManagedObject and NativeInstance management
0xworks Sep 15, 2024
f003b4e
Update example to using() rather than try..finally
0xworks Sep 17, 2024
4bc3a10
AppContext.BaseDirectory is now set properly + assemblies are attempt…
TheCherno Oct 14, 2024
f8a6cd1
Fixes and API additions
TheCherno Nov 7, 2024
da12281
Linux string fix
TheCherno Nov 15, 2024
62b8b07
Add MacOS specific paths and types
ToniPlays Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions Coral.Managed/Source/AssemblyLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ public static class AssemblyLoader
private static readonly Dictionary<Type, AssemblyLoadStatus> s_AssemblyLoadErrorLookup = new();
private static readonly Dictionary<int, AssemblyLoadContext?> s_AssemblyContexts = new();
private static readonly Dictionary<int, Assembly> s_AssemblyCache = new();
#if DEBUG
private static readonly Dictionary<int, List<GCHandle>> s_AllocatedHandles = new();
#endif
Comment on lines +25 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Ensure thread safety when accessing s_AllocatedHandles

The s_AllocatedHandles dictionary may be accessed by multiple threads, potentially leading to race conditions or data corruption. Consider using a thread-safe collection like ConcurrentDictionary<int, List<GCHandle>> or implement proper synchronization when accessing this shared resource.

private static AssemblyLoadStatus s_LastLoadStatus = AssemblyLoadStatus.Success;

private static readonly AssemblyLoadContext? s_CoralAssemblyLoadContext;
Expand Down Expand Up @@ -57,6 +59,8 @@ internal static bool TryGetAssembly(int InAssemblyId, out Assembly? OutAssembly)

internal static Assembly? ResolveAssembly(AssemblyLoadContext? InAssemblyLoadContext, AssemblyName InAssemblyName)
{
LogMessage($"[AssemblyLoader] Resolving assembly: {InAssemblyName.FullName}", MessageLevel.Trace);

try
{
int assemblyId = InAssemblyName.Name!.GetHashCode();
Expand All @@ -83,6 +87,14 @@ internal static bool TryGetAssembly(int InAssemblyId, out Assembly? OutAssembly)
ManagedHost.HandleException(ex);
}

string assemblyPath = Path.Combine(AppContext.BaseDirectory, $"{InAssemblyName.Name}.dll");
LogMessage($"[AssemblyLoader] Trying to find assembly in {assemblyPath}", MessageLevel.Trace);
if (File.Exists(assemblyPath))
{
LogMessage($"[AssemblyLoader] Found assembly {InAssemblyName.FullName}", MessageLevel.Trace);
return InAssemblyLoadContext.LoadFromAssemblyPath(assemblyPath);
}

return null;
}

Expand Down Expand Up @@ -126,6 +138,7 @@ internal static void UnloadAssemblyLoadContext(int InContextId)
return;
}

#if DEBUG
foreach (var assembly in alc.Assemblies)
{
var assemblyName = assembly.GetName();
Expand All @@ -136,8 +149,23 @@ internal static void UnloadAssemblyLoadContext(int InContextId)
continue;
}

// If everything is working properly, then there should not be anything left kicking around in the handles list.
// If you see messages here, it probably means you are mis-managing the lifetime of unmanaged resources.
// Managed objects that wrap an unmanaged resource need to implement IDisposable, and be Dispose()'d properly.
// Example:
// // SceneQueryHitInterop wraps an unmanaged resource. It needs to implement IDisposable
// using(SceneQueryHitInterop hit = new())
// {
// Physics.CastRay(ray, out hit); // Calls into native code, populates the unmanaged resource into hit
//
// // Do something with hit
//
// } // hit is Dispose()'d here
//
foreach (var handle in handles)
{
LogMessage($"Found still-registered handle '{(handle.Target is null? "null" : handle.Target)}' from assembly '{assemblyName}'", MessageLevel.Warning);

if (!handle.IsAllocated || handle.Target == null)
{
continue;
Expand All @@ -149,6 +177,7 @@ internal static void UnloadAssemblyLoadContext(int InContextId)

s_AllocatedHandles.Remove(assemblyId);
}
#endif

ManagedObject.s_CachedMethods.Clear();

Expand Down Expand Up @@ -274,6 +303,9 @@ internal static NativeString GetAssemblyName(int InAssemblyId)
return assemblyName.Name;
}

#if DEBUG
// In DEBUG builds, we track all GCHandles that are allocated by the managed code,
// so that we can check that they've all been freed when the assembly is unloaded.
internal static void RegisterHandle(Assembly InAssembly, GCHandle InHandle)
{
var assemblyName = InAssembly.GetName();
Expand All @@ -288,4 +320,22 @@ internal static void RegisterHandle(Assembly InAssembly, GCHandle InHandle)
handles.Add(InHandle);
}

internal static void DeregisterHandle(Assembly InAssembly, GCHandle InHandle)
{
var assemblyName = InAssembly.GetName();
int assemblyId = assemblyName.Name!.GetHashCode();

if (!s_AllocatedHandles.TryGetValue(assemblyId, out var handles))
{
return;
}

if (!InHandle.IsAllocated)
{
LogMessage($"AssemblyLoader de-registering an already freed object from assembly '{assemblyName}'", MessageLevel.Error);
}

handles.Remove(InHandle);
}
#endif
}
26 changes: 22 additions & 4 deletions Coral.Managed/Source/InteropTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public NativeArray([DisallowNull] T?[] InValues)
}
}

internal NativeArray(IntPtr InArray, IntPtr InHandle, int InLength)
public NativeArray(IntPtr InArray, IntPtr InHandle, int InLength)
{
m_NativeArray = InArray;
m_ArrayHandle = InHandle;
Expand Down Expand Up @@ -168,9 +168,9 @@ public static bool HasFieldArray(object? InTarget, MemberInfo? InArrayMemberInfo
}

[StructLayout(LayoutKind.Sequential)]
public struct NativeInstance<T>
public struct NativeInstance<T> : IDisposable
{
private readonly IntPtr m_Handle;
private IntPtr m_Handle;
private readonly IntPtr m_Unused;

private NativeInstance(IntPtr handle)
Expand All @@ -179,6 +179,24 @@ private NativeInstance(IntPtr handle)
m_Unused = IntPtr.Zero;
}

public void Dispose()
{
if (m_Handle != IntPtr.Zero)
{
var handle = GCHandle.FromIntPtr(m_Handle);
#if DEBUG
var type = handle.Target?.GetType();
if (type is not null)
{
AssemblyLoader.DeregisterHandle(type.Assembly, handle);
}
#endif
handle.Free();
m_Handle = IntPtr.Zero;
}
GC.SuppressFinalize(this);
}

public T? Get()
{
if (m_Handle == IntPtr.Zero)
Expand All @@ -194,7 +212,7 @@ private NativeInstance(IntPtr handle)

public static implicit operator NativeInstance<T>(T instance)
{
return new(GCHandle.ToIntPtr(GCHandle.Alloc(instance, GCHandleType.Pinned)));
return new(GCHandle.ToIntPtr(GCHandle.Alloc(instance, GCHandleType.Normal)));
}

public static implicit operator T?(NativeInstance<T> InInstance)
Expand Down
2 changes: 1 addition & 1 deletion Coral.Managed/Source/ManagedHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

namespace Coral.Managed;

internal enum MessageLevel { Info = 1, Warning = 2, Error = 4 }
internal enum MessageLevel { Trace = 1, Info = 2, Warning = 4, Error = 8 }

internal static class ManagedHost
{
Expand Down
38 changes: 37 additions & 1 deletion Coral.Managed/Source/ManagedObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,36 @@ internal static unsafe IntPtr CreateObject(int InTypeID, Bool32 InWeakRef, IntPt
}

var handle = GCHandle.Alloc(result, InWeakRef ? GCHandleType.Weak : GCHandleType.Normal);
#if DEBUG
AssemblyLoader.RegisterHandle(type.Assembly, handle);
#endif
return GCHandle.ToIntPtr(handle);
}
catch (Exception ex)
{
HandleException(ex);
return IntPtr.Zero;
}
}

[UnmanagedCallersOnly]
internal static unsafe IntPtr CopyObject(IntPtr InObjectHandle)
{
try
{
var target = GCHandle.FromIntPtr(InObjectHandle).Target;

if (target == null)
{
LogMessage($"Cannot copy object with handle {InObjectHandle}. Target was null.", MessageLevel.Error);
return IntPtr.Zero;
}

var handle = GCHandle.Alloc(target, GCHandleType.Normal);
#if DEBUG
var type = target.GetType();
AssemblyLoader.RegisterHandle(type.Assembly, handle);
#endif
Comment on lines +157 to +174
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential misnomer: CopyObject does not create a copy of the object

The CopyObject method creates a new GCHandle to the same target object rather than creating a new object copy. This could lead to confusion for users expecting a deep copy. Consider renaming the method to DuplicateHandle or implementing cloning logic if a true object copy is intended.

Apply this diff to rename the method for clarity:

-internal static unsafe IntPtr CopyObject(IntPtr InObjectHandle)
+internal static unsafe IntPtr DuplicateHandle(IntPtr InObjectHandle)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[UnmanagedCallersOnly]
internal static unsafe IntPtr CopyObject(IntPtr InObjectHandle)
{
try
{
var target = GCHandle.FromIntPtr(InObjectHandle).Target;
if (target == null)
{
LogMessage($"Cannot copy object with handle {InObjectHandle}. Target was null.", MessageLevel.Error);
return IntPtr.Zero;
}
var handle = GCHandle.Alloc(target, GCHandleType.Normal);
#if DEBUG
var type = target.GetType();
AssemblyLoader.RegisterHandle(type.Assembly, handle);
#endif
[UnmanagedCallersOnly]
internal static unsafe IntPtr DuplicateHandle(IntPtr InObjectHandle)
{
try
{
var target = GCHandle.FromIntPtr(InObjectHandle).Target;
if (target == null)
{
LogMessage($"Cannot copy object with handle {InObjectHandle}. Target was null.", MessageLevel.Error);
return IntPtr.Zero;
}
var handle = GCHandle.Alloc(target, GCHandleType.Normal);
#if DEBUG
var type = target.GetType();
AssemblyLoader.RegisterHandle(type.Assembly, handle);
#endif

return GCHandle.ToIntPtr(handle);
}
catch (Exception ex)
Expand All @@ -157,7 +186,14 @@ internal static void DestroyObject(IntPtr InObjectHandle)
{
try
{
GCHandle.FromIntPtr(InObjectHandle).Free();
GCHandle handle = GCHandle.FromIntPtr(InObjectHandle);
#if DEBUG
var type = handle.Target?.GetType();
if (type is not null) {
AssemblyLoader.DeregisterHandle(type.Assembly, handle);
}
#endif
handle.Free();
}
catch (Exception ex)
{
Expand Down
2 changes: 2 additions & 0 deletions Coral.Managed/Source/Marshalling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ namespace Coral.Managed;

public static class Marshalling
{
// This needs to map to Coral::Array, hence the unused ArrayHandle
struct ValueArrayContainer
{
public IntPtr Data;
public IntPtr ArrayHandle;
public int Length;
};

Expand Down
14 changes: 14 additions & 0 deletions Coral.Native/Include/Coral/Array.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,20 @@ namespace Coral {
return result;
}

static Array New(const void* buffer, size_t size)
{
Array<TValue> result;

if (buffer)
{
result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
result.m_Length = static_cast<int32_t>(size);
memcpy(result.m_Ptr, buffer, size);
}

return result;
}
Comment on lines +36 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null check validation for Memory::AllocHGlobal

The allocation could fail, especially with large buffers. MacOS memory pressure situations should be handled gracefully.

Add allocation validation:

 static Array New(const void* buffer, size_t size)
 {
     Array<TValue> result;
 
     if (buffer)
     {
         result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
+        if (!result.m_Ptr)
+            throw std::bad_alloc();
+
         result.m_Length = static_cast<int32_t>(size);
         memcpy(result.m_Ptr, buffer, size);
     }
 
     return result;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static Array New(const void* buffer, size_t size)
{
Array<TValue> result;
if (buffer)
{
result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
result.m_Length = static_cast<int32_t>(size);
memcpy(result.m_Ptr, buffer, size);
}
return result;
}
static Array New(const void* buffer, size_t size)
{
Array<TValue> result;
if (buffer)
{
result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
if (!result.m_Ptr)
throw std::bad_alloc();
result.m_Length = static_cast<int32_t>(size);
memcpy(result.m_Ptr, buffer, size);
}
return result;
}

⚠️ Potential issue

Add size validation to ensure type safety

The new method accepts a raw buffer and size without validating if the size is a multiple of sizeof(TValue). This could lead to buffer overruns or truncated data, especially critical on MacOS where memory alignment is strictly enforced.

Consider this safer implementation:

 static Array New(const void* buffer, size_t size)
 {
     Array<TValue> result;
 
     if (buffer)
     {
+        if (size % sizeof(TValue) != 0)
+            throw std::invalid_argument("Buffer size must be a multiple of sizeof(TValue)");
+
+        size_t elementCount = size / sizeof(TValue);
-        result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
-        result.m_Length = static_cast<int32_t>(size);
+        result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
+        result.m_Length = static_cast<int32_t>(elementCount);
         memcpy(result.m_Ptr, buffer, size);
     }
 
     return result;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static Array New(const void* buffer, size_t size)
{
Array<TValue> result;
if (buffer)
{
result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
result.m_Length = static_cast<int32_t>(size);
memcpy(result.m_Ptr, buffer, size);
}
return result;
}
static Array New(const void* buffer, size_t size)
{
Array<TValue> result;
if (buffer)
{
if (size % sizeof(TValue) != 0)
throw std::invalid_argument("Buffer size must be a multiple of sizeof(TValue)");
size_t elementCount = size / sizeof(TValue);
result.m_Ptr = static_cast<TValue*>(Memory::AllocHGlobal(size));
result.m_Length = static_cast<int32_t>(elementCount);
memcpy(result.m_Ptr, buffer, size);
}
return result;
}


static Array New(std::initializer_list<TValue> InValues)
{
Array result;
Expand Down
13 changes: 11 additions & 2 deletions Coral.Native/Include/Coral/Core.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#include <string_view>

#ifdef CORAL_WINDOWS
#if defined(CORAL_WINDOWS)
#define CORAL_CALLTYPE __cdecl
#define CORAL_HOSTFXR_NAME "hostfxr.dll"

Expand All @@ -19,13 +19,22 @@
using CharType = unsigned short;
using StringView = std::string_view;
#endif
#else
#elif defined(CORAL_LINUX)
#define CORAL_CALLTYPE
#define CORAL_STR(s) s
#define CORAL_HOSTFXR_NAME "libhostfxr.so"

using CharType = char;
using StringView = std::string_view;

#else
#define CORAL_CALLTYPE
#define CORAL_STR(s) s
#define CORAL_HOSTFXR_NAME "libhostfxr.dylib"

using CharType = char;
using StringView = std::string_view;

Comment on lines +30 to +37
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add explicit MacOS platform check

Using #else as a catch-all for MacOS is risky as it would silently include any undefined platform. This could mask configuration errors.

Replace the #else with an explicit check:

-#else
+#elif defined(CORAL_MACOS)
     #define CORAL_CALLTYPE
     #define CORAL_STR(s) s
     #define CORAL_HOSTFXR_NAME "libhostfxr.dylib"

     using CharType = char;
     using StringView = std::string_view;
+#else
+    #error "Unsupported platform"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#else
#define CORAL_CALLTYPE
#define CORAL_STR(s) s
#define CORAL_HOSTFXR_NAME "libhostfxr.dylib"
using CharType = char;
using StringView = std::string_view;
#elif defined(CORAL_MACOS)
#define CORAL_CALLTYPE
#define CORAL_STR(s) s
#define CORAL_HOSTFXR_NAME "libhostfxr.dylib"
using CharType = char;
using StringView = std::string_view;
#else
#error "Unsupported platform"

#endif

#define CORAL_DOTNET_TARGET_VERSION_MAJOR 8
Expand Down
10 changes: 9 additions & 1 deletion Coral.Native/Include/Coral/ManagedObject.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ namespace Coral {
class ManagedObject
{
public:
ManagedObject() = default;
ManagedObject(const ManagedObject& InOther);
ManagedObject(ManagedObject&& InOther) noexcept;
~ManagedObject();

ManagedObject& operator=(const ManagedObject& InOther);
ManagedObject& operator=(ManagedObject&& InOther) noexcept;

template<typename TReturn, typename... TArgs>
TReturn InvokeMethod(std::string_view InMethodName, TArgs&&... InParameters) const
{
Expand Down Expand Up @@ -130,7 +138,7 @@ namespace Coral {

private:
void* m_Handle = nullptr;
const Type* m_Type;
const Type* m_Type = nullptr;

private:
friend class ManagedAssembly;
Expand Down
9 changes: 5 additions & 4 deletions Coral.Native/Include/Coral/MessageLevel.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ namespace Coral {

enum class MessageLevel
{
Info = 1 << 0,
Warning = 1 << 1,
Error = 1 << 2,
All = Info | Warning | Error
Trace = 1 << 0,
Info = 1 << 1,
Warning = 1 << 2,
Error = 1 << 3,
All = Trace | Info | Warning | Error
};

template<typename T>
Expand Down
6 changes: 4 additions & 2 deletions Coral.Native/Include/Coral/Utility.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "Core.hpp"
#include "String.hpp"

namespace Coral {

Expand Down Expand Up @@ -54,7 +55,7 @@ namespace Coral {
return ManagedType::Double;
else if constexpr (std::same_as<TArg, bool>)
return ManagedType::Bool;
else if constexpr (std::same_as<TArg, std::string>)
else if constexpr (std::same_as<TArg, std::string> || std::same_as<TArg, Coral::String>)
return ManagedType::String;
else
return ManagedType::Unknown;
Expand All @@ -63,7 +64,8 @@ namespace Coral {
template <typename TArg, size_t TIndex>
inline void AddToArrayI(const void** InArgumentsArr, ManagedType* InParameterTypes, TArg&& InArg)
{
InParameterTypes[TIndex] = GetManagedType<std::remove_reference_t<TArg>>();
ManagedType managedType = GetManagedType<std::remove_const_t<std::remove_reference_t<TArg>>>();
InParameterTypes[TIndex] = managedType;

if constexpr (std::is_pointer_v<std::remove_reference_t<TArg>>)
{
Expand Down
2 changes: 2 additions & 0 deletions Coral.Native/Source/Coral/CoralManagedFunctions.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ namespace Coral {
#pragma endregion

using CreateObjectFn = void* (*)(TypeId, Bool32, const void**, const ManagedType*, int32_t);
using CopyObjectFn = void* (*)(void*);
using InvokeMethodFn = void (*)(void*, String, const void**, const ManagedType*, int32_t);
using InvokeMethodRetFn = void (*)(void*, String, const void**, const ManagedType*, int32_t, void*);
using InvokeStaticMethodFn = void (*)(TypeId, String, const void**, const ManagedType*, int32_t);
Expand Down Expand Up @@ -141,6 +142,7 @@ namespace Coral {
#pragma endregion

CreateObjectFn CreateObjectFptr = nullptr;
CopyObjectFn CopyObjectFptr = nullptr;
CreateAssemblyLoadContextFn CreateAssemblyLoadContextFptr = nullptr;
InvokeMethodFn InvokeMethodFptr = nullptr;
InvokeMethodRetFn InvokeMethodRetFptr = nullptr;
Expand Down
Loading