New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anonymus nested structs #2045

Open
noaos opened this Issue Dec 18, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@noaos

noaos commented Dec 18, 2017

Hi,

I'm trying to wrap the following struct:

struct ibv_odp_caps {
    uint64_t general_caps;
    struct {
        uint32_t rc_odp_caps;
        uint32_t uc_odp_caps;
        uint32_t ud_odp_caps;
    } per_transport_caps;
};

as follows:

    cdef struct per_transport_caps:
        unsigned int    rc_odp_caps
        unsigned int    uc_odp_caps
        unsigned int    ud_odp_caps

    cdef struct ibv_odp_caps:
        unsigned long       general_caps
        per_transport_caps  per_transport_caps

but get the following error:

warning: 'struct per_transport_caps' declared inside parameter list [enabled by default]
 static PyObject* __pyx_convert__to_py_struct__per_transport_caps(struct per_transport_caps s);
warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
warning: 'struct per_transport_caps' declared inside parameter list [enabled by default]
 static PyObject* __pyx_convert__to_py_struct__per_transport_caps(struct per_transport_caps s) {
error: parameter 1 ('s') has incomplete type
 static PyObject* __pyx_convert__to_py_struct__per_transport_caps(struct per_transport_caps s) {
error: type of formal parameter 1 is incomplete
       member = __pyx_convert__to_py_struct__per_transport_caps(s.per_transport_caps); if (unlikely(!member)) goto bad;

The Oreily book suggested to flatten the inner struct, but this also fails:

    cdef struct ibv_odp_caps:
        unsigned long       general_caps
        unsigned int    rc_odp_caps
        unsigned int    uc_odp_caps
        unsigned int    ud_odp_caps

gets:

error: 'struct ibv_odp_caps' has no member named 'rc_odp_caps'
       member = __Pyx_PyInt_From_unsigned_int(s.rc_odp_caps); if (unlikely(!member)) goto bad;
error: 'struct ibv_odp_caps' has no member named 'uc_odp_caps'
       member = __Pyx_PyInt_From_unsigned_int(s.uc_odp_caps); if (unlikely(!member)) goto bad;
error: 'struct ibv_odp_caps' has no member named 'ud_odp_caps'
       member = __Pyx_PyInt_From_unsigned_int(s.ud_odp_caps); if (unlikely(!member)) goto bad;

How do I wrap this properly?

@TeamSpen210

This comment has been minimized.

Show comment
Hide comment
@TeamSpen210

TeamSpen210 Dec 18, 2017

Contributor

Have a look at the Interfacing with External C Code guide. You'll want to use cdef extern from to include the C header file. Then it'll be declared exactly how you want.

Contributor

TeamSpen210 commented Dec 18, 2017

Have a look at the Interfacing with External C Code guide. You'll want to use cdef extern from to include the C header file. Then it'll be declared exactly how you want.

@noaos

This comment has been minimized.

Show comment
Hide comment
@noaos

noaos Dec 18, 2017

Thank you for the prompt reply.
The header file is already included, I don't see how this can alter the anonymous inner struct.
Checking 'Cython, a guide' it seems like anonymous structs are not supported. A workaround or a fix would be appreciated.

noaos commented Dec 18, 2017

Thank you for the prompt reply.
The header file is already included, I don't see how this can alter the anonymous inner struct.
Checking 'Cython, a guide' it seems like anonymous structs are not supported. A workaround or a fix would be appreciated.

@gmossessian

This comment has been minimized.

Show comment
Hide comment
@gmossessian

gmossessian Dec 28, 2017

Hi,

The following will work, even though ibv_odp_caps_sub is not in your C file:

cdef extern from "my_header.h":
    ctypedef struct ibv_odp_caps_sub:
        unsigned int rc_odp_caps
        unsigned int uc_odp_caps
        unsigned int ud_odp_caps

    cdef struct ibv_odp_caps:
        unsigned long general_caps
        ibv_odp_caps_sub per_transport_caps

gmossessian commented Dec 28, 2017

Hi,

The following will work, even though ibv_odp_caps_sub is not in your C file:

cdef extern from "my_header.h":
    ctypedef struct ibv_odp_caps_sub:
        unsigned int rc_odp_caps
        unsigned int uc_odp_caps
        unsigned int ud_odp_caps

    cdef struct ibv_odp_caps:
        unsigned long general_caps
        ibv_odp_caps_sub per_transport_caps
@noaos

This comment has been minimized.

Show comment
Hide comment
@noaos

noaos Dec 31, 2017

Hi,

Sadly it doesn't:
pyverbs/device.c:1827:56: error: unknown type name 'ibv_odp_caps_sub'
static PyObject* __pyx_convert__to_py_ibv_odp_caps_sub(ibv_odp_caps_sub s);
^
pyverbs/device.c:14540:56: error: unknown type name 'ibv_odp_caps_sub'
static PyObject* __pyx_convert__to_py_ibv_odp_caps_sub(ibv_odp_caps_sub s) {

Code:

   ctypedef struct ibv_odp_caps_sub:
        unsigned int    rc_odp_caps
        unsigned int    uc_odp_caps
        unsigned int    ud_odp_caps

    cdef struct ibv_odp_caps:
        unsigned long       general_caps
        ibv_odp_caps_sub    per_transport_caps

noaos commented Dec 31, 2017

Hi,

Sadly it doesn't:
pyverbs/device.c:1827:56: error: unknown type name 'ibv_odp_caps_sub'
static PyObject* __pyx_convert__to_py_ibv_odp_caps_sub(ibv_odp_caps_sub s);
^
pyverbs/device.c:14540:56: error: unknown type name 'ibv_odp_caps_sub'
static PyObject* __pyx_convert__to_py_ibv_odp_caps_sub(ibv_odp_caps_sub s) {

Code:

   ctypedef struct ibv_odp_caps_sub:
        unsigned int    rc_odp_caps
        unsigned int    uc_odp_caps
        unsigned int    ud_odp_caps

    cdef struct ibv_odp_caps:
        unsigned long       general_caps
        ibv_odp_caps_sub    per_transport_caps
@gmossessian

This comment has been minimized.

Show comment
Hide comment
@gmossessian

gmossessian Dec 31, 2017

I've replicated your error now, but I also have some code in production that looks exactly the same, except it works.

I will try to understand what the difference is in a few days, after the holidays are over. Happy New Year!

gmossessian commented Dec 31, 2017

I've replicated your error now, but I also have some code in production that looks exactly the same, except it works.

I will try to understand what the difference is in a few days, after the holidays are over. Happy New Year!

@imphil

This comment has been minimized.

Show comment
Hide comment
@imphil

imphil Jan 5, 2018

I got the same problem, and solved it (partially) by using the "public" keyword for the embedded (anonymous) struct:

cdef public struct _osd_mem_desc_regions:
    uint64_t baseaddr
    uint64_t memsize

cdef extern from "osd/cl_mam.h":
    cdef struct osd_mem_desc:
        unsigned int di_addr
        uint16_t data_width_bit
        uint16_t addr_width_bit
        uint8_t num_regions
        _osd_mem_desc_regions regions[8]

That works functionally, however I get the following compiler warnings (gcc 5.4.0)

src/osd.c: In function ‘__pyx_convert__to_py_struct__osd_mem_desc’:
src/osd.c:10721:67: warning: passing argument 1 of ‘__Pyx_carray_to_py_struct___osd_mem_desc_regions’ from incompatible pointer type [-Wincompatible-pointer-types]
         member = __Pyx_carray_to_py_struct___osd_mem_desc_regions(s.regions, 8); if (unlikely(!member)) goto bad;
                                                                   ^
src/osd.c:7291:32: note: expected ‘struct _osd_mem_desc_regions *’ but argument is of type ‘struct <anonymous> *’
 static CYTHON_INLINE PyObject *__Pyx_carray_to_py_struct___osd_mem_desc_regions(struct _osd_mem_desc_regions *__pyx_v_v, Py_ssize_t __pyx_v_length) {
                                ^

imphil commented Jan 5, 2018

I got the same problem, and solved it (partially) by using the "public" keyword for the embedded (anonymous) struct:

cdef public struct _osd_mem_desc_regions:
    uint64_t baseaddr
    uint64_t memsize

cdef extern from "osd/cl_mam.h":
    cdef struct osd_mem_desc:
        unsigned int di_addr
        uint16_t data_width_bit
        uint16_t addr_width_bit
        uint8_t num_regions
        _osd_mem_desc_regions regions[8]

That works functionally, however I get the following compiler warnings (gcc 5.4.0)

src/osd.c: In function ‘__pyx_convert__to_py_struct__osd_mem_desc’:
src/osd.c:10721:67: warning: passing argument 1 of ‘__Pyx_carray_to_py_struct___osd_mem_desc_regions’ from incompatible pointer type [-Wincompatible-pointer-types]
         member = __Pyx_carray_to_py_struct___osd_mem_desc_regions(s.regions, 8); if (unlikely(!member)) goto bad;
                                                                   ^
src/osd.c:7291:32: note: expected ‘struct _osd_mem_desc_regions *’ but argument is of type ‘struct <anonymous> *’
 static CYTHON_INLINE PyObject *__Pyx_carray_to_py_struct___osd_mem_desc_regions(struct _osd_mem_desc_regions *__pyx_v_v, Py_ssize_t __pyx_v_length) {
                                ^
@noaos

This comment has been minimized.

Show comment
Hide comment
@noaos

noaos commented Jan 28, 2018

@gmossessian any update?

@gmossessian

This comment has been minimized.

Show comment
Hide comment
@gmossessian

gmossessian Jan 29, 2018

@noaos Thank you for the reminder, I revisited this this weekend.

I have to apologize if my comment gave you hope, I'm completely stumped. The code that I have working in production looks like this:

my_c_file.c:

#include <stdint.h>

typedef struct _my_struct_t {
    uint32_t a;
    uint32_t * b;
    uint32_t * c;
    struct {
        uint32_t x;
        uint32_t y;
    } value;
} _my_struct_t;

my_prog.pxd:

cdef extern from "my_c_file.c":
    ctypedef struct _my_struct_value:
        unsigned int x
        unsigned int y

    cdef struct _my_struct_t:
        unsigned int a
        _my_struct_value value

and in setup.py:

setup(
    ext_modules=cythonize(Extension("my_prog",
                                    ["my_prog.pyx"],
                                    language='c'),
                          language='c'),
)

This works fine for me in my program, but doing the exact same thing in a fresh environment I get the same errors as you.

This has scared me sufficiently that I just went and de-anonymized the struct in my C source, because I have no idea how or why it was working in my code and not in this much-simplified example. I suggest you do the same, if possible...

gmossessian commented Jan 29, 2018

@noaos Thank you for the reminder, I revisited this this weekend.

I have to apologize if my comment gave you hope, I'm completely stumped. The code that I have working in production looks like this:

my_c_file.c:

#include <stdint.h>

typedef struct _my_struct_t {
    uint32_t a;
    uint32_t * b;
    uint32_t * c;
    struct {
        uint32_t x;
        uint32_t y;
    } value;
} _my_struct_t;

my_prog.pxd:

cdef extern from "my_c_file.c":
    ctypedef struct _my_struct_value:
        unsigned int x
        unsigned int y

    cdef struct _my_struct_t:
        unsigned int a
        _my_struct_value value

and in setup.py:

setup(
    ext_modules=cythonize(Extension("my_prog",
                                    ["my_prog.pyx"],
                                    language='c'),
                          language='c'),
)

This works fine for me in my program, but doing the exact same thing in a fresh environment I get the same errors as you.

This has scared me sufficiently that I just went and de-anonymized the struct in my C source, because I have no idea how or why it was working in my code and not in this much-simplified example. I suggest you do the same, if possible...

@noaos

This comment has been minimized.

Show comment
Hide comment
@noaos

noaos Feb 1, 2018

@scoder, can you please explain why there's no such support? Is there any known workaround?

noaos commented Feb 1, 2018

@scoder, can you please explain why there's no such support? Is there any known workaround?

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Feb 1, 2018

Contributor

#2045 (comment) is the correct answer.

My guess for why you are getting the compile error is that you are probably trying to pass such a struct through Python somehow, which results in an auto-generated conversion function for that non-existing type. So it seems that this feature does not currently work with such a fake anonymous struct declarations, simply because it relies on a correct declaration. But the struct declaration itself should otherwise work ok.

The way out of this for us is that we'll eventually have to support anonymous sub-structs in Cython. Until then, I don't see a way to use those two features together.

Contributor

scoder commented Feb 1, 2018

#2045 (comment) is the correct answer.

My guess for why you are getting the compile error is that you are probably trying to pass such a struct through Python somehow, which results in an auto-generated conversion function for that non-existing type. So it seems that this feature does not currently work with such a fake anonymous struct declarations, simply because it relies on a correct declaration. But the struct declaration itself should otherwise work ok.

The way out of this for us is that we'll eventually have to support anonymous sub-structs in Cython. Until then, I don't see a way to use those two features together.

@horeyazub

This comment has been minimized.

Show comment
Hide comment
@horeyazub

horeyazub Sep 24, 2018

hi,
@noaos we were able to add the struct of per_transport_caps by using the following:

cdef struct per_transport_caps:
uint32_t rc_odp_caps
uint32_t uc_odp_caps
uint32_t ud_odp_caps

cdef struct ibv_odp_caps:  
    uint64_t general_caps
    per_transport_caps per_transport_caps

what was missing is the import of :
from libc.stdint cimport uint32_t

and this most likely why you had the error of :
pyverbs/device.c:1827:56: error: unknown type name 'ibv_odp_caps_sub'
static PyObject* __pyx_convert__to_py_ibv_odp_caps_sub(ibv_odp_caps_sub s);

the way to use it is by calling it like so:
self.odp_caps.per_transport_caps.rc_odp_caps

I tried it and it worked for me

horeyazub commented Sep 24, 2018

hi,
@noaos we were able to add the struct of per_transport_caps by using the following:

cdef struct per_transport_caps:
uint32_t rc_odp_caps
uint32_t uc_odp_caps
uint32_t ud_odp_caps

cdef struct ibv_odp_caps:  
    uint64_t general_caps
    per_transport_caps per_transport_caps

what was missing is the import of :
from libc.stdint cimport uint32_t

and this most likely why you had the error of :
pyverbs/device.c:1827:56: error: unknown type name 'ibv_odp_caps_sub'
static PyObject* __pyx_convert__to_py_ibv_odp_caps_sub(ibv_odp_caps_sub s);

the way to use it is by calling it like so:
self.odp_caps.per_transport_caps.rc_odp_caps

I tried it and it worked for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment