Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

potential dangling resources in CommandListResourceStateTracker #44

Closed
BlurryLight opened this issue Jan 27, 2024 · 2 comments
Closed

Comments

@BlurryLight
Copy link
Contributor

A minimal repoducable example is as following:

  void TestRelease() {
    nvrhi::BufferDesc desc{};
    desc.keepInitialState = true;
    desc.initialState = nvrhi::ResourceStates::Common;
    desc.byteSize = 4;

    auto CmdList = GetDevice()->createCommandList();
    CmdList->open();
    {
      auto buf = GetDevice()->createBuffer(desc);
      CmdList->setBufferState(buf,nvrhi::ResourceStates::ConstantBuffer);
    }
    CmdList->close();
  }

It's because CommandListResourceStateTracker store all resources references in raw pointers without ref counter.
When cmdlists are about to close, they try to iterate over these resources ( keepBufferInitialStates / keepTextureInitialStates) and may trigger a crash because these resources may have been freed.

potential fix

CommandListResourceStateTracker might extend resources lifetime until closed.

@BlurryLight
Copy link
Contributor Author

there is another dangling case: CommandList::Set(Permanant)Buffer/TextureState don't record reference in TrackedCommandBuffer.referencedResources;. The resources may have been freed when pipelinebarriers haven't been executed on GPU.

apanteleev added a commit that referenced this issue Feb 2, 2024
@apanteleev
Copy link
Contributor

Thank you for reporting the issue. I figured that storing strong references inside the ..StateTracker class is overkill because its methods are called from many CommandList functions, and those functions already keep strong refs to their resources. Also, IResource* type pointers are not passed to the state tracker at all. So, the user-facing set(Texture|Buffer|AccelStruct)State functions now store strong refs explicitly.
Please let me know if I missed anything.

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

No branches or pull requests

2 participants