Pybind11: Problem with format_descriptor and integral types in VS2015

[patrikhuber]

This is kind of weird but I think it looks to me like a bug in format_descriptor or somewhere related.

I'm doing this inside a .def_buffer(... { ...:

std::cout << py::format_descriptor<float>::format() << std::endl;

and when I call the appropriate function from python, it outputs "f", as expected, and "d" if I plug in double as T. However when I plug in any other type (integral types I guess), like unsigned char or signed int, it outputs a blank string.

I've found the lines PYBIND11_DECL_FMT(float, "f"); for float, double and bool, so the function is not defined for other types, however, there is a function

template <typename T> struct format_descriptor<T, typename std::enable_if<std::is_integral<T>::value>::type> {

which I think should get activated if T is an integral type, however, it doesn't seem to be, as the returned string is an empty "".

After a while of pondering over this I thought it might have something to do with the compile flags of pybind11, since in a "hello-world" repro in a C++ app, it weirdly prints the correct values, for example pybind11::format_descriptor<unsigned char>::format() prints B. However I commented all flags I could find like /LTCG and so on in pybind11's CMakeLists.txt, but there was no change in behaviour.

Initially, what happened was that my code works correctly for float/double, but when I put in unsigned char's in my data type and pybind11::format_descriptor<unsigned char>::format() into the buffer_info, python would complain with a RuntimeWarning: Item size computed from the PEP 3118 buffer format string does not match the actual item size.. Then, I found out what I described above, that in fact the format string is empty when it should be B.

Sorry in case this is something stupid that I overlooked, but I spend quite a lot of time trying to solve this.

[wjakob]

@patrikhuber: could you please provide a self-contained example as requested per https://github.com/pybind/pybind11/blob/master/CONTRIBUTING.md?

cc @aldanor

[aldanor]

@patrikhuber If your VS2015 is older than Update 3, this may (and is known to) fail, could you check your compiler version?

[aldanor]

May be related: #362

[patrikhuber]

362 looks very related on first glance. Seems to be exactly my problem. I'll have a closer look. Also, my code compiles, but fails at runtime, while @valseTrage in the other post said his code didn't compile.

However, I'm already on the newest VS2015, Version 14.0.25425.01 Update 3.
So there might be a difference, since for the person in #362, updating VS seemed to have fixed it.

I'll try to provide a repro case.

[patrikhuber]

I can reproduce it with this simple test case:

#include "pybind11/pybind11.h"

#include <cstddef> // for size_t
#include <iostream>
#include <string>

namespace py = pybind11;

class Test {
public:
    std::size_t rows = 2;
    std::size_t cols = 3;
    unsigned char* data;
};

Test test()
{
    Test t;
    return t;
}

PYBIND11_PLUGIN(eos) {
    py::module m("eos", "Doc");

    py::class_<Test>(m, "Mat")
        .def_buffer([](Test &mat) -> py::buffer_info {

        std::cout << "Converting:" << std::endl;
        std::cout << py::format_descriptor<unsigned char>::format() << std::endl; // all integral types print ""
        std::cout << py::format_descriptor<float>::format() << std::endl; // prints "f"
        std::cout << "End." << std::endl;

        return py::buffer_info(
            mat.data,                               /* Pointer to buffer */
            sizeof(unsigned char),                          /* Size of one scalar */
            py::format_descriptor<unsigned char>::format(), /* Python struct-style format descriptor */
            2,                                      /* Number of dimensions */
            { mat.rows, mat.cols },                 /* Buffer dimensions */
            { sizeof(unsigned char) * mat.cols,             /* Strides (in bytes) for each index */
            sizeof(unsigned char) }
        );
    });

    m.def("test", &test);
    return m.ptr();
};

And then in Python, do:

import eos
import numpy as np

np.array(eos.test())

will produce RuntimeWarning: Item size computed from the PEP 3118 buffer format string does not match the actual item size. because the buffer format string is empty. It prints "f" for float and "" for the unsigned char. It works fine if you replace all unsigned chars with float.

I can also upload my CMake/project files if you can't reproduce it with this.

My system:
VS2015, Version 14.0.25425.01 Update 3
Compiler version 19.00.24213.1
x64 build, it happens in Debug, RelWithDebug, as well as with Release build.
Python 3.5.2, 64bit, downloaded two days ago from python.org, same for NumPy, downloaded through their pip.

As mentioned above, if I do:

int main() {
    std::cout << pybind11::format_descriptor<unsigned char>::format() << std::endl;
}

it outputs "B"... which _is_ quite weird.

[aldanor]

Thanks; I'll only be able to check it on VS2015 tomorrow, but will try to take a look.

@wjakob Could this be constexpr/compile-time strings weirdness again or something like that? (https://github.com/pybind/pybind11/blob/master/include/pybind11/common.h#L443) In which case maybe why don't we just do it like PYBIND11_DECL_FMT(uint64_t, "Q") for all 8 int types? Int types from cstdint are typedefs anyway so it should work unless there's something I'm missing.

[patrikhuber]

Thank you! :-)

The only things that come to my mind that are really different from the int main() case, where it seems to work, are that A) we're building a library, not an .exe, with potentially different flags, B) the format descriptor is called inside a lambda, and the buffer_info is directly initialised with the value. Maybe it's even a weird compiler bug, I think it wouldn't be the first time that the VS compiler has a bug when something is done within a lambda.
Or, C), the mistake is on my part.

[wjakob]

So did this turn out to be a MSVC issue?

[aldanor]

@wjakob I finally got to testing it out on MSVC. Indeed, it does fail like described by @patrikhuber in a peculiar way.

If I add this dummy function (which never gets called) to the code above:

void dummy() {
    (void) py::format_descriptor<unsigned char>::format();
}

then it works (i.e. segfaults as expected) -- @patrikhuber can you confirm?

Basically, accessing the format descriptor in any way anywhere makes a compile-time difference on MSVC for some reason. @wjakob any ideas? (if not, we can try replacing the fancy integer descriptors with DECL_FMT copy/paste and see if that works).

[jagerman]

Try this, which seems to fix the issue under MSVC:

template <typename T> struct format_descriptor<T, detail::enable_if_t<std::is_integral<T>::value>> {
    static constexpr const char *value =
        "b\0B\0h\0H\0i\0I\0q\0Q" + (detail::log2(sizeof(T))*4 + (std::is_unsigned<T>::value ? 2 : 0));
    static std::string format() { return value; }
};

template <typename T> constexpr const char *format_descriptor<
    T, detail::enable_if_t<std::is_integral<T>::value>>::value;

[patrikhuber]

@aldanor I'm glad you were able to reproduce it. I added your dummy function and indeed it then prints "B" as expected so I can confirm your observation.

It seems like the template needs to be instantiated outside of a lambda function for it to work. If it's only instantiated inside the lambda, it doesn't work. It looks like a VS2015 bug, or what do you think? I can also see @jagerman's fix but I'm not sure whether that means that the original code in pybind11 is problematic and his code is "better" or whether the original code is okay and it's an MSVC issue.

[jagerman]

This is a more elegant fix for pybind than the one I posted above:

template <typename T> struct format_descriptor<T, detail::enable_if_t<std::is_integral<T>::value>> {
    static constexpr const char c = "bBhHiIqQ"[detail::log2(sizeof(T))*2 + std::is_unsigned<T>::value];
    static constexpr const char value[2] = {c, '\0'};
    static std::string format() { return std::string(1, c); }
};

[wjakob]

@patrikhuber , can you please check if the commit by @jagerman fixes the issue? This snippet should go into pybind11.h and replace a similar structure (search for "bBhHiIqQ").

[patrikhuber]

@wjakob I can confirm that the latest code from @jagerman fixes the issue.

[wjakob]

committed, thanks.

[patrikhuber]

@wjakob I think what you committed doesn't compile - you additionally need to delete these following two lines (sorry I didn't explicitly mention it - when you gave me the instructions on how to test @jagerman's code, I thought you had already seen that :-) ):

template <typename T> constexpr const char format_descriptor<
    T, detail::enable_if_t<std::is_integral<T>::value>>::value[2];

Otherwise there's an error:

include\pybind11\common.h(441): error C2039: 'value': is not a member of 'pybind11::format_descriptor<T,std::enable_if<std::is_integral<_Ty>::value,void>::type>'
include\pybind11\common.h(440): note: see declaration of 'pybind11::format_descriptor<T,std::enable_if<std::is_integral<_Ty>::value,void>::type>'

[wjakob]

Hmm -- Clang, GCC and MSVC CI builds passed without problems. What line in your codebase specifically causes that error? We should probably add it to the test suite.

[patrikhuber]

The error (L441) is the second line on the code snippet above.

I'll check whether my sources are modified, I played around with it a few days ago. Will update this post in a bit.

_Update:_ Hmm ok, I'm on the master branch now and everything compiles. Sorry for the false alert.

I'm not exactly sure what happened, but I was on commit 29b5064 (which is 2 weeks old) and verified that the only changes I had was the 4 lines changed by @jagerman, and I got above error. Maybe there was some other change to related code in the mean time. I probably should've updated before all this experimenting.
That was a bit weird, but there are no compile issues with the current master branch.

[patrikhuber]

Thank you again to all for the quick response and the fix! This whole project is really awesome! :-)