Make no-python build ABI compatible with python build

[foisya-adsk]

Description of Change(s)

This change makes sure clients compiling against builds with Python
support disabled can still run their application with a runtime USD
with Python enabled.

This is mainly done by wrapping Python binding types in wrappers which
have the same size and alignment in no-python builds.

This is mainly done in existing pyObjWrapper.h and new
pyBoostObjWrapper.h. For example, the applied pattern looks like:

#ifdef PXR_PYTHON_SUPPORT_ENABLED

PXR_NAMESPACE_OPEN_SCOPE

// We define the empty stub for ABI compatibility even if Python support is 
// enabled so we can make sure size and alignment is the same.
class TfPyObjWrapperStub
{
public:
    static constexpr std::size_t Size = 16;
    static constexpr std::size_t Align = 8;

private:
    std::aligned_storage<Size, Align>::type _stub;
};

/// \class TfPyObjWrapper
///
/// Boost Python object wrapper.
/// …
#ifdef PXR_PYTHON_SUPPORT_ENABLED
class TfPyObjWrapper
    : public boost::python::api::object_operators<TfPyObjWrapper>
{
    typedef boost::python::object object;

public:

    TF_API TfPyObjWrapper();

    TF_API TfPyObjWrapper(object obj);

    object const &Get() const {
        return *_objectPtr;
    }

    TF_API PyObject *ptr() const;

    friend inline size_t hash_value(TfPyObjWrapper const &o) {
        return (size_t) o.ptr();
    }

    TF_API bool operator==(TfPyObjWrapper const &other) const;

    TF_API bool operator!=(TfPyObjWrapper const &other) const;

private:

    // Befriend object_operators to allow it access to implicit conversion to
    // boost::python::object.
    friend class boost::python::api::object_operators<TfPyObjWrapper>;
    operator object const &() const {
        return Get();
    }

    // Store a shared_ptr to a python object.
    std::shared_ptr<object> _objectPtr;
};

static_assert(sizeof(TfPyObjWrapper) == sizeof(TfPyObjWrapperStub),
              "ABI break: Incompatible class sizes.");
static_assert(alignof(TfPyObjWrapper) == alignof(TfPyObjWrapperStub),
              "ABI break: Incompatible class alignments.");

#else // PXR_PYTHON_SUPPORT_ENABLED

class TfPyObjWrapper : TfPyObjWrapperStub
{
};

#endif // PXR_PYTHON_SUPPORT_ENABLED

Fixes Issue(s)

• External issue Make USD built with python and without python C++ ABI compatible #1716

Co-authored-by: Martin Bisson [email protected]

[jilliene]

Filed as internal issue #USD-7107

[spiffmon]

Hi @foisya-adsk ! We want to integrate this, but we do not have you listed in Autodesk's CLA. Can you please ask Gordon to add you? Thank you!!

[sunyab]

Left questions about the need for TfPyBoostObjWrapper vs the existing TfPyObjWrapper. Thanks!

[sunyab]

Is there any particular reason why a separate TfPyBoostObjWrapper is needed here instead of using the existing TfPyObjWrapper? It looks like other code like VtValue is using TfPyObjWrapper, so it seems like it should be usable here too.

[foisya-adsk]

Thanks! When we did the changes we wanted to keep things as separate as possible and we kept the return types the same as the ones we wrapped. We will do as you suggest and unify under TfPyObjWrapper.

I think we we were too cautious when considering boost::python::object and boost::python::api::object... One is a "using ..." of the other! We will simplify things.

[bissonm]

The other difference between the two uses is is that TfPyObjWrapper is holding a shared_ptr to an object. However, object is basically a reference-counted holder as well, so it should have the same semantic.

[gitamohr]

The other difference between the two uses is is that TfPyObjWrapper is holding a shared_ptr to an object. However, object is basically a reference-counted holder as well, so it should have the same semantic.

The big important reason why this class exists and uses a shared_ptr is that in order to use boost::python::object you have to be holding the python GIL, since it invokes the python ref-counting operations under the hood, and boost::python::object does not take the GIL itself. This is explained in the class doc for TfPyObjWrapper (https://graphics.pixar.com/usd/release/api/class_tf_py_obj_wrapper.html#details)

Provides a wrapper around boost::python::object that works correctly for the following basic operations regardless of the GIL state: default construction, copy construction, assignment, (in)equality comparison, hash_value(), and destruction.

None of those work correctly in the presence of an unlocked GIL for boost::python::object. This class only actually acquires the GIL for default construction, destruction and for some (in)equality comparisons. The other operations do not require taking the GIL.

[foisya-adsk]

It is worth noting that in the latest commit, in which we use only the wrapper class TfPyObjWrapper, there will be a small overhead when creating the TfPyObjWrapper and deleting it. For example:

`TfPyObjWrapper. TfAnyWeakPtr::_PointerHolder<Ptr>::GetPythonObject() `

will return a TfPyObjWrapper and a subsequent call to the Get() method of a TfPyObjWrapper object will return the boost::python::object.

Thus, on the one hand, there is small cost when going through TfPyObjWrapper, and, on the other hand, it is protected because of the pointer management of TfPyObjWrapper.

Is that OK?

[sunyab]

Is there a particular reason we need both TfPyBoostObjWrapper and TfPyObjWrapper? We weren't sure if there was something important we were missing here.

[foisya-adsk]

No, except that we we wanted to isolate the "proposed changes" as much as possible. We will consolidate the code under TfPyObjWrapper...

[foisya-adsk]

Martin and I just pushed an updated version with no extra TfPyBoostObjWrapper

[foisya-adsk]

@spiffmon Thanks to Gordon, my CLA status has been cleared.

[spiffmon]

Yes! We'll definitely get this in for the next release!

…

--spiff

On Thu, Mar 3, 2022 at 3:41 PM Andre Foisy ***@***.***> wrote: @spiffmon <https://github.com/spiffmon> Thanks to Gordon, my CLA status has been cleared. — Reply to this email directly, view it on GitHub <#1729 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPOU2BPZIDIFAAIP7WEWXDU6FE2VANCNFSM5LIERVAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[MrKepzie]

Just to report that building on Apple clang version 13.0.0 (clang-1300.0.29.30) I get a runtime crash where
the compile believes _getPyObj doesn't exist where, with this patch, it actually should. This leads to an offset in the function pointer table and makes the whole typeid system lead to undefined behavior.
I triple-checked it wasn't because of a mismatching includes and binaries and after rebuilds and verification the bug was still happening. The only way around for me was to revert this patch.

    _EqualPtrFunc _equalPtr;
        _MakeMutableFunc _makeMutable;
-#ifdef PXR_PYTHON_SUPPORT_ENABLED
        _GetPyObjFunc _getPyObj;
-#endif // PXR_PYTHON_SUPPORT_ENABLED
        _StreamOutFunc _streamOut;
        _GetTypeidFunc _getTypeid;

[sunyab]

@MrKepzie That sounds concerning. Were you running into this in a build with Python enabled or disabled? If you have more details it'd be great if you could file an issue so we can dig into it.

[MrKepzie]

Build without python. I was paranoid and thought it was picking up another dylib at runtime, but no this was the one I compiled. Will file an issue