On 11/11/16 13:14, David Sommerseth wrote: > 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
Just for the record, I'm using the following CFLAGS:
CFLAGS="-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions \
-fstack-protector --param=ssp-buffer-size=4"
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
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
