Skip to content
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

Inter-class/file calling of fused functions may result into calling t… #1873

Closed
wants to merge 1 commit into from

Conversation

chris-pcguy
Copy link

@chris-pcguy chris-pcguy commented Sep 15, 2017

…he wrong function

An example from my testcase:
Calling "setFullFlags" from "hirnwichse_main.pyx" will actually call "regWriteWithOpWords".
This results into undefined behaviour.

For some reason (I don't know why, because I don't know the cython source well enough) this only happens when those different classes are in different .pyx/.pxd files.

Here's a snippet of the resulting C-code from a testcase (without the patch):

hirnwichse_main.c:

struct __pyx_vtabstruct_9registers_Registers {
  void (*__pyx_fuse_0setFullFlags)(struct __pyx_obj_9registers_Registers *, uint8_t, uint32_t);
  void (*__pyx_fuse_1setFullFlags)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2setFullFlags)(struct __pyx_obj_9registers_Registers *, uint32_t, uint32_t);
  void (*__pyx_fuse_0regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint16_t);
  void (*__pyx_fuse_1regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint64_t);
};

registers.c:

struct __pyx_vtabstruct_9registers_Registers {
  void (*__pyx_fuse_0regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint16_t);
  void (*__pyx_fuse_1regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint64_t);
  void (*__pyx_fuse_0setFullFlags)(struct __pyx_obj_9registers_Registers *, uint8_t, uint32_t);
  void (*__pyx_fuse_1setFullFlags)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2setFullFlags)(struct __pyx_obj_9registers_Registers *, uint32_t, uint32_t);
};

With the patch, both look like:

struct __pyx_vtabstruct_9registers_Registers {
  void (*__pyx_fuse_0regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint16_t);
  void (*__pyx_fuse_0setFullFlags)(struct __pyx_obj_9registers_Registers *, uint8_t, uint32_t);
  void (*__pyx_fuse_1regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_1setFullFlags)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint64_t);
  void (*__pyx_fuse_2setFullFlags)(struct __pyx_obj_9registers_Registers *, uint32_t, uint32_t);
};

Signed-off-by: Christian Inci chris.gh@broke-the-inter.net

…he wrong function

An example from my testcase:
Calling "setFullFlags" from "hirnwichse_main.pyx" will actually call "regWriteWithOpWords".
This results into undefined behaviour.

For some reason (I don't know why, because I don't know the cython source well enough) this only happens when those different classes are in different .pyx/.pxd files.

Here's a snippet of the resulting C-code from a testcase (without the patch):

hirnwichse_main.c:
"""
struct __pyx_vtabstruct_9registers_Registers {
  void (*__pyx_fuse_0setFullFlags)(struct __pyx_obj_9registers_Registers *, uint8_t, uint32_t);
  void (*__pyx_fuse_1setFullFlags)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2setFullFlags)(struct __pyx_obj_9registers_Registers *, uint32_t, uint32_t);
  void (*__pyx_fuse_0regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint16_t);
  void (*__pyx_fuse_1regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint64_t);
};
"""

registers.c:
"""
struct __pyx_vtabstruct_9registers_Registers {
  void (*__pyx_fuse_0regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint16_t);
  void (*__pyx_fuse_1regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint64_t);
  void (*__pyx_fuse_0setFullFlags)(struct __pyx_obj_9registers_Registers *, uint8_t, uint32_t);
  void (*__pyx_fuse_1setFullFlags)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2setFullFlags)(struct __pyx_obj_9registers_Registers *, uint32_t, uint32_t);
};
"""

With the patch, both look like:
"""
struct __pyx_vtabstruct_9registers_Registers {
  void (*__pyx_fuse_0regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint16_t);
  void (*__pyx_fuse_0setFullFlags)(struct __pyx_obj_9registers_Registers *, uint8_t, uint32_t);
  void (*__pyx_fuse_1regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_1setFullFlags)(struct __pyx_obj_9registers_Registers *, uint16_t, uint32_t);
  void (*__pyx_fuse_2regWriteWithOpWords)(struct __pyx_obj_9registers_Registers *, uint16_t, uint64_t);
  void (*__pyx_fuse_2setFullFlags)(struct __pyx_obj_9registers_Registers *, uint32_t, uint32_t);
};
"""

Signed-off-by: Christian Inci <chris.gh@broke-the-inter.net>
@scoder
Copy link
Contributor

scoder commented Sep 19, 2017

Thanks. But this feels like it's not the right fix. We are talking about externally defined types here, so both sides must agree on the same definition order. And that order must not change with a new release, which it does in your case.

Could you please provide a (failing) test case that shows what exactly you are doing?

@chris-pcguy
Copy link
Author

@scoder
Copy link
Contributor

scoder commented Sep 19, 2017

Thanks for the example. It looks like the order in which they are defined in the cimporting pyx file is determined by the order in which they are called, not in which they are defined. Your example calls the second one first, so it ends up first in the vtable. Clearly a bug.

@scoder scoder closed this in fcc3461 Sep 20, 2017
@scoder
Copy link
Contributor

scoder commented Sep 20, 2017

I've pushed a fix to current master. Please give it a try.

@chris-pcguy
Copy link
Author

It's working fine. Thank you very much.

@scoder
Copy link
Contributor

scoder commented Sep 20, 2017

For completeness, here is the analysis:

The problem was that cdef functions with fused types were expanded arbitrarily on use, and their Entries removed and re-appended to the list of cdef function entries. But that list also defines the order of the entries in the vtable.

Previously, when a method with fused types was defined in a .pxd file, it was the implementation order in the .pyx (!) file that determined the order in the vtable. Even worse, modules that cimported from that .pxd file took the order in which these methods were called to determines the corresponding vtable on the other side, as the entries were expanded at first use, i.e. at their first occurrence in the code, and appended to the current list of cdef functions at this (arbitrary) point.

Thus, there was no way to safely infer the vtable order of an extension type with fused methods from a .pxd file.

The correct way to handle this would have been to replace the original fused entry with the expanded list of specialised entries. But it's too late for that now. All existing modules out there would need to get recompiled if we changed their vtable order, at least if they use fused any types methods anywhere in their hierarchy.

However, I think we can assume code that uses a different method order in the .pxd and .pyx files to be broken already, so the way out of this misery is to make the .pxd order the one and only source, with the twist that we had to keep the "delete and re-append" algorithm to keep the order that existing translated modules are using.

Thus the approach:

  • we switch to a scheme now that is only defined by the first declaration order, which is the order in the .pxd file if there is one.

  • we make sure all fused methods are expanded in that same order, but continue to follow all non-fused methods in the vtable, as before.

  • we break all code now that uses a different order of fused methods in their pyx and pxd files.

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

Successfully merging this pull request may close these issues.

None yet

2 participants