Skip to content

Fix tls leak for non-primary thread (2)#4151

Open
rosenkolev1 wants to merge 1 commit intoshadps4-emu:mainfrom
rosenkolev1:fix-tls-dealloc-issue-2
Open

Fix tls leak for non-primary thread (2)#4151
rosenkolev1 wants to merge 1 commit intoshadps4-emu:mainfrom
rosenkolev1:fix-tls-dealloc-issue-2

Conversation

@rosenkolev1
Copy link
Copy Markdown
Contributor

Attempt 1 - Removed all the fluff and left only the actual change.

Spoke with @red-prig a little and was told this change might make sense after all and that I can try opening a PR again.

The goal

To solve this.

The fix

We call TcbCtor() -> Linker::AllocateTlsForThread() and allocate tls memory for non-primary threads via heap_api->heap_malloc() or std::malloc() at addr_out. In TcbDtor(), we don't call Linker::FreeTlsForNonPrimaryThread(tls_base) (tls_base == addr_out is the sam, we only call it for the dtv_table entries.

  • I don't believe we can call TcbDtor for the primary thread (that is, the "Game:Main" thread). When debugging the game, the code is never entered for that thread, only for non-primary threads. When we exit the emulator, I see we do std::quick_exit and no destructors or additional GC logic for that thread is being hit.

    Still, if there is a case where that logic can be hit for the primary thread (or we want to be safe anyway), we can simply record if the thread is primary or not inside TcbCtor() and guard against that inside TcbDtor() via a simple flag.

  • Linker::FreeTlsForNonPrimaryThread() should handle freeing the memory the same way it was allocated, I don't think the heap_api can change mid-game after its set initially. Besides, we already free the dtv_table entries with the same way. When debugging the game, the heap_api is set before any non-primary threads are created and I didn't hit a case where the heap API changes afterwards.

For what it's worth, I played the game 2 times, each for ~2-3 hours at a time, with the change applied and didn't get any crashes.

@red-prig
Copy link
Copy Markdown
Contributor

@raphaelthegreat Everything seems to be fine?

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.

2 participants