Protect against crashes when MANUAL_LIFETIME is used

wolfpld / tracy

Frame profiler

https://tracy.nereid.pl/

Other

8.67k stars 598 forks source link

Protect against crashes when MANUAL_LIFETIME is used #690

Closed plekakis closed 6 months ago

plekakis commented 6 months ago

Most public API functions are now guarded against TracyIsStarted. This is always true when MANUAL_LIFETIME is not defined and true after StartupProfiler is called. This does mean that no data recording is taking place until after Tracy has been initialised. It doesn't break existing behaviour, this is only a path taken when MANUAL_LIFETIME is defined. In addition to the Scoped classes, the LockCtx is also guarded. This means that the mutexes won't be instrumented if at the time of their context allocation Tracy hasn't been initialised (this is expected in this case).

wolfpld commented 6 months ago

The issues I see with the current solution:

It mixes two different changes.
What is the expected usage pattern where the internal TracyIsStarted check is needed? If you manage the profiler lifetime yourself, you already know where you can and can't make the calls. Adding internal checks just makes things slower.
A blanket check in QueueSerialFinish shows me that this is not a complete solution, but a stop gap measure. There are no use cases where you would call this function directly from your code.
You are making wrong coding style changes for no reason.
You are dealing with the MANUAL_LIFETIME case, but the logic changes you are introducing depend on TRACY_ON_DEMAND.
You add m_active and related handling to a code path that doesn't currently need it, for no good reason.

plekakis commented 6 months ago

Yes, I agree, let's close this PR. It needs more thought, for sure, however as discussed previously: "If you manage the profiler lifetime yourself, you already know where you can and can't make the calls" This is not correct when it comes to mutex instrumentation. For now I can simply use ON_DEMAND which suits my use-case well actually.

wolfpld commented 6 months ago

This is not correct when it comes to mutex instrumentation.

That's true. I have looked at how to solve this myself and it is already non-trivial in the on-demand case. I'm not sure a manual lifetime management solution would be viable here.