Use a helper class with no member variables for storing per-thread untranslated strings (PR #25928)
Arrigo Marchiori
This PR has the purpose of fixing issue #25897
MinGW builds are affected by destroy-after-free situations on thread_local variables.
The proposed class has no data members, and thus its destructor should be callable also on non-existing objects, without triggering exceptions (segmentation faults and the like).
The type of key used in the map must be selected carefully in order to avoid memory leaks.
I tried using the this pointer in the first place, but I saw that freed objects had it corrupted.
IMHO the code will need a good explanation of its purpose; I would like to have an agreement on the code before starting to comment it. For this reason, I am leaving this PR in the draft state.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25928
Commit Summary
- 81aaac5 Use a helper class with no member variables for storing per-thread untranslated strings
File Changes
(1 file)
- M src/common/translation.cpp (28)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25928.patch
- https://github.com/wxWidgets/wxWidgets/pull/25928.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)The class seems to be working fine if I print the thread ID in its methods, and if I run the following test program:
#include <wx/wx.h> class MyThread: public wxThread { public: wxSemaphore semaphore; MyThread(): wxThread(wxTHREAD_JOINABLE) { } virtual ExitCode Entry() { wxString s = wxString::Format("In thread ID %lx\n", wxThread::GetCurrentId()); printf(wxGetTranslation(s).mb_str(wxConvUTF8)); semaphore.Post(); return NULL; } }; int main(int argc, char **argv) { wxInitializer initializer; std::vector<MyThread *> threads; size_t i; size_t count = 10; // Creation for (i = 0; i < count; ++i) { threads.push_back(new MyThread()); } // Launch wxThreadError e; for (i = 0; i < count; ++i) { e = threads[i]->Run(); if (e != wxTHREAD_NO_ERROR) { fprintf(stderr, "Cannot launch thread no. %u\n", i); return -1; } } // Wait on semaphores for (i = 0; i < count; ++i) { threads[i]->semaphore.Wait(); } // Wait() wxThread::ExitCode ex; for (i = 0; i < count; ++i) { ex = threads[i]->Wait(); if (ex != NULL) { fprintf(stderr, "Error with thread %u\n", i); return -1; } } // Deletion for (i = 0; i < count; ++i) { delete threads[i]; } threads.clear(); return 0; }
The output is similar to the following:
UntranslatedStringHloder constructor in thread 0x123
UntranslatedStringHloder constructor in thread 0x456
UntranslatedStringHloder::get in thread 0x123
UntranslatedStringHloder::get in thread 0x456
In thread ID 123
In thread ID 456
UntranslatedStringHloder destructor in thread 0x123
UntranslatedStringHloder destructor in thread 0x456
This should demonstrate that we do not have any memory leaks as long as we use the wxThread class.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)This also seems to work using std::thread (not on MinGW, as it does not seem to provide std::thread):
#include <thread> #include <wx/wx.h> void myThread(wxSemaphore *semaphore) { wxString s = wxString::Format("In thread ID %lx\n", wxThread::GetCurrentId()); printf(wxGetTranslation(s).mb_str(wxConvUTF8)); semaphore->Post(); }; int main(int argc, char **argv) { wxInitializer initializer; std::vector<std::thread *> threads; std::vector<wxSemaphore *> semaphores;
size_t i; size_t count = 10; // Creation for (i = 0
; i < count; ++i) {
semaphores.push_back(new wxSemaphore);
threads.push_back(new std::thread(myThread, semaphores.back()));
}
// Wait on semaphores for (i = 0
; i < count; ++i) {
semaphores[i]->Wait();
}
// thread::join
for (i = 0; i < count; ++i) {
threads[i]->join();
}
// Deletion for (i = 0; i < count; ++i) { delete
threads[i];
delete semaphores[i];
}
threads.clear();
semaphores.clear();
return 0;
}With proper logging added to the UntranslatedStringHolder methods, the program shows the correct sequence of creation, usage and destruction of UntranslatedStringHolder objects.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25928)Sorry, I don't understand this: if we use critical sections and global map, why do we still use thread_local? I thought your plan was to use pointers instead of classes with thread_local, which would make more sense (but you'd have to find some way of deleting these pointers...).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)I thought your plan was to use pointers instead of classes with
thread_local, which would make more sense (but you'd have to find some way of deleting these pointers...).
That's the point: if we just have pointers, they get destroyed and I do not see any way of understanding when it happens.
I could think about the wxThread destructor, but does it get called when a std::thread ends?
If we want thread-localness and no memory leaks, we need a global container of such sets of untranslated strings, and a separate garbage collection logic that somehow frees up any pointers that are not needed any more because their thread ended.
Or alternatively, a fixed-size container, such as a queue that grows up to n elements, and whenever a new set is requested, the nth is cleared. But what would be a good value for n?
Alternative solution: a global container of untranslated strings that is used by all threads, so there is no thread-local data at all. Performance-wise, it should be as efficient as the current proposal when accessing strings, and could possibly avoid duplication if more than one threads use the same strings.
I was just trying to maintain thread-localness.
I hope I could understand your doubts and reply to them.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)Now that I think about it, there is a possible degree of optimization of this class: it could cache the pointer to its own set of untranslated string. The global "set of sets" with its wxCriticalSection etc. would only be accessed at creation and destruction time.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25928)I think I've just realized what's going on here: you think it's actually fine to rely on thread-local variables dtors being executed, but they can't use the object because its memory has already been freed by the time they run, is this so?
If it is, then maybe the current solution is fine, but it should be
- Carefully explained in a comment because it's far from obvious.
- Made conditional for MinGW because I don't think there is any reason for extra complexity and reduced performance for the other compilers.
TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
@ardovm pushed 1 commit.
- 539e976 Differentiate the UntranslatedStringHolder for MinGW and non-MinGW
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)I think I've just realized what's going on here: you think it's actually fine to rely on thread-local variables dtors being executed, but they can't use the object because its memory has already been freed by the time they run, is this so?
Yes, exactly. It's a hack, but it is addressing a ugly bug...
Please check it now. I tried to keep function wxTranslations::GetUntranslatedString as simple as possible.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
@ardovm pushed 1 commit.
- 152c019 Better wording.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
@ardovm pushed 1 commit.
- fc60f5e nullptr instead of NULL
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks, this looks good to me now, I'll just reformat it when applying.
In src/common/translation.cpp:
> @@ -1451,12 +1452,70 @@ wxString wxTranslations::DoGetBestAvailableTranslation(const wxString& domain, c
return lang;
}
+namespace {
+
+/* As of October 2025, MinGW has a bug regarding thread_local
+ * variables: their memory is de-allocated, and then their destructor
+ * is called.
+ *
+ * The UntranslatedStringHolder class works around this issue, having
+ * its destructor access no data members.
+ */
+#if defined __MINGW32__ || defined __MINGW64__
I think __MINGW32__ is defined when using 64-bit MinGW too, so the second check is redundant.
In src/common/translation.cpp:
> + std::unordered_set<wxString> *holder;
+public:
+ UntranslatedStringHolder(): holder(nullptr) {
+ }
Minor but why not
⬇️ Suggested change- std::unordered_set<wxString> *holder;
-public:
- UntranslatedStringHolder(): holder(nullptr) {
- }
+ std::unordered_set<wxString> *holder = nullptr;
+
+public:
+ UntranslatedStringHolder() = default;
In src/common/translation.cpp:
> @@ -1451,12 +1452,70 @@ wxString wxTranslations::DoGetBestAvailableTranslation(const wxString& domain, c
return lang;
}
+namespace {
+
+/* As of October 2025, MinGW has a bug regarding thread_local
+ * variables: their memory is de-allocated, and then their destructor
+ * is called.
+ *
+ * The UntranslatedStringHolder class works around this issue, having
+ * its destructor access no data members.
+ */
+#if defined __MINGW32__ || defined __MINGW64__
+
+class UntranslatedStringHolder {
Probably should make this class non-copyable just in case.
In src/common/translation.cpp:
> + static wxCriticalSection criticalSection; + static std::map<wxThreadIdType, std::unordered_set<wxString> > setsMap;
Normally those should have ms_ prefix to conform to our naming conventions.
In src/common/translation.cpp:
> +namespace {
+
+/* As of October 2025, MinGW has a bug regarding thread_local
+ * variables: their memory is de-allocated, and then their destructor
+ * is called.
+ *
+ * The UntranslatedStringHolder class works around this issue, having
+ * its destructor access no data members.
+ */
+#if defined __MINGW32__ || defined __MINGW64__
+
+class UntranslatedStringHolder {
+private:
+ static wxCriticalSection criticalSection;
+ static std::map<wxThreadIdType, std::unordered_set<wxString> > setsMap;
+ std::unordered_set<wxString> *holder;
And this one m_.
In src/common/translation.cpp:
> @@ -1451,12 +1452,70 @@ wxString wxTranslations::DoGetBestAvailableTranslation(const wxString& domain, c
return lang;
}
+namespace {
Minor style nitpick: we always put { on its own line (here and elsewhere).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
@ardovm pushed 1 commit.
- 205603d Coding style
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)I hope I understood well all your notes :-)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25928)I hope I understood well all your notes :-)
Perfectly, thanks! Will merge soon (should this be backported to 3.2 too?).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arrigo Marchiori
ardovm left a comment (wxWidgets/wxWidgets#25928)(should this be backported to 3.2 too?).
The affected file in the 3.2 branch does not contain any thread_local variables. It is probably still affected by
the memory leaks that were addressed by #23535 and #23543. This PR applies on top of them.
If you wish I can try to do some cherry-picks & merges and propose them as a PR to branch 3.2.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25928)Ah, yes, of course, sorry, I forgot that thread local change was done in 3.3 only.
And thanks for the offer, but it's better to leave this as is in 3.2, we still don't formally require C++11 there.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.