Conversation
HoshimiP
had a problem deploying
to
github-pages
GitHub Actions
Failure
|
Let me fix the CI issue first. |
AsakuraMizu
left a comment
AsakuraMizu
left a comment
There was a problem hiding this comment.
I like the threading and panic::catch_unwind part. Though there are still issues:
- There is some overlap with existing tests; let's merge them.
- What is the purpose of serial_test? If it's due to data conflicts, please isolate the data used by the test cases.
HoshimiP
force-pushed
the
main
branch
2 times, most recently
from
14e03dc to
043a26c
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances test coverage for scope-local functionality by adding three new test scenarios and fixes a linker script issue to ensure tests run properly. The changes add comprehensive testing for multi-threading, panic handling, thread isolation, and nested scope behavior.
Key Changes:
- Added multi-threading and panic recovery tests to the existing
sharedtest - Introduced new
isolationtest to verify thread-local scope independence - Added new
nestedtest to verify scope stacking behavior - Fixed linker script by removing explicit
0x0VMA address from.percpusection
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/scope_local.rs | Adds panic and thread imports, extends shared test with multi-threading and panic scenarios, adds isolation and nested test functions to verify thread safety and scope nesting |
| percpu.x | Removes explicit 0x0 VMA address from .percpu section definition to allow relative positioning and ensure tests run correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ActiveScope::set_global(); |
There was a problem hiding this comment.
Remove this line? I remember percpu delegates to thread local if target_os = "linux"
There was a problem hiding this comment.
Removing this line will cause a SIGSEGV
There was a problem hiding this comment.
Removing this line will cause a SIGSEGV
That's strange, and may indicate a bug in our design.
|
I tested it several times locally and found the results to be unstable. This is likely related to an issue with |
|
Waiting for |
|
Sry I pushed code to your repository again 🐱 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
adjust linker script to ensure tests run