[Bf-cycles] ccl::array == operator

[Brecht Van Lommel]

Doing element by element comparison is fine with me, I'm not concerned
about performance here.

On Thu, Sep 7, 2017 at 8:18 AM, Stefan Werner <stewreo at gmail.com> wrote:

> Hello,
>
> 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.
>
> -Stefan
>
> * A version of Lukas’ AOV patch used for Cryptomatte, hopefully coming
> soon to a differential on d.b.o.
> _______________________________________________
> Bf-cycles mailing list
> Bf-cycles at blender.org
> https://lists.blender.org/mailman/listinfo/bf-cycles
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.blender.org/pipermail/bf-cycles/attachments/20170912/add640b1/attachment.html>