GNOME Bugzilla – Bug 754027
Don't use C constructors for initializing critical sections on Windows
Last modified: 2018-11-03 10:48:04 UTC
Created attachment 309924 [details] [review] Don't use C constructors for initializing critical sections on Windows Instead, use memory barriers and just initialize it as part of orc_init(). Also, require that some sort of threading library be available. C constructors are toolchain-specific which brings its own set of problems. There's no guarantees about the order in which C constructors are called, it makes it impossible to use static libraries built with one toolchain on another one, and so on. Finally, C constructors are only useful when you need a single point of entry for initializing data shared throughout the library. Orc already has a function for doing that; namely orc_init(), so we can just use that. The atomic set/get of cs_inited using MemoryBarrier() was inspired by GLib's g_atomic_int_set/get implementation.
I'm all for it, library constructors are a disease :)
I'll be testing this properly on Windows in a little while; will update the bug when I do so. Please don't merge till then. ;)
Shouldn't the whole of orc_init() happens in a constructor instead...
Calling orc_init() is not required in order to use orc-generated functions. So in general, this patch will lead to crashes due to races. The problems with C constructors listed above are not relevant to Orc.
(In reply to David Schleef from comment #4) > > The problems with C constructors listed above are not relevant to Orc. Could you elaborate what you mean by that? Clearly it's relevant to some use cases..
(In reply to David Schleef from comment #4) > Calling orc_init() is not required in order to use orc-generated functions. > So in general, this patch will lead to crashes due to races. > This is not entirely correct. With this patch, if you do not call orc_init(), the critical sections will not be initialized and your program will immediately segfault when *_mutex_lock is called. > The problems with C constructors listed above are not relevant to Orc. Well, I wrote this patch specifically because of a use-case that I encountered. :) Allow me to elaborate. This patch is only relevant on Windows since pthread mutexes don't need to be initialized at runtime. The code had a toolchain-dependant implementation of C constructors, which meant that if you linked a liborc static library created with MinGW inside a project that was built and linked with MSVC, the C constructor would not be called at all, the CS would not be initialized, and your code would segfault as soon as orc called *_mutex_lock. Orc does build on MSVC, but the build system does not allow it and there's no MSVC binaries available. There are, however, MinGW binaries available. Also, unlike GNU C constructors, there is no way to guarantee the order in which MSVC C constructors are called, so using orc functions inside your program's C constructor was impossible. Finally, using C constructors this way on MSVC is an undocumented hack that might continue to be supported by MSVC because lots of people use it, but is actually an internal compiler feature for implementing global initializers: https://msdn.microsoft.com/en-us/library/bb918180.aspx Going back to... > Calling orc_init() is not required in order to use orc-generated functions. I wasn't aware of this. :) Why does that function exist at all then? Or do you mean that a subset of orc can be used without calling orc_init()? I couldn't find information about this in the docs. If orc_init() is indeed optional, we could just do both things. Keep the C constructor and also initialize inside orc_init(). That way all use cases will keep working. The overhead is the checking of one volatile int via a memory barrier at program load, which is negligible.
FWIW, it seems using C constructors causes problems with Windows 10. See bug 752837. We should almost certainly move away from using it.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/orc/issues/10.