Fix a subtle uninitialized-memory-read in Buffer::for_each_value()

[steven-johnson]

When we flattened dimensions in for_each_value_prep(), we would copy from one past the end, meaning the last element contained uninitialized garbage. (This wasn't noticed as an out-of-bounds read because we overallocated in structure in for_each_value_impl()). This garbage stride was later used to advance ptrs in for_each_value_helper()... but only on the final iteration, so even if the ptr was wrong, it didn't matter, as the ptr was never used again. Under certain MSAN configurations, though, the read would be (correctly) flagged as uninitialized.

This fixes the MSAN bug, and also (slightly) improves the efficiency by returning the post-flattened number of dimensions, potentially reducing the number of iterations f for_each_value_helper() needed.

[vitalybuka]

Added few comments, but I know nothing about this code.

The patch fixes sanitizer issue we had.

[vitalybuka]

this d can be after d-- on 2226, is this expected?

[steven-johnson]

Yes, this is deliberate; if we 'flatten' multiple dimensions into one, we want to know the new, smaller number of dimensions.

[vitalybuka]

t[d-1].extent will be set on line 2231, what is about .stride?

[steven-johnson]

No, we want to leave that alone -- @abadams to confirm.

[abadams]

Do we still need to set the extent, given that we're not going to iterate over that dimension?

[steven-johnson]

Oooh, good point. We can probably elide that now. Let me do some hackery to verify.

[steven-johnson]

yes, that was definitely unnecessary.

[steven-johnson]

I now have another thing I'm not sure is optimal; this section in for_each_value_prep():

        bool innermost_strides_are_one = true;
        if (dimensions > 0) {
            for (int i = 0; i < N; i++) {
                innermost_strides_are_one &= (t[0].stride[i] == 1);
            }
        }

I presume the if (dimensions > 0) check is just for paranoia? Currently this function should never be called when dimensions == 0 (there's only one caller). I added an assert() to indicate this is expected as an invariant.