[Mlir-commits] [mlir] 970cb57 - [mlir] fix crash in PybindAdaptors.h

[Alex Zinenko]

Author: Alex Zinenko
Date: 2022-01-17T10:28:01+01:00
New Revision: 970cb57ef72c9045250e0492cb00127b49ddfea8

URL: https://github.com/llvm/llvm-project/commit/970cb57ef72c9045250e0492cb00127b49ddfea8
DIFF: https://github.com/llvm/llvm-project/commit/970cb57ef72c9045250e0492cb00127b49ddfea8.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. 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

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..68942e560355c 100644
--- a/mlir/include/mlir/Bindings/Python/PybindAdaptors.h
+++ b/mlir/include/mlir/Bindings/Python/PybindAdaptors.h
@@ -320,7 +320,11 @@ 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(
@@ -336,7 +340,9 @@ class mlir_attribute_subclass : public pure_subclass {
           }
           superClass.attr("__init__")(self, 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,7 +377,11 @@ 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(
@@ -387,7 +397,9 @@ class mlir_type_subclass : public pure_subclass {
           }
           superClass.attr("__init__")(self, 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;