On 28/09/16 13:08, Steffan Karger wrote: > Fixes compiler warnings (undefined behavior) by making the copy explicit > to comply to strict aliasing rules. With newer GCC the old code could > actually lead to undefined behaviour. > > See e.g. http://blog.regehr.org/archives/959. > > Signed-off-by: Steffan Karger <stef...@karger.me> > --- > src/openvpn/mroute.h | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) >
I've taken another stab on this one, read up [1,2,3] more carefully on strict aliasing , type punning and undefined behaviours in compilers. I do consider fixing this important, despite it might not be strictly needed with today's compilers [4], we don't know if and how this undefined behaviour will change in a future compiler update - despite the risk of doing so is very low in our case. The last time I looked at this, I identified that the assembly being generated by the compiler is pretty much the same and it uses some simple and mov instructions. In addition I also have a better understanding why the code ends up as the same result, as we're just storing an "fresh" integer value to the address extracted via the dereferencing - and not manipulating data extracted from that address and putting something else back, so the compiler's optimizer won't be able to mess this up too badly. When considering that the optimized code will be most optimal regardless if we use type punning and breaking strict alias or we will use memcpy() in the C code, it makes little sense to me to not fix this warning. Today's compilers are truly clever in this aspect when being asked to optimise variable assignments with static lengths. So all in all, I am willing to give this a full ACK now. However, compiling this with gcc and the common CFLAGS on RHEL and Fedora, more issues related to strict aliasing appeared. With clang, there were no more warnings with this patch. I think it would be good to resolve the gcc issues as well. The issues located are found in the attachment. [1] <http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html> [2] <http://blog.regehr.org/archives/213> [3] <http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html> [4] I've tested with gcc-4.8.5 (Red Hat 4.8.5-4) and clang-3.4.2 on EL7.2 -- kind regards, David Sommerseth OpenVPN Technologies, Inc
mroute.c: In function ‘mroute_get_in_addr_t’:
mroute.c:93:7: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
*(in_addr_t*)ma->addr = src;
^
mroute.c: In function ‘mroute_get_in6_addr’:
mroute.c:105:16: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
*(struct in6_addr *)ma->addr = src;
^
mroute.c: In function ‘mroute_addr_mask_host_bits’:
mroute.c:329:3: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
in_addr_t addr = ntohl(*(in_addr_t*)ma->addr);
^
mroute.c:333:7: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
*(in_addr_t*)ma->addr = htonl (addr);
^
mroute.c: In function ‘mroute_addr_print_ex’:
mroute.c:432:8: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
print_in_addr_t( *(in_addr_t*)(&maddr.addr[12]), IA_NET_ORDER, gc));
^
mroute.c:437:34: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
print_in6_addr( *(struct in6_addr*)&maddr.addr, 0, gc));
ps.c: In function ‘port_share_sendmsg’:
ps.c:226:4: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
*((socket_descriptor_t*)CMSG_DATA(h)) = sd_send;
^
ps.c:231:4: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
*((socket_descriptor_t*)CMSG_DATA(h)) = sd_null[0];
^
ps.c: In function ‘control_message_from_parent’:
ps.c:505:3: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
*((socket_descriptor_t*)CMSG_DATA(h)) = SOCKET_UNDEFINED;
^
ps.c:519:4: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
const socket_descriptor_t received_fd =
*((socket_descriptor_t*)CMSG_DATA(h));
^
socket.c: In function ‘setenv_sockaddr’:
socket.c:2601:4: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
ia.s_addr = *(in_addr_t *)&addr->addr.in6.sin6_addr.s6_addr[12] ;
^
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
