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.
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.
// Martin
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
