Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-8631

Robustify pybind11 holder type inheritance

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      DRP S17-2, DRP S17-3
    • Team:
      Data Release Production

      Description

      One frequently occurring problem with pybind11 is segfaults happening when two classes in an inheritance hierarchy have different holder types (i.e. parent has unique_ptr holder type while derived class has shared_ptr).
      This ticket aims to investigate and implement (if possible) a compile time (or import time) check for matching holder types.
      While this check is not strictly necessary, it would make the pybind11 stack significantly more robust.

        Attachments

          Activity

          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Ok, Jim Bosch can you take a quick look at the PR?
          I think upstream will probably think segfaults are a problem but it is a thing to keep in mind with these kind of tickets.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Ok, Jim Bosch can you take a quick look at the PR? I think upstream will probably think segfaults are a problem but it is a thing to keep in mind with these kind of tickets.
          Hide
          jbosch Jim Bosch added a comment -

          Looks good to me. Just a few minor comments:

          • Perhaps the void_holder<std::unique_ptr<T>> specialization should have another template parameter for custom deleters?
          • I thought I once read something somewhere about either shared_ptr<void> or unique_ptr<void> only being legal in C++14. I can't find that now, but could you please double-check that those are valid instantiations in C++11?
          Show
          jbosch Jim Bosch added a comment - Looks good to me. Just a few minor comments: Perhaps the void_holder<std::unique_ptr<T>> specialization should have another template parameter for custom deleters? I thought I once read something somewhere about either shared_ptr<void> or unique_ptr<void> only being legal in C++14. I can't find that now, but could you please double-check that those are valid instantiations in C++11?
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Jim Bosch. Good catch about the deleter, without that it fails to recognise a holder type that specified one.

          I'm not sure about the problem with void. I can't find anything about that, and it compiles just fine with -std=c++11 without warnings. However, do note that creating a unique_ptr<void> is of course impossible without specifying a custom deleter. But asking for the typeid is valid (doesn't require a complete type when given a type instead of a reference).
          Granted, this may be a bit of a hack, hence my expectation that the upstream developers may push back. But I can't think of another good way to encode the type information of the (smart) pointer that would not in itself be a hack. Of course I could just pick typeid(unique_ptr<int>) (or any other) and be done with it, but aesthetics pointed me to void.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Jim Bosch . Good catch about the deleter, without that it fails to recognise a holder type that specified one. I'm not sure about the problem with void . I can't find anything about that, and it compiles just fine with -std=c++11 without warnings. However, do note that creating a unique_ptr<void> is of course impossible without specifying a custom deleter. But asking for the typeid is valid (doesn't require a complete type when given a type instead of a reference). Granted, this may be a bit of a hack, hence my expectation that the upstream developers may push back. But I can't think of another good way to encode the type information of the (smart) pointer that would not in itself be a hack. Of course I could just pick typeid(unique_ptr<int>) (or any other) and be done with it, but aesthetics pointed me to void .
          Hide
          jbosch Jim Bosch added a comment -

          If you didn't find any mention of shared_ptr<void> being disallowed, I probably imagined it. Carry on.

          Show
          jbosch Jim Bosch added a comment - If you didn't find any mention of shared_ptr<void> being disallowed, I probably imagined it. Carry on.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Merged upstream.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Merged upstream.

            People

            Assignee:
            pschella Pim Schellart [X] (Inactive)
            Reporter:
            pschella Pim Schellart [X] (Inactive)
            Watchers:
            Jim Bosch, John Swinbank, Pim Schellart [X] (Inactive), Russell Owen
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins Builds

                No builds found.