Musl patches for 2.1.0 #35

staehle · 2020-06-16T00:25:22Z

Hello again:

We have various build targets with different archs, C libraries, etc. Most of them use Musl, but one of the ARM boards this is targeted for actually does use traditional glibc. Everything I add also has to work for our 32- and 64-bit simulation environments as well.

These are the cross-compile targets I'm working with:

arm-glibc
x86-musl
x86_64-musl

There are 2 commits here.

The first commit addresses the usage of pthread NP:

In applying the same kind of Musl patch for 'libnetconf2', CESNET/libnetconf2@153fe40 , I noticed that it actually doesn't work. check_function_exists(pthread_rwlockattr_setkind_np HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP) returns false / 'not found' on that arm-glibc target, which definitely does have it in that toolchain's "pthread.h" file.

So I ended up writing a patch for 'IPFIXcol2' that uses the slightly different CMake check_symbol_exists, which is actually recommended as a replacement by CMake. See note at the bottom of their docs here: https://cmake.org/cmake/help/latest/module/CheckFunctionExists.html

You might want to do a similar change in your 'libnetconf2'

arm-glibc target now:

-- The C compiler identification is GNU 5.5.0
-- The CXX compiler identification is GNU 5.5.0
...
-- Found LibFds: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libfds.so (found suitable version "0.2.0", minimum required is "0.2.0") 
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - not found
-- Check if compiler accepts -pthread
-- Check if compiler accepts -pthread - yes
-- Found Threads: TRUE  
-- Looking for pthread_rwlockattr_setkind_np
-- Looking for pthread_rwlockattr_setkind_np - found
-- Found ZLIB: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libz.so (found version "1.2.11") 
...
[ 75%] Building CXX object src/plugins/output/json/CMakeFiles/json-output.dir/src/File.cpp.o
cd /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json && /usr/bin/ccache /opt/toolchains/crosstools-arm-gcc-5.5-linux-4.1-glibc-2.26-binutils-2.28.1/bin/arm-buildroot-linux-gnueabi-g++  -Djson_output_EXPORTS -I/mnt/ipfixcol2.2.1.0-patch/include -I/mnt/ipfixcol2.2.1.0-patch/src -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include  -Os -pipe -g3 -fno-caller-saves -rdynamic -DNGX -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include -I/opt/toolchains/crosstools-arm-gcc-5.5-linux-4.1-glibc-2.26-binutils-2.28.1/arm-buildroot-linux-gnueabi/include -I/opt/toolchains/crosstools-arm-gcc-5.5-linux-4.1-glibc-2.26-binutils-2.28.1/arm-buildroot-linux-gnueabi/sysroot/usr/include -fvisibility=hidden -std=gnu++11 -DHAVE_PTHREAD_RWLOCKATTR_SETKIND_NP -O2 -DNDEBUG -fPIC   -o CMakeFiles/json-output.dir/src/File.cpp.o -c /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp

Notice it adds -DHAVE_PTHREAD_RWLOCKATTR_SETKIND_NP to CFLAGS. I don't know if this was the best way to do it or not.

x86_64-musl target now:

-- The C compiler identification is GNU 7.3.0
-- The CXX compiler identification is GNU 7.3.0
...
-- Found LibFds: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libfds.so (found suitable version "0.2.0", minimum required is "0.2.0") 
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE  
-- Looking for pthread_rwlockattr_setkind_np
-- Looking for pthread_rwlockattr_setkind_np - not found
-- Found ZLIB: /mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/lib/libz.so (found version "1.2.11") 
...
[ 75%] Building CXX object src/plugins/output/json/CMakeFiles/json-output.dir/src/File.cpp.o
cd /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json && /usr/bin/ccache /opt/toolchains/bin/x86_64-openwrt-linux-musl-g++  -Djson_output_EXPORTS -I/mnt/ipfixcol2.2.1.0-patch/include -I/mnt/ipfixcol2.2.1.0-patch/src -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include  -Os -pipe -fno-caller-saves -g3 -rdynamic -fhonour-copts -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro -Wno-error=unused-const-variable -I/opt/toolchains/include -I/mnt/ipfixcol2.2.1.0-patch/.odedeps/usr/include -I/opt/toolchains/x86_64-openwrt-linux-musl/include -fvisibility=hidden -std=gnu++11 -O2 -DNDEBUG -fPIC   -o CMakeFiles/json-output.dir/src/File.cpp.o -c /mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp

The second commit addresses the usage of strerror_r:

From manpage of 'strerror' group: https://man7.org/linux/man-pages/man3/strerror.3.html There are 2 signatures of strerror_r: XSI and GNU. Musl does not do any GNU extensions, so it only provides the XSI signature returning an int. strerror_r is considered non-portable and deprecated anyways, so replace with strerror_l.

"libnl" did something similar: https://lists.infradead.org/pipermail/libnl/2016-August/002196.html

/mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp: In static member function 'static int File::dir_create(ipx_ctx_t*, const string&)':
/mnt/ipfixcol2.2.1.0-patch/src/plugins/output/json/src/File.cpp:346:45: error: invalid conversion from 'int' to 'const char*' [-fpermissive]
             const char *err_str = strerror_r(errno, buffer, 128);
                                   ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

With these 2 commits, I can successfully build for all my targets, and I'll consider that it fixes #34

…pport. Addresses GitHub CESNET/ipfixcol2 issue CESNET#34 Signed-off-by: Jake Staehle <[email protected]>

…patible. Addresses GitHub CESNET/ipfixcol2 issue CESNET#34 Signed-off-by: Jake Staehle <[email protected]>

Lukas955 · 2020-06-17T08:30:25Z

Thank you for the patch.
I will try to merge it soon but I will have to resolve some collisions with newer changes in devel branch. Since I'm quite busy right now, I will do it early next week.

Using strerror_r and strerror_l is always painful. The first one has two different definitions and the latter cannot use LC_GLOBAL_LOCALE. These functions are just stupidly designed.
However, just after brief look at the code of the patch, I can tell that I'm not fan of code duplication and I'm not sure whether the string returned by strerror_l can be considered as valid after freelocale().

staehle · 2020-06-17T14:48:14Z

Hey, sounds good. This PR is just an equivalent to the patches that we are now using internally, and I wanted to make sure it got shared upstream :)

That's also why I split this into 2 commits -- the second one with the strerror changes is definitely more hacky. I do agree though, the copy/pasted segments are bad and it would be much better as a function somewhere. I initially did that, however, I didn't really see a good common place in the 'json' plugin to put it. Sorry about that!

For the strerror_l code though, it was largely taken from the popular "libnl" doing the same thing here, so it should be valid: https://github.com/thom311/libnl/blob/master/lib/utils.c#L120

You don't have to actually approve & merge this PR btw. More or less, I'm just pointing out some issues with Musl and how I resolved them. If you just want to git cherry-pick -x <sha> the individual commits into your "devel" branch and clean it up as you see fit, or just ignore this entirely, both are totally OK. I don't mean to create work for you :)

Thanks!

staehle added 2 commits June 15, 2020 18:33

Add check for 'pthread_rwlockattr_setkind_np', which Musl does not su…

c789c06

…pport. Addresses GitHub CESNET/ipfixcol2 issue CESNET#34 Signed-off-by: Jake Staehle <[email protected]>

Replace nonportable 'strerror_r' with 'strerror_l', which is Musl-com…

7342013

…patible. Addresses GitHub CESNET/ipfixcol2 issue CESNET#34 Signed-off-by: Jake Staehle <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Musl patches for 2.1.0 #35

Musl patches for 2.1.0 #35

staehle commented Jun 16, 2020

Lukas955 commented Jun 17, 2020

staehle commented Jun 17, 2020

Musl patches for 2.1.0 #35

Are you sure you want to change the base?

Musl patches for 2.1.0 #35

Conversation

staehle commented Jun 16, 2020

Lukas955 commented Jun 17, 2020

staehle commented Jun 17, 2020