Skip to content

WIP#8

Closed
jbachorik wants to merge 1 commit into
mainfrom
jb/move_tests_to_gitlab
Closed

WIP#8
jbachorik wants to merge 1 commit into
mainfrom
jb/move_tests_to_gitlab

Conversation

@jbachorik
Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 23, 2023

🔧 Report generated by pr-comment-scanbuild

Scan-Build Report

User:runner@fv-az167-45
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 clean all
Clang Version:Ubuntu clang version 14.0.0-1ubuntu1.1
Date:Wed Aug 23 11:00:56 2023

Bug Summary

Bug TypeQuantityDisplay?
All Bugs4
Logic error
Assigned value is garbage or undefined1
Dereference of null pointer1
Result of operation is garbage or undefined1
Unused code
Dead nested assignment1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorAssigned value is garbage or undefineddwarf.cppparseInstructions23120
Unused codeDead nested assignmentvmStructs.cppcheckNativeBinding6351
Logic errorDereference of null pointersymbols_linux.cppElfParser15435
Logic errorResult of operation is garbage or undefineddwarf.hgetSLeb12325

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 23, 2023

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Style Violations (151)

@jbachorik jbachorik force-pushed the jb/move_tests_to_gitlab branch from a66828d to 0e53b45 Compare August 23, 2023 10:59
@jbachorik jbachorik closed this Aug 24, 2023
@jbachorik jbachorik deleted the jb/move_tests_to_gitlab branch August 24, 2023 12:26
jbachorik added a commit that referenced this pull request May 25, 2026
The always-on _in_signal_handler_depth TLS variable, accessed first from
inside our SIGPROF/SIGVTALRM handlers via SIGNAL_HANDLER_GUARD(), was
declared with the default global-dynamic TLS model.  On first access in
a given thread glibc lazily allocates the dtv slot via malloc() and
takes the heap lock — both async-signal-unsafe.

Reproduced deterministically on Graal aarch64 (glibc 17-graal debug)
running ClassGCTest: SIGPROF arrived on the VM Thread while Graal's
JVMCI compiler held the malloc heap lock through
c2v_notifyCompilerPhaseEvent.  Stack:

  #2  __libc_malloc                    -- waiting on heap lock
  #4  allocate_dtv_entry
  #7  _dl_tlsdesc_dynamic
  #8  TLS wrapper for _in_signal_handler_depth
  #9  SignalHandlerScope::SignalHandlerScope
  #10 CTimer::signalHandler

The heap holder is itself blocked at a safepoint waiting for VM Thread
to check in, and VM Thread is stuck in malloc -> full process deadlock.

Switch the variable to the initial-exec TLS model so the loader
allocates its slot from the static TLS surplus at libjavaProfiler.so
load time.  Every existing thread is fixed up at dlopen and every new
thread receives the slot at pthread_create.  Access is then a
register-relative load — async-signal-safe, lock-free, malloc-free.

Also narrow the type to uint8_t (realistic max depth ~3) to make the
intent explicit; alignment-wise this is the same slot.

Refresher tick reduced from 5 s to 500 ms so a library lazily loaded
from signal context becomes resolvable by the stack walker within
half a second.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant