nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.29k stars 325 forks source link

Fix build on musl libc with clang. #941

Closed ac000 closed 10 months ago

ac000 commented 10 months ago

As reported by @andypost on GitHub, if you try to build Unit on a system that uses musl libc (such as Alpine Linux) with clang then you get the following

clang -c -pipe -fPIC -fvisibility=hidden -O -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -fstrict-aliasing -Wstrict-overflow=5 -Wmissing-prototypes -Werror -g -I src -I build/include \ \ \ -o build/src/nxt_socketpair.o \ -MMD -MF build/src/nxt_socketpair.dep -MT build/src/nxt_socketpair.o \ src/nxt_socketpair.c In file included from src/nxt_socketpair.c:8: src/nxt_socket_msg.h:138:17: error: comparison of integers of different signs: 'unsigned long' and 'long' [-Werror,-Wsign-compare] cmsg = CMSG_NXTHDR(&msg, cmsg)) ^~~~~~~ /usr/include/sys/socket.h:358:44: note: expanded from macro 'CMSG_NXTHDR' __CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \


In file included from src/nxt_socketpair.c:8:
src/nxt_socket_msg.h:177:17: error: comparison of integers of different signs: 'unsigned long' and 'long' [-Werror,-Wsign-compare]
         cmsg = CMSG_NXTHDR(&msg, cmsg))
                ^~~~~~~~~~~~~~~~~~~~~~~
/usr/include/sys/socket.h:358:44: note: expanded from macro 'CMSG_NXTHDR'
        __CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
make: *** [build/Makefile:261: build/src/nxt_socketpair.o] Error 1

GCC works fine, it seems to have some smarts so that it doesn't give warnings on system header files.

This seems to be a long standing issue with musl libc (bad casting in the CMSG_NXTHDR macro) and the workaround employed by several projects is to disable the -Wsign-compare clang warning for the code in question.

So, that's what we do. We wrap the CMSG_NXTHDR macro in a function, so we can use the pre-processor in it to selectively disable the warning.
ac000 commented 10 months ago

Added missing 'Closes' tag.

andypost commented 10 months ago

Thank you, posted review to https://github.com/nginx/unit/issues/936#issuecomment-1708938752 that fixed build on 4 arches

but it still fail on arm(7/hf/64) and riscv64 because of atomics - there's pipelines for clang 15 and 16 https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/51157/pipelines

ac000 commented 10 months ago

Thank you, posted review to #936 (comment) that fixed build on 4 arches

but it still fail on arm(7/hf/64) and riscv64 because of atomics - there's pipelines for clang 15 and 16 https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/51157/pipelines

To avoid any confusion. This is not related to this patch and is being tracked in a separate issue.

hongzhidao commented 10 months ago

LGTM. The only question is if we need to add a BB test or not. @thresheek @ac000 @lcrilly.

ac000 commented 10 months ago

Do we want a BB with say, Alpine Linux + clang?

Apart from the extra resources and possible extra time to complete a run, I don't think having more clang coverage would be bad thing in general. AFAICT the only builders that currently use clang are the FreeBSD ones and the code analyser.

hongzhidao commented 10 months ago

Do we want a BB with say, Alpine Linux + clang?

Yep, that's what I mean.

ac000 commented 10 months ago

Rebase onto master