gh-157660: Fix stale TLBC caches in _remote_debugging - #157732
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
This is ready for review. The TLBC regression test passes locally, including repeated runs with frame caching enabled and disabled. The two failing CI jobs appear unrelated to this change, as noted above. |
pablogsal
left a comment
pablogsal
left a comment
There was a problem hiding this comment.
Thanks for the PR! I think the approach is the right one: the frame's own tlbc_index is enough evidence that the cached array is stale, so refreshing lazily and keeping the bounds check is fine.
But this only fixes the first case in the issue. The second reproducer (slot going from NULL to a pointer with no growth) still gives lineno -1 with this branch. Same mechanism covers it with one more condition, see inline. This also needs a backport to 3.14.
Not from this PR, but _Py_hashtable_set(self->tlbc_cache, key, NULL) in get_tlbc_cache_entry asserts the key is absent in debug builds and leaks the old entry. It is unreachable today because a generation change clears the table, but since you are here, maybe use the same _Py_hashtable_steal + destroy pattern you added?
| tlbc_entry = get_tlbc_cache_entry(unwinder, real_address, unwinder->tlbc_generation); | ||
| } | ||
|
|
||
| if (tlbc_entry && ctx->tlbc_index >= tlbc_entry->tlbc_array_size) { |
There was a problem hiding this comment.
We need to refresh also when the cached slot at a nonzero index is NULL. A live frame with tlbc_index != 0 always has a populated slot (_PyFrame_InitializeTLBC sets the index to 0 when there is no copy yet), so a NULL here means the cache predates the slot being filled. Otherwise we fall back to the main bytecode with an instr_ptr from the TLBC copy and report garbage line numbers.
Something like (hoisting entries above this check):
if (tlbc_entry && ctx->tlbc_index >= 0 &&
(ctx->tlbc_index >= tlbc_entry->tlbc_array_size ||
entries[ctx->tlbc_index] == 0)) {I checked locally and this makes both reproducers from the issue pass.
| int | ||
| cache_tlbc_array(RemoteUnwinderObject *unwinder, uintptr_t code_addr, uintptr_t tlbc_array_addr, uint32_t generation) | ||
| cache_tlbc_array(RemoteUnwinderObject *unwinder, uintptr_t code_addr, | ||
| uintptr_t tlbc_array_addr, uint32_t generation, bool force_refresh) |
There was a problem hiding this comment.
I don't think we need force_refresh. The page cache is cleared at the end of every get_stack_trace, so across samples paged reads are already fresh, and within a sample the initial load has the same window. Let's keep the old signature.
| sys.platform == "linux" and not PROCESS_VM_READV_SUPPORTED, | ||
| "Requires process_vm_readv", | ||
| ) | ||
| def test_tlbc_cache_refresh_after_growth(self): |
There was a problem hiding this comment.
This covers growth only. Can you add a test for the slot-fill case as well? Two workers is enough, it is the second script in the issue and it fails on this branch today.
Small nit: this runs both cache_frames modes, so TestFrameCaching is a bit of an odd home for it.
| @@ -0,0 +1,2 @@ | |||
| Fix persistent sampling errors in :mod:`!_remote_debugging` when a code | |||
There was a problem hiding this comment.
Once the NULL slot case is in, this should mention incorrect line numbers too, and better to name the user-facing module:
Fix :mod:`profiling.sampling` reporting errors or incorrect line numbers in free-threaded
builds when a thread-local bytecode array grows or gains entries after being cached.
99b225b to
69a4191
Compare
liuzhijie-0614
changed the title
69a4191 to
ad535c6
Compare
4da4976 to
f9611ac
Compare
|
@pablogsal Thank you very much for your guidance! It has resolved my confusion. I completed the code modification based on the suggestions. Please review it again :) |
|
Thanks! The cache changes address my earlier comments. Can we replace the sleeps in the tests with worker synchronization? The Android and iOS failures are unrelated download failures. We also still need to arrange the 3.14 backport. |
`profiling.sampling` reporting errors or incorrect line numbers in free-threaded builds when a thread-local bytecode array grows or gains entries after being cached.
f9611ac to
a7e7c06
Compare
|
GH-158007 is a backport of this pull request to the 3.14 branch. |
Thanks for your feedback! I’ve replaced 3.14 backport PR：#158007 |
|
@pablogsal I investigated the Windows ARM64 free-threading CI failure and reproduced the same error locally. It looks related to #157605. After |
039483a to
a7e7c06
Compare
|
GH-158302 is a backport of this pull request to the 3.15 branch. |
|
Great work on the TLBC cache fix, @liuzhijie-0614! Thanks a lot! ❤️ |
…) (#158007) * gh-157660: Fix stale TLBC caches in _remote_debugging `profiling.sampling` reporting errors or incorrect line numbers in free-threaded builds when a thread-local bytecode array grows or gains entries after being cached. * gh-157660: Retry transient sampling races in TLBC tests --------- Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
2 participants
Fixes #157660.
In free-threaded builds, TLBC arrays can grow or gain entries without changing tlbc_generation, leaving _remote_debugging with stale cached data. This can cause persistent Invalid tlbc_index errors or incorrect line numbers.
Refresh the cached array when a frame’s index exceeds its capacity or points to a cached NULL slot, while retaining the existing bounds checks.
Adds regression tests based on both reproducers in the issue.