[SSSD] [PATCHES] Alignment issues reported by Clang

Simo Sorce simo at redhat.com
Fri Sep 20 00:28:19 UTC 2013


On Thu, 2013-09-19 at 18:22 +0200, Lukas Slebodnik wrote:
> On (19/09/13 09:00), Simo Sorce wrote:
> >On Thu, 2013-09-19 at 09:17 +0200, Sumit Bose wrote:
> >> On Wed, Sep 18, 2013 at 07:07:46PM +0200, Lukas Slebodnik wrote:
> >> > On (12/09/13 16:55), Michal Židek wrote:
> >> > >On 09/12/2013 02:03 PM, Lukas Slebodnik wrote:
> >> > >>On (11/09/13 17:05), Michal Židek wrote:
> >> > >>>Patches 1-4 and 9:
> >> > >>>These patches use the SAFEALIGN macros where it is appropriate. I
> >> > >>>split them into several patches for easier review, so that client
> >> > >>>code changes are not mixed with the rest of the code and changes that
> >> > >>>are a little different than the rest have their own patch.
> >> > >>>
> >> > >>>Patches 5-6:
> >> > >>>Here I think it is not needed to use uint8_t* or char*. We cast the
> >> > >>>pointer anyway and there are no real alignment issues (the memory is
> >> > >>>aligned properly). I was thinking about suppressing the warnings with
> >> > >>>#pragma here, but in this particular situation it is IMO better to
> >> > >>>have just void*.
> >> > >>>
> >> > >>>Patches 7-8:
> >> > >>>These were all false positive warnings. It is possible to suppress
> >> > >>>them with additional casting but I found it less readable so I used
> >> > >>>#pragma.
> >> > >>>
> >> > >>>Patch 10:
> >> > >>>We had improperly aligned part of a buffer used in client code. Also
> >> > >>>some code blocks here must rely on fact that the buffer they get is
> >> > >>>aligned properly (they can do nothing about it if it isn't). In these
> >> > >>>cases it is better to silence the warnings, so that it does not make
> >> > >>>permanent noise during compilation.
> >> > >>>
> >> > >>>Thanks
> >> > >>>Michal
> >> > >>
> >> > >>You forgot to fix two warnings in tests.
> >> > >>
> >> > >>   CC       src/tests/cmocka/dyndns_tests-test_dyndns.o
> >> > >>src/tests/cmocka/test_dyndns.c:130:10: warning: cast from 'struct sockaddr *' to
> >> > >>       'struct sockaddr_in *' increases required alignment from 2 to 4 [-Wcast-align]
> >> > >>         ((struct sockaddr_in *) ifap->ifa_addr)->sin_family = AF_INET;
> >> > >>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> > >>src/tests/cmocka/test_dyndns.c:134:26: warning: cast from 'struct sockaddr *' to
> >> > >>       'struct sockaddr_in *' increases required alignment from 2 to 4 [-Wcast-align]
> >> > >>                       &(((struct sockaddr_in *) ifap->ifa_addr)->sin_addr)) != 1) {
> >> > >>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> > >>2 warnings generated.
> >> > >>
> >> > >>LS
> >> > >
> >> > >Thanks, new patches are attached.
> >> > >
> >> > >Michal
> >> > >
> >> > 
> >> > I tested some compilers and patches work fine with clang and gcc.
> >> > But there are also another compilers in the world :-)
> >> > ant they needn't understand "pragma GCC". This pragma is not standardized.
> >> > 
> >> > 
> >> > src/providers/dp_dyndns.c(145): warning #161: unrecognized #pragma
> >> >   #pragma GCC diagnostic push
> >> >           ^
> >> > 
> >> > src/providers/dp_dyndns.c(146): warning #161: unrecognized #pragma
> >> >   #pragma GCC diagnostic ignored "-Wcast-align"
> >> >           ^
> >> > 
> >> > src/providers/dp_dyndns.c(206): warning #161: unrecognized #pragma
> >> >   #pragma GCC diagnostic pop
> >> >           ^
> >> > 
> >> > If you are 100% sure about alignment, you can define own macro.
> >> > 
> >> > Something like this:
> >> > #define ignore_align(var, dest_type) \
> >> >     (dest_type *)((void*)(var))
> >> > 
> >> > And usage:
> >> >     wrapper->refcount = ignore_align(wrapper->ptr + refcount_offset, int);
> >> > 
> >> > 
> >> > Does anybody have any better ideas?
> >> 
> >> Compiler specific #pragma should not be used. And I think in general
> >> #pragma should be used with great care. Currently there is only one use
> >> case in sssd in the memcache code to align some structs which make
> >> sense.
> >
> >Indeed, agree 100%
> >
> >> Especially I think ingnoring a warning is not a good idea. If currently
> >> no one has a good idea how to handle the sockaddr and in_addr structs in
> >> a way which does not produce a warning I would prefer to keep the
> >> warning around until someone has a good idea. Imo a comment in the code
> >> that the warning can be ignored would be sufficient for the time being.
> >
> >casting sockaddr is a bug, unless you know for sure it was created as
> >sockaddr_un and then simply casted to sockaddr, but in that case I would
> >consider the casting to pass it around just a different bug.
> >
> >Please use sockaddr_un anywhere you need to perform casts. Pass around
> >sockaddr_un and cast it only at the time of use when needed.
> It is not possible to use sockaddr_un(or sockaddr_storage).
> 
> We use function getifaddrs to retrieve addresses of network interfaces.
> 
> Manual page says (man 3 getifaddrs)
> --------------------------------------------------------------------------
> int getifaddrs(struct ifaddrs **ifap);
> 
>        The  getifaddrs()  function  creates a linked list of structures
>        describing the network interfaces of the local system, and stores
>        the address of the first item of the list in *ifap.
>        The list consists of ifaddrs structures, defined as follows:
> 
>            struct ifaddrs {
>                struct ifaddrs  *ifa_next;    /* Next item in list */
>                char            *ifa_name;    /* Name of interface */
>                unsigned int     ifa_flags;   /* Flags from SIOCGIFFLAGS */
>                struct sockaddr *ifa_addr;    /* Address of interface */
>                ^^^^^^^^^^^^^^^
> We cannot change this type
>                struct sockaddr *ifa_netmask; /* Netmask of interface */
>                union {
>                    struct sockaddr *ifu_broadaddr;
>                                     /* Broadcast address of interface */
>                    struct sockaddr *ifu_dstaddr;
>                                     /* Point-to-point destination address */
>                } ifa_ifu;
>            #define              ifa_broadaddr ifa_ifu.ifu_broadaddr
>            #define              ifa_dstaddr   ifa_ifu.ifu_dstaddr
>                void            *ifa_data;    /* Address-specific data */
>            };
> 
> --------------------------------------------------------------------------

In this case you should copy the contents of sockaddr to a new variable
of the right type, and not cast.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list