Print stacktrace on precondition failure & assertion on pointer dereference #1695
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1695 +/- ##
=======================================
Coverage 88.47% 88.47%
=======================================
Files 382 382
Lines 31759 31759
=======================================
Hits 28100 28100
Misses 3659 3659 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error diagnostics by printing a backtrace on assertion failures, adds null-pointer checks to unsafe pointer subscripts, and removes a flaky concurrent sort example.
- Add
preconditionchecks for null inPointerToMutableandPointerunsafe subscripts - Extend
Assertto callprint_backtrace()on failure and define backtrace helpers - Remove the flaky
concurrent_sortend-to-end test
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/EndToEndTests/TestCases/Concurrency/concurrent_sort.hylo | Deleted flaky concurrent sort test |
| StandardLibrary/Sources/Core/PointerToMutable.hylo | Added null checks before dereferencing in unsafe subscripts |
| StandardLibrary/Sources/Core/Pointer.hylo | Added null check before dereferencing in unsafe subscript |
| StandardLibrary/Sources/Core/Assert.hylo | Invoke print_backtrace() on assertion failure and implement backtrace utilities |
Comments suppressed due to low confidence (3)
StandardLibrary/Sources/Core/Assert.hylo:67
- There are no tests exercising the new backtrace printing. Adding a test that triggers a failure and verifies the backtrace output would help ensure this works across platforms.
fun print_backtrace() {
StandardLibrary/Sources/Core/PointerToMutable.hylo:15
- The added null-pointer precondition isn't covered by existing tests. Consider adding unit tests that verify an assertion is triggered when dereferencing a null
PointerToMutable.
precondition(self != .null(), "dereferencing null pointer")
StandardLibrary/Sources/Core/Pointer.hylo:14
- Similarly, add tests to confirm that dereferencing a null
Pointertriggers the intended precondition failure.
precondition(self != .null(), "dereferencing null pointer")
| @@ -45,6 +45,7 @@ fun log_failure_description(_ title: String, _ message: String, in file: String, | |||
| print(title, terminator: ": ") | |||
| print(message) | |||
| } | |||
| print_backtrace() | |||
There was a problem hiding this comment.
I wonder if we should print the backtrace in release mode. Do we have a compiler directive to distinguish #debug or #release? I don't remember.
There was a problem hiding this comment.
We don't seem to have such a feature yet.
(and to be honest, I would keep it in release as well, but add a different flag to disable this)
| { | ||
| &buffer.with_mutable_contiguous_storage(fun (_ p: PointerToMutable<MemoryAddress>) -> Void { | ||
| var size = backtrace(p, C.int(truncating_or_extending: backtrace_depth)) | ||
| backtrace_symbols_fd(p, size, 2) // write to stderr | ||
| }) | ||
| } |
There was a problem hiding this comment.
Is this supposed to delimit a scope? Why is that scope necessary?
There was a problem hiding this comment.
Sometimes (probably due to compiler bugs) the scopes are needed. In this case, it seems to work without one. I think this was a leftover from me trying to solve a lifetime issue. Will drop.
| @@ -11,6 +11,7 @@ public type Pointer<Pointee>: Regular { | |||
| /// - Requires: `self` is the address of an object of type `Pointee` and its storage | |||
| /// is accessed only through this projection during the projection's lifetime. | |||
| public subscript unsafe(): Pointee { | |||
| precondition(self != .null(), "dereferencing null pointer") | |||
There was a problem hiding this comment.
I am not convinced about the value of this precondition. We can't in general test that dereferencing a pointer is safe and (perhaps more importantly) we should guarantee that using the unsafe API is as fast as possible. Therefore adding branches on this path seems wrong.
There was a problem hiding this comment.
null appears so often in low-level code, that, I think, it deserves a special check. It's just the checks that we do to ensure we are not past the end of a buffer.
In terms of performance, most of these branches should be removed by the compiler once it figures out that the pointer is valid (for most of the cases that I can think of).
(see also: https://security.googleblog.com/2024/11/retrofitting-spatial-safety-to-hundreds.html)
Co-authored-by: Dimi Racordon <[email protected]>
Print stack trace when a precondition is violated.
Also add assertions on pointer derefence, checking for null pointers.
Also remove
concurrent_sortexample as is is flaky.