[Bf-cycles] ccl::array == operator
stewreo at gmail.com
Thu Sep 7 08:18:23 CEST 2017
when tracing an odd bug in some code I’m working on*, I noticed something in ccl::array. The == operator for the array uses memcmp() to test two arrays for equality, as opposed to an element by element == compare which std::array::== is using. Using memcmp() for a comparison of classes or structs is not guaranteed to work and can return false negatives. This can cause subtle bugs which may not be obvious at first.
In my case, it was comparing two arrays of structs, of which all elements are equal. However, since the compiler added padding for alignment, there were undefined bytes in between struct members, causing memcmp() to fail. This padding is not visible in your average debugger, only once you inspect raw memory you do see what’s at odds there.
I noticed that in Cycles we’re using ccl::array::== on some arrays where it is not guaranteed to work. Prime candidate is using it in Node::equals_value(), where it is applied to arrays of OIIO::ustring - using memcmp() on 3rd party C++ classes is likely to break, if not today, then at some point in the future. I’m also worried about using it for float3 types, which internally are float4s with the 4th float being undefined.
Note that in case where Node::equals_value() is comparing single elements that are not arrays, it is also doing memcmp() on ustrings instead of using ustring::==.
I have no direct bug in master/HEAD that is caused by this behaviour, but a failing ccl::array::== operator caused problems in my custom code. As far as I can see from here, the main use of it seems to be the optimisation of shader graphs, where false negatives would prevent possible optimisations from happening.
My proposal would be to replace ccl::array::== with an element-by-element comparison as in std::array::==, with template specialisations for hand picked cases where memcmp() is known to be safe (these may be compiler and architecture specific). This will slow down some things, but I prefer slow and correct code over fast and broken. Since it’s code that, to my knowledge, we’re not using inside the path tracing kernel, performance penalties should not be too painful.
* A version of Lukas’ AOV patch used for Cryptomatte, hopefully coming soon to a differential on d.b.o.
More information about the Bf-cycles