Skip to content

fix: provide non-static recursive mutex initializer #1113

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

Merged
merged 10 commits into from
Mar 6, 2025

Conversation

supervacuus
Copy link
Collaborator

Potentially fixes #1090 by providing another mutex initializer that manually constructs a recursive mutex instead of relying on a static initialization formula (which either the pthread library or we provide).

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.85%. Comparing base (fe3b6d9) to head (10c6c9e).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
- Coverage   82.85%   82.85%   -0.01%     
==========================================
  Files          53       53              
  Lines        7991     8002      +11     
  Branches     1252     1252              
==========================================
+ Hits         6621     6630       +9     
  Misses       1255     1255              
- Partials      115      117       +2     
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@supervacuus
Copy link
Collaborator Author

Hi @rubiefawn, can you try this branch and check whether it fixes the mutex initialization issue you see on FreeBSD?

@rubiefawn
Copy link

Sorry for the late response, I don't have FreeBSD installed on bare metal anymore and kept forgetting to create a VM for this.

This looks like a step in the right direction, but it still doesn't build on FreeBSD. SENTRY__MUTEX_INIT is used in src/sentry_core.c:27. I'll take a closer look at it later; I don't have any other support for FreeBSD prepared so it's likely still not going to build immediately even after that one line is fixed.

@supervacuus
Copy link
Collaborator Author

This looks like a step in the right direction, but it still doesn't build on FreeBSD. SENTRY__MUTEX_INIT is used in src/sentry_core.c:27. I'll take a closer look at it later; I don't have any other support for FreeBSD prepared so it's likely still not going to build immediately even after that one line is fixed.

That is excellent feedback. I only quickly prepared something for the general mutex usage but did not consider that the static initializer might be used directly. That means we must introduce a pthread_once-based dynamic initialization for those usages. I will prepare something, I don't know when I will find the time.

@supervacuus supervacuus force-pushed the fix/mutex_init_wo_static_recursive_initializer branch from 0edcbab to 39b3038 Compare February 13, 2025 11:38
@supervacuus
Copy link
Collaborator Author

@rubiefawn, can you give the latest changes a try? Thx!

@thesunexpress
Copy link

@rubiefawn, can you give the latest changes a try? Thx!

Partial build on FreeBSD 15-CURRENT -- going down some compile error rabbit holes for inspection & switching to 14-STABLE in a bit to be more relevant.

@supervacuus
Copy link
Collaborator Author

@rubiefawn, can you give the latest changes a try? Thx!

Partial build on FreeBSD 15-CURRENT -- going down some compile error rabbit holes for inspection & switching to 14-STABLE in a bit to be more relevant.

I don't understand what you mean by your comment. But just to be clear: this PR is not meant to be a FreeBSD support PR. We currently do not have the resources to achieve support, and it is also not a short to mid-term goal.

However, we are happy to receive PRs that allow the Native SDK to run on FreeBSD. This would entail more changes (see below) than allowing recursive mutexes on platforms without respective static initializers. However, this is an issue on more platforms than only FreeBSD and, as such, would be a valuable addition in isolation. I only used the FreeBSD #ifdef since that would be the primary platform @rubiefawn could test this on who initiated the investigation.

Also, if you no longer have time to work on this topic, it is OK to close it.


In case you want to attempt basic FreeBSD support:

  • you will need to add SENTRY_PLATFORM_FREEBSD to the public header
  • see how far you can take it by also defining SENTRY_PLATFORM_UNIX for FreeBSD
  • replace the __FreeBSD__ preprocessor check with your newly created SENTRY_PLATFORM_FREEBSD
  • Since our CMake script currently has no awareness of FreeBSD as a target, it will default to the inproc backend, which should be a good backend for an initial implementation (which you will have to ensure works on FreeBSD, but it uses POSIX signal handling, so should primarily be a question of compile-defs)
  • you will have to make sure that CMake selects curl as a transport for FreeBSD
  • you will have to implement a FreeBSD module finder that uses the libprocstat API to get the modules loaded
  • there might be some utility functions (typically string functions) that your standard library does not support
  • other things that I currently can't think of 😸
  • use the unit and integration tests to verify that your changes work (or skip them on FreeBSD if not applicable)

So, as you can see, there is a considerable effort beyond pthread static initializers, even if most of the other UNIX-related code works OOTB.

@rubiefawn
Copy link

Indeed, in order to test this on FreeBSD so far I have locally (and hackily) done the first few bullet points. Full FreeBSD support is out of scope and I'm not expecting the SDK to fully build on that platform even after this PR is merged.

@vaind vaind requested a review from shana February 27, 2025 19:12
@supervacuus supervacuus force-pushed the fix/mutex_init_wo_static_recursive_initializer branch from b849925 to 48b93ad Compare March 4, 2025 13:09
@supervacuus
Copy link
Collaborator Author

Since it recently became clear, this is also relevant for other concrete platforms, we can merge this whenever @shana and @vaind give it the thumbs up. Since this is an internal change, I am happy to get this out for usage sooner. We can always adapt interfaces later on.

@supervacuus supervacuus requested a review from vaind March 4, 2025 13:15
@@ -262,14 +264,36 @@ typedef pthread_cond_t sentry_cond_t;
PTHREAD_MUTEX_RECURSIVE \
} \
}
# elif defined(__FreeBSD__) || defined(SENTRY_PLATFORM_NX)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I got sick and haven't been able to look at this much.

Could we avoid gating this based on hardcoded platform checks if possible? Platform checks can set defaults, but it would be great if SENTRY__MUTEX_INIT_DYN could also be set based on a define that the build system can control and with an opt-in mechanism. i.e., instead of "if hardcoded platform check then define it", something like this instead:

#if defined(__FreeBSD__) || defined(SENTRY_PLATFORM_NX) || SOME OTHER PLATFORM
#define NEEDS_SENTRY_MUTEX_DYN
 #endif

#if defined(NEEDS_SENTRY_MUTEX_DYN) && !defined(SENTRY__MUTEX_INIT_DYN)
#define SENTRY__MUTEX_INIT_DYN(...
#endif

This way, any platform can define its own implementation if it needs to, and it can opt-in to the default implementation if it can use it, without having to go and change the header for every new platform that comes along. It is literally not possible to whitelist every platform even if you want to (some platforms have proprietary codenames that you can't make public, and those codenames usually include defines that let you detect them, so there is no way to legally publish that code), so ideally platform detection only sets defaults and generic flags, and the macro implementations are gated on those flags instead of the platforms themselves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, @shana. We addressed these points in our recent Discord chat. Let's go ahead and continue the discussion there to reduce redundant points to a minimum.

In short, nothing is set in stone. If we need to open implementation details to anyone rather than binding them to a specific platform, we can do that, but it shouldn't be the default and we can make these decisions on a case-by-case basis.

Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seems to do the job on NX: #1165

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…izer
@supervacuus supervacuus merged commit c2d08ed into master Mar 6, 2025
26 checks passed
@supervacuus supervacuus deleted the fix/mutex_init_wo_static_recursive_initializer branch March 6, 2025 09:56
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.

Recursive mutex initializer is not portable
5 participants