-
Notifications
You must be signed in to change notification settings - Fork 0
V0.7.0 new traces #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
V0.7.0 new traces #369
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #369 +/- ##
==========================================
+ Coverage 81.73% 81.89% +0.15%
==========================================
Files 381 381
Lines 33912 34012 +100
==========================================
+ Hits 27719 27854 +135
+ Misses 6193 6158 -35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found several issues including leftover debug logs, a potential crash from a removed nil check, an unsafe force-unwrap in a test, and a confusing comment change. The remaining changes are bug fixes or acceptable refactoring.
Suggestions that couldn't be attached to a specific line
Blockchain/Sources/Blockchain/RuntimeProtocols/Accumulation.swift:649, 662
The file contains debug logging statements that should be removed before merging.
Blockchain/Sources/Blockchain/VMInvocations/HostCall/HostCalls.swift:297, 316
The file contains debug logging statements that should be removed before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found potential correctness and robustness issues in memory initialization, instruction name parsing, a test using force-unwrapping without bounds checks, and a writable-check regression for nil values with non-zero length.
Suggestions that couldn't be attached to a specific line
PolkaVM/Sources/PolkaVM/Memory/StandardMemory.swift:59
heapZone data is now set directly to readWriteData without extending/zero-filling to cover heapEmptyPagesSize. This can cause out-of-bounds or undefined reads/writes for the uninitialized heap region since Zone's backing data may be shorter than endAddress - startAddress. Actionable fix: reintroduce zero-extension to the full heap zone size before assigning. For example:
var heapData = readWriteData
let totalHeapSize = Int(heapDataPagesLen + heapEmptyPagesSize)
if heapData.count < totalHeapSize {
let oldSize = heapData.count
heapData.count = totalHeapSize
heapData.withUnsafeMutableBytes { bytes in
memset(bytes.baseAddress!.advanced(by: oldSize), 0, totalHeapSize - oldSize)
}
}
heapZone = Zone(startAddress: heapStart, endAddress: heapStart + heapDataPagesLen + heapEmptyPagesSize, data: heapData)
PolkaVM/Sources/PolkaVM/Memory/StandardMemory.swift:65
stackZone data is now initialized to an empty Data(), whereas previously it was zero-filled to stackPageAlignedSize. Any reads from the stack zone can panic or return invalid data due to missing backing storage. Actionable fix: restore zero-initialization to the stack zone size:
let stackData = Data(count: Int(stackPageAlignedSize))
stackZone = Zone(startAddress: stackStartAddr, endAddress: UInt32(config.pvmProgramInitStackBaseAddress), data: stackData)
PolkaVM/Sources/PolkaVM/Engine.swift:101
Instruction name cleanup now replaces "Instructions::" but no longer removes "Instructions.". If typeName uses dot notation (common in Swift), this will leave the module prefix intact and break the intended formatting. Actionable fix: handle both separators or strip the prefix more generically. For example:
let cleanName = typeName
.replacingOccurrences(of: "Instructions.", with: "")
.replacingOccurrences(of: "Instructions::", with: "")
|
|
||
| let totalSize = 5 * ZZ + readOnlyAlignedSize + readWriteAlignedSize + stackAlignedSize + ZI | ||
| guard totalSize <= UInt32.max else { | ||
| guard totalSize - 1 <= UInt32.max else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can never be true if totalSize is UInt32. we simply want to ensure the above any UInt32 is <= UInt32.max.
what we wanted is to ensure the above calculation can never overflow and handle the overflow error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totalSize is Int, and we want to check <= 2^32, but UInt32.max + 1 part will have overflow so moved it to the left
changed to totalSize <= 0x1_0000_0000 to be simpler
minor fixes, and leave the rest since not easy to find source of mismatch, can revisit later, and maybe some can be fix during 0.7.1 traces