This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Rework subclass construction in PybindAdaptors.h
ClosedPublic

Authored by ftynse on Jan 19 2022, 1:16 AM.

Details

Summary

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++. Even if the correct name is provided, the mechanism used by
PybindAdaptors.h to bind constructors directly as "init" functions taking
"self" is deprecated by Pybind. The new mechanism does not seem to have access
to a fully-constructed "self" object (i.e., the constructor in C++ takes a
pybind11::detail::value_and_holder that cannot be forwarded back to Python).

Instead, redefine "new" to perform the required checks (there are no
additional initialization needed for attributes and types as they are all
wrappers around a C++ pointer). "new" can call its equivalent on a
superclass without needing "self".

Bump pybind11 dependency to 3.8.0, which is the first version that allows one
to redefine "new".

Diff Detail

Event Timeline

ftynse created this revision.Jan 19 2022, 1:16 AM
ftynse requested review of this revision.Jan 19 2022, 1:16 AM
ftynse updated this revision to Diff 401132.Jan 19 2022, 1:30 AM

Bump pybind11 requirement to 2.8.0.

ftynse updated this revision to Diff 401158.Jan 19 2022, 3:23 AM

Add a standalone test for PybindAdaptors.h

stellaraccident accepted this revision.Jan 19 2022, 8:33 AM

This makes sense to me, and I'm trying to remember why I didn't do it originally (I remember considering the new approach but can't remember why I avoided it). I think I was aliasing the prohibition that regular pybind classes cannot further modify new with these not actually being pybind classes.

In any case, thank you for the heroic debugging and fix.

mlir/cmake/modules/MLIRDetectPythonEnv.cmake
35 ↗(On Diff #401158)

Can you include in the patch description the reason for the version bump? Based on offline discussion, it sounded like this version was chosen based on presence of something in the changelog. Would be good to capture that.

This revision is now accepted and ready to land.Jan 19 2022, 8:33 AM
ftynse edited the summary of this revision. (Show Details)Jan 19 2022, 9:05 AM
ftynse marked an inline comment as done.Jan 19 2022, 9:07 AM

This makes sense to me, and I'm trying to remember why I didn't do it originally (I remember considering the new approach but can't remember why I avoided it). I think I was aliasing the prohibition that regular pybind classes cannot further modify new with these not actually being pybind classes.

It was not possible to redefine __new__ when you wrote this, it was only added in September 2021 (https://github.com/pybind/pybind11/issues/3253), hence the version bump to the 3.8 release from October 2021.

mlir/cmake/modules/MLIRDetectPythonEnv.cmake
35 ↗(On Diff #401158)

Yes, I updated the git commit but arc doesn't pick up new description. Amended manually.

This revision was landed with ongoing or failed builds.Jan 19 2022, 9:09 AM
This revision was automatically updated to reflect the committed changes.
ftynse marked an inline comment as done.