On Mon, Feb 17, 2014 at 03:31:20PM +0200, Martin Storsjö wrote:
> On Mon, 17 Feb 2014, Diego Biurrun wrote:
> >--- a/configure
> >+++ b/configure
> >@@ -1606,7 +1606,7 @@ atomics_gcc_if="sync_val_compare_and_swap"
> >atomics_suncc_if="atomic_cas_ptr machine_rw_barrier"
> >atomics_win32_if="MemoryBarrier"
> >atomics_native_if_any="$ATOMICS_LIST"
> >-threads_if_any="$THREADS_LIST"
> >+threads_if_any="atomics_native pthreads"
>
> Ok, so this takes some time to think through again since the thread
> was long dead before reviving it...
Yeah, sorry, I should have followed up more quickly...
> What your current patch does is not ok. Consider that you're
> building for a platform where there is no threading implementation
> at all, but your compiler supports something from $ATOMICS_LIST.
> That will give you atomics_native=yes, and will thus also end up
> with threads=yes even though you don't have threading whatsoever.
Good point.
> Also, separately, even if you made this patch work as you intended
> it to, there's still another issue in the case where a non-pthread
> threading implementation exists (e.g. w32threads), but there's no
> atomics. Then you will end up with thread_type=w32threads, configure
> will happily print "threading support: w32threads", the user will be
> happy to have threading ... although configure silently set
> HAVE_THREADS=0 since not all threading dependecies were available.
This patch sort of depends on the next one in the series that fixes the
informative configure output. I think it makes sense to squash the two.
I'll resend a squashed version just for reference. Or maybe not, just
changing this patch to do the right thing might be enough.
> 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.
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.
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
