add PR template#13
Merged
Merged
Conversation
Contributor
Contributor
|
🔧 Report generated by pr-comment-scanbuild Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jbachorik
approved these changes
Aug 24, 2023
Collaborator
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 0s) you can cancel this operation by commenting your pull request with |
|
🚨 MergeQueue Tests failed on this commit c67894a You should fix those tests and then re-add your pull request to the queue! Details
checks are failing:
If you need support, contact us on slack #ci-interfaces with those details! |
jbachorik
added a commit
that referenced
this pull request
May 25, 2026
Code review surfaced 7 findings — 4 CONFIRMED, 3 PLAUSIBLE. This commit addresses all 7: #8 (CONFIRMED) — split isInSignalContext into a strict variant. dlopen_hook was treating null ProfiledThread as "in signal" and deferring refresh on every dlopen from uninstrumented JVM threads (VM Thread, JIT, GC), delaying wasmtime sigaction patching by up to 500 ms. Add isInTrackedSignalContext() that returns false on null (only true when one of our SignalHandlerScopes is positively on the stack); dlopen_hook now uses it so JVM-internal threads get synchronous refresh again. isInSignalContext() retains its conservative semantics for any future caller that wants AS-safe-by-default. #7 (CONFIRMED) — switchLibraryTrap was called before startRefresher despite the invariant comment. Reorder so the refresher is running before the trap can fire. #9 (CONFIRMED) — DEBUG_ASSERT_NOT_IN_SIGNAL was on the 1-arg and 2-arg Dictionary::lookup overloads but missed the 4-arg form that actually mallocs. bounded_lookup's runtime-decided for_insert path was uncovered. Move the assertion into the 4-arg lookup, gated on for_insert (read-only lookups are AS-safe). #4 (CONFIRMED) — Comments referenced "REFRESH_INTERVAL_NS (5s)" but the actual constant is 500 ms. Fix both stale mentions. #13 (PLAUSIBLE) — SIGNAL_HANDLER_GUARD_RELEASE before chaining leaves depth == 0 inside a chained handler that returns normally; DEBUG_ASSERT_NOT_IN_SIGNAL inside such a handler would not fire. Document the trade-off in segvHandler — the longjmp safety property is more important than the sanitizer coverage gap, which is bounded to third-party signal handler code we don't own. #2 (PLAUSIBLE) — refresherLoop used OS::sleep without an EINTR loop; any unmasked signal (SIGCHLD, SIGURG, RT signals) would cause premature ticks. Wrap the sleep in an explicit elapsed-time loop using OS::nanotime so the refresher ticks at 500 ms regardless of stray signals. #14 (PLAUSIBLE) — refresherLoop published _refresher_tid before blocking SIGPROF/SIGVTALRM; a stale per-thread timer from a previous lifecycle could fire on the refresher during the window. Block signals first, then publish the TID. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.