Update thread related tests to 3.14.5#8018
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe ChangesThread Stack Size and Timeout Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/threading.py dependencies:
dependent tests: (157 tests)
[x] test: cpython/Lib/test/test_thread.py (TODO: 3) dependencies: dependent tests: (13 tests) Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 56-61: The constant PY_OS_LOG2_STACK_MARGIN currently uses
debug_assertions to pick 12 vs 11 which mismatches CPython; update the selection
for PY_OS_LOG2_STACK_MARGIN to detect sanitizers instead (ASan/TSan) rather than
debug builds — e.g. change the cfg_select! condition used in the const
PY_OS_LOG2_STACK_MARGIN to check for sanitizer cfgs (Address/Thread sanitizer)
or drive it from a build-time flag so it returns 12 only when a sanitizer is
enabled and 11 otherwise; ensure any related MIN_SIZE/_thread.stack_size tests
rely on that corrected constant or are adjusted to accept the
sanitizer-dependent minimum.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: da590a8a-2db9-4db3-965f-f017e95cb8e3
⛔ Files ignored due to path filters (5)
Lib/test/test_thread.pyis excluded by!Lib/**Lib/test/test_thread_local_bytecode.pyis excluded by!Lib/**Lib/test/test_threadedtempfile.pyis excluded by!Lib/**Lib/test/test_threading.pyis excluded by!Lib/**Lib/test/test_threading_local.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/_thread.rs
| // TODO: Check for sanitize once https://github.com/rust-lang/rust/issues/39699 is closed | ||
| /// [CPython `_PyOS_LOG2_STACK_MARGIN`](https://github.com/python/cpython/blob/v3.14.5/Include/internal/pycore_pythonrun.h#L41-L47) | ||
| const PY_OS_LOG2_STACK_MARGIN: u32 = cfg_select! { | ||
| debug_assertions => 12, | ||
| _ => 11, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython 3.14.5 _PyOS_LOG2_STACK_MARGIN definition pycore_pythonrun.h sanitizer
💡 Result:
In CPython 3.14, the macro _PyOS_LOG2_STACK_MARGIN is an internal configuration macro defined in the internal header file pycore_pythonrun.h [1][2]. It is used to define the C stack margin—the amount of stack space reserved to prevent stack overflows [2][3]. As of the current CPython 3.14 codebase, _PyOS_LOG2_STACK_MARGIN is set based on the environment and build configuration [2]: 1. If AddressSanitizer (_Py_ADDRESS_SANITIZER) or ThreadSanitizer (_Py_THREAD_SANITIZER) are enabled, _PyOS_LOG2_STACK_MARGIN is set to 12 (corresponding to 4096 pointers) [2][4]. 2. In other cases, it defaults to 11 (corresponding to 2048 pointers) [2]. This macro is a private, internal-only implementation detail [2][5]. It was moved from public headers to pycore_pythonrun.h to hide it from the public API, as it is intended solely for CPython's internal stack overflow protection mechanisms [2][5]. The value is used to calculate the actual stack margin in bytes (_PyOS_STACK_MARGIN_BYTES), which is used by CPython to check for stack exhaustion, particularly on platforms like Windows or when using specific sanitizers [2][3][4]. It is not intended for use by third-party C extensions [5].
Citations:
- 1: https://fossies.org/linux/Python/Include/internal/pycore_pythonrun.h?m=b
- 2: nascheme/cpython@28940e8
- 3: https://github.com/python/cpython/blob/2b74c835a7280840a853e3a9aaeb83758b13a458/Python/pythonrun.c
- 4: hugovk/cpython@4162bc1
- 5: Implement stack overflow protection for linux based on actual stack depth python/cpython#130396
Align _PyOS_LOG2_STACK_MARGIN logic with CPython’s sanitizer-based values.
CPython sets _PyOS_LOG2_STACK_MARGIN to 12 only when ASan/TSan is enabled; otherwise it uses 11. Using Rust’s debug_assertions to select 12 makes debug (non-sanitized) runs compute a larger MIN_SIZE than CPython, so any _thread.stack_size boundary tests can diverge between debug and release—either select based on actual sanitizer cfgs or ensure the updated tests don’t assume a single fixed minimum across profiles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/stdlib/_thread.rs` around lines 56 - 61, The constant
PY_OS_LOG2_STACK_MARGIN currently uses debug_assertions to pick 12 vs 11 which
mismatches CPython; update the selection for PY_OS_LOG2_STACK_MARGIN to detect
sanitizers instead (ASan/TSan) rather than debug builds — e.g. change the
cfg_select! condition used in the const PY_OS_LOG2_STACK_MARGIN to check for
sanitizer cfgs (Address/Thread sanitizer) or drive it from a build-time flag so
it returns 12 only when a sanitizer is enabled and 11 otherwise; ensure any
related MIN_SIZE/_thread.stack_size tests rely on that corrected constant or are
adjusted to accept the sanitizer-dependent minimum.
ShaharNaveh
force-pushed
the
update-test-threads
branch
from
cf903e5 to
52acc28
Compare
Brings the branch up to date with main so CI job names match the required status checks (the windows snippets/cpython job was renamed by RustPython#8004 when its matrix skips were cleared). Also picks up RustPython#8018's _thread.rs stack-margin updates, which are orthogonal to the apply_thread_stack_size fix in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Summary by CodeRabbit
Release Notes