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

Fix the MinGW build. #1903

Merged
merged 2 commits into from
Jun 29, 2015
Merged

Fix the MinGW build. #1903

merged 2 commits into from
Jun 29, 2015

Conversation

vkargov
Copy link
Contributor

@vkargov vkargov commented Jun 29, 2015

Fix the MinGW builds (vanilla & w64) .

tritao added a commit that referenced this pull request Jun 29, 2015
@tritao tritao merged commit 2221dfc into mono:master Jun 29, 2015
@kumpera
Copy link
Contributor

kumpera commented Jun 30, 2015

Why is the handled mutex use after cleanup only triggered on windows?

Shouldn't we fix the use-after-free instead of hacking our way around it?

@vkargov
Copy link
Contributor Author

vkargov commented Jun 30, 2015

Why is the handled mutex use after cleanup only triggered on windows?

It is triggered on Linux and OS X too, but does not lead to a crash due to the way how mutexes are implemented (pthread_mutex_destroy(), which mono_mutex_destroy() translates to on Linux, is essentially a stub), only causing a crash on Windows where the implementation is different.

Shouldn't we fix the use-after-free instead of hacking our way around it?

A fix that does not introduce a second cleanup phase would definitely be better, but I failed to find any other simple solution that does not require some non-trivial reworking of the code logic due to the fact that mono_gc_cleanup() (which destroys the mutex in question) and mono_domain_free() (which uses that prematurely destroyed mutex) rely on each other's effects and cannot be simply interchanged.
Considering that the changes were causing crashes in the Windows build, and since the trouble making mutex code in gc.c will be removed shortly anyway (see https://github.com/schani/mono/tree/weak-refs), I thought this hotfix would be good enough for the time being, and will be removed along with other obsolete code in the longer run anyway.
If it's not okay to have this kind of code in master, I can definitely put more time into finding a cleaner solution, though it just does not seem that the problem will be relevant for a long enough time to justify that.

@kumpera
Copy link
Contributor

kumpera commented Jul 2, 2015

It's normal to have temporary workarounds on master until we land a work-in-progress better fix.

The problem is that the commit message didn't have the long form explanation that you just did
and probably no one but you would know it otherwise.

So, as a rule of thumb, favor big commit messages, specially when fixing very tricky problems like this
one. It makes clear your intent and educate the rest of the team of problems the runtime have. I, for one, didn't know about this bad interaction between domain unload and GC cleanup.

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.

3 participants