| Message ID | 20180426195309.26042-1-asafka7@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] zeromq: add libatomic linkage when available | expand |
Hello Asaf, On Thu, 26 Apr 2018 22:53:09 +0300, Asaf Kahlon wrote: > Fixes: > http://autobuild.buildroot.net/results/ff5f7d3bcd713db9c9d92a746d6ad7134748fe16 > http://autobuild.buildroot.net/results/9a743da1c3be673288c3482a9e7ddea12750206f > http://autobuild.buildroot.net/results/818b12387e6117569c4fab310b98c11fac80c047 > > If libatomic is available, it should be added to LIBS in CONF_ENV, since without > it we can get into a state in which symbols like __atomic_fetch_sub_4, > __atomic_compare_exchange_4 and __atomic_fetch_add_4 can't be found. > > Signed-off-by: Asaf Kahlon <asafka7@gmail.com> > --- > package/zeromq/zeromq.mk | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/package/zeromq/zeromq.mk b/package/zeromq/zeromq.mk > index 8273cad763..214bd9e35c 100644 > --- a/package/zeromq/zeromq.mk > +++ b/package/zeromq/zeromq.mk > @@ -58,4 +58,8 @@ else > ZEROMQ_CONF_OPTS += --disable-libunwind > endif > > +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y) > +ZEROMQ_CONF_OPTS += LIBS=-latomic > +endif While this does fix the problem, I don't think it's the most ideal fix. Indeed, the solution should be upstreamed. Currently, zeromq configure.ac uses LIBZMQ_CHECK_ATOMIC_INTRINSICS, which is implemented in acinclude.m4, to detect the availability of atomic intrinsics. It goes like this: AC_DEFUN([LIBZMQ_CHECK_ATOMIC_INTRINSICS], [{ AC_MSG_CHECKING(whether compiler supports __atomic_Xxx intrinsics) AC_COMPILE_IFELSE([AC_LANG_SOURCE([ /* atomic intrinsics test */ int v = 0; int main (int, char **) { int t = __atomic_add_fetch (&v, 1, __ATOMIC_ACQ_REL); return t; } ])], [AC_MSG_RESULT(yes) ; libzmq_cv_has_atomic_instrisics="yes" ; $1], [AC_MSG_RESULT(no) ; libzmq_cv_has_atomic_instrisics="no" ; $2] ) }]) It does only an AC_COMPILE_IFELSE(), so it succeeds on Sparc, as the need for -latomic only appears at link time. So I believe it should be changed to test with AC_LINK_IFELSE, and if it fails, test with AC_LINK_IFELSE with -latomic, to determine (1) If atomic intrinsics are available (2) If -latomic is needed to use atomic intrinsics Do you think you could have a look at implementing something like this ? That being said, I agree that a number of other packages in Buildroot are fixed by just passing LIBS=-latomic. Best regards, Thomas
Thomas, All, I completely agree that the solution should be on upstream, and the current solution is easiest one but not the best one. I'll try to build a patch on the upstream and send it here. Asaf. On Sat, Apr 28, 2018, 09:39 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello Asaf, > > On Thu, 26 Apr 2018 22:53:09 +0300, Asaf Kahlon wrote: > > Fixes: > > > http://autobuild.buildroot.net/results/ff5f7d3bcd713db9c9d92a746d6ad7134748fe16 > > > http://autobuild.buildroot.net/results/9a743da1c3be673288c3482a9e7ddea12750206f > > > http://autobuild.buildroot.net/results/818b12387e6117569c4fab310b98c11fac80c047 > > > > If libatomic is available, it should be added to LIBS in CONF_ENV, since > without > > it we can get into a state in which symbols like __atomic_fetch_sub_4, > > __atomic_compare_exchange_4 and __atomic_fetch_add_4 can't be found. > > > > Signed-off-by: Asaf Kahlon <asafka7@gmail.com> > > --- > > package/zeromq/zeromq.mk | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/package/zeromq/zeromq.mk b/package/zeromq/zeromq.mk > > index 8273cad763..214bd9e35c 100644 > > --- a/package/zeromq/zeromq.mk > > +++ b/package/zeromq/zeromq.mk > > @@ -58,4 +58,8 @@ else > > ZEROMQ_CONF_OPTS += --disable-libunwind > > endif > > > > +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y) > > +ZEROMQ_CONF_OPTS += LIBS=-latomic > > +endif > > While this does fix the problem, I don't think it's the most ideal fix. > Indeed, the solution should be upstreamed. Currently, zeromq > configure.ac uses LIBZMQ_CHECK_ATOMIC_INTRINSICS, which is implemented > in acinclude.m4, to detect the availability of atomic intrinsics. > > It goes like this: > > AC_DEFUN([LIBZMQ_CHECK_ATOMIC_INTRINSICS], [{ > AC_MSG_CHECKING(whether compiler supports __atomic_Xxx intrinsics) > AC_COMPILE_IFELSE([AC_LANG_SOURCE([ > /* atomic intrinsics test */ > int v = 0; > int main (int, char **) > { > int t = __atomic_add_fetch (&v, 1, __ATOMIC_ACQ_REL); > return t; > } > ])], > [AC_MSG_RESULT(yes) ; libzmq_cv_has_atomic_instrisics="yes" ; $1], > [AC_MSG_RESULT(no) ; libzmq_cv_has_atomic_instrisics="no" ; $2] > ) > }]) > > It does only an AC_COMPILE_IFELSE(), so it succeeds on Sparc, as the > need for -latomic only appears at link time. > > So I believe it should be changed to test with AC_LINK_IFELSE, and if > it fails, test with AC_LINK_IFELSE with -latomic, to determine > > (1) If atomic intrinsics are available > > (2) If -latomic is needed to use atomic intrinsics > > Do you think you could have a look at implementing something like this ? > > That being said, I agree that a number of other packages in Buildroot > are fixed by just passing LIBS=-latomic. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) > Embedded Linux and Kernel engineering > https://bootlin.com > <div dir="auto">Thomas, All,<div dir="auto"><br></div><div dir="auto">I completely agree that the solution should be on upstream, and the current solution is easiest one but not the best one. I'll try to build a patch on the upstream and send it here.</div><div dir="auto"><br></div><div dir="auto">Asaf.</div></div><br><div class="gmail_quote"><div dir="ltr">On Sat, Apr 28, 2018, 09:39 Thomas Petazzoni <<a href="mailto:thomas.petazzoni@bootlin.com">thomas.petazzoni@bootlin.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello Asaf,<br> <br> On Thu, 26 Apr 2018 22:53:09 +0300, Asaf Kahlon wrote:<br> > Fixes:<br> > <a href="http://autobuild.buildroot.net/results/ff5f7d3bcd713db9c9d92a746d6ad7134748fe16" rel="noreferrer noreferrer" target="_blank">http://autobuild.buildroot.net/results/ff5f7d3bcd713db9c9d92a746d6ad7134748fe16</a><br> > <a href="http://autobuild.buildroot.net/results/9a743da1c3be673288c3482a9e7ddea12750206f" rel="noreferrer noreferrer" target="_blank">http://autobuild.buildroot.net/results/9a743da1c3be673288c3482a9e7ddea12750206f</a><br> > <a href="http://autobuild.buildroot.net/results/818b12387e6117569c4fab310b98c11fac80c047" rel="noreferrer noreferrer" target="_blank">http://autobuild.buildroot.net/results/818b12387e6117569c4fab310b98c11fac80c047</a><br> > <br> > If libatomic is available, it should be added to LIBS in CONF_ENV, since without<br> > it we can get into a state in which symbols like __atomic_fetch_sub_4,<br> > __atomic_compare_exchange_4 and __atomic_fetch_add_4 can't be found.<br> > <br> > Signed-off-by: Asaf Kahlon <<a href="mailto:asafka7@gmail.com" target="_blank" rel="noreferrer">asafka7@gmail.com</a>><br> > ---<br> > package/zeromq/<a href="http://zeromq.mk" rel="noreferrer noreferrer" target="_blank">zeromq.mk</a> | 4 ++++<br> > 1 file changed, 4 insertions(+)<br> > <br> > diff --git a/package/zeromq/<a href="http://zeromq.mk" rel="noreferrer noreferrer" target="_blank">zeromq.mk</a> b/package/zeromq/<a href="http://zeromq.mk" rel="noreferrer noreferrer" target="_blank">zeromq.mk</a><br> > index 8273cad763..214bd9e35c 100644<br> > --- a/package/zeromq/<a href="http://zeromq.mk" rel="noreferrer noreferrer" target="_blank">zeromq.mk</a><br> > +++ b/package/zeromq/<a href="http://zeromq.mk" rel="noreferrer noreferrer" target="_blank">zeromq.mk</a><br> > @@ -58,4 +58,8 @@ else<br> > ZEROMQ_CONF_OPTS += --disable-libunwind<br> > endif<br> > <br> > +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)<br> > +ZEROMQ_CONF_OPTS += LIBS=-latomic<br> > +endif<br> <br> While this does fix the problem, I don't think it's the most ideal fix.<br> Indeed, the solution should be upstreamed. Currently, zeromq<br> <a href="http://configure.ac" rel="noreferrer noreferrer" target="_blank">configure.ac</a> uses LIBZMQ_CHECK_ATOMIC_INTRINSICS, which is implemented<br> in acinclude.m4, to detect the availability of atomic intrinsics.<br> <br> It goes like this:<br> <br> AC_DEFUN([LIBZMQ_CHECK_ATOMIC_INTRINSICS], [{<br> AC_MSG_CHECKING(whether compiler supports __atomic_Xxx intrinsics)<br> AC_COMPILE_IFELSE([AC_LANG_SOURCE([<br> /* atomic intrinsics test */<br> int v = 0;<br> int main (int, char **)<br> {<br> int t = __atomic_add_fetch (&v, 1, __ATOMIC_ACQ_REL);<br> return t;<br> }<br> ])],<br> [AC_MSG_RESULT(yes) ; libzmq_cv_has_atomic_instrisics="yes" ; $1],<br> [AC_MSG_RESULT(no) ; libzmq_cv_has_atomic_instrisics="no" ; $2]<br> )<br> }])<br> <br> It does only an AC_COMPILE_IFELSE(), so it succeeds on Sparc, as the<br> need for -latomic only appears at link time.<br> <br> So I believe it should be changed to test with AC_LINK_IFELSE, and if<br> it fails, test with AC_LINK_IFELSE with -latomic, to determine<br> <br> (1) If atomic intrinsics are available<br> <br> (2) If -latomic is needed to use atomic intrinsics<br> <br> Do you think you could have a look at implementing something like this ?<br> <br> That being said, I agree that a number of other packages in Buildroot<br> are fixed by just passing LIBS=-latomic.<br> <br> Best regards,<br> <br> Thomas<br> -- <br> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)<br> Embedded Linux and Kernel engineering<br> <a href="https://bootlin.com" rel="noreferrer noreferrer" target="_blank">https://bootlin.com</a><br> </blockquote></div>
Hello Asaf, On Sun, 29 Apr 2018 18:40:05 +0000, Asaf Kahlon wrote: > I completely agree that the solution should be on upstream, and the current > solution is easiest one but not the best one. I'll try to build a patch on > the upstream and send it here. Do you have any news about this ? If needed, you can leverage some code from Mesa3D, which I've just patched earlier today: https://lists.freedesktop.org/archives/mesa-dev/2018-May/194214.html It would be nice to have a patch in the near future, to fix this build issue. Let me know if you need help for this. Thanks! Thomas
diff --git a/package/zeromq/zeromq.mk b/package/zeromq/zeromq.mk index 8273cad763..214bd9e35c 100644 --- a/package/zeromq/zeromq.mk +++ b/package/zeromq/zeromq.mk @@ -58,4 +58,8 @@ else ZEROMQ_CONF_OPTS += --disable-libunwind endif +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y) +ZEROMQ_CONF_OPTS += LIBS=-latomic +endif + $(eval $(autotools-package))
Fixes: http://autobuild.buildroot.net/results/ff5f7d3bcd713db9c9d92a746d6ad7134748fe16 http://autobuild.buildroot.net/results/9a743da1c3be673288c3482a9e7ddea12750206f http://autobuild.buildroot.net/results/818b12387e6117569c4fab310b98c11fac80c047 If libatomic is available, it should be added to LIBS in CONF_ENV, since without it we can get into a state in which symbols like __atomic_fetch_sub_4, __atomic_compare_exchange_4 and __atomic_fetch_add_4 can't be found. Signed-off-by: Asaf Kahlon <asafka7@gmail.com> --- package/zeromq/zeromq.mk | 4 ++++ 1 file changed, 4 insertions(+)