On Mon, Feb 17, 2014 at 05:53:32PM +0200, Martin Storsjö wrote: > On Mon, 17 Feb 2014, Diego Biurrun wrote: > >On Mon, Feb 17, 2014 at 03:31:20PM +0200, Martin Storsjö wrote: > >>On Mon, 17 Feb 2014, Diego Biurrun wrote: > > > >>Doing this kinda gives you a circular dependency, enabling 'threads' > >>requires one of the elements in $THREADS_LIST to be enabled. But if > >>'threads' was disabled due to missing atomics, we should disable all > >>the elements in $THREADS_LIST. > >> > >>Namely, there's even another aspect to this which you need to keep > >>in mind. Currently, you can't do a build with any threading > >>mechanism enabled unless there is some sort of working atomics. If > >>atomics aren't available (even emulated via pthreads), the build > >>will fail. If you instead silently disable HAVE_THREADS, but keep > >>HAVE_<foo>THREADS enabled, there could be some codepath that checks > >>for HAVE_<foo>THREADS which actually will run threads, expecting > >>there to be working atomics, giving you silent bugs which may crop > >>up much later. Currently we only have such code for HAVE_PTHREADS > >>(yes, we have some code that uses HAVE_THREADS and a few other > >>occurrances that uses HAVE_PTHREADS directly) and since we can > >>implement atomics using pthreads, this isn't an issue. But if we > >>later add a special codepath for HAVE_W32THREADS, we would have > >>introduced a bug, long after this discussion here at hand is > >>settled, in case your patch was merged. > > > >The current situation is also non-ideal and brittle because the logic > >for enabling threading is scattered over configure and the C code. > > > >First off, the build should never fail due to a configuration that we > >consider invalid. configure should detect such situations and abort > >instead of having a user go through an expensive build run. At the > >end of such a build run it's probably not clear to users that they > >need to use configure options to resolve the situation. > > Indeed, although failing to build is better than producing builds > that can fail randomly (builds with some sort of threading enabled, > but no implementation of atomics). Failing (or resolving it in > another way) at the configure stage obviously is better.
We're on the same page then. > >This is just to explain why I consider it an improvement to move all > >of the logic to configure. Of course, we need to get the logic right > >in the process. > > > >The dependency tree is the following one (correct me if I'm wrong): > > > >threads --+--> pthreads --- atomics (optional) > > | > > +--> w32threads --> native atomics > > > >In configure-speak that translates to > > > >w32threads_deps="atomics_native" > >threads_if_any"$THREADS_LIST" > > > >and avoids any problem of HAVE_THREADS being disabled at the same time > >that HAVE_<foo>THREADS is enabled. If a special codepath for W32THREADS > >gets added, it would not be used unless atomics were available as well. > > That would indeed solve the issue quite neatly. > > The only thing that does get lost in this formulation is the fact > that every element in $THREADS_LIST (except for pthreads) need to > have that dependency - in case we add support for another threading > backend we would have to make sure that this dependency gets added > there as well. This obviously isn't something that is happening too > often, but we should at least make sure we scatter notes in > configure at the right places to document this issue. Do you have suggestions where and how to document this on top of the patch I just sent? Diego _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
