Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a POSIX host target and tooling: CI workflows and Azure pipeline POSIX builds, POSIX-compatible CLR/HAL/PAL implementations and headers, a cross-platform native library loader, .NET host/packaging updates, and a POSIX test harness for macOS/Linux. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Managed Host App
participant Loader as NativeNanoClrLoader
participant FS as Filesystem
participant Runtime as NativeLibrary API
App->>Loader: EnsureInitialized()
App->>Loader: ConfigureSearchDirectory(path)?
App->>Loader: Request load "nanoFramework.nanoCLR"
Loader->>Loader: if cached -> return handle
Loader->>FS: Probe candidate paths (configured, AppContext, runtimes/*, NanoCLR/*, assembly dir)
alt Found candidate
FS-->>Loader: candidate path
Loader->>Runtime: NativeLibrary.TryLoad(candidate)
Runtime-->>Loader: handle on success
Loader-->>App: return handle/path
else None found
Loader->>Runtime: NativeLibrary.TryLoad("nanoFramework.nanoCLR")
Runtime-->>Loader: handle or fail
Loader-->>App: success or throw (DllNotFoundException on macOS)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
targets/netcore/nanoFramework.nanoCLR.Host/packages.lock.json (1)
4-14:⚠️ Potential issue | 🟡 MinorUpdate System.IO.Ports to align with net10.0 target.
The lock file specifies System.IO.Ports 8.0.0 for the net10.0 target framework. Newer stable versions (10.0.5, 10.0.6) are available and should be used instead for better compatibility with .NET 10.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/netcore/nanoFramework.nanoCLR.Host/packages.lock.json` around lines 4 - 14, The lock file pins System.IO.Ports to 8.0.0 under the net10.0 target; update the resolved and requested entries for "System.IO.Ports" (and its "runtime.native.System.IO.Ports" dependency) to a current stable .NET 10 compatible version (e.g., 10.0.5 or 10.0.6) so the net10.0 section reflects the newer package; ensure both the "requested" range and the "resolved" value for "System.IO.Ports" are adjusted and that the "dependencies" entry for "runtime.native.System.IO.Ports" is updated to the matching version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/posix-nanoclr.yml:
- Around line 64-71: The smoke step is invoking the wrong artifact; replace
references to running and uploading build/posix/bin/nanoFramework.nanoCLR with
the actual runnable test binary nanoFramework.nanoCLR.test (built by
targets/posix/nanoCLR/CMakeLists.txt) located under build/posix/bin, and when
uploading artifacts point to the produced shared library nanoFramework.nanoCLR
in build/posix/lib only if you intend to publish the shared library; otherwise
ensure both the test executable (nanoFramework.nanoCLR.test) and the shared
library (nanoFramework.nanoCLR) paths are correctly used for run and upload
respectively so the grep validation runs against the real executable output.
In @.vscode/posix-port-plan.md:
- Line 90: The fenced code blocks in the Markdown use plain ``` fences and
trigger markdownlint MD040; update each unlabeled triple-backtick fence (the
ones currently containing shell commands or plain output) by appending an
explicit language identifier such as ```bash for shell snippets or ```text for
non-code output so that each fenced block is labeled (apply to the fences
flagged at the positions referenced in the review).
In `@src/CLR/Include/nanoCLR_Hardware.h`:
- Around line 98-99: Add a compile-time size assertion for 64-bit POSIX builds:
when PLATFORM_POSIX_HOST and __LP64__ are defined, add a CT_ASSERT verifying
sizeof(HalInterruptRecord) equals 40 (accounting for 8-byte m_context pointer
and alignment padding) instead of skipping the check; reference
HalInterruptRecord and CT_ASSERT so the assertion sits alongside the existing
conditional for PLATFORM_POSIX_HOST / __LP64__.
In `@src/CLR/Include/nanoCLR_Runtime.h`:
- Around line 3911-3913: The POSIX 64-bit exemption is inconsistently applied:
update the preprocessor guards around CLR_RT_HeapBlock_Raw and
CLR_RT_DataTypeLookup to require both PLATFORM_POSIX_HOST and __LP64__ (i.e.,
replace checks that only test PLATFORM_POSIX_HOST with PLATFORM_POSIX_HOST &&
__LP64__) so the same LP64 requirement used for CLR_RT_HeapBlock is enforced for
those layout checks; locate the existing guards around the CLR_RT_HeapBlock_Raw
and CLR_RT_DataTypeLookup definitions and change them to include the __LP64__
condition (or alternatively document in CMake that POSIX targets must be
64-bit).
In `@targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cs`:
- Around line 48-50: The current code in ExecuteCommandProcessor (the branch
using RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) merely prints a
warning for unsupported transports (--serialport / --namedpipe) and continues;
change this to fail fast with a non-zero CLI error: detect when the user
requested --serialport or --namedpipe on non-Windows inside the same branch, log
a clear error via the existing logging mechanism, and then terminate the command
with a non-zero exit (e.g., throw a user-facing exception or return a non-zero
status from the method) so the process does not return 0; update the handling in
the same code paths referenced by
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) and any related branches
around those transport flag checks to ensure consistent non-zero failure.
In `@targets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csproj`:
- Line 37: Update the mismatched framework identifiers so the project metadata
uses the same TargetFramework everywhere: change the PackagePath value
(currently "tools\net8.0\any\NanoCLR") and PackageTfmSubFolder (the nodes around
lines 74-76) to match the declared TargetFramework "net10.0"; ensure the XML
elements PackagePath and PackageTfmSubFolder are updated consistently with
TargetFramework so all references (TargetFramework, PackagePath,
PackageTfmSubFolder) use "net10.0".
- Around line 32-49: The macOS and Linux <None> items currently set Pack="False"
will exclude their native libraries from the NuGet package; change the <None>
elements referencing ..\..\..\build\posix\lib\nanoFramework.nanoCLR.dylib and
..\..\..\build\posix\lib\nanoFramework.nanoCLR.so to Pack="True" and add
appropriate PackagePath values that use runtime identifiers (mirroring the
Windows entry pattern, e.g., tools\net8.0\any\NanoCLR or runtime-specific paths)
so the .dylib and .so are included in the nuget; update the two <None> elements
for the macOS and Linux entries to set Pack="True" and provide matching
PackagePath entries.
In `@targets/netcore/nanoFramework.nanoCLR.CLI/Program.cs`:
- Around line 111-113: The non-Windows branch currently throws a bare
CLIException with ExitCode.E9000 making failures cryptic; update the throw in
the ternary fallback (the branch used when
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) is false) to construct a
CLIException that includes a clear "virtual-serial/Windows-only" message (e.g.,
"virtual-serial command is supported only on Windows") alongside the
ExitCode.E9000 so VirtualSerialDeviceCommandProcessor.ProcessVerb callers get a
self-describing error.
In `@targets/netcore/nanoFramework.nanoCLR.Host/Interop/NativeNanoClrLoader.cs`:
- Around line 188-194: The duplicate-path check in NativeNanoClrLoader.cs
(inside the loop comparing probeDirectory to normalizedPath) uses
StringComparison.Ordinal which is case-sensitive and will treat semantically
identical paths as different on case-insensitive file systems; update the
comparison to use StringComparison.OrdinalIgnoreCase (or otherwise normalize
casing) when comparing probeDirectory and normalizedPath in the method that adds
probe directories so that paths are compared case-insensitively on
Windows/macOS.
In `@targets/posix/CMakeLists.txt`:
- Line 29: The POSIX top-level CMakeLists currently only calls
add_subdirectory(nanoCLR), violating the target layout rule requiring
nanoBooter, nanoCLR, and common subdirs; update targets/posix/CMakeLists.txt to
include stubbed subdirectories for nanoBooter and common (e.g.,
add_subdirectory(nanoBooter) and add_subdirectory(common)) and add a short
comment documenting that POSIX intentionally has no booter implementation (or
point to the exception documentation) so the directory structure matches the
repository convention while clearly noting the special case for the nanoBooter
stub.
- Around line 16-24: The cache default for NANO_POSIX_ARCH should use the host
processor instead of hardcoding "arm64"; update the initialization of
NANO_POSIX_ARCH to default to CMAKE_HOST_SYSTEM_PROCESSOR (while still declaring
it as a CACHE STRING and keeping the allowed values via set_property and
validation), so a plain cmake configure on an x86_64 Mac picks x86_64 unless the
user explicitly overrides NANO_POSIX_ARCH; then continue to set
CMAKE_OSX_ARCHITECTURES to NANO_POSIX_ARCH as before.
In `@targets/posix/Include/nanoPAL.h`:
- Around line 26-27: The single-line guard clauses like "if (!dst || size == 0)
return 1;" in the inline helper(s) in nanoPAL.h must be converted to
Allman-style braced blocks; replace the single-line if bodies with a braced form
(opening brace on its own line, closing brace on its own line) for the shown
guard and the other occurrence (the similar guard at lines 34-35) so both inline
helpers follow the repo's C/C++ Allman brace style.
- Around line 24-40: Validate src != NULL at the start of both hal_strcpy_s and
hal_strncpy_s and return 1 on violation; compute the actual source length (use
strlen for hal_strcpy_s and strnlen(src, count) for hal_strncpy_s), detect
truncation (if src length >= size or count >= size as appropriate), perform a
bounded copy up to size-1 and null-terminate, and return 1 when
truncation/constraint violation occurs (return 0 only on full success).
Reference functions: hal_strcpy_s and hal_strncpy_s.
In `@targets/posix/Include/target_BlockStorage.h`:
- Around line 1-2: Replace the current top-of-file license text in
target_BlockStorage.h with the standard POSIX header license block used across
other new POSIX headers in this repo; locate the file target_BlockStorage.h and
update its header comment to match the project's canonical license format (same
wording, ordering, and SPDX/notice style used by other POSIX headers) so it is
consistent with the rest of the codebase.
In `@targets/posix/Include/target_os.h`:
- Around line 1-2: Replace the current license block in
targets/posix/Include/target_os.h so it matches the project standard used in
target_common.h: start the file with the exact two-line header "// Copyright (c)
.NET Foundation and Contributors" followed by "// See LICENSE file in the
project root for full license information." and remove or replace any differing
comment lines so the header text and order exactly match the project's canonical
license header.
In `@targets/posix/Include/targetHAL_Time.h`:
- Around line 1-2: Update the file header in
targets/posix/Include/targetHAL_Time.h to match the coding guidelines by
replacing the existing two-line license comment with the exact required header:
the first line should be "// Copyright (c) .NET Foundation and Contributors" and
the second line should be "// See LICENSE file in the project root for full
license information."; ensure there is no extra text or formatting around these
two lines and keep them at the very top of the file.
In `@targets/posix/nanoCLR/CLRStartup.cpp`:
- Around line 428-440: WcharToString currently masks the low byte and silently
corrupts non-ASCII wchar_t values; update WcharToString to assert or validate
the ASCII-only assumption before converting by checking each wchar_t for
high-byte data (e.g., ensure (ch & ~0x7F) == 0 or ch <= 0x7F) and fail fast in
debug builds (assert or log+return) if a non-ASCII character is encountered;
keep the existing low-byte conversion for valid ASCII values so callers using
WcharToString (function name) will get an explicit debug-time failure if the
ASCII assumption is violated.
- Around line 391-419: The destructor currently duplicates the cleanup logic by
deleting entries in m_assemblies; replace the manual loop in ~Settings() with a
call to Cleanup() so cleanup logic is centralized: change ~Settings() to invoke
Cleanup() (and keep the existing Settings() constructor unchanged), and ensure
Cleanup() remains idempotent and safe to call multiple times (no assumptions
about prior state or double-free of m_assemblies entries).
- Around line 131-156: In LoadAssembly the buffer is deleted inside the
header->GoodAssembly() failure branch then the cleanup path also deletes it on
FAILED(hr), causing a double-delete; remove the immediate delete in the failure
branch (or if you prefer keep it there then set buffer = nullptr) and ensure
only the cleanup block deletes buffer when hr indicates failure and buffer is
non-null; reference function LoadAssembly, local variable buffer,
header->GoodAssembly(), m_assemblies assignment, and the FAILED(hr) cleanup
check so the buffer is either stored once or deleted exactly once.
In `@targets/posix/nanoCLR/FileStore_POSIX.cpp`:
- Around line 13-31: In nanoCLR_POSIX_LoadFile, refuse non-regular or unbounded
inputs before reading: use POSIX stat on the provided path and return an error
if !(S_ISREG(st_mode)) to reject FIFOs/devices, then check st_size from stat and
ensure it is non-negative and below a reasonable cap (or SIZE_MAX) before
allocating; if the size is acceptable, reserve/resize content and read exactly
st_size bytes rather than using istreambuf_iterator; update error messages
(e.g., "Not a regular file: " or "File too large: ") and return false on any
check failure.
In `@targets/posix/nanoCLR/main.cpp`:
- Around line 83-89: The loop that populates peFiles currently pushes every
positional argument except the literal "--assemblies"; change it to only push
arguments that have a ".pe" extension (e.g., check argv[i] ends with ".pe",
case-insensitive if needed) and skip everything else (including other flags and
non-PE paths). Update the block that iterates argc/argv and the push_back call
to validate the extension before calling peFiles.push_back(argv[i]) so only
valid managed assembly paths are enqueued.
- Around line 51-59: The current read logic allows short/partial reads to be
treated as success; after calling in.read(reinterpret_cast<char *>(out.data()),
size) check that the number of bytes actually read equals the requested size and
treat any short read as a failure: use in.gcount() (or compare out.size()/size)
and fail if in.gcount() != size or if the stream has an error (i.e., !in), then
log the same error message referencing path and return false so a truncated
buffer cannot be passed to nanoCLR_LoadAssembly().
In `@targets/posix/nanoCLR/Memory.cpp`:
- Around line 14-17: The file-scope heap variables s_MemoryStart,
s_CustomHeapStart, s_DefaultHeapOnce and s_CustomHeapOnce must be renamed to
follow the repo global prefix; rename them to g_MemoryStart, g_CustomHeapStart,
g_DefaultHeapOnce and g_CustomHeapOnce respectively, keep their types and
initialization the same, and update every use/assignment in Memory.cpp (and any
internal headers/translation units that reference them) to the new names so all
symbol references compile correctly.
In `@targets/posix/nanoCLR/nanoCLR_native_posix.cpp`:
- Around line 162-164: The requiredSize calculation undercounts per-assembly
bytes causing potential buffer overruns; update the calculation that sets
requiredSize (currently using g_CLR_InteropAssembliesCount * (5 *
sizeof(uint16_t) + 128)) to use the actual bytes written per assembly:
sizeof(uint32_t) for checksum plus 4 * sizeof(uint16_t) for version fields plus
128 for the name buffer, i.e. g_CLR_InteropAssembliesCount * (sizeof(uint32_t) +
4 * sizeof(uint16_t) + 128), and keep the existing size < requiredSize check to
return false when insufficient.
- Around line 30-37: Several global variables violate naming conventions: rename
gDebugPrintCallback to g_DebugPrintCallback, WireProtocolTransmitCallback to
g_WireProtocolTransmitCallback, WireProtocolReceiveCallback to
g_WireProtocolReceiveCallback, and change _wireProtocolStopProcess to
g_WireProtocolStopProcess (remove leading underscore and use g_ prefix). Update
all references accordingly (e.g., any usage in posix_stubs.cpp or other files
that reference gDebugPrintCallback, WireProtocolTransmitCallback,
WireProtocolReceiveCallback, or _wireProtocolStopProcess) and ensure
declarations like DebugPrintCallback g_DebugPrintCallback, WireTransmitCallback
g_WireProtocolTransmitCallback, WireReceiveCallback
g_WireProtocolReceiveCallback, and static bool g_WireProtocolStopProcess replace
the old names consistently across the codebase.
- Around line 113-119: The stop flag _wireProtocolStopProcess is accessed by
nanoCLR_WireProtocolProcess and set by nanoCLR_WireProtocolClose from different
threads; make it thread-safe by including <atomic> and changing the flag's
declaration to std::atomic<bool> (replace the current bool
_wireProtocolStopProcess). Update any reads/writes (in
nanoCLR_WireProtocolProcess loop and nanoCLR_WireProtocolClose) to use the
atomic variable directly (e.g., store/load or assignment) and ensure
initialization uses a boolean value.
In `@targets/posix/nanoCLR/platform_selector.h`:
- Around line 11-17: Replace the use of bare `#if` checks with defined() checks:
update the preprocessor conditions that set PLATFORM_APPLE_HOST and
PLATFORM_LINUX_HOST to use `#if` defined(NANOCLR_HOST_APPLE) and `#if`
defined(NANOCLR_HOST_LINUX) respectively, so change the guards around the
PLATFORM_APPLE_HOST and PLATFORM_LINUX_HOST defines to use defined(...) for
robustness and consistency with the codebase.
In `@targets/posix/nanoCLR/posix_stubs.cpp`:
- Around line 37-44: The current debug path uses a fixed stack buffer char
buf[512] and silently truncates when (size_t)len >= 512; change this to detect
truncation and either allocate a dynamic buffer for the full message or at
minimum emit a warning when truncation happens: if (size_t)len < sizeof(buf) use
the existing stack buffer, otherwise allocate a heap buffer of size (len+1) (or
use std::string.resize(len+1)), copy the bytes, null terminate, call
gDebugPrintCallback(buf) and then free/delete the heap buffer; ensure
null-termination and no leaks and keep the gDebugPrintCallback(buf) call path
identical.
- Around line 90-107: The array g_CLR_InteropAssembliesNativeData is not mutated
after init, so make the array variable itself const to be defensive: change its
declaration to an array of const pointers (e.g. add a trailing const so the type
becomes "const CLR_RT_NativeAssemblyData * const
g_CLR_InteropAssembliesNativeData[]") and keep the initializer and sentinel
unchanged; adjust any corresponding extern/declaration signatures if they exist
to match the new const-qualified symbol and leave g_CLR_InteropAssembliesCount
computation as-is.
In `@targets/posix/nanoCLR/Target_HAL.cpp`:
- Around line 9-24: The HAL_Windows_Debug_Print function accepts a mutable char*
though it only reads the input; change the parameter type to const char* to
reflect immutability and allow callers to pass string literals safely, update
any internal uses (constructing std::string from szText, null check) to accept
const char*, and adjust any call sites if necessary to match the new signature
in HAL_Windows_Debug_Print.
In `@targets/posix/nanoCLR/targetHAL_Time.cpp`:
- Line 10: Replace the deprecated C header by changing the include directive
from `#include` <time.h> to `#include` <ctime> in targetHAL_Time.cpp (i.e., update
the existing include line). After switching headers, qualify any C library
symbols (e.g., time_t, tm, time(), localtime()) with std:: if needed or add
using declarations to match the rest of the codebase.
- Line 36: The constant k_epochOffset100ns uses the wrong prefix; rename it to
c_epochOffset100ns (keep it as static constexpr uint64_t with the same value)
and update all usages of k_epochOffset100ns in this file to the new name (e.g.,
the occurrences that currently reference k_epochOffset100ns). Ensure compilation
by updating all references in functions/expressions that rely on the constant.
- Around line 1-4: The file targets/posix/nanoCLR/targetHAL_Time.cpp has an
extra leading '//' line in its header; remove that extra line so the file begins
exactly with the two-line license header: "// Copyright (c) .NET Foundation and
Contributors" followed by "// See LICENSE file in the project root for full
license information."; update the top of targetHAL_Time.cpp accordingly so the
header matches project convention.
- Around line 97-114: HAL_Time_CurrentDateTimeToString is not thread-safe
because it uses a single static buffer (buf) and calls gmtime(), which may
return a static struct; change the static char buf[128] to a per-thread buffer
(e.g., thread_local char buf[128]) and replace gmtime(&sec) with the reentrant
gmtime_r(&sec, &tm_res) pattern: declare a local struct tm tm_res, call
gmtime_r(&sec, &tm_res) and use tm_res fields when formatting into buf (via
CLR_SafeSprintf) so concurrent calls to HAL_Time_CurrentDateTimeToString are
safe.
In `@targets/posix/nanoCLR/targetPAL_Events.cpp`:
- Around line 1-4: The file header in targetPAL_Events.cpp has a leading empty
comment line before the license; remove that blank "//" line so the first line
of the file begins exactly with "// Copyright (c) .NET Foundation and
Contributors" (ensure the rest of the existing license lines remain unchanged
and spacing preserved).
In `@targets/posix/README.md`:
- Around line 11-13: The README line that states "Managed PE assemblies are
loaded from memory buffers and executed end-to-end; the nanoFramework.Json test
suite passes 249/249 tests." hard-codes a test count that will drift; update the
sentence (the line starting with "Managed PE assemblies are loaded...") to use a
softer claim such as "the nanoFramework.Json test suite passes in CI" or "all
tests currently pass" and/or add a link to the CI job/results for the live
passing count instead of the literal "249/249".
---
Outside diff comments:
In `@targets/netcore/nanoFramework.nanoCLR.Host/packages.lock.json`:
- Around line 4-14: The lock file pins System.IO.Ports to 8.0.0 under the
net10.0 target; update the resolved and requested entries for "System.IO.Ports"
(and its "runtime.native.System.IO.Ports" dependency) to a current stable .NET
10 compatible version (e.g., 10.0.5 or 10.0.6) so the net10.0 section reflects
the newer package; ensure both the "requested" range and the "resolved" value
for "System.IO.Ports" are adjusted and that the "dependencies" entry for
"runtime.native.System.IO.Ports" is updated to the matching version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b46a336-4fcf-4b25-a0df-60e3a121eb8b
📒 Files selected for processing (59)
.github/workflows/posix-nanoclr.yml.vscode/posix-port-plan.mdazure-pipelines.ymlsrc/CLR/Core/Thread.cppsrc/CLR/Helpers/nanoprintf/nanoprintf.hsrc/CLR/Include/nanoCLR_Hardware.hsrc/CLR/Include/nanoCLR_PlatformDef.hsrc/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Include/nanoCLR_Runtime__HeapBlock.hsrc/CLR/Startup/CLRStartup.cppsrc/HAL/Include/nanoHAL.hsrc/HAL/Include/nanoHAL_v2.htargets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cstargets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cstargets/netcore/nanoFramework.nanoCLR.CLI/Program.cstargets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csprojtargets/netcore/nanoFramework.nanoCLR.CLI/packages.lock.jsontargets/netcore/nanoFramework.nanoCLR.Host/Interop/NativeNanoClrLoader.cstargets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cstargets/netcore/nanoFramework.nanoCLR.Host/nanoFramework.nanoCLR.Host.csprojtargets/netcore/nanoFramework.nanoCLR.Host/packages.lock.jsontargets/posix/CMakeLists.txttargets/posix/Include/TargetHAL_Spi.htargets/posix/Include/Target_System_IO_FileSystem.htargets/posix/Include/nanoCLR_native.htargets/posix/Include/nanoHAL_Boot.htargets/posix/Include/nanoHAL_Capabilites.htargets/posix/Include/nanoPAL.htargets/posix/Include/nanoPAL_Network.htargets/posix/Include/nf_config.htargets/posix/Include/stdafx.htargets/posix/Include/targetHAL.htargets/posix/Include/targetHAL_Power.htargets/posix/Include/targetHAL_Time.htargets/posix/Include/targetPAL_time.htargets/posix/Include/target_BlockStorage.htargets/posix/Include/target_board.htargets/posix/Include/target_common.htargets/posix/Include/target_os.htargets/posix/Include/target_platform.htargets/posix/README.mdtargets/posix/nanoCLR/CLRStartup.cpptargets/posix/nanoCLR/CMakeLists.txttargets/posix/nanoCLR/FileStore_POSIX.cpptargets/posix/nanoCLR/Memory.cpptargets/posix/nanoCLR/Target_BlockStorage.cpptargets/posix/nanoCLR/Target_HAL.cpptargets/posix/nanoCLR/Various.cpptargets/posix/nanoCLR/WatchDog.cpptargets/posix/nanoCLR/main.cpptargets/posix/nanoCLR/nanoCLR_native_posix.cpptargets/posix/nanoCLR/platform_heap.cpptargets/posix/nanoCLR/platform_selector.htargets/posix/nanoCLR/posix_headers_shim_test.cpptargets/posix/nanoCLR/posix_stubs.cpptargets/posix/nanoCLR/targetHAL_Time.cpptargets/posix/nanoCLR/targetPAL_Events.cpptargets/posix/nanoCLR/targetPAL_Time.cpptargets/posix/nanoCLR/targetRandom.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CLR/Startup/CLRStartup.cpp`:
- Line 170: The CRC call uses a narrowed 32-bit length that can wrap if assEnd
<= assStart or the span exceeds 32-bit; before calling SUPPORT_ComputeCRC,
validate that assEnd > assStart, compute the span in a size_t (e.g., size_t span
= (uintptr_t)assEnd - (uintptr_t)assStart), then if span exceeds UINT_MAX clamp
or handle as an error (e.g., log/set g_buildCRC to a safe default) and only pass
a safe unsigned int length to SUPPORT_ComputeCRC; reference g_buildCRC,
SUPPORT_ComputeCRC, assStart, and assEnd when making this change.
- Around line 164-167: The debug print uses "%x" which loses pointer width on
64-bit: change the CLR_Debug::Printf call to use "%p" and cast assStart and
assEnd to (void*) so full pointer values are logged; additionally, before the
CRC length computation (where you compute assEnd - assStart) add an explicit
check that assEnd >= assStart (or compare pointers) and handle the invalid case
(log an error via CLR_Debug::Printf and skip/return error) to avoid unsigned
wraparound and an enormous CRC length.
In `@src/HAL/Include/nanoHAL.h`:
- Line 892: Update the comment in nanoHAL.h that currently reads "// nothing to
define here just to help the nanoCLR VS project to build hapilly" by correcting
the typo "hapilly" to "happily" so the comment reads "...to build happily";
locate and edit the comment string in the header (near the existing placeholder
comment) to apply the fix.
- Around line 891-898: The current branch defines GLOBAL_LOCK(),
GLOBAL_UNLOCK(), and ASSERT_IRQ_MUST_BE_ON() as silent no-ops for
__arm__/PLATFORM_ESP32/PLATFORM_POSIX_HOST which can mask missing HAL
implementations; change the logic so that only PLATFORM_POSIX_HOST gets the
no-op fallback, and for __arm__ and PLATFORM_ESP32 remove the silent fallback
and emit a compile-time failure (e.g., use `#error` or a static_assert) requiring
the platform HAL to provide GLOBAL_LOCK/GLOBAL_UNLOCK/ASSERT_IRQ_MUST_BE_ON;
update the conditional around the macros (referencing GLOBAL_LOCK,
GLOBAL_UNLOCK, ASSERT_IRQ_MUST_BE_ON and the platform macros __arm__,
PLATFORM_ESP32, PLATFORM_POSIX_HOST) accordingly so embedded targets fail build
if these macros are not supplied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad046c39-d2d0-496c-a0d4-807dc86ba483
📒 Files selected for processing (4)
src/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Startup/CLRStartup.cppsrc/HAL/Include/nanoHAL.htargets/posix/Include/nanoCLR_native.h
Comment on lines
+164
to
+167
| CLR_Debug::Printf( | ||
| " Loading start at %x, end %x\r\n", | ||
| (unsigned int)(uintptr_t)assStart, | ||
| (unsigned int)(uintptr_t)assEnd); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what format specifiers CLR_Debug::Printf supports before applying %p.
# Expected: formatter implementation confirms %p support, or shows required alternative specifier.
rg -nP -C3 '\bCLR_Debug::Printf\s*\(' --type=cpp --type=c --type=h --type=hpp
rg -nP -C3 '(%p|PRIxPTR|PRIXPTR|%llx|%lx)' --type=cpp --type=c --type=h --type=hpp
rg -nP -C5 '(vsnprintf|snprintf|va_list|format)' --type=cpp --type=c --type=h --type=hppRepository: nanoframework/nf-interpreter
Length of output: 155
🏁 Script executed:
# First, check the actual file and lines mentioned
wc -l src/CLR/Startup/CLRStartup.cppRepository: nanoframework/nf-interpreter
Length of output: 106
🏁 Script executed:
# Read the specific lines in question (164-170 area)
sed -n '160,175p' src/CLR/Startup/CLRStartup.cppRepository: nanoframework/nf-interpreter
Length of output: 633
🏁 Script executed:
# Find CLR_Debug::Printf implementation
rg -n 'CLR_Debug::Printf\s*\(' --type=cpp --type=c -A 2 -B 2 | head -50Repository: nanoframework/nf-interpreter
Length of output: 3965
🏁 Script executed:
# Search for CLR_Debug class/namespace definition
rg -n 'class\s+CLR_Debug|namespace.*CLR_Debug' --type=cpp --type=c --type=hRepository: nanoframework/nf-interpreter
Length of output: 54
🏁 Script executed:
# Check how other code formats pointers in printf-style calls
rg -n '%p' --type=cpp --type=c | head -20Repository: nanoframework/nf-interpreter
Length of output: 2677
🏁 Script executed:
# Find CLR_Debug class definition/implementation
rg -n 'CLR_Debug' --type=cpp --type=c | grep -E '(class|struct|^\s*(namespace|void|int|void\s+\w+::))' | head -30Repository: nanoframework/nf-interpreter
Length of output: 354
🏁 Script executed:
# Search for CLR_Debug.h or similar header files
fd -e h -e hpp 'CLR_Debug' src/Repository: nanoframework/nf-interpreter
Length of output: 107
🏁 Script executed:
# Look for CLR_Debug::Printf implementation
rg -n 'CLR_Debug.*Printf' --type=cpp --type=c -B 5 -A 10 | head -80Repository: nanoframework/nf-interpreter
Length of output: 7691
🏁 Script executed:
# Find SUPPORT_ComputeCRC signature
rg -n 'SUPPORT_ComputeCRC' --type=cpp --type=c --type=h -B 2 -A 2 | head -40Repository: nanoframework/nf-interpreter
Length of output: 3448
🏁 Script executed:
# Check if there are any range validation checks before SUPPORT_ComputeCRC calls elsewhere
rg -n 'SUPPORT_ComputeCRC' --type=cpp --type=c -B 10 | head -80Repository: nanoframework/nf-interpreter
Length of output: 7112
🏁 Script executed:
# Read CLR_Debug class definition
sed -n '950,970p' src/CLR/Include/nanoCLR_Types.hRepository: nanoframework/nf-interpreter
Length of output: 515
🏁 Script executed:
# Find CLR_Debug::Printf implementation
fd -type f -name 'nanoCLR_Debugging.h' -o -name 'nanoCLR_Types.h' | xargs grep -l 'Printf'Repository: nanoframework/nf-interpreter
Length of output: 244
🏁 Script executed:
# Look for CLR_Debug implementation in nanoCLR_Debugging.h
cat src/CLR/Include/nanoCLR_Debugging.h | head -100Repository: nanoframework/nf-interpreter
Length of output: 4091
🏁 Script executed:
# Check the exact context around line 170 in the file under review
sed -n '165,175p' src/CLR/Startup/CLRStartup.cppRepository: nanoframework/nf-interpreter
Length of output: 505
🏁 Script executed:
# Verify the Windows version to compare differences
sed -n '215,225p' targets/win32/nanoCLR/CLRStartup.cppRepository: nanoframework/nf-interpreter
Length of output: 569
🏁 Script executed:
# Check if CheckKnownAssembliesForNonXIP validates the range
rg -n 'CheckKnownAssembliesForNonXIP' --type=cpp --type=c -B 3 -A 15 | head -60Repository: nanoframework/nf-interpreter
Length of output: 4854
🏁 Script executed:
# Look for the definition of CheckKnownAssembliesForNonXIP
rg -n 'void.*CheckKnownAssembliesForNonXIP|HRESULT.*CheckKnownAssembliesForNonXIP' --type=cpp --type=c -A 20 | head -80Repository: nanoframework/nf-interpreter
Length of output: 5799
🏁 Script executed:
# Verify if standard C printf %p is used elsewhere in CLR_Debug calls
rg -n 'CLR_Debug::Printf.*%p' --type=cpp --type=c | head -20Repository: nanoframework/nf-interpreter
Length of output: 373
🏁 Script executed:
# Check if there's any validation of assStart/assEnd relationship before the CRC call
sed -n '140,175p' src/CLR/Startup/CLRStartup.cppRepository: nanoframework/nf-interpreter
Length of output: 1123
Fix pointer format specifiers and add range validation for CRC computation.
Lines 164-167: Use %p instead of %x with casts to preserve full pointer values on 64-bit hosts where debug logs are essential for troubleshooting.
Line 170: Add explicit validation that assEnd >= assStart before computing the CRC length. The current unsigned subtraction can silently wrap to an invalid large value if the range is invalid.
Suggested fixes
- CLR_Debug::Printf(
- " Loading start at %x, end %x\r\n",
- (unsigned int)(uintptr_t)assStart,
- (unsigned int)(uintptr_t)assEnd);
+ CLR_Debug::Printf(" Loading start at %p, end %p\r\n", assStart, assEnd);+ if (assEnd < assStart)
+ {
+ CLR_Debug::Printf("ERROR: Invalid assembly range (end < start)\r\n");
+ NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER);
+ }
+
g_buildCRC = SUPPORT_ComputeCRC(assStart, (unsigned int)((uintptr_t)assEnd - (uintptr_t)assStart), 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLR/Startup/CLRStartup.cpp` around lines 164 - 167, The debug print uses
"%x" which loses pointer width on 64-bit: change the CLR_Debug::Printf call to
use "%p" and cast assStart and assEnd to (void*) so full pointer values are
logged; additionally, before the CRC length computation (where you compute
assEnd - assStart) add an explicit check that assEnd >= assStart (or compare
pointers) and handle the invalid case (log an error via CLR_Debug::Printf and
skip/return error) to avoid unsigned wraparound and an enormous CRC length.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| #endif | ||
|
|
||
| g_buildCRC = SUPPORT_ComputeCRC(assStart, (unsigned int)assEnd - (unsigned int)assStart, 0); | ||
| g_buildCRC = SUPPORT_ComputeCRC(assStart, (unsigned int)((uintptr_t)assEnd - (uintptr_t)assStart), 0); |
Contributor
There was a problem hiding this comment.
Validate assembly span before CRC length narrowing (Line 170).
This computation can wrap/truncate if assEnd < assStart or span exceeds 32-bit, potentially causing SUPPORT_ComputeCRC to read invalid memory.
🛠️ Suggested fix
- g_buildCRC = SUPPORT_ComputeCRC(assStart, (unsigned int)((uintptr_t)assEnd - (uintptr_t)assStart), 0);
+ if (assEnd < assStart)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER);
+ }
+
+ const uintptr_t assemblySpan = static_cast<uintptr_t>(assEnd - assStart);
+ if (assemblySpan > UINT32_MAX)
+ {
+ NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER);
+ }
+
+ g_buildCRC = SUPPORT_ComputeCRC(assStart, static_cast<uint32_t>(assemblySpan), 0);📝 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
| g_buildCRC = SUPPORT_ComputeCRC(assStart, (unsigned int)((uintptr_t)assEnd - (uintptr_t)assStart), 0); | |
| if (assEnd < assStart) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); | |
| } | |
| const uintptr_t assemblySpan = static_cast<uintptr_t>(assEnd - assStart); | |
| if (assemblySpan > UINT32_MAX) | |
| { | |
| NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); | |
| } | |
| g_buildCRC = SUPPORT_ComputeCRC(assStart, static_cast<uint32_t>(assemblySpan), 0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLR/Startup/CLRStartup.cpp` at line 170, The CRC call uses a narrowed
32-bit length that can wrap if assEnd <= assStart or the span exceeds 32-bit;
before calling SUPPORT_ComputeCRC, validate that assEnd > assStart, compute the
span in a size_t (e.g., size_t span = (uintptr_t)assEnd - (uintptr_t)assStart),
then if span exceeds UINT_MAX clamp or handle as an error (e.g., log/set
g_buildCRC to a safe default) and only pass a safe unsigned int length to
SUPPORT_ComputeCRC; reference g_buildCRC, SUPPORT_ComputeCRC, assStart, and
assEnd when making this change.
Member
Author
There was a problem hiding this comment.
@josesimoes I guess we can forget about this one as we always be under 32-bit, correct?
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+891
to
+898
| #elif defined(__arm__) || defined(PLATFORM_ESP32) || defined(PLATFORM_POSIX_HOST) | ||
| // nothing to define here just to help the nanoCLR VS project to build hapilly | ||
| // for POSIX: GLOBAL_LOCK/UNLOCK will be provided by the platform HAL layer | ||
| #ifndef GLOBAL_LOCK | ||
| #define GLOBAL_LOCK() | ||
| #define GLOBAL_UNLOCK() | ||
| #define ASSERT_IRQ_MUST_BE_ON() | ||
| #endif |
Contributor
There was a problem hiding this comment.
Avoid silent no-op lock fallback for embedded branches.
Line 894 currently allows GLOBAL_LOCK()/GLOBAL_UNLOCK() to become no-ops for the combined __arm__/ESP32/POSIX branch when target macros are missing. This can hide embedded HAL integration defects and introduce runtime races instead of failing at build time.
🔧 Proposed fix
-#elif defined(__arm__) || defined(PLATFORM_ESP32) || defined(PLATFORM_POSIX_HOST)
-// nothing to define here just to help the nanoCLR VS project to build hapilly
-// for POSIX: GLOBAL_LOCK/UNLOCK will be provided by the platform HAL layer
-#ifndef GLOBAL_LOCK
-#define GLOBAL_LOCK()
-#define GLOBAL_UNLOCK()
-#define ASSERT_IRQ_MUST_BE_ON()
-#endif
+#elif defined(__arm__) || defined(PLATFORM_ESP32)
+// Embedded targets must provide lock primitives in targetHAL.h
+
+#elif defined(PLATFORM_POSIX_HOST)
+// nothing to define here just to help the nanoCLR VS project to build happily
+// for POSIX: GLOBAL_LOCK/UNLOCK may be provided by the platform HAL layer
+#ifndef GLOBAL_LOCK
+#define GLOBAL_LOCK()
+#define GLOBAL_UNLOCK()
+#define ASSERT_IRQ_MUST_BE_ON()
+#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HAL/Include/nanoHAL.h` around lines 891 - 898, The current branch defines
GLOBAL_LOCK(), GLOBAL_UNLOCK(), and ASSERT_IRQ_MUST_BE_ON() as silent no-ops for
__arm__/PLATFORM_ESP32/PLATFORM_POSIX_HOST which can mask missing HAL
implementations; change the logic so that only PLATFORM_POSIX_HOST gets the
no-op fallback, and for __arm__ and PLATFORM_ESP32 remove the silent fallback
and emit a compile-time failure (e.g., use `#error` or a static_assert) requiring
the platform HAL to provide GLOBAL_LOCK/GLOBAL_UNLOCK/ASSERT_IRQ_MUST_BE_ON;
update the conditional around the macros (referencing GLOBAL_LOCK,
GLOBAL_UNLOCK, ASSERT_IRQ_MUST_BE_ON and the platform macros __arm__,
PLATFORM_ESP32, PLATFORM_POSIX_HOST) accordingly so embedded targets fail build
if these macros are not supplied.
Member
Author
There was a problem hiding this comment.
@josesimoes I also would love to get your feedback on this one
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
targets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csproj (1)
5-5:⚠️ Potential issue | 🟡 MinorTargetFramework mismatch with PackagePath and PackageTfmSubFolder.
The project targets
net10.0(Line 5), butPackagePath(Lines 37, 43, 49) andPackageTfmSubFolder(Line 77) still referencenet8.0. These should be aligned to avoid confusion during packaging.💡 Proposed fix
<!-- Windows: DLL is produced by the WIN32 build under build/bin/$(Configuration)/ --> <None Condition="$([MSBuild]::IsOSPlatform('Windows'))" Include="..\..\..\build\bin\$(Configuration)\nanoFramework.nanoCLR.dll" Link="NanoCLR\nanoFramework.nanoCLR.dll"> <Pack>True</Pack> - <PackagePath>tools\net8.0\any\NanoCLR</PackagePath> + <PackagePath>tools\net10.0\any\NanoCLR</PackagePath> <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> </None> <!-- macOS: dylib is produced by the POSIX build under build/posix/lib/ --> <None Condition="$([MSBuild]::IsOSPlatform('OSX'))" Include="..\..\..\build\posix\lib\nanoFramework.nanoCLR.dylib" Link="NanoCLR\nanoFramework.nanoCLR.dylib"> <Pack>True</Pack> - <PackagePath>tools\net8.0\any\NanoCLR</PackagePath> + <PackagePath>tools\net10.0\any\NanoCLR</PackagePath> <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> </None> <!-- Linux: .so is produced by the POSIX build under build/posix/lib/ --> <None Condition="$([MSBuild]::IsOSPlatform('Linux'))" Include="..\..\..\build\posix\lib\nanoFramework.nanoCLR.so" Link="NanoCLR\nanoFramework.nanoCLR.so"> <Pack>True</Pack> - <PackagePath>tools\net8.0\any\NanoCLR</PackagePath> + <PackagePath>tools\net10.0\any\NanoCLR</PackagePath> <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> </None><PropertyGroup> - <!-- set this to 'net8.0\' to package contents in this TFM folder --> - <PackageTfmSubFolder>net8.0\</PackageTfmSubFolder> + <!-- set this to 'net10.0\' to package contents in this TFM folder --> + <PackageTfmSubFolder>net10.0\</PackageTfmSubFolder> </PropertyGroup>Also applies to: 37-37, 75-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csproj` at line 5, The Package metadata values are inconsistent with the project's TargetFramework; update all occurrences of "net8.0" used in PackagePath and PackageTfmSubFolder to match the TargetFramework "net10.0" so packaging paths align with TargetFramework; specifically change the PackagePath attributes (the entries that currently reference net8.0) and the PackageTfmSubFolder property to "net10.0" so they match the TargetFramework element.targets/posix/Include/nanoPAL.h (1)
26-27: 🛠️ Refactor suggestion | 🟠 MajorUse braced Allman
ifblocks for guard clauses.The single-line
ifbodies at lines 26-27 and 41-42 should use Allman-style braces per the project's coding guidelines.♻️ Proposed fix
inline int hal_strcpy_s(char *dst, size_t size, const char *src) { - if (!dst || !src || size == 0) - return 1; + if (!dst || !src || size == 0) + { + return 1; + } size_t srcLen = strlen(src);inline int hal_strncpy_s(char *dst, size_t size, const char *src, size_t count) { - if (!dst || !src || size == 0) - return 1; + if (!dst || !src || size == 0) + { + return 1; + } size_t n = count < size - 1 ? count : size - 1;As per coding guidelines,
**/*.{cpp,c,h,hpp}: Use Allman brace style: opening brace on its own line after functions, classes, and control statements.Also applies to: 41-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/posix/Include/nanoPAL.h` around lines 26 - 27, The guard clause using a single-line if (if (!dst || !src || size == 0) return 1;) should be converted to Allman-style braced form; replace the single-line body with a block using opening brace on its own line, the return inside the block, and closing brace on its own line. Apply the same change to the other single-line guard (the similar if at lines containing if (!dst || !src || size == 0) and the second occurrence noted in the review) so both guard clauses follow the project's Allman brace style for control statements.targets/posix/nanoCLR/main.cpp (2)
82-88: 🧹 Nitpick | 🔵 TrivialConsider validating
.pefile extension.The loop accepts any positional argument as an assembly path. While the comment documents that only
.pefiles should be provided, stray arguments or typos will fail later with a less obvious error fromnanoCLR_LoadAssembly.💡 Proposed validation
for (int i = 1; i < argc; i++) { - if (std::strcmp(argv[i], "--assemblies") == 0) - continue; // skip the flag itself + std::string arg = argv[i]; - peFiles.push_back(argv[i]); + if (arg == "--assemblies") + { + continue; // skip the flag itself + } + + // Validate .pe extension + if (arg.size() < 3 || arg.substr(arg.size() - 3) != ".pe") + { + std::cerr << "warning: skipping non-.pe argument '" << arg << "'\n"; + continue; + } + + peFiles.push_back(arg); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/posix/nanoCLR/main.cpp` around lines 82 - 88, The loop in main.cpp that populates peFiles accepts any positional argument; validate that each argv[i] has a .pe extension (case-insensitive) before pushing to peFiles and either skip/emit an error or exit with non-zero so nanoCLR_LoadAssembly won’t get unexpected inputs; update the for-loop around peFiles.push_back to check the filename suffix (using std::string or std::filesystem::path) and log a clear error mentioning the offending argument when it doesn’t end with ".pe".
84-85: 🛠️ Refactor suggestion | 🟠 MajorUse braced Allman
ifblocks.Several single-line
ifstatements lack braces, which violates the project's coding guidelines.♻️ Proposed fix
for (int i = 1; i < argc; i++) { if (std::strcmp(argv[i], "--assemblies") == 0) - continue; // skip the flag itself + { + continue; // skip the flag itself + } peFiles.push_back(argv[i]); }std::vector<uint8_t> data; if (!ReadFile(path.c_str(), data)) - return 1; + { + return 1; + }if (!peFiles.empty()) - std::cout << "\n"; + { + std::cout << "\n"; + }As per coding guidelines,
**/*.{cpp,c,h,hpp}: Use Allman brace style: opening brace on its own line after functions, classes, and control statements.Also applies to: 94-95, 109-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/posix/nanoCLR/main.cpp` around lines 84 - 85, Several single-line if statements (e.g., the check using std::strcmp(argv[i], "--assemblies") == 0 and the other ifs around the same block at lines referenced in the review) are missing braces; update each to use Allman-style braced blocks: replace single-line if ... continue; with a two-line braced form (open brace on its own line, statement(s) inside, closing brace on its own line) so the control statements around argv parsing use proper braces and match the project's Allman style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@targets/posix/Include/nanoPAL.h`:
- Around line 52-54: The macro name __isnanf is reserved; rename it to a
non-reserved identifier (e.g., nanoPAL_isnanf or hal_isnanf) and update its
definition to `#define` nanoPAL_isnanf(x) std::isnan(static_cast<float>(x)); then
search and replace all call sites that reference __isnanf to use the new macro
name (nanoPAL_isnanf) so the behavior (casting to float and calling std::isnan)
remains identical.
In `@targets/posix/Include/targetPAL_Time.h`:
- Around line 4-7: The header is empty but must declare the functions
implemented in targetPAL_Time.cpp; add prototypes for Time_SetCompare (with
extern "C" linkage) and nanoCLR_POSIX_GetNextCompareValue using the exact
signatures used in the .cpp so other translation units can link to them; wrap
the prototypes in an extern "C" { } block when __cplusplus is defined and
include any needed standard typedef headers (e.g., stdint.h) if those types are
used by the function signatures.
In `@targets/posix/nanoCLR/CLRStartup.cpp`:
- Around line 160-200: In LoadAssembliesSet, when
CLR_RT_Assembly::CreateInstance(headerSub, assm) fails, don’t silently break—log
a debug/error message containing identifying info (e.g., header summary, asmSize
or header->m_szName if available) and delete bufferSub, then continue parsing
the next assembly instead of breaking; update the failure branch around
CLR_RT_Assembly::CreateInstance to call your logging facility, clean up
bufferSub, and advance header (same rounding logic) to keep processing remaining
assemblies before returning S_OK.
In `@targets/posix/nanoCLR/targetPAL_Time.cpp`:
- Around line 21-24: The function nanoCLR_POSIX_GetNextCompareValue is defined
but not declared in the public header; either add its prototype to
targetPAL_Time.h (e.g., uint64_t nanoCLR_POSIX_GetNextCompareValue(void);) so
other translation units can use it, or if it should be internal to the
compilation unit make the function static (change its definition to static
uint64_t nanoCLR_POSIX_GetNextCompareValue(void)). Update the header or the
function linkage accordingly and rebuild to confirm no unresolved symbol
warnings remain.
---
Duplicate comments:
In `@targets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csproj`:
- Line 5: The Package metadata values are inconsistent with the project's
TargetFramework; update all occurrences of "net8.0" used in PackagePath and
PackageTfmSubFolder to match the TargetFramework "net10.0" so packaging paths
align with TargetFramework; specifically change the PackagePath attributes (the
entries that currently reference net8.0) and the PackageTfmSubFolder property to
"net10.0" so they match the TargetFramework element.
In `@targets/posix/Include/nanoPAL.h`:
- Around line 26-27: The guard clause using a single-line if (if (!dst || !src
|| size == 0) return 1;) should be converted to Allman-style braced form;
replace the single-line body with a block using opening brace on its own line,
the return inside the block, and closing brace on its own line. Apply the same
change to the other single-line guard (the similar if at lines containing if
(!dst || !src || size == 0) and the second occurrence noted in the review) so
both guard clauses follow the project's Allman brace style for control
statements.
In `@targets/posix/nanoCLR/main.cpp`:
- Around line 82-88: The loop in main.cpp that populates peFiles accepts any
positional argument; validate that each argv[i] has a .pe extension
(case-insensitive) before pushing to peFiles and either skip/emit an error or
exit with non-zero so nanoCLR_LoadAssembly won’t get unexpected inputs; update
the for-loop around peFiles.push_back to check the filename suffix (using
std::string or std::filesystem::path) and log a clear error mentioning the
offending argument when it doesn’t end with ".pe".
- Around line 84-85: Several single-line if statements (e.g., the check using
std::strcmp(argv[i], "--assemblies") == 0 and the other ifs around the same
block at lines referenced in the review) are missing braces; update each to use
Allman-style braced blocks: replace single-line if ... continue; with a two-line
braced form (open brace on its own line, statement(s) inside, closing brace on
its own line) so the control statements around argv parsing use proper braces
and match the project's Allman style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 209940af-8038-4836-ab01-3da41b98fc53
📒 Files selected for processing (15)
.github/workflows/posix-nanoclr.ymlsrc/CLR/Include/nanoCLR_Hardware.hsrc/CLR/Include/nanoCLR_Runtime.htargets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csprojtargets/posix/CMakeLists.txttargets/posix/Include/nanoCLR_native.htargets/posix/Include/nanoPAL.htargets/posix/Include/stdafx.htargets/posix/Include/targetPAL_Time.htargets/posix/Include/target_common.htargets/posix/nanoCLR/CLRStartup.cpptargets/posix/nanoCLR/main.cpptargets/posix/nanoCLR/nanoCLR_native_posix.cpptargets/posix/nanoCLR/posix_stubs.cpptargets/posix/nanoCLR/targetPAL_Time.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
targets/posix/nanoCLR/FileStore_POSIX.cpp (1)
31-50:⚠️ Potential issue | 🟠 MajorBounded read is still bypassable by post-
statfile growth.Line 44 reads to EOF with iterators. If file size changes between
stat()and read, this can exceed the intended cap and allocate beyond the validated size. Prefer reading exactlyst.st_sizebytes.🛠️ Suggested change
- if (st.st_size > k_MaxFileBytes) + if (st.st_size < 0 || st.st_size > c_MaxFileBytes) { error = "File too large (> 64 MiB): " + path; return false; } @@ - content.assign(std::istreambuf_iterator<char>(in), std::istreambuf_iterator<char>()); - - if (in.bad() || (in.fail() && !in.eof())) + const auto expectedSize = static_cast<size_t>(st.st_size); + content.resize(expectedSize); + + if (expectedSize > 0) + { + in.read(reinterpret_cast<char *>(content.data()), static_cast<std::streamsize>(expectedSize)); + if (in.gcount() != static_cast<std::streamsize>(expectedSize)) + { + error = "Failed reading file: " + path; + content.clear(); + return false; + } + } + + if (in.bad()) { error = "Failed reading file: " + path; content.clear(); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/posix/nanoCLR/FileStore_POSIX.cpp` around lines 31 - 50, The current read uses streambuf iterators (content.assign(std::istreambuf_iterator<char>(in), ...)) which can read to EOF and be tricked if the file grows after stat(); instead, read exactly st.st_size bytes and enforce k_MaxFileBytes: after opening the std::ifstream (in) allocate content.resize(st.st_size) and use in.read(&content[0], st.st_size) (or readsome/seek+read) then check in.gcount()/in.fail()/in.bad() to detect short/failed reads; ensure you still clear content and set error on failure and preserve the earlier size check against k_MaxFileBytes (reference st, k_MaxFileBytes, content.assign, and in).targets/posix/nanoCLR/posix_stubs.cpp (1)
115-132: 🧹 Nitpick | 🔵 TrivialConsider making
g_CLR_InteropAssembliesNativeDatafully const.The array of pointers is never modified after initialization. Making the variable itself
constwould be more defensive and silence the static analysis warning.♻️ Suggested fix
-const CLR_RT_NativeAssemblyData *g_CLR_InteropAssembliesNativeData[] = { +const CLR_RT_NativeAssemblyData *const g_CLR_InteropAssembliesNativeData[] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@targets/posix/nanoCLR/posix_stubs.cpp` around lines 115 - 132, The array g_CLR_InteropAssembliesNativeData should be made fully const to prevent modification and satisfy static analysis: change its declaration so the pointer array is const as well (e.g., const CLR_RT_NativeAssemblyData * const g_CLR_InteropAssembliesNativeData[]), leaving the initializer and the sentinel nullptr intact; no other logic changes are needed, and g_CLR_InteropAssembliesCount can remain as-is referencing ARRAYSIZE(g_CLR_InteropAssembliesNativeData) - 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@targets/netcore/nanoFramework.nanoCLR.Host/Interop/NativeNanoClrLoader.cs`:
- Around line 199-222: The NormalizeDirectory method compares fullPath to
rootPath using StringComparison.Ordinal which can mismatch the OrdinalIgnoreCase
used elsewhere for directory deduplication; update the comparison in
NormalizeDirectory (the equality check between fullPath and rootPath) to use
StringComparison.OrdinalIgnoreCase so drive letters and casing are treated
consistently with the rest of the codebase.
In `@targets/posix/Include/targetPAL_Time.h`:
- Line 23: The function declared as nanoCLR_POSIX_GetNextCompareValue must be
renamed to use the PAL prefix: change the declaration/definition and all call
sites to nanoPAL_POSIX_GetNextCompareValue; update the header prototype in
targetPAL_Time.h, the corresponding implementation function name, and any
references (calls, forward declarations, externs) to the old symbol so they
match the new nanoPAL_ prefixed name.
- Line 7: Replace the C++ header include with the C-compatible header by
changing the include of <cstdint> to <stdint.h> so that nanoPAL_Time.h can be
included inside its extern "C" block without breaking C compilation; also rename
the PAL function declaration currently using the nanoCLR_ prefix to the PAL
naming convention by replacing the declaration of
nanoCLR_POSIX_GetNextCompareValue (or similarly prefixed symbol) with
nanoPAL_POSIX_GetNextCompareValue so it matches other PAL APIs.
In `@targets/posix/nanoCLR/CLRStartup.cpp`:
- Around line 131-143: Reject null or undersized assembly blobs before
constructing or indexing the CLR_RT_Buffer: in LoadAssembly (and the similar
loader around the 161-167 region) check that data != nullptr and size >=
sizeof(CLR_RECORD_ASSEMBLY) (and any other needed minimum like bulk size) and
return a proper error (e.g., CLR_E_FAIL) before creating CLR_RT_Buffer or
accessing (*buffer)[0] or bulk[bulk.size()-1]; ensure you free/avoid
constructing the buffer when the preconditions fail and keep the existing
error-handling path (NANOCLR_SET_AND_LEAVE) intact.
- Around line 476-480: The unconditional ASSERT in ClrStartup comparing
sizeof(CLR_RT_HeapBlock_Raw) and sizeof(CLR_RT_HeapBlock) must be guarded the
same way as the POSIX LP64 header; surround the ASSERT in ClrStartup with the
same conditional used in src/CLR/Include/nanoCLR_Runtime.h (i.e., skip the check
when PLATFORM_POSIX_HOST && __LP64__ is true) so that the size-equality assert
is not executed on POSIX LP64 builds (reference symbols: ClrStartup,
CLR_RT_HeapBlock_Raw, CLR_RT_HeapBlock).
- Around line 206-240: The Resolve() implementation (nanoCLR_Resolve) currently
only walks g_CLR_RT_TypeSystem but never processes staged raw buffers stored by
nanoCLR_LoadAssembly (m_assemblies), so callers get a false-success; before
iterating assemblies or calling g_CLR_RT_TypeSystem.ResolveAll() ensure you load
the staged buffers into the type system by invoking the existing loader path
(call the class method that processes m_assemblies—e.g., the instance Load() or
the appropriate overload LoadAssembly() that links buffers into
g_CLR_RT_TypeSystem) for each entry in m_assemblies, propagate and check load
errors, and only then perform the existing missing-assembly checks and call
g_CLR_RT_TypeSystem.ResolveAll().
In `@targets/posix/nanoCLR/FileStore_POSIX.cpp`:
- Line 13: Rename the constant k_MaxFileBytes to follow the repository's c_
prefix convention (e.g., c_MaxFileBytes) wherever it is declared and referenced;
update the definition (static constexpr std::streamoff k_MaxFileBytes) and
replace all usages of k_MaxFileBytes in this translation unit (and any headers)
with the new name so symbol references remain correct and compilation succeeds.
Ensure only the identifier changes and the value/type (static constexpr
std::streamoff 64LL * 1024 * 1024) and linkage remain unchanged.
In `@targets/posix/nanoCLR/Memory.cpp`:
- Around line 22-33: Replace the std::call_once-based initialization that uses
g_DefaultHeapOnce with a mutex-guarded retryable initialization: protect
allocation with a std::mutex, on entry check if g_MemoryStart is null, attempt
std::malloc(c_DefaultHeapSizeBytes) and if successful memset it, and leave
g_MemoryStart null on failure so future calls (from HeapLocation() and
CustomHeapLocation()) can retry; ensure BaseAddress and SizeInBytes are set from
g_MemoryStart after the guarded allocation attempt and keep the null-check
inside the mutex to maintain thread-safety.
In `@targets/posix/nanoCLR/nanoCLR_native_posix.cpp`:
- Around line 158-191: The output buffer parameter on
nanoCLR_GetNativeAssemblyInformation is incorrectly declared const while the
function writes into it; change the signature from const uint8_t *data to
uint8_t *data (and analogous CLR_UINT8 const to CLR_UINT8 *) in the function
definition and its header prototypes (and in the other native function
declarations referenced), update any corresponding implementations (e.g.,
nanoCLR_native.cpp variants) and callers to use the non-const pointer, and
remove the unsafe (void*) casts around data (e.g., inside
memset/memcpy/hal_strcpy_s) so the code writes to the buffer without const
violations.
In `@targets/posix/nanoCLR/Target_HAL.cpp`:
- Around line 9-24: The HAL_Windows_Debug_Print function currently writes
directly to stdout instead of invoking the registered host callback; update it
to check the global callback set by nanoCLR_SetDebugPrintCallback
(g_DebugPrintCallback in nanoCLR_native_posix.cpp) and, if non-null, forward the
trimmed text to that callback, otherwise fall back to writing to std::cout;
preserve the current null-check and CR/LF trimming behavior before calling
g_DebugPrintCallback so existing formatting is unchanged.
In `@targets/posix/nanoCLR/targetHAL_Time.cpp`:
- Around line 34-36: Replace the duplicate local constant c_epochOffset100ns
with the shared TIME_UNIX_EPOCH_AS_TICKS from nanoHAL_Time.h: remove the
c_epochOffset100ns declaration and update all uses of c_epochOffset100ns in this
file to reference TIME_UNIX_EPOCH_AS_TICKS instead (the header is already
included), ensuring consistency with other ports like ESP32 and avoiding the
duplicate definition.
In `@targets/win32/nanoCLR/Target_HAL.cpp`:
- Line 12: Replace the redundant C-style cast when constructing the std::string:
instead of using (std::string)szText to initialize the variable str, construct
or assign the string directly from the const char* (szText) — e.g., use
std::string str(szText) or std::string str = szText — in the Target_HAL.cpp
location where str and szText are used.
---
Duplicate comments:
In `@targets/posix/nanoCLR/FileStore_POSIX.cpp`:
- Around line 31-50: The current read uses streambuf iterators
(content.assign(std::istreambuf_iterator<char>(in), ...)) which can read to EOF
and be tricked if the file grows after stat(); instead, read exactly st.st_size
bytes and enforce k_MaxFileBytes: after opening the std::ifstream (in) allocate
content.resize(st.st_size) and use in.read(&content[0], st.st_size) (or
readsome/seek+read) then check in.gcount()/in.fail()/in.bad() to detect
short/failed reads; ensure you still clear content and set error on failure and
preserve the earlier size check against k_MaxFileBytes (reference st,
k_MaxFileBytes, content.assign, and in).
In `@targets/posix/nanoCLR/posix_stubs.cpp`:
- Around line 115-132: The array g_CLR_InteropAssembliesNativeData should be
made fully const to prevent modification and satisfy static analysis: change its
declaration so the pointer array is const as well (e.g., const
CLR_RT_NativeAssemblyData * const g_CLR_InteropAssembliesNativeData[]), leaving
the initializer and the sentinel nullptr intact; no other logic changes are
needed, and g_CLR_InteropAssembliesCount can remain as-is referencing
ARRAYSIZE(g_CLR_InteropAssembliesNativeData) - 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3af8ad8f-b2ce-4634-9d20-b424edaf6ed5
📒 Files selected for processing (20)
src/CLR/Core/CLR_RT_StackFrame.cppsrc/CLR/Include/nanoCLR_Runtime.hsrc/CLR/Include/nanoCLR_Win32.hsrc/HAL/Include/nanoHAL.htargets/netcore/nanoFramework.nanoCLR.CLI/Program.cstargets/netcore/nanoFramework.nanoCLR.Host/Interop/NativeNanoClrLoader.cstargets/netcore/nanoFramework.nanoCLR/Target_HAL.cpptargets/posix/CMakeLists.txttargets/posix/Include/targetPAL_Time.htargets/posix/README.mdtargets/posix/nanoCLR/CLRStartup.cpptargets/posix/nanoCLR/FileStore_POSIX.cpptargets/posix/nanoCLR/Memory.cpptargets/posix/nanoCLR/Target_HAL.cpptargets/posix/nanoCLR/main.cpptargets/posix/nanoCLR/nanoCLR_native_posix.cpptargets/posix/nanoCLR/platform_selector.htargets/posix/nanoCLR/posix_stubs.cpptargets/posix/nanoCLR/targetHAL_Time.cpptargets/win32/nanoCLR/Target_HAL.cpp
Member
|
@Ellerbach there are issues with the code style on the source files. Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CLR/Include/nanoCLR_Win32.h`:
- Line 29: The access-specifier "public:" is indented with 2 spaces; update its
leading indentation to 4 spaces to comply with the project's 4-space indentation
style (adjust the "public:" token in nanoCLR_Win32.h so it aligns with other
class/struct members).
In `@targets/posix/nanoCLR/CMakeLists.txt`:
- Around line 280-291: The compile definitions for the POSIX host are
duplicated; extract them into a single reusable target or variable and reuse it
from both the library and test targets to avoid drift. Create a CMake variable
or INTERFACE target (e.g., POSIX_HOST_DEFINITIONS or target
POSIXHostDefinitions) that contains the definitions used in
target_compile_definitions (PLATFORM_POSIX_HOST, NANOCLR_HOST_POSIX,
NANOCLR_HOST_APPLE, NANOCLR_HOST_LINUX, NANOCLR_HOST_WINDOWS,
NANO_POSIX_ENABLE_SMOKE, NANOCLR_POSIX_VERSION_STRING, NANOCLR_PLATFORM_NAME)
and replace the duplicated target_compile_definitions(...) calls for
nanoFramework.nanoCLR and the test target with a single inclusion (either
add_definitions from the variable or target_link_libraries(<target> PRIVATE
POSIXHostDefinitions)).
- Around line 270-278: The current logic in the if/else that sets
NANOCLR_HOST_APPLE, NANOCLR_HOST_LINUX and NANOCLR_PLATFORM_NAME incorrectly
treats every non-Apple host as Linux; replace the else branch with an explicit
Linux check and a fatal error for unsupported hosts (e.g. use if(APPLE) ...
elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") ... else(message(FATAL_ERROR
"Unsupported host: ${CMAKE_SYSTEM_NAME} - nanoCLR supports only macOS and
Linux")) to set NANOCLR_HOST_APPLE, NANOCLR_HOST_LINUX and NANOCLR_PLATFORM_NAME
accordingly so unknown UNIX systems fail configure instead of being treated as
Linux.
- Around line 313-340: The nanoFramework.nanoCLR.test binary and its setup
(add_executable(main.cpp),
target_compile_features/target_link_libraries/target_include_directories/target_compile_definitions/set_target_properties)
should only be created when the NANO_POSIX_ENABLE_SMOKE option is enabled;
currently the target is always built. Wrap or gate the block that defines the
nanoFramework.nanoCLR.test target (the add_executable for main.cpp and all
subsequent target_* and set_target_properties calls) with a conditional that
checks NANO_POSIX_ENABLE_SMOKE (e.g., if(NANO_POSIX_ENABLE_SMOKE) ... endif())
so the smoke harness is only configured and built when that option is ON. Ensure
any compile definitions relying on NANO_POSIX_ENABLE_SMOKE remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3b31e47-e596-4053-93b8-c9290bf90706
📒 Files selected for processing (8)
src/CLR/Include/nanoCLR_Win32.hsrc/nanoFramework.Runtime.Events/nf_rt_events_native_nanoFramework_Runtime_Events_EventSink.cpptargets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cstargets/netcore/nanoFramework.nanoCLR.CLI/Utilities.cstargets/netcore/nanoFramework.nanoCLR.CLI/nanoFramework.nanoCLR.CLI.csprojtargets/netcore/nanoFramework.nanoCLR.CLI/packages.lock.jsontargets/posix/nanoCLR/CMakeLists.txttargets/win32/nanoCLR/Target_HAL.cpp
Member
|
@Ellerbach there are issues with the code style on the source files. Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that. |
13 tasks
Skip executing the test binary when the host CPU architecture differs from the build target architecture (e.g. osx-x64 binary on an arm64 runner). The library artifact is still staged and published regardless. Normalize 'aarch64' (Linux) and 'arm64' (macOS) to the same canonical name before comparing, so the guard works on both Linux and macOS ARM64 runners.
macos-13 (Intel) has been retired from Azure Pipelines and GitHub Actions hosted runners. Switch the osx-x64 leg to macOS-latest (Apple Silicon arm64) and cross-compile for x86_64. The smoke-test arch guard already handles this case: the test binary is skipped when host != target, and the library artifact is still published.
No ARM64 Ubuntu hosted runner is available in Azure Pipelines and the ubuntu-22.04-arm image doesn't exist. Instead cross-compile linux-arm64 from an x86_64 Ubuntu runner using the aarch64-linux-gnu GCC toolchain. Changes: - Add targets/posix/toolchain-aarch64-linux-gnu.cmake (new file) - ADO: install gcc-aarch64-linux-gnu when POSIX_RID=linux-arm64; pass CMAKE_TOOLCHAIN_FILE to cmake configure - GitHub Actions: same logic via a conditional 'Install arm64 cross-toolchain' step and TOOLCHAIN_ARG in Configure step - Both CI files: linux-arm64 leg now uses ubuntu-22.04 (x64) runner - The existing smoke-test arch guard already skips execution of the arm64 binary on the x86_64 runner; the .so is still staged and published as the linux-arm64 artifact
Add a 'nanoclr_nuget' pipeline artifact that publishes the .nupkg produced by the CLI pack step. This lets reviewers download it, rename to .zip, and verify all four POSIX RID native libs are present alongside the Windows DLL. The existing 'nanoclr_cli' artifact (Windows DLL only) is unchanged.
Member
Author
|
nuget perfectly working on Mac OS as well |
PackagePath was hardcoded to 'tools\net8.0\any\NanoCLR\...' but the tool targets net10.0. When installed, the runtime locates assemblies under tools/net10.0/any/ and NativeNanoClrLoader probes that same subtree -- so the native .so/.dylib at net8.0 was never found. Fix: derive PackageTfmSubFolder from $(TargetFramework) so all PackagePath values track the TargetFramework automatically. No manual update needed if the TFM changes again.
On distro-specific runtimes (e.g. Ubuntu 24.04), .NET reports a distro-specific RuntimeIdentifier such as 'ubuntu.24.04-x64' rather than the portable 'linux-x64'. NativeNanoClrLoader was only probing the exact RuntimeIdentifier, so the native library packaged under runtimes/linux-x64/native/ was never found. Fix: after probing the distro-specific RID, also probe the portable RID derived from the OS platform and ProcessArchitecture (linux-x64, osx-arm64, etc.). The portable probe is skipped when it equals the exact RID to avoid duplicate filesystem hits. Affected platforms: Ubuntu (any version), other Linux distros.
Member
Author
|
and working also as expected (test fails on purpose) on WSL Linux x64: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add nanoCLR POSIX for MacOS and Linux
Motivation and Context
How Has This Been Tested?
On a Mac OS ARM64, WSL on its way
Screenshots
Types of changes
Checklist