Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Building with Clang 16 fails with sign-compare #936

Closed
andypost opened this issue Sep 3, 2023 · 13 comments
Closed

Building with Clang 16 fails with sign-compare #936

andypost opened this issue Sep 3, 2023 · 13 comments
Assignees

Comments

@andypost
Copy link

andypost commented Sep 3, 2023

Trying to build for Alpinelinux using Clang --cc=clang but getting error and the only workaround is to use --cc-opt="$CFLAGS -Eno-sign-compare"

checking for C compiler: clang
 + using Clang C compiler
 + Alpine clang version 16.0.6
building an "echo" program
checking for endianness ... little endian
checking for int size ... 4
checking for long size ... 8
checking for long long size ... 8
checking for void * size ... 8
checking for size_t size ... 8
checking for off_t size ... 8
checking for time_t size ... 8
....
In file included from src/nxt_socket_msg.c:7:
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_socket_msg.c:7:
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:276: build/src/nxt_socket_msg.o] Error 1
make: *** Waiting for unfinished jobs....
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.
@ac000
Copy link
Member

ac000 commented Sep 4, 2023

A long known issue between musl libc and clang.

One way to workaround this would be to create our own wrapper around CMSG_NXTHDR and do the #pragma thing...

Something like the following perhaps...

diff --git a/src/nxt_socket_msg.h b/src/nxt_socket_msg.h
index 04de1761..fca82f05 100644
--- a/src/nxt_socket_msg.h
+++ b/src/nxt_socket_msg.h
@@ -69,6 +69,19 @@ NXT_EXPORT ssize_t nxt_recvmsg(nxt_socket_t s,
     nxt_iobuf_t *iob, nxt_uint_t niob, nxt_recv_oob_t *oob);
 
 
+static inline struct cmsghdr *
+NXT_CMSG_NXTHDR(struct msghdr *msg, struct cmsghdr *cmsg)
+{
+#ifndef __GLIBC__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wsign-compare"
+#endif
+    return CMSG_NXTHDR(msg, cmsg);
+#ifndef __GLIBC__
+#pragma clang diagnostic pop
+#endif
+}
+
 nxt_inline void
 nxt_socket_msg_oob_init(nxt_send_oob_t *oob, int *fds)
 {
@@ -135,7 +148,7 @@ nxt_socket_msg_oob_get_fds(nxt_recv_oob_t *oob, nxt_fd_t *fd
)
 
     for (cmsg = CMSG_FIRSTHDR(&msg);
          cmsg != NULL;
-         cmsg = CMSG_NXTHDR(&msg, cmsg))
+         cmsg = NXT_CMSG_NXTHDR(&msg, cmsg))
     {
         size = cmsg->cmsg_len - CMSG_LEN(0);
 
@@ -174,7 +187,7 @@ nxt_socket_msg_oob_get(nxt_recv_oob_t *oob, nxt_fd_t *fd, nx
t_pid_t *pid)
 
     for (cmsg = CMSG_FIRSTHDR(&msg);
          cmsg != NULL;
-         cmsg = CMSG_NXTHDR(&msg, cmsg))
+         cmsg = NXT_CMSG_NXTHDR(&msg, cmsg))
     {
         size = cmsg->cmsg_len - CMSG_LEN(0);
 

ac000 added a commit to ac000/unit that referenced this issue Sep 6, 2023
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.

Link: <dotnet/runtime#16438>
Link: <https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-devtools/breakpad/breakpad/0001-Turn-off-sign-compare-for-musl-libc.patch>
Link: <https://github.com/dotnet/corefx/blob/57ff88bb75a0/src/Native/Unix/System.Native/pal_networking.c#L811-L829>
Link: <https://patchwork.yoctoproject.org/project/oe/patch/20220407191438.3696227-1-stefan@datenfreihafen.org/>
Closes: <nginx#936>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@ac000 ac000 self-assigned this Sep 6, 2023
@hongzhidao
Copy link
Contributor

Hi,

@ac000 @andypost
Could you try NGINX with the same OS?

> git clone https://github.com/nginx/nginx.git
> cd nginx
> ./auto/configure && make

@andrey-zelenkov Do we need to add it to BB?

@ac000
Copy link
Member

ac000 commented Sep 6, 2023

nginx has the same issue

$ auto/configure --with-cc=clang && make -j4
...
clang -c -pipe  -O -Wall -Wextra -Wpointer-arith -Wconditional-uninitialized -Wno-unused-parameter -Werror -g  -I src/core -I src/event -I src/event/modules -I src/event/quic -I src/os/unix -I objs \
        -o objs/src/event/ngx_event_connect.o \
        src/event/ngx_event_connect.c
src/event/ngx_event_udp.c:143:25: 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) \
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

@hongzhidao
Copy link
Contributor

Good catch, thanks.

@thresheek
Copy link
Member

See https://trac.nginx.org/nginx/ticket/2534 for the related issue in nginx. They decided not to fix that.

@ac000
Copy link
Member

ac000 commented Sep 6, 2023

That's their perogative. I don't see a compelling reason why we shouldn't.

@andypost
Copy link
Author

andypost commented Sep 6, 2023

@ac000 thank you for patch! it allows to build meantime exposed issues on armv7 and armv8 and riscv64

checking for GCC __builtin_expect() ... not found
checking for GCC __builtin_unreachable() ... not found
checking for GCC __builtin_prefetch() ... not found
checking for GCC __builtin_clz() ... not found
checking for GCC __builtin_popcount() ... not found
checking for GCC __attribute__ visibility ... not found
checking for GCC __attribute__ aligned ... not found
checking for GCC __attribute__ malloc ... not found
checking for GCC __attribute__ packed ... not found
checking for GCC __attribute__ unused ... not found
checking for GCC builtin atomic operations ... not found
checking for Solaris builtin atomic operations ... not found
checking for xlC builtin atomic operations ... not found
./configure: error: no atomic operations found.

maybe it's related to clang for this arches but at least "GCC" prefix for atomics is confusing when building with clang

@andypost
Copy link
Author

andypost commented Sep 6, 2023

Using clang 15.0.7 I'm getting the same error

@ac000
Copy link
Member

ac000 commented Sep 6, 2023

@andypost

Thanks for testing.

@ac000 thank you for patch! it allows to build meantime exposed issues on armv7 and armv8 and riscv64

checking for GCC __builtin_expect() ... not found
checking for GCC __builtin_unreachable() ... not found
checking for GCC __builtin_prefetch() ... not found
checking for GCC __builtin_clz() ... not found
checking for GCC __builtin_popcount() ... not found
checking for GCC __attribute__ visibility ... not found
checking for GCC __attribute__ aligned ... not found
checking for GCC __attribute__ malloc ... not found
checking for GCC __attribute__ packed ... not found
checking for GCC __attribute__ unused ... not found
checking for GCC builtin atomic operations ... not found
checking for Solaris builtin atomic operations ... not found
checking for xlC builtin atomic operations ... not found
./configure: error: no atomic operations found.

maybe it's related to clang for this arches but at least "GCC" prefix for atomics is confusing when building with clang

Hmm, we just have the three above checks here, one for GCC 4.1+ (heh, I'm just quoting the auto/atomic script), Solaris 10 & AIX xlC. The mind boggles, I suspect these just came straight from nginx...

It's also weird that it failed to find all the builtins and attributes...

I assume GCC builds Unit fine on such systems?

Some more investigation is probably required...

@andypost
Copy link
Author

andypost commented Sep 6, 2023

Yes, using GCC it builds/works fine, I just attempted to use clang to see effect

@ac000
Copy link
Member

ac000 commented Sep 6, 2023

Thanks for confirming.

No, sure, while GCC is my compiler of choice I see value in also testing with clang, they both find different issues in code.

@ac000
Copy link
Member

ac000 commented Sep 6, 2023

Lets move this to a separate issue...

ac000 added a commit to ac000/unit that referenced this issue Sep 11, 2023
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.

Link: <dotnet/runtime#16438>
Link: <https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-devtools/breakpad/breakpad/0001-Turn-off-sign-compare-for-musl-libc.patch>
Link: <https://github.com/dotnet/corefx/blob/57ff88bb75a0/src/Native/Unix/System.Native/pal_networking.c#L811-L829>
Link: <https://patchwork.yoctoproject.org/project/oe/patch/20220407191438.3696227-1-stefan@datenfreihafen.org/>
Closes: <nginx#936>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@ac000
Copy link
Member

ac000 commented Sep 11, 2023

Fix was committed.

@ac000 ac000 closed this as completed Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants