Skip to content

Commit 970cb57

Browse files
committed
[mlir] fix crash in PybindAdaptors.h
The constructor function was being defined without indicating its "__init__" name, which made it interpret it as a regular fuction rather than a constructor. When overload resolution failed, Pybind would attempt to print the arguments actually passed to the function, including "self", which is not initialized since the constructor couldn't be called. This would result in "__repr__" being called with "self" referencing an uninitialized MLIR C API object, which in turn would cause undefined behavior when attempting to print in C++. Fix this by specifying the correct name. This in turn uncovers the fact the the mechanism used by PybindAdaptors.h to bind constructors directly as "__init__" functions taking "self" is deprecated by Pybind. The modern method requires using "py::init", which seems to rely on the C++ equivalent of the bound class to be available, which is not the case in PybindAdaptors.h. A deeper inspection shows that the deprecation concerns old-style pybind11 constructors that had to allocate the object using placement new with "self" as memory. The PybindAdaptors.h only provides extension classes and never allocates (the object construction is delegated to the base class), so it does not use the deprecated functionality. Use the implementation detail tag class to convince pybind11 that we are using the modern constructor binding method and suppress the warning. On top of that, the definition of the function was incorrectly indicated as the method on the "None" object instead of being the method of its parent class. This would result in a second problem when Pybind would attempt to print warnings pointing to the parent class since the "None" does not have a "__name__" field or its C API equivalent. Fix this by specifying the correct parent class by looking it up by name in the parent module. Reviewed By: stellaraccident Differential Revision: https://reviews.llvm.org/D117325
1 parent 85def34 commit 970cb57

File tree

1 file changed

+16
-4
lines changed

1 file changed

+16
-4
lines changed

mlir/include/mlir/Bindings/Python/PybindAdaptors.h

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,11 @@ class mlir_attribute_subclass : public pure_subclass {
320320
// Casting constructor. Note that defining an __init__ method is special
321321
// and not yet generalized on pure_subclass (it requires a somewhat
322322
// different cpp_function and other requirements on chaining to super
323-
// __init__ make it more awkward to do generally).
323+
// __init__ make it more awkward to do generally). It is marked as
324+
// `is_new_style_constructor` to suppress the deprecation warning from
325+
// pybind11 related to placement-new since we are not doing any allocation
326+
// here but relying on the superclass constructor that does "new-style"
327+
// allocation for pybind11.
324328
std::string captureTypeName(
325329
typeClassName); // As string in case if typeClassName is not static.
326330
py::cpp_function initCf(
@@ -336,7 +340,9 @@ class mlir_attribute_subclass : public pure_subclass {
336340
}
337341
superClass.attr("__init__")(self, otherType);
338342
},
339-
py::arg("cast_from_type"), py::is_method(py::none()),
343+
py::name("__init__"), py::arg("cast_from_type"),
344+
py::is_method(scope.attr(typeClassName)),
345+
py::detail::is_new_style_constructor(),
340346
"Casts the passed type to this specific sub-type.");
341347
thisClass.attr("__init__") = initCf;
342348

@@ -371,7 +377,11 @@ class mlir_type_subclass : public pure_subclass {
371377
// Casting constructor. Note that defining an __init__ method is special
372378
// and not yet generalized on pure_subclass (it requires a somewhat
373379
// different cpp_function and other requirements on chaining to super
374-
// __init__ make it more awkward to do generally).
380+
// __init__ make it more awkward to do generally). It is marked as
381+
// `is_new_style_constructor` to suppress the deprecation warning from
382+
// pybind11 related to placement-new since we are not doing any allocation
383+
// here but relying on the superclass constructor that does "new-style"
384+
// allocation for pybind11.
375385
std::string captureTypeName(
376386
typeClassName); // As string in case if typeClassName is not static.
377387
py::cpp_function initCf(
@@ -387,7 +397,9 @@ class mlir_type_subclass : public pure_subclass {
387397
}
388398
superClass.attr("__init__")(self, otherType);
389399
},
390-
py::arg("cast_from_type"), py::is_method(py::none()),
400+
py::name("__init__"), py::arg("cast_from_type"),
401+
py::is_method(scope.attr(typeClassName)),
402+
py::detail::is_new_style_constructor(),
391403
"Casts the passed type to this specific sub-type.");
392404
thisClass.attr("__init__") = initCf;
393405

0 commit comments

Comments
 (0)