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.
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.
bye,
Sumit
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
