diff mbox

[v2,1/2] libffi: make thread support optional

Message ID 1410784002-8659-1-git-send-email-jezz@sysmic.org
State Changes Requested
Headers show

Commit Message

Jérôme Pouiller Sept. 15, 2014, 12:26 p.m. UTC 
From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Fixes:

  http://autobuild.buildroot.org/results/7ee57d01917ea72d1811469e482513dda2ceb1ea/build-end.log

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
---
v2:
   - Fix boolean expression in patch
   - Renumber patch
   - Add my Signed-off

 ...bffi-006-Make-thread-support-conditionnal.patch | 74 ++++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 package/libffi/libffi-006-Make-thread-support-conditionnal.patch

Comments

Thomas Petazzoni Oct. 29, 2014, 9:24 p.m. UTC | #1 
Dear Jérôme Pouiller,

On Mon, 15 Sep 2014 14:26:41 +0200, Jérôme Pouiller wrote:
> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> Fixes:
> 
>   http://autobuild.buildroot.org/results/7ee57d01917ea72d1811469e482513dda2ceb1ea/build-end.log
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>

To be honest, I am wondering if we really should bother merging a patch
to make libffi usable on configurations that have thread support
disabled. Nowadays, not having thread support is also impossible due to
the large number of libraries/applications that rely on threads. libffi
is generally used in "big" things like Python or glib, so not having
thread support in systems using such big things is a bit unlikely.

In addition, I'm not sure the patch could be upstreamed as is, which
means we would have to carry it in Buildroot forever.

Peter, Arnout, Gustavo, what is your point of view on this?

Thomas
Gustavo Zacarias Oct. 29, 2014, 9:54 p.m. UTC | #2 
On 10/29/2014 06:24 PM, Thomas Petazzoni wrote:

> To be honest, I am wondering if we really should bother merging a patch
> to make libffi usable on configurations that have thread support
> disabled. Nowadays, not having thread support is also impossible due to
> the large number of libraries/applications that rely on threads. libffi
> is generally used in "big" things like Python or glib, so not having
> thread support in systems using such big things is a bit unlikely.
> 
> In addition, I'm not sure the patch could be upstreamed as is, which
> means we would have to carry it in Buildroot forever.
> 
> Peter, Arnout, Gustavo, what is your point of view on this?

+1, ditching threads (uClibc) is for really small targets which these
days would mostly mean noMMU.
Regards.
Arnout Vandecappelle Oct. 30, 2014, 8:09 a.m. UTC | #3 
On 29/10/14 22:24, Thomas Petazzoni wrote:
> Dear Jérôme Pouiller,
> 
> On Mon, 15 Sep 2014 14:26:41 +0200, Jérôme Pouiller wrote:
>> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>
>> Fixes:
>>
>>   http://autobuild.buildroot.org/results/7ee57d01917ea72d1811469e482513dda2ceb1ea/build-end.log
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
> 
> To be honest, I am wondering if we really should bother merging a patch
> to make libffi usable on configurations that have thread support
> disabled. Nowadays, not having thread support is also impossible due to
> the large number of libraries/applications that rely on threads. libffi
> is generally used in "big" things like Python or glib, so not having
> thread support in systems using such big things is a bit unlikely.
> 
> In addition, I'm not sure the patch could be upstreamed as is, which
> means we would have to carry it in Buildroot forever.
> 
> Peter, Arnout, Gustavo, what is your point of view on this?

 I agree. Removing toolchain requirements from a package is really a feature
patch, so we shouldn't do it unless it's upstreamable or when it's more of a
build issue than a code issue. There may be an exception when a package that has
a lot of dependants suddenly grows a toolchain dependency, but that's not the
case here.

 On the other hand, why shouldn't this patch be upstreamable?


 Regards,
 Arnout
Thomas Petazzoni Oct. 30, 2014, 10:31 a.m. UTC | #4 
Dear Arnout Vandecappelle,

On Thu, 30 Oct 2014 09:09:35 +0100, Arnout Vandecappelle wrote:

>  I agree. Removing toolchain requirements from a package is really a feature
> patch, so we shouldn't do it unless it's upstreamable or when it's more of a
> build issue than a code issue. There may be an exception when a package that has
> a lot of dependants suddenly grows a toolchain dependency, but that's not the
> case here.

Agreed.

>  On the other hand, why shouldn't this patch be upstreamable?

Because it shouldn't hardcode:

+#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)

But instead detect in configure.ac if threads are available, and then
use that.

So, what I would propose is that if Jérôme is really interested in
having libffi available in non-threaded configurations, he works on a
patch that is upstreamable, submit it upstream, and once it's upstream,
we backport it in Buildroot, until upstream does a new release.

What do you think?

Thomas
Jérôme Pouiller Oct. 30, 2014, 10:51 a.m. UTC | #5 
On Thursday 30 October 2014 11:31:34 Thomas Petazzoni wrote:
> Dear Arnout Vandecappelle,
> 
> On Thu, 30 Oct 2014 09:09:35 +0100, Arnout Vandecappelle wrote:
> >  I agree. Removing toolchain requirements from a package is really a
> >  feature
> > 
> > patch, so we shouldn't do it unless it's upstreamable or when it's more of
> > a build issue than a code issue. There may be an exception when a package
> > that has a lot of dependants suddenly grows a toolchain dependency, but
> > that's not the case here.
> 
> Agreed.
> 
> >  On the other hand, why shouldn't this patch be upstreamable?
> 
> Because it shouldn't hardcode:
> 
> +#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)
> 
> But instead detect in configure.ac if threads are available, and then
> use that.
> 
> So, what I would propose is that if Jérôme is really interested in
> having libffi available in non-threaded configurations, he works on a
> patch that is upstreamable, submit it upstream, and once it's upstream,
> we backport it in Buildroot, until upstream does a new release.
My only objective was to fix properly an autobuilder failure :). 
We can keep current implementation and mark this patch as rejected.


I have noticed that dependency to BR2_TOOLCHAIN_HAS_THREADS is not properly 
propagated to all packages that depend on python. I will check that (and send 
a patch before 2014.11rc1 release).
diff mbox

Patch

diff --git a/package/libffi/libffi-006-Make-thread-support-conditionnal.patch b/package/libffi/libffi-006-Make-thread-support-conditionnal.patch
new file mode 100644
index 0000000..72e7e5d
--- /dev/null
+++ b/package/libffi/libffi-006-Make-thread-support-conditionnal.patch
@@ -0,0 +1,74 @@ 
+From a7f6342120060564a829704cceb843e53e0b34a9 Mon Sep 17 00:00:00 2001
+From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+Date: Sat, 17 Nov 2012 18:44:16 +0100
+Subject: [PATCH 3/3] Make thread support conditionnal
+
+When libffi is linked against a C library that does not have thread
+support, it is not necessary to use a mutex to protect global
+variables, since the application calling libffi cannot be
+multi-threaded.
+
+Therefore, make the libffi thread support conditionnal: when we're
+building against uClibc with no thread support, don't use the
+pthread_mutex.
+
+Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
+---
+ src/closures.c |   12 ++++++++++++
+ 1 file changed, 12 insertions(+)
+
+diff --git a/src/closures.c b/src/closures.c
+index 1b37827..3d151f6 100644
+--- a/src/closures.c
++++ b/src/closures.c
+@@ -70,7 +70,10 @@
+ 
+ # elif FFI_MMAP_EXEC_WRIT /* !FFI_EXEC_TRAMPOLINE_TABLE */
+ 
++#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)
+ #define USE_LOCKS 1
++#endif
++
+ #define USE_DL_PREFIX 1
+ #ifdef __GNUC__
+ #ifndef USE_BUILTIN_FFS
+@@ -116,7 +119,10 @@
+ #include <mntent.h>
+ #endif /* HAVE_MNTENT */
+ #include <sys/param.h>
++
++#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)
+ #include <pthread.h>
++#endif
+ 
+ /* We don't want sys/mman.h to be included after we redefine mmap and
+    dlmunmap.  */
+@@ -214,8 +220,10 @@ static int dlmunmap(void *, size_t);
+ 
+ #if !(defined(X86_WIN32) || defined(X86_WIN64) || defined(__OS2__)) || defined (__CYGWIN__) || defined(__INTERIX)
+ 
++#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)
+ /* A mutex used to synchronize access to *exec* variables in this file.  */
+ static pthread_mutex_t open_temp_exec_file_mutex = PTHREAD_MUTEX_INITIALIZER;
++#endif
+ 
+ /* A file descriptor of a temporary file from which we'll map
+    executable pages.  */
+@@ -473,9 +481,13 @@ dlmmap (void *start, size_t length, int prot,
+ 
+   if (execsize == 0 || execfd == -1)
+     {
++#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)
+       pthread_mutex_lock (&open_temp_exec_file_mutex);
++#endif
+       ptr = dlmmap_locked (start, length, prot, flags, offset);
++#if !defined(__UCLIBC__) || !defined(__HAS_NO_THREADS__)
+       pthread_mutex_unlock (&open_temp_exec_file_mutex);
++#endif
+ 
+       return ptr;
+     }
+-- 
+1.7.9.5
+