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

Protect against crashes when MANUAL_LIFETIME is used #690

Closed
wants to merge 4 commits into from

Conversation

plekakis
Copy link

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).

…t MANUAL_LIFETIME can be used correctly. Mutexes that have been allocated *before* Tracy is initialised will not be tracked.
@wolfpld
Copy link
Owner

wolfpld commented Dec 20, 2023

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
Copy link
Author

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.

@plekakis plekakis closed this Dec 21, 2023
@wolfpld
Copy link
Owner

wolfpld commented Dec 21, 2023

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.

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