[RFC] Fix type_caster::{handle,load} under ubsan in gcc-4.9

[ajtulloch]

ubsan in gcc-4.9 doesn't allow iteration over a zero-sized std::array, so essentially all pybind11 bindings fail ubsan. https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02617.html is a fix as well, but it might be nice to support gcc-4.9 sanitizers as well.

[jagerman]

The extra checks seem to incur a small increase in .so size (about .3% for the test suite). I also don't think the second one is going to work properly: it's added after the first for loop over the 0-sized array (or is that one different for some reason--perhaps because it iterates by const ref?)

Either way, I think you could address both issues by providing overloads for empty-argument versions by keeping the current method as they are, then these new overloads to handle the 0-argument calls trivially:

    bool load(handle, bool, index_sequence<>) { return true; }
    static handle cast(const type &, return_value_policy, handle, index_sequence<>) { return tuple(0).release(); }

[jagerman]

The load() overload would also allow the non-trivial load() to be simplified nicely:

    bool load(handle, bool, index_sequence<>) { return true; }
    template <size_t ... Indices> bool load(handle src, bool convert, index_sequence<Indices...>) {
        for (bool r : { std::get<Indices>(value).load(PyTuple_GET_ITEM(src.ptr(), Indices), convert)... })
            if (!r)
                return false;
        return true;
    }

which is a nice gain on its own (while also addressing the ubsan compatibility for load()). But note that this requires the overload (even if ubsan doesn't) because otherwise it fails because of an inability to deduce the initializer_list type for an empty list.

[jagerman]

And, just for fun, if this was pybind17 instead of pybind11, the whole thing could become nicer still:

template <size_t ... Indices> bool load(handle src, bool convert, index_sequence<Indices...>) {
    return std::get<Indices>(value).load(PyTuple_GET_ITEM(src.ptr(), Indices), convert) && ...;
}

[aldanor]

The extra checks seem to incur a small increase in .so size

That's a bit strange since it's essentially a compile-time check.

[aldanor]

Agreed though, overloads might be a cleaner way to do this.

[wjakob]

@ajtulloch: Interesting -- I'm curious: is Facebook planning to use pybind11 for something? :)

[jagerman]

@aldanor - agreed, but it might just have been gcc being finicky. After I pulled some other (completely unrelated) changes from master the difference mostly disappeared, and doesn't appear at all under clang. Still, I think the cleanup on load() is nice.

[jagerman]

@ajtulloch ping? I can submit a PR to fix this if you're too busy (but I don't want to steal the commit credit).

[wjakob]

@ajtulloch: ping

[jagerman]

@wjakob -- I think #534 already includes the fix for this.

[wjakob]

Ok, then I'll close the ticket.