MacOS Support #27
MacOS Support #27
Conversation
- ManagedObject is basically a wrapper around GCHandle. Each ManagedObject needs to have its own GCHandle (each one is basically a reference to the managed resource, and prevents the GC from collecting the managed object if there are no references to it on the C# side). - NativeInstance helps managed objects wrap unmanaged resources. These need to be disposed properly (otherwise the corresponding GCHandle kicks around forever). NativeInstance now implements IDisposable. - If everything is done properly, then there should be no need to track GCHandles via AssemblyLoader::RegisterHandle() etc. However, there is no guarantee that users of Coral will do everything properly. In particular calling Dispose() on NativeInstance instances may be forgotten. To this end DEBUG build still tracks GCHandles and will produce warnings on assembly unload if there are GCHandles kicking around.
…ed to be resolved from that path - Also added MessageLevel.Trace
- Changed GCHandleType to Normal instead of Pinned for NativeInstances to work in some cases - Fixed values arrays not being marshalled correctly due to incorrect memory offsets - Fixed const not being removed in ManagedType deduction - Added Coral::Array::New(const void*, size_t)
WalkthroughThe changes in this pull request involve enhancements across multiple files in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
Coral.Native/Include/Coral/Core.hpp (1)
31-36: Fix indentation inconsistencyThe MacOS section uses spaces while the rest of the file uses tabs. Maintain consistent indentation.
- #define CORAL_CALLTYPE - #define CORAL_STR(s) s - #define CORAL_HOSTFXR_NAME "libhostfxr.dylib" - - using CharType = char; - using StringView = std::string_view; + #define CORAL_CALLTYPE + #define CORAL_STR(s) s + #define CORAL_HOSTFXR_NAME "libhostfxr.dylib" + + using CharType = char; + using StringView = std::string_view;Coral.Managed/Source/InteropTypes.cs (2)
Line range hint
72-77: Add safety documentation for the public constructorMaking this constructor public exposes direct pointer manipulation. While necessary for the Map/Unmap functionality, it could be dangerous if misused.
Consider adding:
- XML documentation explaining proper usage and risks
- Debug assertions to validate input parameters
[EditorBrowsable(EditorBrowsableState.Advanced)]attribute to indicate it's for advanced scenarios+ /// <summary> + /// Creates a NativeArray from existing native memory. Use with extreme caution. + /// </summary> + /// <param name="InArray">Pointer to native memory containing the array data</param> + /// <param name="InHandle">Optional GC handle for pinned managed arrays</param> + /// <param name="InLength">Number of elements in the array</param> + [EditorBrowsable(EditorBrowsableState.Advanced)] public NativeArray(IntPtr InArray, IntPtr InHandle, int InLength) { + Debug.Assert(InArray != IntPtr.Zero || InLength == 0, "Array pointer cannot be null for non-empty arrays"); + Debug.Assert(InLength >= 0, "Length must be non-negative"); m_NativeArray = InArray; m_ArrayHandle = InHandle; m_NativeLength = InLength; }
Line range hint
171-198: Enhance the Dispose pattern implementationThe current implementation is good but could be improved for better safety and maintainability.
Consider these enhancements:
public struct NativeInstance<T> : IDisposable { + private bool m_IsDisposed; private IntPtr m_Handle; private readonly IntPtr m_Unused; + private void ThrowIfDisposed() + { + if (m_IsDisposed) + throw new ObjectDisposedException(typeof(T).Name); + } public void Dispose() { - if (m_Handle != IntPtr.Zero) + if (!m_IsDisposed) { - var handle = GCHandle.FromIntPtr(m_Handle); -#if DEBUG - var type = handle.Target?.GetType(); - if (type is not null) + if (m_Handle != IntPtr.Zero) { - AssemblyLoader.DeregisterHandle(type.Assembly, handle); + 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; } -#endif - handle.Free(); - m_Handle = IntPtr.Zero; + m_IsDisposed = true; } GC.SuppressFinalize(this); }Also update the
Getmethod to check for disposed state:public T? Get() { + ThrowIfDisposed(); if (m_Handle == IntPtr.Zero) return default;Coral.Native/Source/Coral/CoralManagedFunctions.hpp (1)
72-72: Document the purpose and ownership semantics of CopyObjectFnWhile the function signature is clear, it would be beneficial to document:
- Whether this performs a deep or shallow copy
- Memory ownership responsibilities
- Any platform-specific considerations for memory management
Add documentation above the function pointer type:
+// Copies a managed object. Returns a pointer to the new copy. +// Note: Caller is responsible for managing the returned object's lifecycle. using CopyObjectFn = void* (*)(void*);Coral.Managed/Source/Marshalling.cs (2)
Line range hint
20-25: Ensure consistent struct layout with ValueArrayContainerThe
ObjectArrayContainermirrors the layout ofValueArrayContainer, which is good for consistency. However, we should document why these structs need separate implementations despite having identical layouts.Consider adding a comment explaining the distinction between these containers and why they can't be unified using generics.
Line range hint
12-25: Consider using platform-specific attributes for struct layoutSince these structs need to match the native
Coral::Arraylayout, consider usingStructLayoutattribute with explicit field offsets to ensure consistent layout across platforms. This would make the platform-specific requirements more explicit and maintainable.Example approach:
[StructLayout(LayoutKind.Explicit)] struct ValueArrayContainer { [FieldOffset(0)] public IntPtr Data; [FieldOffset(8)] public IntPtr ArrayHandle; [FieldOffset(16)] public int Length; }Coral.Managed/Source/AssemblyLoader.cs (2)
Line range hint
141-180: Ensure thread safety when iterating overs_AllocatedHandlesduring unloadingWhen unloading the
AssemblyLoadContext, the code iterates overs_AllocatedHandlesto check for unfreed handles. If accessed concurrently, this may lead to race conditions. Implement synchronization mechanisms or use a thread-safe collection to prevent concurrent modification issues.
Line range hint
306-340: Implement thread safety inRegisterHandleandDeregisterHandlemethodsThe
RegisterHandleandDeregisterHandlemethods modify thes_AllocatedHandlesdictionary, which can be accessed by multiple threads. To prevent race conditions and ensure data integrity, consider synchronizing access to this shared resource or using a thread-safe collection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
Coral.Managed/Source/AssemblyLoader.cs(8 hunks)Coral.Managed/Source/InteropTypes.cs(4 hunks)Coral.Managed/Source/ManagedHost.cs(1 hunks)Coral.Managed/Source/ManagedObject.cs(2 hunks)Coral.Managed/Source/Marshalling.cs(1 hunks)Coral.Native/Include/Coral/Array.hpp(1 hunks)Coral.Native/Include/Coral/Core.hpp(2 hunks)Coral.Native/Include/Coral/ManagedObject.hpp(2 hunks)Coral.Native/Include/Coral/MessageLevel.hpp(1 hunks)Coral.Native/Include/Coral/Utility.hpp(3 hunks)Coral.Native/Source/Coral/CoralManagedFunctions.hpp(2 hunks)Coral.Native/Source/Coral/HostInstance.cpp(5 hunks)Coral.Native/Source/Coral/ManagedObject.cpp(1 hunks)
🔇 Additional comments (33)
Coral.Native/Include/Coral/MessageLevel.hpp (2)
11-15:
While the new enum values follow a logical progression (1,2,4,8), changing the underlying values of existing enum members is a breaking change that could affect:
- Serialized logs or configurations using these values
- External tools or systems interpreting these values
- Cross-platform compatibility if different platforms use different values
Please ensure all dependent code and systems are updated accordingly.
Let's check for potential usage that might be affected:
11-15: Clarify relationship to MacOS support
The PR is titled "MacOS Support", but these enum changes appear unrelated to platform-specific functionality. Please clarify:
- How these message level changes relate to MacOS support
- Whether these changes should be in a separate PR focused on logging enhancements
Let's check if there are platform-specific uses of these message levels:
Coral.Managed/Source/ManagedHost.cs (1)
10-10: Review the MessageLevel enum changes
The enum values follow a binary flag pattern (1, 2, 4, 8), which is good for bitwise operations. However, ensure that all message handlers and logging systems are updated to handle the new Trace level correctly.
Let's verify the usage of MessageLevel throughout the codebase:
✅ Verification successful
MessageLevel enum changes are properly handled throughout the codebase
Based on the verification results, all usages of MessageLevel are consistent with the binary flag pattern. The codebase shows proper handling of all levels including Trace:
- AssemblyLoader.cs uses Trace level appropriately for detailed assembly resolution logging
- Error level is used consistently for error conditions across all files
- Warning level is used for non-critical issues like unfreed objects
- Info level is used for general status updates like assembly loading
- No instances found where message levels are combined with bitwise operations, suggesting the binary flag pattern is currently unused
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of MessageLevel enum to ensure all handlers support the new Trace level
# Find all files using MessageLevel
rg -l "MessageLevel"
# Look for message handling or logging related code
ast-grep --pattern 'MessageLevel.$_'
Length of output: 7245
Coral.Native/Include/Coral/Core.hpp (4)
Line range hint 5-21: LGTM: Improved preprocessor directive clarity
The change from #ifdef to #if defined() makes the platform check more explicit and consistent with other platform checks.
22-29: LGTM: Explicit Linux platform check
The change to use #elif defined(CORAL_LINUX) makes the Linux-specific code path explicit rather than a fallback case, improving code clarity.
33-33: Verify the hostfxr path on MacOS
The path to the .NET host FXR library needs verification on MacOS systems.
✅ Verification successful
The hostfxr path on MacOS is correctly defined
The verification shows that the codebase has proper platform-specific definitions for the hostfxr library path:
- Windows:
hostfxr.dll - Linux:
libhostfxr.so - MacOS:
libhostfxr.dylib
The libhostfxr.dylib naming follows the standard MacOS dynamic library convention, and the implementation in HostInstance.cpp correctly handles the loading of this library across all platforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing references to hostfxr paths in the codebase
rg -i "hostfxr" --type cpp --type hpp
# Search for any MacOS-specific path handling code
rg -i "getexepath|getapplicationpath|bundlepath" --type cpp
Length of output: 127
Script:
#!/bin/bash
# Search for hostfxr references with correct file extensions
rg -i "hostfxr" -g "*.h" -g "*.hpp" -g "*.cpp"
# Search for MacOS-specific path handling with correct file extensions
rg -i "getexepath|getapplicationpath|bundlepath" -g "*.h" -g "*.hpp" -g "*.cpp"
# Look for other platform-specific definitions
rg -i "CORAL_HOSTFXR" -g "*.h" -g "*.hpp" -g "*.cpp"
Length of output: 8155
Line range hint 5-37: Verify platform-specific type usage
The changes to platform-specific types and definitions might affect dependent code.
✅ Verification successful
Platform-specific type changes are properly integrated across the codebase
The verification shows that the platform-specific types (CharType and StringView) are consistently used throughout the codebase:
- Core type definitions in
Core.hppare correctly referenced in string handling (String.hpp,String.cpp) - Memory management properly uses these types (
Memory.hpp,Memory.cpp) - Host instance functionality correctly handles platform-specific string types
- Platform-specific code paths are properly guarded with
CORAL_WINDOWSandCORAL_LINUXdefines
The changes maintain consistency across the codebase's string handling and platform-specific functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of CharType and StringView
ast-grep --pattern 'CharType $_'
ast-grep --pattern 'StringView $_'
# Search for platform-specific code paths
rg "#if.*CORAL_(WINDOWS|LINUX|MACOS)" --type cpp --type hpp
Length of output: 161
Script:
#!/bin/bash
# Search for usage of CharType and StringView with correct file extensions
rg "CharType" -t cpp -t h -t hpp -A 2 -B 2
rg "StringView" -t cpp -t h -t hpp -A 2 -B 2
# Search for platform-specific code paths with correct file extensions
rg "#if.*CORAL_(WINDOWS|LINUX)" -t cpp -t h -t hpp
# Find all header and source files to understand the codebase structure
fd -e cpp -e h -e hpp
Length of output: 1816
Script:
#!/bin/bash
# Search with correct paths for the specific files we found
cat "Coral.Native/Include/Coral/Core.hpp" "Coral.Native/Include/Coral/String.hpp" "Coral.Native/Source/Coral/String.cpp"
# Search for CharType and StringView usage in specific paths
rg "CharType" "Coral.Native/Include/Coral/" "Coral.Native/Source/Coral/"
rg "StringView" "Coral.Native/Include/Coral/" "Coral.Native/Source/Coral/"
# Search for platform-specific defines in specific paths
rg "#if.*CORAL_(WINDOWS|LINUX)" "Coral.Native/Include/Coral/" "Coral.Native/Source/Coral/"
Length of output: 7366
Coral.Native/Include/Coral/Utility.hpp (4)
4-4: LGTM: Header inclusion is platform-independent
The String.hpp header inclusion uses relative paths which are compatible across platforms, including MacOS.
67-68: LGTM: Improved type handling ensures cross-platform consistency
The enhanced type handling with std::remove_const_t and std::remove_reference_t ensures consistent behavior across all platforms, including MacOS.
58-59: Verify string type handling across platforms
While the addition of Coral::String support is good, we should ensure consistent string handling across platforms.
Let's check for any platform-specific string handling:
✅ Verification successful
Let me gather more information about the string handling implementation and its usage:
String handling is properly implemented across platforms
The implementation of Coral::String is well-designed with proper platform-specific handling:
- Windows: Uses
wchar_t(wide chars) when_WCHAR_T_DEFINED - Linux/MacOS: Uses standard
chartype - Proper UTF-8/Wide string conversions are implemented where needed
- Memory management is consistent across platforms using
CoTaskMem
The addition of Coral::String alongside std::string in GetManagedType is correct and safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for platform-specific string handling
rg -g '*.{hpp,cpp,h}' -l 'ifdef.*APPLE.*String|ifdef.*WIN32.*String|ifdef.*LINUX.*String'
# Check String.hpp implementation for platform specifics
fd -g 'String.hpp' --exec cat {}
Length of output: 1501
Script:
#!/bin/bash
# Check Core.hpp for CharType definition and platform specifics
fd -g 'Core.hpp' --exec cat {}
# Look for String class usage across the codebase
rg -g '*.{hpp,cpp}' 'Coral::String' -C 2
# Check String class implementation
fd -g 'String.cpp' --exec cat {}
Length of output: 6095
Line range hint 1-100: Verify relationship to MacOS support
While the changes in this file are good improvements to type handling and string support, they appear to be platform-agnostic and don't directly address MacOS-specific paths or functionality as mentioned in the PR objectives.
Let's verify if there are other files that contain the MacOS-specific changes:
✅ Verification successful
MacOS support changes are present and relevant
The changes in Utility.hpp are indeed part of the MacOS support effort. The file contains platform-agnostic type system improvements that are necessary for cross-platform compatibility, working in conjunction with the MacOS-specific changes in HostInstance.cpp, which includes:
- Platform-specific .NET runtime path detection for MacOS at
/usr/local/share/dotnet/host/fxr/ - Conditional compilation blocks for MacOS using
CORAL_MACOSX - Cross-platform dynamic library loading using
dlopenon Unix-like systems (including MacOS)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MacOS-specific code paths
rg -g '*.{hpp,cpp,h}' '(?i)(ifdef|if defined).*APPLE|darwin|macos'
# Search for Dotnet path handling mentioned in PR description
rg -g '*.{hpp,cpp,h}' 'GetDotNetPath|dotnet.*path'
Length of output: 192
Script:
#!/bin/bash
# Let's check the content of HostInstance.cpp to understand the MacOS-specific changes
cat "Coral.Native/Source/Coral/HostInstance.cpp"
# Also search for any platform-specific defines or configurations
rg -g '*.{hpp,cpp,h}' 'CORAL_MACOSX|CORAL_PLATFORM'
Length of output: 18635
Coral.Native/Include/Coral/Array.hpp (2)
Line range hint 109-112: Verify usage of unused member variables
The class contains member variables that appear unused in the implementation:
m_ArrayHandle: No usage found in the provided codem_IsDisposed: No disposal logic implemented
This could lead to memory leaks or undefined behavior, especially important for MacOS support.
Let's check for their usage across the codebase:
40-45: Consider memory alignment requirements for MacOS
MacOS has strict alignment requirements. The current implementation doesn't ensure proper alignment of the allocated memory, which could cause crashes on MacOS.
Let's verify the alignment requirements in the codebase:
Coral.Native/Include/Coral/ManagedObject.hpp (3)
141-141: LGTM! Consistent member initialization.
The nullptr initialization of m_Type matches m_Handle's initialization, ensuring consistent object state and making IsValid() checks more reliable.
15-21: Consider adding self-assignment check and thread safety guarantees.
While the addition of special member functions improves resource management, consider the following suggestions:
- Add self-assignment check in the copy assignment operator
- Document thread safety guarantees for these operations
- Consider using RAII wrappers for platform-independent resource management
Example implementation pattern for the assignment operator:
ManagedObject& operator=(const ManagedObject& InOther) {
if (this != &InOther) { // Self-assignment check
Destroy();
// ... copy implementation
}
return *this;
}These changes seem unrelated to MacOS support. Let's verify if there are any platform-specific implementations:
Line range hint 1-146: Verify alignment with MacOS support objectives.
While these changes improve the codebase through better resource management, they don't appear to directly contribute to MacOS support as mentioned in the PR objectives. Please clarify if there are platform-specific implementations of these methods elsewhere in the codebase.
Let's check for platform-specific implementations:
Coral.Managed/Source/InteropTypes.cs (1)
215-215: Verify the safety of changing GCHandleType from Pinned to Normal
The change from GCHandleType.Pinned to GCHandleType.Normal improves GC performance by allowing object relocation, but we need to verify that no native code assumes the object's memory location remains fixed.
Let's scan for potential unsafe usage:
✅ Verification successful
Based on the search results, I can make a determination. The codebase shows that the GCHandle is primarily used for managed-to-native interop through the NativeInstance<T> type, where the handle is used to keep track of managed objects that are referenced by native code. The key observations:
- The GCHandle is only used to maintain references to managed objects that are passed to native code
- The native code never directly accesses the memory of these objects - it only uses the handle as an opaque reference
- All actual memory access is done through proper marshaling in the
Marshalling.csclass - No unsafe pointer operations are performed on the GCHandled objects themselves
The change from Pinned to Normal GCHandle is safe
The change from GCHandleType.Pinned to GCHandleType.Normal is safe because:
- The handle is only used as an opaque reference by native code
- No direct pointer operations are performed on the memory of GCHandled objects
- All data marshaling is done through proper Marshal API calls that handle object movement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for unsafe pointer usage that might assume pinned memory
# Search for unsafe blocks or IntPtr usage that might indicate direct pointer manipulation
rg -A 5 "unsafe|IntPtr|fixed|Marshal\.Copy|Marshal\.Write|Marshal\.Read" --type cs
# Search for P/Invoke declarations that might pass this object's memory to native code
ast-grep --pattern 'DllImport($_)'
Length of output: 57504
Coral.Native/Source/Coral/CoralManagedFunctions.hpp (2)
72-72: Clarify relationship with MacOS support
The changes appear to be general memory management improvements rather than MacOS-specific path modifications as described in the PR objectives. Please clarify how these changes relate to MacOS support or consider submitting them in a separate PR focused on memory management enhancements.
Let's verify if there are related MacOS-specific changes:
Also applies to: 145-145
145-145: Verify integration with MacOS memory management
The addition of CopyObjectFptr enhances object management capabilities, but we should ensure it works correctly across all platforms.
Run the following to check for platform-specific implementations:
✅ Verification successful
Let me gather more information about platform-specific implementations.
Let me do one final check to specifically look for MacOS-related memory management code.
Memory management implementation is platform-agnostic and safe
The CopyObjectFptr implementation is built on .NET's platform-agnostic memory management system using GCHandle, which provides consistent behavior across all platforms including MacOS. The implementation:
- Uses standard .NET memory management through
GCHandlefor object lifetime control - Employs consistent marshaling patterns for cross-platform interop
- Has no platform-specific branches in the memory management code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for platform-specific implementations of CopyObject
rg -g '!*.hpp' "CopyObject"
# Check for any platform-specific memory management code
ast-grep --pattern 'if ($PLATFORM) {
$$$
CopyObject
$$$
}'
Length of output: 627
Script:
#!/bin/bash
# Check for platform-specific code around CopyObject usage
rg -B 5 -A 5 "CopyObject"
# Look for any platform-specific defines or conditions in relevant files
rg -g "*.cpp" -g "*.cs" "#if.*PLATFORM|#ifdef.*PLATFORM|#if.*MACOS|#ifdef.*MACOS" Coral.Native/ Coral.Managed/
Length of output: 6893
Script:
#!/bin/bash
# Check for any MacOS specific memory management
rg -g "*.cpp" -g "*.hpp" -g "*.cs" -i "macos|darwin|apple|osx" -B 3 -A 3 Coral.Native/ Coral.Managed/
# Look for platform-specific memory management in ManagedObject implementation
rg -g "*.cs" "GCHandle|Marshal" Coral.Managed/Source/ManagedObject.cs
Length of output: 2812
Coral.Managed/Source/Marshalling.cs (1)
12-17: Verify the struct layout matches Coral::Array on MacOS
The addition of ArrayHandle for MacOS compatibility looks good. However, we should verify that the struct layout exactly matches Coral::Array on MacOS to prevent ABI issues.
✅ Verification successful
The struct layout correctly matches Coral::Array native implementation
The ValueArrayContainer struct layout in the managed code correctly mirrors the native Coral::Array template class implementation, which also includes the m_ArrayHandle field. The field ordering matches:
- Data pointer (
m_Ptr) - Array handle pointer (
m_ArrayHandle) - Length (
m_Length)
This ensures proper ABI compatibility across platforms, including MacOS.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Coral::Array definition to verify the layout
ast-grep --pattern 'struct Array {
$$$
}'
# Search for any platform-specific struct definitions
rg -g '*.{h,hpp,cpp}' -A 5 '#ifdef __APPLE__.*struct\s+Array'
Length of output: 109
Script:
#!/bin/bash
# Let's try searching for Array in header files with broader context
rg -g '*.{h,hpp}' -B 2 -A 5 'struct\s+Array'
# Also search for any references to Array in source files
rg -g '*.{cpp}' -B 2 -A 5 'Array::'
# Look for any platform-specific code blocks
rg -g '*.{h,hpp,cpp}' -B 2 -A 5 '#if.*(__APPLE__|__linux__|_WIN32).*Array'
# Search for files that might contain the native Array definition
fd -e h -e hpp "Array"
Length of output: 219
Script:
#!/bin/bash
# Let's examine the Array.hpp file content
cat Coral.Native/Include/Coral/Array.hpp
# Also search for any platform-specific implementations in this file
rg -p --context 5 "(__APPLE__|__linux__|_WIN32)" Coral.Native/Include/Coral/Array.hpp
Length of output: 2699
Coral.Native/Source/Coral/ManagedObject.cpp (4)
11-18: Copy constructor implementation looks good
The copy constructor correctly duplicates the object's handle and type when the source object has a valid handle.
20-24: Move constructor correctly transfers ownership
The move constructor properly transfers ownership of m_Handle and m_Type from the source object and nullifies the source's members.
26-29: Destructor ensures proper resource cleanup
The destructor calls Destroy() to release any resources held by the object, ensuring proper cleanup.
44-57: Copy assignment operator correctly manages resources
The copy assignment operator properly handles self-assignment, calls Destroy() to release existing resources, and duplicates the handle and type when the source object has a valid handle.
Coral.Managed/Source/AssemblyLoader.cs (2)
62-63: Logging assembly resolution at Trace level is appropriate
Adding this log statement at the Trace level aids in debugging assembly resolution without impacting performance in production environments.
90-97:
Review security implications of loading assemblies from base directory
Loading assemblies from AppContext.BaseDirectory may introduce security risks if untrusted assemblies are present in the directory. Ensure that only trusted directories are used for assembly loading, and consider implementing strong-name verification or other security measures to validate the assemblies.
Run the following script to list all assemblies in the base directory and check for untrusted files:
✅ Verification successful
Loading assemblies from base directory is controlled within a secure context
The analysis reveals that the assembly loading implementation is secure for the following reasons:
- The AssemblyLoader operates within isolated AssemblyLoadContexts that provide security boundaries
- Assembly loading is controlled through a dedicated resolver (ResolveAssembly) that's registered with the load context
- The loader maintains a cache of trusted assemblies and validates assemblies before loading
- The base directory usage is limited to the application's own context (s_CoralAssemblyLoadContext) which is initialized at startup
- Assembly loading is tracked with proper error handling and logging for security auditing
The implementation follows secure assembly loading practices by using isolated contexts and maintaining proper security boundaries. No additional security measures are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all .dll files in the base directory to identify potential untrusted assemblies.
# Expected: Only trusted assemblies should be present.
cd "$(dirname "$0")" || exit
find . -maxdepth 1 -name '*.dll' -exec ls -l {} \;
Length of output: 135
Script:
#!/bin/bash
# Let's analyze the codebase to understand the assembly loading context and security measures
# Check for any security validation or verification before loading assemblies
rg -B 2 -A 2 "LoadFromAssemblyPath|LoadFrom" --type cs
# Look for any strong name or assembly verification
rg -B 2 -A 2 "StrongName|AssemblyName|AssemblyVerification" --type cs
# Search for any security-related configuration or validation
ast-grep --pattern 'class AssemblyLoader {
$$$
}'
Length of output: 3412
Script:
#!/bin/bash
# Let's analyze how this assembly loader is used and what security context it operates in
# Check for any security-related configuration or assembly validation
rg -B 5 -A 5 "AssemblyLoader" --type cs
# Look for any custom assembly load contexts or security boundaries
rg -B 3 -A 3 "AssemblyLoadContext" --type cs
# Check if there's any validation of assembly sources or restrictions
ast-grep --pattern 'try {
$$$
LoadFromAssemblyPath($$$)
$$$
}'
Length of output: 17356
Coral.Managed/Source/ManagedObject.cs (3)
145-155: Good enhancement: Added exception handling in CreateObject method
The addition of the try-catch block in the CreateObject method improves robustness by handling potential exceptions during object creation, ensuring the application doesn't crash unexpectedly.
145-149: Proper registration of GCHandle in DEBUG mode
Registering the GCHandle with AssemblyLoader in DEBUG mode aids in tracking handle usage and facilitates debugging. This practice enhances resource management during development.
189-196: Good addition: Deregistering GCHandle in DestroyObject method
Including the deregistration of the GCHandle with AssemblyLoader in the DestroyObject method ensures that handles are properly tracked and resources are cleaned up in DEBUG mode. This enhancement improves memory management and debugging processes.
Coral.Native/Source/Coral/HostInstance.cpp (4)
20-20: Addition of SetRuntimePropertyValue Function Pointer
The SetRuntimePropertyValue function pointer is appropriately added to the CoreCLRFunctions struct to enable setting runtime properties, enhancing runtime configuration flexibility.
219-219: Loading SetRuntimePropertyValue Function Pointer
The SetRuntimePropertyValue function pointer is correctly loaded using LoadFunctionPtr, ensuring that runtime properties can be set as needed.
335-335: Addition of CopyObjectFptr to Managed Functions
The CopyObjectFptr function pointer is properly added to the s_ManagedFunctions struct, extending object management capabilities within the managed environment.
243-249: Verify Character Encoding Consistency When Setting Runtime Properties
When setting the APP_CONTEXT_BASE_DIRECTORY property, ensure that coralDirectoryPath.c_str() returns the correct string type matching the expected character encoding of SetRuntimePropertyValue. This is especially important when handling wide characters with CORAL_WIDE_CHARS defined. Inconsistent encoding may lead to unexpected behavior on different platforms.
To confirm that the character types are consistent across platforms, you can review the definitions of CORAL_WIDE_CHARS and verify the types used:
✅ Verification successful
Character encoding is correctly handled in the runtime property setting
The implementation is correct and type-safe. Here's why:
- When
CORAL_WIDE_CHARSis defined,char_tis typedef'd aswchar_tin hostfxr.h, matching the wide-string literalL"APP_CONTEXT_BASE_DIRECTORY"andcoralDirectoryPath.c_str()which returnswchar_t*due toCharTypebeingwchar_t - When
CORAL_WIDE_CHARSis not defined,char_tis typedef'd aschar, matching the narrow string literal and the narrow string returned byc_str()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the character types match when setting runtime properties.
# Check where CORAL_WIDE_CHARS is defined
rg "define\s+CORAL_WIDE_CHARS"
# Search for the definition of SetRuntimePropertyValue
rg "typedef.*hostfxr_set_runtime_property_value_fn"
# Verify the types of coralDirectoryPath.c_str()
Length of output: 241
Script:
#!/bin/bash
# Let's check the implementation and usage of SetRuntimePropertyValue to verify type consistency
# Check the CoreCLRFunctions class definition and implementation
rg -A 5 "class CoreCLRFunctions"
# Check the hostfxr.h file content around the typedef to see parameter types
rg -B 2 -A 5 "typedef.*hostfxr_set_runtime_property_value_fn" NetCore/hostfxr.h
# Look for any string type definitions or conversions related to this
rg "SetRuntimePropertyValue.*c_str" -A 2 -B 2
Length of output: 1032
Script:
#!/bin/bash
# Let's check the char_t definition and its relation to CORAL_WIDE_CHARS
# Check char_t definition
rg "typedef.*char_t"
# Check if there are any platform-specific character type definitions
rg -B 3 -A 3 "CORAL_WIDE_CHARS" Coral.Native/Include/Coral/Core.hpp
# Look for any string/character type handling in the codebase
rg "using.*char_t|typedef.*char_t"
Length of output: 1411
| #else | ||
| #define CORAL_CALLTYPE | ||
| #define CORAL_STR(s) s | ||
| #define CORAL_HOSTFXR_NAME "libhostfxr.dylib" | ||
|
|
||
| using CharType = char; | ||
| using StringView = std::string_view; | ||
|
|
There was a problem hiding this comment.
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.
| #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" |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | |
| } |
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.
| 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; | |
| } |
| ManagedObject& ManagedObject::operator=(ManagedObject&& InOther) noexcept | ||
| { | ||
| if (this != &InOther) | ||
| { | ||
| m_Handle = InOther.m_Handle; | ||
| m_Type = InOther.m_Type; | ||
| InOther.m_Handle = nullptr; | ||
| InOther.m_Type = nullptr; | ||
| } | ||
|
|
||
| return *this; | ||
| } |
There was a problem hiding this comment.
Potential resource leak in move assignment operator
The move assignment operator does not release existing resources before overwriting m_Handle and m_Type. This could lead to resource leaks if the object already owns resources.
Apply the following diff to fix the issue:
ManagedObject& ManagedObject::operator=(ManagedObject&& InOther) noexcept
{
+ if (this != &InOther)
+ {
+ Destroy();
m_Handle = InOther.m_Handle;
m_Type = InOther.m_Type;
InOther.m_Handle = nullptr;
InOther.m_Type = nullptr;
+ }
return *this;
}Committable suggestion skipped: line range outside the PR's diff.
| #if DEBUG | ||
| private static readonly Dictionary<int, List<GCHandle>> s_AllocatedHandles = new(); | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
| [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 |
There was a problem hiding this comment.
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.
| [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 |
| #elif defined(CORAL_MACOSX) | ||
| auto searchPaths = std::array | ||
| { | ||
| std::filesystem::path("/usr/lib/dotnet/host/fxr/"), | ||
| std::filesystem::path("/usr/local/share/dotnet/host/fxr/"), | ||
| }; |
There was a problem hiding this comment.
Specify Template Arguments for std::array Initialization
The std::array requires explicit template arguments specifying the element type and size. Currently, searchPaths is initialized without these arguments, which may lead to compilation errors. Please specify the template arguments as shown below:
#elif defined(CORAL_MACOSX)
- auto searchPaths = std::array
+ auto searchPaths = std::array<std::filesystem::path, 2>
{
std::filesystem::path("/usr/lib/dotnet/host/fxr/"),
std::filesystem::path("/usr/local/share/dotnet/host/fxr/"),
};📝 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.
| #elif defined(CORAL_MACOSX) | |
| auto searchPaths = std::array | |
| { | |
| std::filesystem::path("/usr/lib/dotnet/host/fxr/"), | |
| std::filesystem::path("/usr/local/share/dotnet/host/fxr/"), | |
| }; | |
| #elif defined(CORAL_MACOSX) | |
| auto searchPaths = std::array<std::filesystem::path, 2> | |
| { | |
| std::filesystem::path("/usr/lib/dotnet/host/fxr/"), | |
| std::filesystem::path("/usr/local/share/dotnet/host/fxr/"), | |
| }; |
Updated paths for MacOS Dotnet, slightly different than on Linux, also added MacOS in Core.h. Quite self-explanatory to be fair.
Summary by CodeRabbit
Release Notes
New Features
Tracelevel to message logging for improved detail.Arrayclass functionality to initialize from raw memory buffers.Bug Fixes
Documentation
Refactor
Chores