Conversation
2 tasks
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This comment has been minimized.
This comment has been minimized.
Contributor
CI Test ResultsRun: #35750635785 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-22 16:16:40 UTC |
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 17, 2026 19:08
67ad3f7 to
b827f2b
Compare
Contributor
jbachorik
added this pull request to stack #803
September 17, 2026 20:02
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 17, 2026 20:05
b827f2b to
93d1e5a
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 17, 2026 20:19
93d1e5a to
6d92306
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 17, 2026 20:20
6d92306 to
2d93136
Compare
2 tasks
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 17, 2026 21:01
2d93136 to
97bb7f9
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 17, 2026 21:33
97bb7f9 to
bc388ff
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
2 times, most recently
from
September 18, 2026 06:04
2c64891 to
79de5c8
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 18, 2026 07:06
79de5c8 to
98b801a
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 18, 2026 08:04
98b801a to
1b1c03e
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 18, 2026 11:22
1b1c03e to
5f8aaa1
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 18, 2026 12:59
5f8aaa1 to
28e1a01
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 18, 2026 14:33
28e1a01 to
b99fee4
Compare
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 21, 2026 08:16
b99fee4 to
8a93d16
Compare
* Model crashNow's intentional crash as a trap under scan-build The null-pointer store is the deliberate never-returning crash, but clang scan-build reports a null dereference. Under __clang_analyzer__, replace it with __builtin_trap() - the analyzer understands the trap as a halt and explores no false path past it; the real build keeps the store. * Add JFR event types for reference chains Introduces the ReferenceChainEvent/ReferenceChainAbandonedEvent payloads (event.h), their JFR metadata (jfrMetadata.*), and the FlightRecorder emission paths that serialize chain events into JFR recording buffers, including the constant-pool handling for per-hop edge labels. Emission is pull-style: profiler.cpp snapshots events and hands them to FlightRecorder; this layer does not depend on the tracker itself. * Keep chain events inside the recording buffer; keep truncated labels Review findings on the JFR plumbing layer: - MAX_REFERENCE_CHAIN_EVENT_HOPS was a fixed 4096, permitting a ~438 KB worst-case event (near-limit edge labels) against a ~61 KB recording buffer - the reservation flushed first but the margin underflowed, so the write ran past the buffer (debug assert, release corruption). The cap is now derived from RECORDING_BUFFER_LIMIT minus the event's fixed fields, divided by the per-hop worst case, so a full-cap event always fits. - Truncation dropped ALL edge labels: the label count was gated on _edges.size() == emitted_size, which only holds for untruncated chains. Labels align with the chain's leaf-first element order, so truncation now emits the first emitted_size labels and loses only the root-side ones. - ObjectLivenessEvent::leak_tag is default-initialized to 0 so any construction path that forgets to set it serializes a defined untagged value (flush_table() overwrites it from the entry, which track() zeroes at insert). Moves the JFR round-trip and arguments parsing unit tests into this layer (they test exactly this code), rewrites the round-trip test to construct events directly instead of through the tracker, and adds byte-level boundary tests: oversize-chain truncation with label preservation, the size-prefix invariant, and the default leak tag. * Drop transient and stale line-number references from comments Uncommitted plan documents, rotted .cpp:NNN line references, and a nonexistent j9WallClock.cpp path replaced with symbol references that stay valid as the code moves. * Address review: merge chain hops, trim comments, drop doc/jira refs - ReferenceChainEvent carries one vector of ReferenceChainHop (klass id + retention-edge label) instead of two parallel vectors - Compress the sub-option floor/ceiling rationale and the provisional default constant comments to one concise statement each - Drop design-doc and Jira references from code comments; revert the unrelated LineNumberTable comment rewrite * Make comments layer-local: no references to later stack layers The event/argument comments named collector classes, methods and files that do not exist at this layer of the stack; describe the contracts without those forward references instead. * Drop a later-layer class name from the arguments test comment * Clamp reference-chain edge labels to the reserved cap when writing * Reuse reservation constants and drop edge_labels stack array in recordReferenceChain
ReferenceChainTracker: per-klass class tags, the frontier table of retained references, the BFS expansion thread, chain resolution into per-sample ReferenceChainEvent payloads, and leak-tag correlation. LivenessTracker: the per-klass population table, heap-floor ring with time-to-OOM projection, and leak-candidate selection feeding the tracker's search gate. Adds the referencechains Arguments block, the string-dictionary generation counter used to invalidate the class-tag cache, and the container-memory/os queries the projection needs.
hopLabelClassFor() was the only GetObjectsWithTags() call site that did not Deallocate() the returned object/tag arrays - a per-cache-miss leak on the BFS thread's hop-label path (caught by the asan gtest run). Free both right after the class object is extracted, and null-guard the error path, matching the file's other call sites.
- collectStaticFieldAnchorsForRotation: the other tier is the last consumer of the anchor budget - stop accumulating its leftover back into budget_left (dead store). - buildCanaryChainEvent: capture the chain size before moving the vector into the event instead of reading the moved-from object for the log. - secondsToOOM: check ringThirdsStats()'s return for the time ring instead of reading time_stats uninitialized on its (unreachable-in- practice, but analyzer-visible) failure path - same head/fill/min-fill gate as the byte call, so it cannot trigger once the byte call passed.
- Leak-tag pool: start() reset the free list while the preserved tracking table still had tagged entries (the table survives stop()/start()), so a new object could receive a tag another live object owns and a later double release could write past the free list. Reclaim owned tags first and keep their correlation info. - track() published a reserved slot before initializing it while shared-mode scanners (tagLeakInstances, getLiveTraceIds) could read the uninitialized malloc storage; slots now carry a release/acquire ready flag and fresh table regions start unpublished. - secondsToOOM projects BOTH the heap and the container boundary and takes the shorter time instead of picking a ring by the raw limit comparison - container usage includes native memory and siblings, so a container with a numerically larger limit can still be closer to exhaustion. - cleanup_table() claimed the GC epoch before acquiring the table lock, so a newer epoch's fold could enter the population history before an older one's; the claim now happens under the lock (the pre-lock check remains as an advisory early exit). - threadLoop's urgency ramp multiplied the budget by four on every rounded pause-target change and never restored it; the boost now applies once per urgency episode and the configured budget is restored when it ends. - The terminal restart gate now charges the finished search's accumulated safepoint cost BEFORE checking affordability, so an expensive search no longer earns one free immediate successor (restartSearch() no longer spends it itself; the pain-budget test asserts the new order). - hopLabelClassFor() deleted cls twice on the superclass-walk path. - The class-shape reconciliation loop never deleted the class-object local refs GetObjectsWithTags() returned (BFS thread - pins classes against unload). - walkStaticFieldAnchors() early breaks left later anchors' local refs undeleted; a cleanup pass now releases them. - The static-field sweep's truncation cursor resumed by a visited-count index that assumes HotSpot's LIFO FollowReferences order; it now redoes the chunk, which is order-independent (shared code must not rely on HotSpot internals). - buildDiscoveredInstanceChains() treated a cache hit from an earlier search generation as current; the generation check now mirrors the representative-refresh paths. - cacheResolvedChain() reports success so coverage accounting (found bits, resolved counts) only advances for a chain that was actually stored. - os_linux: container usage is now read from the same cgroup level that supplied the selected limit (an ancestor limit covers sibling cgroups whose usage the leaf excludes). Moves referenceChains_ut.cpp and livenessTracker_ut.cpp into this layer - they test exactly this code, and the pain-budget ordering change requires its test to land with it.
A stale full scratch surviving klassPopulationResetForTest() lets the first post-reset GC fold fill the population table in one pass, making the synthetic-epoch seeded entry the permanent LRU-eviction victim - a fold landing mid-seeding (few-ms GC cadence on slow runners) resets the seeded ring and breaks the trend gate. Observed as the shouldSelectSeededKlassAsLeakCandidateOnPositiveSlope flake on musl-aarch64 (librca 21 and 11).
Comments pointing at locally-kept plan documents, rotted .cpp:NNN references, and a duplicated four-times comment block consolidated to the constant it documents.
fillHopEdgeLabels() fills hop edge labels in place; buildChainEvent() and the canary builder assemble ReferenceChainEvent::_hops from the parallel internal vectors.
No references to Java integration tests, stresstest repros, or uncommitted plan documents from this layer; the gtest files carry the comment cleanups with the code they test.
staleLeaf handling and the line-number-table copy boundary tests.
jbachorik
force-pushed
the
jb/rc-5-gtest
branch
from
September 22, 2026 15:55
8a93d16 to
cb32970
Compare
This branch has not been deployed
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
1 participant
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.
What does this PR do?:
Adds the C++ unit tests for reference chains (622 cases): frontier-table behavior, BFS expansion and cursor advancement, chain resolution/improvement (including the cycle guard), leak-candidate selection and the heap-floor OOM projection,
referencechainsarguments parsing, stale-leaf handling, and JFR round-tripping of chain events. Also extends the existing liveness-tracker and line-number-table tests.Motivation:
Test layer for PROF-15341, stacked on the engine it verifies.
Additional Notes:
Stacked on #798. All gtest binaries pass (2 skipped by design).
How to test the change?:
./gradlew :ddprof-lib:gtestDebug— the tests in this PR are exactly the ones it adds/extends.For Datadog employees: