openwrt / packages

Community maintained packages for OpenWrt. Documentation for submitting pull requests is in CONTRIBUTING.md
GNU General Public License v2.0
4.05k stars 3.51k forks source link

bind: Missing libatomic dependency #6360

Closed sjjgo closed 6 years ago

sjjgo commented 6 years ago

Maintainer: @nmeyerhans Environment: MIPS, BT HomeHub 5a, master

Description: The updated version of BIND doesn't build without the libatomic dependency. I have fixed this issue in https://github.com/openwrt/packages/pull/6359. It's a minuscule change – hopefully you can approve it. Thanks.

sjjgo commented 6 years ago

I have done a bit more digging on this bug. In the interests of minimising dependencies I wondered if libatomic was really necessary.

There were changes in bind's configure script between 9.11.2-P1 and 9.11.3. These are the changes: https://gitlab.isc.org/isc-projects/bind9/commit/fd82c70695888c134287b8018296028c252d100e

It seems that this script means that when building libatomic has gone from not being required to being required. Therefore it is appropriate that the dependency is added to the Makefile.

nmeyerhans commented 6 years ago

No, bind does not have a dependency on libatomic. The upstream change that you referenced was a change to auto-detection of libatomic, if it's available, but is not intended to introduce a hard dependency.

I do not have difficulty building bind on mvebu or omap without libatomic. Can you please provide a complete transcript from the following commands so we can debug this further?

make package/bind/clean V=s
make package/bind/compile V=s
nmeyerhans commented 6 years ago

It may also be worthwhile to look at bind's config.log, or at least the section from the line containing checking if -latomic is needed to a couple lines after the one containing checking architecture type for atomic operations

You can find this file as build_dir/*/bind-9.11.3/config.log in your build tree.

It's possible that libatomic is needed on certain architectures. The config.log details should tell us for sure, or if the test is misbehaving in some way.

sjjgo commented 6 years ago

Thanks for picking this up. I think that change means that if libatomic is present it will try to build with it, though. And then when it can't find a built libatomic the build fails. From my reading 9.11.2-P1 doesn't check for stdatomic.h at all. It seems to be that 9.11.3 configure sees stdatomic.h and says 'great, I'll use that'. At least that's my understanding of it. Perhaps this is not the case for the other architectures that you mentioned?

The most relevant lines from the build in 9.11.3 are:

checking for usable stdatomic.h... yes
checking if -latomic is needed to use 64-bit stdatomic.h primitives... yes
checking architecture type for atomic operations... mips

And the build subsequently fails missing libatomic.

Note that if I roll this back to 9.11.2-P1 then I get only:

checking architecture type for atomic operations... mips

And the build goes fine.

I have attached the full build transcripts for 9.11.3 and 9.11.2-P1 here: bind-9.11.3-mips.txt bind-9.11.2-P1-mips.txt

I was able to build 9.11.3 by setting --enable-atomic=no but I don't know enough about the role of atomic operations in bind to know if this is an acceptable thing to apply to all platforms, or if we might need some architecture-specific dependencies here.

Here's the build log for 9.11.3 with --enable-atomic=no set: bind-9.11.3-mips--enable-atomic=no.txt

sjjgo commented 6 years ago

Ah, sorry I didn't get your follow-up until I posted mine. I've checked the config.log as you suggest. Here's the output:

bind-9.11.3-mips-config.log

Here are what seems to be the important lines:

configure:20308: checking for usable stdatomic.h
configure:20326: mips-openwrt-linux-musl-gcc -c -Os -pipe -mno-branch-likely -mips32r2 -mtune=24kc -fno-caller-saves -fno-plt -fhonour-copts -Wno-error=unused-but-set-variable -Wno-error=unused-result -msoft-float -iremap/home/person/source/build_dir/target-mips_24kc_musl/bind-9.11.3:bind-9.11.3 -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro   -I/home/person/source/staging_dir/target-mips_24kc_musl/usr/include -I/home/person/source/staging_dir/target-mips_24kc_musl/include -I/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/usr/include -I/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/include/fortify -I/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/include  -D_GNU_SOURCE conftest.c >&5
configure:20326: $? = 0
configure:20327: result: yes
configure:20422: checking if -latomic is needed to use 64-bit stdatomic.h primitives
configure:20435: mips-openwrt-linux-musl-gcc -o conftest -Os -pipe -mno-branch-likely -mips32r2 -mtune=24kc -fno-caller-saves -fno-plt -fhonour-copts -Wno-error=unused-but-set-variable -Wno-error=unused-result -msoft-float -iremap/home/person/source/build_dir/target-mips_24kc_musl/bind-9.11.3:bind-9.11.3 -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro   -I/home/person/source/staging_dir/target-mips_24kc_musl/usr/include -I/home/person/source/staging_dir/target-mips_24kc_musl/include -I/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/usr/include -I/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/include/fortify -I/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/include  -D_GNU_SOURCE -L/home/person/source/staging_dir/target-mips_24kc_musl/usr/lib -L/home/person/source/staging_dir/target-mips_24kc_musl/lib -L/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/usr/lib -L/home/person/source/staging_dir/toolchain-mips_24kc_gcc-7.3.0_musl/lib -znow -zrelro  conftest.c -lz  >&5
/home/person/source/tmp/ccroqla8.o: In function `main':
conftest.c:(.text.startup+0x24): undefined reference to `__atomic_fetch_add_8'
collect2: error: ld returned 1 exit status
configure:20435: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "BIND"
| #define PACKAGE_TARNAME "bind"
| #define PACKAGE_VERSION "9.11"
| #define PACKAGE_STRING "BIND 9.11"
| #define PACKAGE_BUGREPORT "info@isc.org"
| #define PACKAGE_URL "https://www.isc.org/downloads/BIND/"
| #define STDC_HEADERS 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_MEMORY_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_UNISTD_H 1
| #define HAVE_DLFCN_H 1
| #define LT_OBJDIR ".libs/"
| #define ISC_BUFFER_USEINLINE 1
| #define STDC_HEADERS 1
| #define HAVE_FCNTL_H 1
| #define HAVE_REGEX_H 1
| #define HAVE_SYS_TIME_H 1
| #define HAVE_UNISTD_H 1
| #define HAVE_SYS_MMAN_H 1
| #define HAVE_SYS_SELECT_H 1
| #define HAVE_SYS_PARAM_H 1
| #define HAVE_SYS_SOCKET_H 1
| #define HAVE_NET_ROUTE_H 1
| #define HAVE_LINUX_NETLINK_H 1
| #define HAVE_LINUX_RTNETLINK_H 1
| #define FLEXIBLE_ARRAY_MEMBER /**/
| #define HAVE_MMAP 1
| #define HAVE_SETEUID 1
| #define HAVE_SETEGID 1
| #define HAVE_SETRESGID 1
| #define HAVE_FTELLO 1
| #define HAVE_FSEEKO 1
| #define ISC_SOCKADDR_LEN_T socklen_t
| #define TIME_WITH_SYS_TIME 1
| #define HAVE_UNAME 1
| #define WORDS_BIGENDIAN 1
| #define PATH_RANDOMDEV "/dev/urandom"
| #define HAVE_FIPS_MODE 1
| #define HAVE_OPENSSL_DSA 1
| #define HAVE_EVP_SHA256 1
| #define HAVE_EVP_SHA384 1
| #define HAVE_EVP_SHA512 1
| #define HAVE_OPENSSL_ECDSA 1
| #define HAVE_OPENSSL_EVP_AES 1
| #define AES_CC 1
| #define HAVE_CLOCK_GETTIME 1
| #define HAVE_ZLIB 1
| #define HAVE_FLOCKFILE 1
| #define HAVE_GETCUNLOCKED 1
| #define HAVE_CATGETS 1
| #define WANT_IPV6 1
| #define HAVE_ADDRINFO 1
| #define IRS_GETNAMEINFO_SOCKLEN_T socklen_t
| #define IRS_GETNAMEINFO_BUFLEN_T size_t
| #define IRS_GETNAMEINFO_FLAGS_T int
| #define IRS_GAISTRERROR_RETURN_T const char *
| #define HAVE_GETADDRINFO 1
| #define HAVE_GAISTRERROR 1
| #define HAVE_GETIFADDRS 1
| #define HAVE_STRERROR 1
| #define HAVE_CHROOT 1
| #define HAVE_SYS_PRCTL_H 1
| #define HAVE_SYS_UN_H 1
| #define HAVE_TZSET 1
| #define HAVE_STRINGS_H 1
| #define HAVE_IF_NAMETOINDEX 1
| #define HAVE_NANOSLEEP 1
| #define HAVE_USLEEP 1
| /* end confdefs.h.  */
| #include <stdatomic.h>
| int
| main ()
| {
| atomic_int_fast64_t val = 0; atomic_fetch_add_explicit(&val, 1, memory_order_relaxed);
|   ;
|   return 0;
| }
configure:20440: result: yes
configure:20560: checking architecture type for atomic operations
configure:20562: result: mips

The failure in there just seems to be a test to see if it needs to use 64-bit stdatomic.h primitives. So I think the configure script is working correctly?

sjjgo commented 6 years ago

I've closed my PR too, it's clearly not the correct fix for this given your success with other architectures.

nmeyerhans commented 6 years ago

Thanks. I'm not sure what the correct solution will be at this time. The obvious options are:

I don't fully understand the ramifications of the first two options. I don't like the last option because that list would need to be maintained by hand. I'm open to suggestions...

easyteacher commented 6 years ago

ramips needs it...

sjjgo commented 6 years ago

Are all openwrt platforms able to build libatomic? Presumably the check in bind's configure is there because it is not universally supported?

I think musl comes with stdatomic.h, but not sure which architectures are covered. For the platforms you mentioned, what happens with the stdatomic.h checks? Presumably it fails the test and builds without it?

That suggests that forcing it as a dependency means a redundant dependency for some platforms?

So perhaps disabling it for all is the best quick fix. So far as I can tell the atomic operations provide a performance boost on multicore machines. Not sure how critical this is for openwrt deployments. My first thought is 'not very', I am guessing it only makes a difference if bind is doing a lot of work.

The final option does seem the best, but I appreciate the platform by platform issue. There seems a good chance of it breaking when bind changes and when toolchains are updated. I will have a look at the libatomic package to see if someone has already done the work.

sjjgo commented 6 years ago

@easyteacher Have you tried building with --enable-atomic=no?

easyteacher commented 6 years ago

@sjjgo I haven't tried. But after I know it can boost performance on multicore platforms, I decide to keep it enabled.

sjjgo commented 6 years ago

@easyteacher I imagine that any such speed-up would only be noticeable on a machine that is primarily an DNS server and is dealing with a large number of requests. If your use case demands maximum throughput for bind though, I'd be curious to hear about it. If it's an edge case then it might be necessary for you to build it yourself with your own flags set.

nmeyerhans commented 6 years ago

Yeah, considering that previous versions of the bind package never enabled libatomic support, my inclination is to explicitly disable it everywhere, at least temporarily. This will at least get us back to the previous state on all platforms.

sjjgo commented 6 years ago

I agree. That will at least mean everything builds.

nmeyerhans commented 6 years ago

Closed via #6375