strict aliasing

[PixelRick]

I don't see "-fno-strict-aliasing" in the linux mk file. Is it somewhere else ?

[seladb]

You're right, there is no reference to strict aliasing in the makefiles. Do you see any issue that requires this flag?

[PixelRick]

I have "apparently" not encountered issues, but since type punning is used everywhere someone might stall on a release-only bug that he has all chances to not understand without looking at the assembly for hours. I have looked at the implementation of different layers and I don't recall places where the raw buffer pointer and the punned pointer are both used for read/writes in the same scope so that would explain the "no bug yet". If such case exists then if the compiler can statically deduce that the char pointer (uint8_t is not always an aliasing type, it depends on the compiler) points to the defined buffer the object type referenced by the the char is known and it is no longer a "may alias" with the struct pointer -> possible reordering or constant propagation. I will personally use the flag as it seems to be the usual practice for networking libs since the only true portable way is a performance killer: memcpy the relevant buffer slice into the trivially copiable target struct. The standard is not always practical, compilers are ! As a good industry example, the Unreal Engine 4 (game engine) violates strict aliasing everywhere and thus use -fno-strict-aliasing on gcc/clang. Losing strict aliasing optimizations is a really small price to pay for the performance gained from direct serialization/reinterpretation.

[seladb]

Thanks for the thorough explanation. I'm definitely not an expert on strict aliasing (you seem far more knowledgeable than me), but from the little I've read it seems to be less of an issue on x86 machines because of the relatively small number of registers. PcapPlusPlus is currently only officially supported on x86, simply because I've never tested it on any other arch (like RISC etc). So I'm not sure how critical it is to fix it.

Having said that, you're more than welcome to add this flag and run some tests to make sure there is no impact on functionality and performance. If this is indeed the case, I'll be more than happy to add it.

[seladb]

@TehRick I'd really appreciate your input, let's try to make a decision here

[PixelRick]

I will check with bindiff for differences and run examples with type sanitizer and report my findings here. VS does not use aliasing analysis in its optimization phase, so using the flag shouldn't bring GCC doing worse than VS. I have checked libtins and they seem to not consider the problem either, mixing pointer to char[] and struct is violating the rule.

[seladb]

thanks @TehRick please keep me posted on this

[PixelRick]

1) The mk file does include -O2 for which strict-aliasing optimizations are enabled. 2) When checking for violations at -O2/3 using -Wstrict-aliasing=1 some violations are detected, but the one I thought of does not seem to show up (I think raw storage and char arrays might be equivalent for g++ in order to keep allocators working). Examples of detections:

src/IPv4Layer.cpp:343:47: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing] ScalarBuffer scalar = { (uint16_t)ipHdr, (size_t)(ipHdr->internetHeaderLength4) } ;

src/IcmpLayer.cpp:646:42: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing] buffer.buffer = (uint16_t*)getIcmpHeader(); 3) DPDK net modules do use -fno-strict-aliasing. 4) Checking the validity of the changes between -O0 with and without strict-aliasing optimization is too time consuming for me at the moment.

[seladb]

Thanks for taking the time to check this. Based on your experience with strict aliasing, is it sufficient to make sure the unit-tests pass (and maybe also run PcapPlusPlus examples) or should we do a more thorough analysis after adding the -fno-strict-aliasing flag?

[PixelRick]

Ok let's conclude ;)

1) Type-punning is simply not portable. But it often works as expected on the popular ABIs.

2) It's sweet when the compiler can assume more in order to optimize more. As strict aliasing can be respected in most projects, it feels wrong to not use it. But sadly, relying on it currently makes things compile quite poorly on MSVC that has no such optimization capability. It can let developers forget about what a compiler can no longer assume in the presence of a "may-alias" type. A good example (that also works badly in gcc since it uses char that may alias): ` class A { char buf; void foo() { for (int i = 0; i < N; ++i) buf[i] = i; } }; ` Here buf is not a local pointer, it's a class attribute. So in foo(), buf could alias *this and thus ITSELF (this->buf). The compiler has no other choice than reloading buf from this on each iteration because each write could overwrite it. Using a const char* dest = buf; and doing the writes using dest[i] would prevent this reload. In MSVC caching pointers is a good practice to compensate for the absence of aliasing analysis-based optimization, but unfortunately must be done everywhere.

3) There is a type attribute in gcc that specifies may alias https://www.ibm.com/support/knowledgecenter/en/SSGH3R_16.1.0/com.ibm.xlcpp161.aix.doc/language_ref/type_attr_may_alias.html I never used it but it seems like a good solution when targeting gcc/clang. There is no reason for msvc to not add such attribute whenever they end up adding strict aliasing optimizations.

4) I checked that char[] are considered may-alias and they are. Here is my small test: https://wandbox.org/permlink/wSKZilweaxVyCf8F The compiler did consider char[8] as potentially aliasing a float and reloaded i[0] (returning 0 instead of 1). Thus pcappp is safe using a char[] as raw data. The other type-punnings (between scalar types) would be easy to identify if a bug happens. I think you can continue >not< using -fno-strict-aliasing for this project. However if you now somewhere in the code where two different struct types are aliasing then you should consider using gcc's type attribute locally.

Thanks for this library that is of great value for many networking projects :)

[seladb]

Thanks for the detailed information, I really appreciate your help. As per your recommendation I'll close the issue now. Please reopen it if needed