Fix dangling-pointer walk in ResidencySet during DiscardCommandList#114
Conversation
|
It seems like all this complexity (and extra locking) could be short-circuited by making |
681b713 to
96a38e3
Compare
|
Thanks Jesse, pivoted to your approach. Force-pushed the branch: the new ResidencyManager::DiscardResidencySet is called from CommandListManager::DiscardCommandList, no API signature changes, no extra locking. Also closing the sibling D3D9On12 / D3D11On12 PRs (#112 / #57) - under your design there's no API break so they're no longer needed. |
| // Resources alive until Close() runs. That breaks at teardown, where a synchronous DDI release | ||
| // (e.g. D3D9on12::DestroyResource -> Resource::Release -> ~Resource) can destroy a Resource | ||
| // while its tracking bit and Set entry are still live. Teardown therefore routes through | ||
| // ResidencyManager::DiscardResidencySet, which skips the walk. See bug 53802456. |
There was a problem hiding this comment.
GitHub comments shouldn't be referencing internal bug IDs.
96a38e3 to
402a917
Compare
jenatali
left a comment
There was a problem hiding this comment.
Comments are still too verbose / referring to past state for my taste, but the change itself seems fine.
402a917 to
3408c6f
Compare
Problem
Access violation in
D3D12TranslationLayer::ResidencySet::Closeduring device shutdown when a Resource is destroyed while still tracked in an open per-CL residency set. Surfaces in release builds as:Root cause
ResidencySet::Setholds rawManagedObject*with no ownership and no destroy-time notification.Close()walksSetand dereferences every entry to clear itsCommandListsUsedOnbit, so it assumes every Resource backing an entry is still alive at Close-time.That assumption holds for the submit paths (
ExecuteCommandList,SubmitCommandQueueCommand,PreExecuteCommandQueueCommand), where Resources are kept alive by the deferred-deletion queue + GPU fence until well after the CL completes.It does not hold for the teardown path. At
~ImmediateContext::Shutdown, a CL that was recorded but never submitted may have entries whose Resources were already released synchronously via a DDI callback (the D3D9 pathD3D9on12::DestroyResource->CleanupResourceBindingsAndRelease->Resource::Release->~Resourcedestroys immediately when the refcount hits zero, with no deferred-deletion-queue intermediation).Close()then walks dangling pointers and AVs.Fix
Per Jesse's review suggestion, the teardown path now routes through a new method on
ResidencyManagerthat drops the per-CL set's tracking state without walkingSet:ResidencyManager::DiscardResidencySet(ResidencySet*)-- new, the teardown entry point.ResidencySet::Discard()-- private, friended toResidencyManager. Resets state without iterating.CommandListManager::DiscardCommandList()-- now callsGetResidencyManager().DiscardResidencySet(...)instead ofClose().The submit paths are unchanged: they still go through
Close()and walkSet. Only the teardown discard path -- where the lifetime guarantee cannot hold -- skips the walk.A short lifetime-contract comment on
ResidencySetdocuments the invariant + the teardown carve-out for the next reader.No API signature changes. No new locking. Two files, ~16 net lines.
Verification
Tested on x64
d3d9on12.dllagainst the failingCheckVideoProcessorCaps#MockD3D12RuntimeTAEF test under AppVerifier:LeakTestTest note: the verifying system has a hardware GPU, so
CheckVideoProcessorCaps#MockD3D12Runtimeitself reports[Skipped]("9on12 Video Tests can only run on the Basic Display Driver"). The failure exercised reproduces duringClassCleanupregardless; the basic-display gating in the original repro is sufficient but not necessary to surface the failure.Known issue (DBG builds only)
The existing
#if TRANSLATION_LAYER_DBGtrip-wire atResidency.h:74(assert(!val)inside~ManagedObject, checking thatCommandListsUsedOn[i]is all false at Resource destruction) still fires on this repro under DBG builds. Empirically verified: 2/2 runs againstd3d9on12.dllbuilt from this branch.The trip-wire fires from
D3D9on12::DestroyResourceduringClassCleanup-- before~ImmediateContext::Shutdown->DiscardCommandListever runs -- so this fix'sClose->Discardswap is downstream of the trip-wire's trigger. The bit-leak originates at recording time whenInsertset the bit; the canonical clear path (ClosewalkingSet) never runs in the teardown scenario.This PR is intentionally scoped to fix the release-build AV described above. The DBG-only trip-wire is a separate concern with three options worth considering as a follow-up:
~ManagedObject's assert behind something narrower thanTRANSLATION_LAYER_DBG(e.g., a flag that excludes the known-benign teardown bit-leak).Discard()for entries we know are about to be destroyed.Happy to spin a follow-up PR for whichever the team prefers.