[musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual

mailing list of musl libc
 help / color / mirror / code / Atom feed

* [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
@ 2024-08-02 21:27 Brad House
  2024-08-02 23:38 ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Brad House @ 2024-08-02 21:27 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 4120 bytes --]

I'm the maintainer of c-ares (https://c-ares.org) and have been scanning 
the CI build logs for various systems to catch warnings, and on Alpine 
Linux (which obviously uses musl c) we get these warnings, specifically 
when using clang (but not oddly not on gcc):

/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast from 
'const struct in6_addr *' to 'unsigned char *' drops const qualifier 
[-Wcast-qual]
    93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
       |         ^
/usr/include/netinet/in.h:120:48: note: expanded from macro 
'IN6_IS_ADDR_MULTICAST'
   120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
       |                                                ^
/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:95:16: warning: cast 
from 'const struct in6_addr *' to 'unsigned int *' drops const qualifier 
[-Wcast-qual]
    95 |     } else if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr) ||
       |                ^
/usr/include/netinet/in.h:115:24: note: expanded from macro 
'IN6_IS_ADDR_LOOPBACK'
   115 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 
0 && \
       |                        ^
/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:96:16: warning: cast 
from 'const struct in6_addr *' to 'unsigned char *' drops const 
qualifier [-Wcast-qual]
    96 | IN6_IS_ADDR_LINKLOCAL(&addr6->sin6_addr)) {
       |                ^
/usr/include/netinet/in.h:123:24: note: expanded from macro 
'IN6_IS_ADDR_LINKLOCAL'
   123 |         ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) 
(a))[1] & 0xc0) == 0x80)
       |                        ^
/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:141:16: warning: cast 
from 'const struct in6_addr *' to 'unsigned int *' drops const qualifier 
[-Wcast-qual]
   141 |     } else if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
       |                ^
/usr/include/netinet/in.h:129:24: note: expanded from macro 
'IN6_IS_ADDR_V4MAPPED'
   129 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 
0 && \
       |                        ^
/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:149:16: warning: cast 
from 'const struct in6_addr *' to 'unsigned int *' drops const qualifier 
[-Wcast-qual]
   149 |     } else if (IN6_IS_ADDR_V4COMPAT(&addr6->sin6_addr)) {
       |                ^
/usr/include/netinet/in.h:134:24: note: expanded from macro 
'IN6_IS_ADDR_V4COMPAT'
   134 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 
0 && \
       |                        ^
/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:151:16: warning: cast 
from 'const struct in6_addr *' to 'unsigned char *' drops const 
qualifier [-Wcast-qual]
   151 |     } else if (IN6_IS_ADDR_SITELOCAL(&addr6->sin6_addr)) {
       |                ^
/usr/include/netinet/in.h:126:24: note: expanded from macro 
'IN6_IS_ADDR_SITELOCAL'
   126 |         ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) 
(a))[1] & 0xc0) == 0xc0)
       |                        ^
/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:189:16: warning: cast 
from 'const struct in6_addr *' to 'unsigned int *' drops const qualifier 
[-Wcast-qual]
   189 |     } else if (IN6_IS_ADDR_V4COMPAT(&addr6->sin6_addr) ||
       |                ^
/usr/include/netinet/in.h:134:24: note: expanded from macro 
'IN6_IS_ADDR_V4COMPAT'
   134 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 
0 && \
       |                        ^

Full build output: 
https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865

I've attached a patch that will silence this warning by always casting 
to the comparison to const, but otherwise not impact the behavior.

-Brad

[-- Attachment #2: musl-netinet-warning-fix.patch --]
[-- Type: text/plain, Size: 3868 bytes --]

diff --git a/include/netinet/in.h b/include/netinet/in.h
index fb628b61..f04657f3 100644
--- a/include/netinet/in.h
+++ b/include/netinet/in.h
@@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
 #define IPPROTO_MAX      263
 
 #define IN6_IS_ADDR_UNSPECIFIED(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
+        (((const uint32_t *) (a))[0] == 0 && \
+         ((const uint32_t *) (a))[1] == 0 && \
+         ((const uint32_t *) (a))[2] == 0 && \
+         ((const uint32_t *) (a))[3] == 0)
 
 #define IN6_IS_ADDR_LOOPBACK(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && \
-         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
-         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
+        (((const uint32_t *) (a))[0] == 0 && \
+         ((const uint32_t *) (a))[1] == 0 && \
+         ((const uint32_t *) (a))[2] == 0 && \
+         ((const uint8_t *) (a))[12] == 0 && \
+         ((const uint8_t *) (a))[13] == 0 && \
+         ((const uint8_t *) (a))[14] == 0 && \
+         ((const uint8_t *) (a))[15] == 1 )
 
-#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
+#define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)
 
 #define IN6_IS_ADDR_LINKLOCAL(a) \
-        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
+        ((((const uint8_t *) (a))[0]) == 0xfe && \
+         (((const uint8_t *) (a))[1] & 0xc0) == 0x80)
 
 #define IN6_IS_ADDR_SITELOCAL(a) \
-        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
+        ((((const uint8_t *) (a))[0]) == 0xfe && \
+         (((const uint8_t *) (a))[1] & 0xc0) == 0xc0)
 
 #define IN6_IS_ADDR_V4MAPPED(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
-         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
+        (((const uint32_t *) (a))[0] == 0 && \
+         ((const uint32_t *) (a))[1] == 0 && \
+         ((const uint8_t *) (a))[8] == 0 && \
+         ((const uint8_t *) (a))[9] == 0 && \
+         ((const uint8_t *) (a))[10] == 0xff && \
+         ((const uint8_t *) (a))[11] == 0xff)
 
 #define IN6_IS_ADDR_V4COMPAT(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
+        (((const uint32_t *) (a))[0] == 0 && \
+         ((const uint32_t *) (a))[1] == 0 && \
+         ((const uint32_t *) (a))[2] == 0 && \
+         ((const uint8_t *) (a))[15] > 1)
 
 #define IN6_IS_ADDR_MC_NODELOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((const uint8_t *) (a))[1] & 0xf) == 0x1))
 
 #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((const uint8_t *) (a))[1] & 0xf) == 0x2))
 
 #define IN6_IS_ADDR_MC_SITELOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((const uint8_t *) (a))[1] & 0xf) == 0x5))
 
 #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((const uint8_t *) (a))[1] & 0xf) == 0x8))
 
 #define IN6_IS_ADDR_MC_GLOBAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((const uint8_t *) (a))[1] & 0xf) == 0xe))
 
 #define __ARE_4_EQUAL(a,b) \
 	(!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-02 21:27 [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual Brad House
@ 2024-08-02 23:38 ` Rich Felker
  2024-08-03  0:02   ` Brad House
  0 siblings, 1 reply; 8+ messages in thread
From: Rich Felker @ 2024-08-02 23:38 UTC (permalink / raw)
  To: Brad House; +Cc: musl

On Fri, Aug 02, 2024 at 05:27:26PM -0400, Brad House wrote:
> I'm the maintainer of c-ares (https://c-ares.org) and have been
> scanning the CI build logs for various systems to catch warnings,
> and on Alpine Linux (which obviously uses musl c) we get these
> warnings, specifically when using clang (but not oddly not on gcc):
> 
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast
> from 'const struct in6_addr *' to 'unsigned char *' drops const
> qualifier [-Wcast-qual]
>    93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
>       |         ^
> /usr/include/netinet/in.h:120:48: note: expanded from macro
> 'IN6_IS_ADDR_MULTICAST'
>   120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
>       |                                                ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:95:16: warning: cast
> from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>    95 |     } else if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr) ||
>       |                ^
> /usr/include/netinet/in.h:115:24: note: expanded from macro
> 'IN6_IS_ADDR_LOOPBACK'
>   115 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:96:16: warning: cast
> from 'const struct in6_addr *' to 'unsigned char *' drops const
> qualifier [-Wcast-qual]
>    96 | IN6_IS_ADDR_LINKLOCAL(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:123:24: note: expanded from macro
> 'IN6_IS_ADDR_LINKLOCAL'
>   123 |         ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *)
> (a))[1] & 0xc0) == 0x80)
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:141:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>   141 |     } else if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:129:24: note: expanded from macro
> 'IN6_IS_ADDR_V4MAPPED'
>   129 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:149:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>   149 |     } else if (IN6_IS_ADDR_V4COMPAT(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:134:24: note: expanded from macro
> 'IN6_IS_ADDR_V4COMPAT'
>   134 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:151:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned char *' drops const
> qualifier [-Wcast-qual]
>   151 |     } else if (IN6_IS_ADDR_SITELOCAL(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:126:24: note: expanded from macro
> 'IN6_IS_ADDR_SITELOCAL'
>   126 |         ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *)
> (a))[1] & 0xc0) == 0xc0)
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:189:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>   189 |     } else if (IN6_IS_ADDR_V4COMPAT(&addr6->sin6_addr) ||
>       |                ^
> /usr/include/netinet/in.h:134:24: note: expanded from macro
> 'IN6_IS_ADDR_V4COMPAT'
>   134 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> 
> Full build output: https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865
> 
> I've attached a patch that will silence this warning by always
> casting to the comparison to const, but otherwise not impact the
> behavior.
> 
> -Brad

> diff --git a/include/netinet/in.h b/include/netinet/in.h
> index fb628b61..f04657f3 100644
> --- a/include/netinet/in.h
> +++ b/include/netinet/in.h
> @@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
>  #define IPPROTO_MAX      263
>  
>  #define IN6_IS_ADDR_UNSPECIFIED(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint32_t *) (a))[2] == 0 && \
> +         ((const uint32_t *) (a))[3] == 0)
>  
>  #define IN6_IS_ADDR_LOOPBACK(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && \
> -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
> -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint32_t *) (a))[2] == 0 && \
> +         ((const uint8_t *) (a))[12] == 0 && \
> +         ((const uint8_t *) (a))[13] == 0 && \
> +         ((const uint8_t *) (a))[14] == 0 && \
> +         ((const uint8_t *) (a))[15] == 1 )
>  
> -#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
> +#define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)
>  
>  #define IN6_IS_ADDR_LINKLOCAL(a) \
> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
> +        ((((const uint8_t *) (a))[0]) == 0xfe && \
> +         (((const uint8_t *) (a))[1] & 0xc0) == 0x80)
>  
>  #define IN6_IS_ADDR_SITELOCAL(a) \
> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
> +        ((((const uint8_t *) (a))[0]) == 0xfe && \
> +         (((const uint8_t *) (a))[1] & 0xc0) == 0xc0)
>  
>  #define IN6_IS_ADDR_V4MAPPED(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
> -         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint8_t *) (a))[8] == 0 && \
> +         ((const uint8_t *) (a))[9] == 0 && \
> +         ((const uint8_t *) (a))[10] == 0xff && \
> +         ((const uint8_t *) (a))[11] == 0xff)
>  
>  #define IN6_IS_ADDR_V4COMPAT(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint32_t *) (a))[2] == 0 && \
> +         ((const uint8_t *) (a))[15] > 1)
>  
>  #define IN6_IS_ADDR_MC_NODELOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((const uint8_t *) (a))[1] & 0xf) == 0x1))
>  
>  #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((const uint8_t *) (a))[1] & 0xf) == 0x2))
>  
>  #define IN6_IS_ADDR_MC_SITELOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((const uint8_t *) (a))[1] & 0xf) == 0x5))
>  
>  #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((const uint8_t *) (a))[1] & 0xf) == 0x8))
>  
>  #define IN6_IS_ADDR_MC_GLOBAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((const uint8_t *) (a))[1] & 0xf) == 0xe))
>  
>  #define __ARE_4_EQUAL(a,b) \
>  	(!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))

It looks like there's a lot wrong with these macros. They should not
be doing random pointer casts like they are. Per the standard, they
take an argument of type const struct in6_addr *, so they should
almost surely be operating on that type directly. That would make them
actually type-safe (diagnostic if called with wrong argument type).

I guess we should look at whether there's any good reason they were
written the way they were..

Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-02 23:38 ` Rich Felker
@ 2024-08-03  0:02   ` Brad House
  2024-08-14  0:47     ` Brad House
  2024-08-14 21:16     ` Rich Felker
  0 siblings, 2 replies; 8+ messages in thread
From: Brad House @ 2024-08-03  0:02 UTC (permalink / raw)
  To: musl; +Cc: Rich Felker

[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]

On 8/2/24 7:38 PM, Rich Felker wrote:

> On Fri, Aug 02, 2024 at 05:27:26PM -0400, Brad House wrote:
>> I'm the maintainer of c-ares (https://c-ares.org) and have been
>> scanning the CI build logs for various systems to catch warnings,
>> and on Alpine Linux (which obviously uses musl c) we get these
>> warnings, specifically when using clang (but not oddly not on gcc):
>>
>> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast
>> from 'const struct in6_addr *' to 'unsigned char *' drops const
>> qualifier [-Wcast-qual]
>>     93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
>>        |         ^
>> /usr/include/netinet/in.h:120:48: note: expanded from macro
>> 'IN6_IS_ADDR_MULTICAST'
>>    120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
>>        |                                                ^
>> ...       ^
>>
>> Full build output: https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865
>>
>> I've attached a patch that will silence this warning by always
>> casting to the comparison to const, but otherwise not impact the
>> behavior.
>>
>> -Brad
>> diff --git a/include/netinet/in.h b/include/netinet/in.h
>> index fb628b61..f04657f3 100644
>> --- a/include/netinet/in.h
>> +++ b/include/netinet/in.h
>> @@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
>>   #define IPPROTO_MAX      263
>>   
>> ...
>>   #define IN6_IS_ADDR_LOOPBACK(a) \
>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>> -         ((uint32_t *) (a))[2] == 0 && \
>> -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
>> -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
>> +        (((const uint32_t *) (a))[0] == 0 && \
>> +         ((const uint32_t *) (a))[1] == 0 && \
>> +         ((const uint32_t *) (a))[2] == 0 && \
>> +         ((const uint8_t *) (a))[12] == 0 && \
>> +         ((const uint8_t *) (a))[13] == 0 && \
>> +         ((const uint8_t *) (a))[14] == 0 && \
>> +         ((const uint8_t *) (a))[15] == 1 )
>> ...
> It looks like there's a lot wrong with these macros. They should not
> be doing random pointer casts like they are. Per the standard, they
> take an argument of type const struct in6_addr *, so they should
> almost surely be operating on that type directly. That would make them
> actually type-safe (diagnostic if called with wrong argument type).
>
> I guess we should look at whether there's any good reason they were
> written the way they were..
>
> Rich

Yep, I see what you mean.  There are already accessors for 8, 16, and 
32bit into struct in6_addr so its odd not to use those.  I've attached a 
v2 patch that uses those instead which also cleans up the warnings.

-Brad

[-- Attachment #2: musl-netinet-warning-fix-v2.patch --]
[-- Type: text/plain, Size: 3845 bytes --]

diff --git a/include/netinet/in.h b/include/netinet/in.h
index fb628b61..c6afeed8 100644
--- a/include/netinet/in.h
+++ b/include/netinet/in.h
@@ -108,51 +108,68 @@ uint16_t ntohs(uint16_t);
 #define IPPROTO_MAX      263
 
 #define IN6_IS_ADDR_UNSPECIFIED(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr32)[2] == 0 && \
+         ((a)->s6_addr32)[3] == 0)
 
 #define IN6_IS_ADDR_LOOPBACK(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && \
-         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
-         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr32)[2] == 0 && \
+         ((a)->s6_addr)[12] == 0 && \
+         ((a)->s6_addr)[13] == 0 && \
+         ((a)->s6_addr)[14] == 0 && \
+         ((a)->s6_addr)[15] == 1 )
 
-#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
+#define IN6_IS_ADDR_MULTICAST(a) (((a)->s6_addr)[0] == 0xff)
 
 #define IN6_IS_ADDR_LINKLOCAL(a) \
-        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
+        ((((a)->s6_addr)[0]) == 0xfe && \
+         (((a)->s6_addr)[1] & 0xc0) == 0x80)
 
 #define IN6_IS_ADDR_SITELOCAL(a) \
-        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
+        ((((a)->s6_addr)[0]) == 0xfe && \
+         (((a)->s6_addr)[1] & 0xc0) == 0xc0)
 
 #define IN6_IS_ADDR_V4MAPPED(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
-         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr)[8] == 0 && \
+         ((a)->s6_addr)[9] == 0 && \
+         ((a)->s6_addr)[10] == 0xff && \
+         ((a)->s6_addr)[11] == 0xff)
 
 #define IN6_IS_ADDR_V4COMPAT(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr32)[2] == 0 && \
+         ((a)->s6_addr)[15] > 1)
 
 #define IN6_IS_ADDR_MC_NODELOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x1))
 
 #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x2))
 
 #define IN6_IS_ADDR_MC_SITELOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x5))
 
 #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x8))
 
 #define IN6_IS_ADDR_MC_GLOBAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0xe))
 
 #define __ARE_4_EQUAL(a,b) \
 	(!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))
 #define IN6_ARE_ADDR_EQUAL(a,b) \
-	__ARE_4_EQUAL((const uint32_t *)(a), (const uint32_t *)(b))
+	__ARE_4_EQUAL((a)->s6_addr32, (b)->s6_addr32)
 
 #define	IN_CLASSA(a)		((((in_addr_t)(a)) & 0x80000000) == 0)
 #define	IN_CLASSA_NET		0xff000000

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-03  0:02   ` Brad House
@ 2024-08-14  0:47     ` Brad House
  2024-08-14 21:16     ` Rich Felker
  1 sibling, 0 replies; 8+ messages in thread
From: Brad House @ 2024-08-14  0:47 UTC (permalink / raw)
  To: musl

On 8/2/24 8:02 PM, Brad House wrote:

>
> Yep, I see what you mean.  There are already accessors for 8, 16, and 
> 32bit into struct in6_addr so its odd not to use those.  I've attached 
> a v2 patch that uses those instead which also cleans up the warnings.
>
> -Brad

Any feedback?

-Brad


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-03  0:02   ` Brad House
  2024-08-14  0:47     ` Brad House
@ 2024-08-14 21:16     ` Rich Felker
  2024-08-14 21:24       ` enh
  1 sibling, 1 reply; 8+ messages in thread
From: Rich Felker @ 2024-08-14 21:16 UTC (permalink / raw)
  To: Brad House; +Cc: musl

On Fri, Aug 02, 2024 at 08:02:16PM -0400, Brad House wrote:
> On 8/2/24 7:38 PM, Rich Felker wrote:
> 
> >On Fri, Aug 02, 2024 at 05:27:26PM -0400, Brad House wrote:
> >>I'm the maintainer of c-ares (https://c-ares.org) and have been
> >>scanning the CI build logs for various systems to catch warnings,
> >>and on Alpine Linux (which obviously uses musl c) we get these
> >>warnings, specifically when using clang (but not oddly not on gcc):
> >>
> >>/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast
> >>from 'const struct in6_addr *' to 'unsigned char *' drops const
> >>qualifier [-Wcast-qual]
> >>    93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
> >>       |         ^
> >>/usr/include/netinet/in.h:120:48: note: expanded from macro
> >>'IN6_IS_ADDR_MULTICAST'
> >>   120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
> >>       |                                                ^
> >>...       ^
> >>
> >>Full build output: https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865
> >>
> >>I've attached a patch that will silence this warning by always
> >>casting to the comparison to const, but otherwise not impact the
> >>behavior.
> >>
> >>-Brad
> >>diff --git a/include/netinet/in.h b/include/netinet/in.h
> >>index fb628b61..f04657f3 100644
> >>--- a/include/netinet/in.h
> >>+++ b/include/netinet/in.h
> >>@@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
> >>  #define IPPROTO_MAX      263
> >>...
> >>  #define IN6_IS_ADDR_LOOPBACK(a) \
> >>-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> >>-         ((uint32_t *) (a))[2] == 0 && \
> >>-         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
> >>-         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
> >>+        (((const uint32_t *) (a))[0] == 0 && \
> >>+         ((const uint32_t *) (a))[1] == 0 && \
> >>+         ((const uint32_t *) (a))[2] == 0 && \
> >>+         ((const uint8_t *) (a))[12] == 0 && \
> >>+         ((const uint8_t *) (a))[13] == 0 && \
> >>+         ((const uint8_t *) (a))[14] == 0 && \
> >>+         ((const uint8_t *) (a))[15] == 1 )
> >>...
> >It looks like there's a lot wrong with these macros. They should not
> >be doing random pointer casts like they are. Per the standard, they
> >take an argument of type const struct in6_addr *, so they should
> >almost surely be operating on that type directly. That would make them
> >actually type-safe (diagnostic if called with wrong argument type).
> >
> >I guess we should look at whether there's any good reason they were
> >written the way they were..
> >
> >Rich
> 
> Yep, I see what you mean.  There are already accessors for 8, 16,
> and 32bit into struct in6_addr so its odd not to use those.  I've
> attached a v2 patch that uses those instead which also cleans up the
> warnings.
> 
> -Brad

> diff --git a/include/netinet/in.h b/include/netinet/in.h
> index fb628b61..c6afeed8 100644
> --- a/include/netinet/in.h
> +++ b/include/netinet/in.h
> @@ -108,51 +108,68 @@ uint16_t ntohs(uint16_t);
>  #define IPPROTO_MAX      263
>  
>  #define IN6_IS_ADDR_UNSPECIFIED(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
> +        (((a)->s6_addr32)[0] == 0 && \
> +         ((a)->s6_addr32)[1] == 0 && \
> +         ((a)->s6_addr32)[2] == 0 && \
> +         ((a)->s6_addr32)[3] == 0)
>  
>  #define IN6_IS_ADDR_LOOPBACK(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && \
> -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
> -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
> +        (((a)->s6_addr32)[0] == 0 && \
> +         ((a)->s6_addr32)[1] == 0 && \
> +         ((a)->s6_addr32)[2] == 0 && \
> +         ((a)->s6_addr)[12] == 0 && \
> +         ((a)->s6_addr)[13] == 0 && \
> +         ((a)->s6_addr)[14] == 0 && \
> +         ((a)->s6_addr)[15] == 1 )
>  
> -#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
> +#define IN6_IS_ADDR_MULTICAST(a) (((a)->s6_addr)[0] == 0xff)
>  
>  #define IN6_IS_ADDR_LINKLOCAL(a) \
> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
> +        ((((a)->s6_addr)[0]) == 0xfe && \
> +         (((a)->s6_addr)[1] & 0xc0) == 0x80)
>  
>  #define IN6_IS_ADDR_SITELOCAL(a) \
> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
> +        ((((a)->s6_addr)[0]) == 0xfe && \
> +         (((a)->s6_addr)[1] & 0xc0) == 0xc0)
>  
>  #define IN6_IS_ADDR_V4MAPPED(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
> -         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
> +        (((a)->s6_addr32)[0] == 0 && \
> +         ((a)->s6_addr32)[1] == 0 && \
> +         ((a)->s6_addr)[8] == 0 && \
> +         ((a)->s6_addr)[9] == 0 && \
> +         ((a)->s6_addr)[10] == 0xff && \
> +         ((a)->s6_addr)[11] == 0xff)
>  
>  #define IN6_IS_ADDR_V4COMPAT(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
> +        (((a)->s6_addr32)[0] == 0 && \
> +         ((a)->s6_addr32)[1] == 0 && \
> +         ((a)->s6_addr32)[2] == 0 && \
> +         ((a)->s6_addr)[15] > 1)
>  
>  #define IN6_IS_ADDR_MC_NODELOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((a)->s6_addr)[1] & 0xf) == 0x1))
>  
>  #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((a)->s6_addr)[1] & 0xf) == 0x2))
>  
>  #define IN6_IS_ADDR_MC_SITELOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((a)->s6_addr)[1] & 0xf) == 0x5))
>  
>  #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((a)->s6_addr)[1] & 0xf) == 0x8))
>  
>  #define IN6_IS_ADDR_MC_GLOBAL(a) \
> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
> +        (IN6_IS_ADDR_MULTICAST(a) && \
> +         ((((a)->s6_addr)[1] & 0xf) == 0xe))
>  
>  #define __ARE_4_EQUAL(a,b) \
>  	(!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))
>  #define IN6_ARE_ADDR_EQUAL(a,b) \
> -	__ARE_4_EQUAL((const uint32_t *)(a), (const uint32_t *)(b))
> +	__ARE_4_EQUAL((a)->s6_addr32, (b)->s6_addr32)
>  
>  #define	IN_CLASSA(a)		((((in_addr_t)(a)) & 0x80000000) == 0)
>  #define	IN_CLASSA_NET		0xff000000

I think this looks fine. Anyone willing to point a second set of eyes
at it (or maybe write a test to check whether codegen is same before
and after) and make sure before I merge it?

Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-14 21:16     ` Rich Felker
@ 2024-08-14 21:24       ` enh
  2024-08-19 15:36         ` Brad House
  2024-11-07 10:33         ` Brad House
  0 siblings, 2 replies; 8+ messages in thread
From: enh @ 2024-08-14 21:24 UTC (permalink / raw)
  To: musl; +Cc: Brad House

not quite the question you asked, but the new implementation is what
bionic has shipped since 2016, and had the historical castful
implementation before that. (citation needed?
https://android-review.googlesource.com/c/platform/bionic/+/250098)

On Wed, Aug 14, 2024 at 5:16 PM Rich Felker <dalias@libc.org> wrote:
>
> On Fri, Aug 02, 2024 at 08:02:16PM -0400, Brad House wrote:
> > On 8/2/24 7:38 PM, Rich Felker wrote:
> >
> > >On Fri, Aug 02, 2024 at 05:27:26PM -0400, Brad House wrote:
> > >>I'm the maintainer of c-ares (https://c-ares.org) and have been
> > >>scanning the CI build logs for various systems to catch warnings,
> > >>and on Alpine Linux (which obviously uses musl c) we get these
> > >>warnings, specifically when using clang (but not oddly not on gcc):
> > >>
> > >>/__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast
> > >>from 'const struct in6_addr *' to 'unsigned char *' drops const
> > >>qualifier [-Wcast-qual]
> > >>    93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
> > >>       |         ^
> > >>/usr/include/netinet/in.h:120:48: note: expanded from macro
> > >>'IN6_IS_ADDR_MULTICAST'
> > >>   120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
> > >>       |                                                ^
> > >>...       ^
> > >>
> > >>Full build output: https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865
> > >>
> > >>I've attached a patch that will silence this warning by always
> > >>casting to the comparison to const, but otherwise not impact the
> > >>behavior.
> > >>
> > >>-Brad
> > >>diff --git a/include/netinet/in.h b/include/netinet/in.h
> > >>index fb628b61..f04657f3 100644
> > >>--- a/include/netinet/in.h
> > >>+++ b/include/netinet/in.h
> > >>@@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
> > >>  #define IPPROTO_MAX      263
> > >>...
> > >>  #define IN6_IS_ADDR_LOOPBACK(a) \
> > >>-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> > >>-         ((uint32_t *) (a))[2] == 0 && \
> > >>-         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
> > >>-         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
> > >>+        (((const uint32_t *) (a))[0] == 0 && \
> > >>+         ((const uint32_t *) (a))[1] == 0 && \
> > >>+         ((const uint32_t *) (a))[2] == 0 && \
> > >>+         ((const uint8_t *) (a))[12] == 0 && \
> > >>+         ((const uint8_t *) (a))[13] == 0 && \
> > >>+         ((const uint8_t *) (a))[14] == 0 && \
> > >>+         ((const uint8_t *) (a))[15] == 1 )
> > >>...
> > >It looks like there's a lot wrong with these macros. They should not
> > >be doing random pointer casts like they are. Per the standard, they
> > >take an argument of type const struct in6_addr *, so they should
> > >almost surely be operating on that type directly. That would make them
> > >actually type-safe (diagnostic if called with wrong argument type).
> > >
> > >I guess we should look at whether there's any good reason they were
> > >written the way they were..
> > >
> > >Rich
> >
> > Yep, I see what you mean.  There are already accessors for 8, 16,
> > and 32bit into struct in6_addr so its odd not to use those.  I've
> > attached a v2 patch that uses those instead which also cleans up the
> > warnings.
> >
> > -Brad
>
> > diff --git a/include/netinet/in.h b/include/netinet/in.h
> > index fb628b61..c6afeed8 100644
> > --- a/include/netinet/in.h
> > +++ b/include/netinet/in.h
> > @@ -108,51 +108,68 @@ uint16_t ntohs(uint16_t);
> >  #define IPPROTO_MAX      263
> >
> >  #define IN6_IS_ADDR_UNSPECIFIED(a) \
> > -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> > -         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
> > +        (((a)->s6_addr32)[0] == 0 && \
> > +         ((a)->s6_addr32)[1] == 0 && \
> > +         ((a)->s6_addr32)[2] == 0 && \
> > +         ((a)->s6_addr32)[3] == 0)
> >
> >  #define IN6_IS_ADDR_LOOPBACK(a) \
> > -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> > -         ((uint32_t *) (a))[2] == 0 && \
> > -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
> > -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
> > +        (((a)->s6_addr32)[0] == 0 && \
> > +         ((a)->s6_addr32)[1] == 0 && \
> > +         ((a)->s6_addr32)[2] == 0 && \
> > +         ((a)->s6_addr)[12] == 0 && \
> > +         ((a)->s6_addr)[13] == 0 && \
> > +         ((a)->s6_addr)[14] == 0 && \
> > +         ((a)->s6_addr)[15] == 1 )
> >
> > -#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
> > +#define IN6_IS_ADDR_MULTICAST(a) (((a)->s6_addr)[0] == 0xff)
> >
> >  #define IN6_IS_ADDR_LINKLOCAL(a) \
> > -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
> > +        ((((a)->s6_addr)[0]) == 0xfe && \
> > +         (((a)->s6_addr)[1] & 0xc0) == 0x80)
> >
> >  #define IN6_IS_ADDR_SITELOCAL(a) \
> > -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
> > +        ((((a)->s6_addr)[0]) == 0xfe && \
> > +         (((a)->s6_addr)[1] & 0xc0) == 0xc0)
> >
> >  #define IN6_IS_ADDR_V4MAPPED(a) \
> > -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> > -         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
> > -         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
> > +        (((a)->s6_addr32)[0] == 0 && \
> > +         ((a)->s6_addr32)[1] == 0 && \
> > +         ((a)->s6_addr)[8] == 0 && \
> > +         ((a)->s6_addr)[9] == 0 && \
> > +         ((a)->s6_addr)[10] == 0xff && \
> > +         ((a)->s6_addr)[11] == 0xff)
> >
> >  #define IN6_IS_ADDR_V4COMPAT(a) \
> > -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> > -         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
> > +        (((a)->s6_addr32)[0] == 0 && \
> > +         ((a)->s6_addr32)[1] == 0 && \
> > +         ((a)->s6_addr32)[2] == 0 && \
> > +         ((a)->s6_addr)[15] > 1)
> >
> >  #define IN6_IS_ADDR_MC_NODELOCAL(a) \
> > -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
> > +        (IN6_IS_ADDR_MULTICAST(a) && \
> > +         ((((a)->s6_addr)[1] & 0xf) == 0x1))
> >
> >  #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
> > -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
> > +        (IN6_IS_ADDR_MULTICAST(a) && \
> > +         ((((a)->s6_addr)[1] & 0xf) == 0x2))
> >
> >  #define IN6_IS_ADDR_MC_SITELOCAL(a) \
> > -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
> > +        (IN6_IS_ADDR_MULTICAST(a) && \
> > +         ((((a)->s6_addr)[1] & 0xf) == 0x5))
> >
> >  #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
> > -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
> > +        (IN6_IS_ADDR_MULTICAST(a) && \
> > +         ((((a)->s6_addr)[1] & 0xf) == 0x8))
> >
> >  #define IN6_IS_ADDR_MC_GLOBAL(a) \
> > -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
> > +        (IN6_IS_ADDR_MULTICAST(a) && \
> > +         ((((a)->s6_addr)[1] & 0xf) == 0xe))
> >
> >  #define __ARE_4_EQUAL(a,b) \
> >       (!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))
> >  #define IN6_ARE_ADDR_EQUAL(a,b) \
> > -     __ARE_4_EQUAL((const uint32_t *)(a), (const uint32_t *)(b))
> > +     __ARE_4_EQUAL((a)->s6_addr32, (b)->s6_addr32)
> >
> >  #define      IN_CLASSA(a)            ((((in_addr_t)(a)) & 0x80000000) == 0)
> >  #define      IN_CLASSA_NET           0xff000000
>
> I think this looks fine. Anyone willing to point a second set of eyes
> at it (or maybe write a test to check whether codegen is same before
> and after) and make sure before I merge it?
>
> Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-14 21:24       ` enh
@ 2024-08-19 15:36         ` Brad House
  2024-11-07 10:33         ` Brad House
  1 sibling, 0 replies; 8+ messages in thread
From: Brad House @ 2024-08-19 15:36 UTC (permalink / raw)
  To: musl

On 8/14/24 5:24 PM, enh wrote:

> not quite the question you asked, but the new implementation is what
> bionic has shipped since 2016, and had the historical castful
> implementation before that. (citation needed?
> https://android-review.googlesource.com/c/platform/bionic/+/250098)
>
> On Wed, Aug 14, 2024 at 5:16 PM Rich Felker <dalias@libc.org> wrote:
>> I think this looks fine. Anyone willing to point a second set of eyes
>> at it (or maybe write a test to check whether codegen is same before
>> and after) and make sure before I merge it?
>>
>> Rich

@enh, great, I'm guessing Android hit the same types of warnings at some 
point :)

-Brad


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual
  2024-08-14 21:24       ` enh
  2024-08-19 15:36         ` Brad House
@ 2024-11-07 10:33         ` Brad House
  1 sibling, 0 replies; 8+ messages in thread
From: Brad House @ 2024-11-07 10:33 UTC (permalink / raw)
  To: musl

Any update on maybe merging the updated patch from this chain?

Thanks.

-Brad

On 8/14/24 5:24 PM, enh wrote:
> not quite the question you asked, but the new implementation is what
> bionic has shipped since 2016, and had the historical castful
> implementation before that. (citation needed?
> https://android-review.googlesource.com/c/platform/bionic/+/250098)
>
> On Wed, Aug 14, 2024 at 5:16 PM Rich Felker <dalias@libc.org> wrote:
>> On Fri, Aug 02, 2024 at 08:02:16PM -0400, Brad House wrote:
>>> On 8/2/24 7:38 PM, Rich Felker wrote:
>>>
>>>> On Fri, Aug 02, 2024 at 05:27:26PM -0400, Brad House wrote:
>>>>> I'm the maintainer of c-ares (https://c-ares.org) and have been
>>>>> scanning the CI build logs for various systems to catch warnings,
>>>>> and on Alpine Linux (which obviously uses musl c) we get these
>>>>> warnings, specifically when using clang (but not oddly not on gcc):
>>>>>
>>>>> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast
>>>> >from 'const struct in6_addr *' to 'unsigned char *' drops const
>>>>> qualifier [-Wcast-qual]
>>>>>     93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
>>>>>        |         ^
>>>>> /usr/include/netinet/in.h:120:48: note: expanded from macro
>>>>> 'IN6_IS_ADDR_MULTICAST'
>>>>>    120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
>>>>>        |                                                ^
>>>>> ...       ^
>>>>>
>>>>> Full build output: https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865
>>>>>
>>>>> I've attached a patch that will silence this warning by always
>>>>> casting to the comparison to const, but otherwise not impact the
>>>>> behavior.
>>>>>
>>>>> -Brad
>>>>> diff --git a/include/netinet/in.h b/include/netinet/in.h
>>>>> index fb628b61..f04657f3 100644
>>>>> --- a/include/netinet/in.h
>>>>> +++ b/include/netinet/in.h
>>>>> @@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
>>>>>   #define IPPROTO_MAX      263
>>>>> ...
>>>>>   #define IN6_IS_ADDR_LOOPBACK(a) \
>>>>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>>>>> -         ((uint32_t *) (a))[2] == 0 && \
>>>>> -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
>>>>> -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
>>>>> +        (((const uint32_t *) (a))[0] == 0 && \
>>>>> +         ((const uint32_t *) (a))[1] == 0 && \
>>>>> +         ((const uint32_t *) (a))[2] == 0 && \
>>>>> +         ((const uint8_t *) (a))[12] == 0 && \
>>>>> +         ((const uint8_t *) (a))[13] == 0 && \
>>>>> +         ((const uint8_t *) (a))[14] == 0 && \
>>>>> +         ((const uint8_t *) (a))[15] == 1 )
>>>>> ...
>>>> It looks like there's a lot wrong with these macros. They should not
>>>> be doing random pointer casts like they are. Per the standard, they
>>>> take an argument of type const struct in6_addr *, so they should
>>>> almost surely be operating on that type directly. That would make them
>>>> actually type-safe (diagnostic if called with wrong argument type).
>>>>
>>>> I guess we should look at whether there's any good reason they were
>>>> written the way they were..
>>>>
>>>> Rich
>>> Yep, I see what you mean.  There are already accessors for 8, 16,
>>> and 32bit into struct in6_addr so its odd not to use those.  I've
>>> attached a v2 patch that uses those instead which also cleans up the
>>> warnings.
>>>
>>> -Brad
>>> diff --git a/include/netinet/in.h b/include/netinet/in.h
>>> index fb628b61..c6afeed8 100644
>>> --- a/include/netinet/in.h
>>> +++ b/include/netinet/in.h
>>> @@ -108,51 +108,68 @@ uint16_t ntohs(uint16_t);
>>>   #define IPPROTO_MAX      263
>>>
>>>   #define IN6_IS_ADDR_UNSPECIFIED(a) \
>>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>>> -         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
>>> +        (((a)->s6_addr32)[0] == 0 && \
>>> +         ((a)->s6_addr32)[1] == 0 && \
>>> +         ((a)->s6_addr32)[2] == 0 && \
>>> +         ((a)->s6_addr32)[3] == 0)
>>>
>>>   #define IN6_IS_ADDR_LOOPBACK(a) \
>>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>>> -         ((uint32_t *) (a))[2] == 0 && \
>>> -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
>>> -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
>>> +        (((a)->s6_addr32)[0] == 0 && \
>>> +         ((a)->s6_addr32)[1] == 0 && \
>>> +         ((a)->s6_addr32)[2] == 0 && \
>>> +         ((a)->s6_addr)[12] == 0 && \
>>> +         ((a)->s6_addr)[13] == 0 && \
>>> +         ((a)->s6_addr)[14] == 0 && \
>>> +         ((a)->s6_addr)[15] == 1 )
>>>
>>> -#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
>>> +#define IN6_IS_ADDR_MULTICAST(a) (((a)->s6_addr)[0] == 0xff)
>>>
>>>   #define IN6_IS_ADDR_LINKLOCAL(a) \
>>> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
>>> +        ((((a)->s6_addr)[0]) == 0xfe && \
>>> +         (((a)->s6_addr)[1] & 0xc0) == 0x80)
>>>
>>>   #define IN6_IS_ADDR_SITELOCAL(a) \
>>> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
>>> +        ((((a)->s6_addr)[0]) == 0xfe && \
>>> +         (((a)->s6_addr)[1] & 0xc0) == 0xc0)
>>>
>>>   #define IN6_IS_ADDR_V4MAPPED(a) \
>>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>>> -         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
>>> -         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
>>> +        (((a)->s6_addr32)[0] == 0 && \
>>> +         ((a)->s6_addr32)[1] == 0 && \
>>> +         ((a)->s6_addr)[8] == 0 && \
>>> +         ((a)->s6_addr)[9] == 0 && \
>>> +         ((a)->s6_addr)[10] == 0xff && \
>>> +         ((a)->s6_addr)[11] == 0xff)
>>>
>>>   #define IN6_IS_ADDR_V4COMPAT(a) \
>>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>>> -         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
>>> +        (((a)->s6_addr32)[0] == 0 && \
>>> +         ((a)->s6_addr32)[1] == 0 && \
>>> +         ((a)->s6_addr32)[2] == 0 && \
>>> +         ((a)->s6_addr)[15] > 1)
>>>
>>>   #define IN6_IS_ADDR_MC_NODELOCAL(a) \
>>> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
>>> +        (IN6_IS_ADDR_MULTICAST(a) && \
>>> +         ((((a)->s6_addr)[1] & 0xf) == 0x1))
>>>
>>>   #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
>>> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
>>> +        (IN6_IS_ADDR_MULTICAST(a) && \
>>> +         ((((a)->s6_addr)[1] & 0xf) == 0x2))
>>>
>>>   #define IN6_IS_ADDR_MC_SITELOCAL(a) \
>>> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
>>> +        (IN6_IS_ADDR_MULTICAST(a) && \
>>> +         ((((a)->s6_addr)[1] & 0xf) == 0x5))
>>>
>>>   #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
>>> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
>>> +        (IN6_IS_ADDR_MULTICAST(a) && \
>>> +         ((((a)->s6_addr)[1] & 0xf) == 0x8))
>>>
>>>   #define IN6_IS_ADDR_MC_GLOBAL(a) \
>>> -        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
>>> +        (IN6_IS_ADDR_MULTICAST(a) && \
>>> +         ((((a)->s6_addr)[1] & 0xf) == 0xe))
>>>
>>>   #define __ARE_4_EQUAL(a,b) \
>>>        (!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))
>>>   #define IN6_ARE_ADDR_EQUAL(a,b) \
>>> -     __ARE_4_EQUAL((const uint32_t *)(a), (const uint32_t *)(b))
>>> +     __ARE_4_EQUAL((a)->s6_addr32, (b)->s6_addr32)
>>>
>>>   #define      IN_CLASSA(a)            ((((in_addr_t)(a)) & 0x80000000) == 0)
>>>   #define      IN_CLASSA_NET           0xff000000
>> I think this looks fine. Anyone willing to point a second set of eyes
>> at it (or maybe write a test to check whether codegen is same before
>> and after) and make sure before I merge it?
>>
>> Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-11-07 10:33 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-08-02 21:27 [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual Brad House
2024-08-02 23:38 ` Rich Felker
2024-08-03  0:02   ` Brad House
2024-08-14  0:47     ` Brad House
2024-08-14 21:16     ` Rich Felker
2024-08-14 21:24       ` enh
2024-08-19 15:36         ` Brad House
2024-11-07 10:33         ` Brad House

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).