[Mlir-commits] [mlir] 289021a - [mlir] fix crash in PybindAdaptors.h
Alex Zinenko
llvmlistbot at llvm.org
Tue Jan 18 01:24:18 PST 2022
Author: Alex Zinenko
Date: 2022-01-18T10:24:11+01:00
New Revision: 289021a45decdef8ed61d054b9c90ce775ca9c96
URL: https://github.com/llvm/llvm-project/commit/289021a45decdef8ed61d054b9c90ce775ca9c96
DIFF: https://github.com/llvm/llvm-project/commit/289021a45decdef8ed61d054b9c90ce775ca9c96.diff
LOG: [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. Instead, leverage the fact that the adaptors are intended for
attrbutes/types that cannot have additional data members and are all ultimately
instances of "PyAttribute"/"PyType" C++ class. In constructors of derived
classes, construct an instance of the base class first, then steal its internal
pointer to the C++ object to construct the instance of the derived class.
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
Added:
Modified:
mlir/include/mlir/Bindings/Python/PybindAdaptors.h
Removed:
################################################################################
diff --git a/mlir/include/mlir/Bindings/Python/PybindAdaptors.h b/mlir/include/mlir/Bindings/Python/PybindAdaptors.h
index 0340e9cc4b87f..becbf01806f44 100644
--- a/mlir/include/mlir/Bindings/Python/PybindAdaptors.h
+++ b/mlir/include/mlir/Bindings/Python/PybindAdaptors.h
@@ -291,6 +291,29 @@ class pure_subclass {
py::object get_class() const { return thisClass; }
protected:
+ /// Defers to the constructor of the superClass the configuration of the
+ /// pybind11 object from the given arguments. Pybind11 has a special handling
+ /// for constructors such that they don't accept a "self" reference, unlike
+ /// Python "__init__" calls. Therefore, one cannot just call the "__init__" of
+ /// the parent class, which would require access to "self". Instead, create an
+ /// instance of the superclass and take its instance pointer to the base C++
+ /// object to populate the instance pointer of the constructed object. Since
+ /// we only deal with _pure_ subclasses, this should be sufficient as derived
+ /// classes cannot have more data fields.
+ template <typename... Args>
+ static void deferToSuperclassConstructor(py::detail::value_and_holder &vh,
+ py::object superClass,
+ Args &&...args) {
+ py::object super = superClass(std::forward<Args>(args)...);
+ py::detail::type_info *ti =
+ py::detail::get_type_info((PyTypeObject *)superClass.ptr());
+ auto *instance = reinterpret_cast<py::detail::instance *>(super.ptr());
+
+ // Take ownership of the value pointer from the base class.
+ vh.value_ptr() = instance->get_value_and_holder(ti, true).value_ptr();
+ super.release();
+ }
+
py::object superClass;
py::object thisClass;
};
@@ -320,12 +343,16 @@ class mlir_attribute_subclass : public pure_subclass {
// Casting constructor. Note that defining an __init__ method is special
// and not yet generalized on pure_subclass (it requires a somewhat
//
diff erent cpp_function and other requirements on chaining to super
- // __init__ make it more awkward to do generally).
+ // __init__ make it more awkward to do generally). It is marked as
+ // `is_new_style_constructor` to suppress the deprecation warning from
+ // pybind11 related to placement-new since we are not doing any allocation
+ // here but relying on the superclass constructor that does "new-style"
+ // allocation for pybind11.
std::string captureTypeName(
typeClassName); // As string in case if typeClassName is not static.
py::cpp_function initCf(
- [superClass, isaFunction, captureTypeName](py::object self,
- py::object otherType) {
+ [superClass, isaFunction, captureTypeName](
+ py::detail::value_and_holder &vh, py::object otherType) {
MlirAttribute rawAttribute = py::cast<MlirAttribute>(otherType);
if (!isaFunction(rawAttribute)) {
auto origRepr = py::repr(otherType).cast<std::string>();
@@ -334,9 +361,12 @@ class mlir_attribute_subclass : public pure_subclass {
" (from " + origRepr + ")")
.str());
}
- superClass.attr("__init__")(self, otherType);
+ pure_subclass::deferToSuperclassConstructor(vh, superClass,
+ otherType);
},
- py::arg("cast_from_type"), py::is_method(py::none()),
+ py::name("__init__"), py::arg("cast_from_type"),
+ py::is_method(scope.attr(typeClassName)),
+ py::detail::is_new_style_constructor(),
"Casts the passed type to this specific sub-type.");
thisClass.attr("__init__") = initCf;
@@ -371,12 +401,16 @@ class mlir_type_subclass : public pure_subclass {
// Casting constructor. Note that defining an __init__ method is special
// and not yet generalized on pure_subclass (it requires a somewhat
//
diff erent cpp_function and other requirements on chaining to super
- // __init__ make it more awkward to do generally).
+ // __init__ make it more awkward to do generally). It is marked as
+ // `is_new_style_constructor` to suppress the deprecation warning from
+ // pybind11 related to placement-new since we are not doing any allocation
+ // here but relying on the superclass constructor that does "new-style"
+ // allocation for pybind11.
std::string captureTypeName(
typeClassName); // As string in case if typeClassName is not static.
py::cpp_function initCf(
- [superClass, isaFunction, captureTypeName](py::object self,
- py::object otherType) {
+ [superClass, isaFunction, captureTypeName](
+ py::detail::value_and_holder &vh, py::object otherType) {
MlirType rawType = py::cast<MlirType>(otherType);
if (!isaFunction(rawType)) {
auto origRepr = py::repr(otherType).cast<std::string>();
@@ -385,9 +419,12 @@ class mlir_type_subclass : public pure_subclass {
origRepr + ")")
.str());
}
- superClass.attr("__init__")(self, otherType);
+ pure_subclass::deferToSuperclassConstructor(vh, superClass,
+ otherType);
},
- py::arg("cast_from_type"), py::is_method(py::none()),
+ py::name("__init__"), py::arg("cast_from_type"),
+ py::is_method(scope.attr(typeClassName)),
+ py::detail::is_new_style_constructor(),
"Casts the passed type to this specific sub-type.");
thisClass.attr("__init__") = initCf;
More information about the Mlir-commits
mailing list