From ee3e3f46f797a744456693c756c036fd451baf0a Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Thu, 26 Oct 2023 17:46:34 -0400 Subject: [PATCH 1/3] Fixing the static py ptrs with the trivial destructor --- src/clp_ffi_py/PyObjectUtils.hpp | 21 +++++++++++++++++++ src/clp_ffi_py/Py_utils.cpp | 4 ++-- src/clp_ffi_py/ir/native/PyDecoder.cpp | 2 +- src/clp_ffi_py/ir/native/PyDecoder.hpp | 2 +- src/clp_ffi_py/ir/native/PyDecoderBuffer.cpp | 4 ++-- src/clp_ffi_py/ir/native/PyDecoderBuffer.hpp | 4 ++-- .../ir/native/PyFourByteEncoder.cpp | 2 +- .../ir/native/PyFourByteEncoder.hpp | 2 +- src/clp_ffi_py/ir/native/PyLogEvent.cpp | 2 +- src/clp_ffi_py/ir/native/PyLogEvent.hpp | 2 +- src/clp_ffi_py/ir/native/PyMetadata.cpp | 2 +- src/clp_ffi_py/ir/native/PyMetadata.hpp | 2 +- src/clp_ffi_py/ir/native/PyQuery.cpp | 4 ++-- src/clp_ffi_py/ir/native/PyQuery.hpp | 4 ++-- 14 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/clp_ffi_py/PyObjectUtils.hpp b/src/clp_ffi_py/PyObjectUtils.hpp index fd6885ec..c64c20bf 100644 --- a/src/clp_ffi_py/PyObjectUtils.hpp +++ b/src/clp_ffi_py/PyObjectUtils.hpp @@ -18,6 +18,17 @@ class PyObjectDeleter { void operator()(PyObjectType* ptr) { Py_XDECREF(reinterpret_cast(ptr)); } }; +/** + * An empty deleter that has an empty implementation to ensure object trivial + * destruction. + * @tparam PyObjectType + */ +template +class PyObjectTrivialDeleter { +public: + void operator()(PyObjectType* ptr) {} +}; + /** * A type of smart pointer that maintains a reference to a Python object for the * duration of its lifetime. @@ -25,5 +36,15 @@ class PyObjectDeleter { */ template using PyObjectPtr = std::unique_ptr>; + +/** + * A type of smart pointer that holds a static Python object raw pointer. For + * static/global variables, the destructor will be executed after the Python + * interpreter exits. Therefore, the destructor is set to empty to avoid unsafe + * memory operations. + * @tparam PyObjectType + */ +template +using PyObjectStaticPtr = std::unique_ptr>; } // namespace clp_ffi_py #endif // CLP_FFI_PY_PY_OBJECT_PTR_HPP diff --git a/src/clp_ffi_py/Py_utils.cpp b/src/clp_ffi_py/Py_utils.cpp index 0ffe837d..231c9972 100644 --- a/src/clp_ffi_py/Py_utils.cpp +++ b/src/clp_ffi_py/Py_utils.cpp @@ -7,10 +7,10 @@ namespace clp_ffi_py { namespace { constexpr char const* const cPyFuncNameGetFormattedTimestamp{"get_formatted_timestamp"}; -PyObjectPtr Py_func_get_formatted_timestamp; +PyObjectStaticPtr Py_func_get_formatted_timestamp; constexpr char const* const cPyFuncNameGetTimezoneFromTimezoneId{"get_timezone_from_timezone_id"}; -PyObjectPtr Py_func_get_timezone_from_timezone_id; +PyObjectStaticPtr Py_func_get_timezone_from_timezone_id; /** * Wrapper of PyObject_CallObject. diff --git a/src/clp_ffi_py/ir/native/PyDecoder.cpp b/src/clp_ffi_py/ir/native/PyDecoder.cpp index 1b4f4a6a..05128565 100644 --- a/src/clp_ffi_py/ir/native/PyDecoder.cpp +++ b/src/clp_ffi_py/ir/native/PyDecoder.cpp @@ -86,7 +86,7 @@ PyType_Spec PyDecoder_type_spec{ }; } // namespace -PyObjectPtr PyDecoder::m_py_type{nullptr}; +PyObjectStaticPtr PyDecoder::m_py_type{nullptr}; auto PyDecoder::module_level_init(PyObject* py_module) -> bool { static_assert(std::is_trivially_destructible()); diff --git a/src/clp_ffi_py/ir/native/PyDecoder.hpp b/src/clp_ffi_py/ir/native/PyDecoder.hpp index d7fb1531..81a5f1cc 100644 --- a/src/clp_ffi_py/ir/native/PyDecoder.hpp +++ b/src/clp_ffi_py/ir/native/PyDecoder.hpp @@ -25,7 +25,7 @@ class PyDecoder { private: PyObject_HEAD; - static PyObjectPtr m_py_type; + static PyObjectStaticPtr m_py_type; }; } // namespace clp_ffi_py::ir::native diff --git a/src/clp_ffi_py/ir/native/PyDecoderBuffer.cpp b/src/clp_ffi_py/ir/native/PyDecoderBuffer.cpp index 0ea7c302..4c9dd229 100644 --- a/src/clp_ffi_py/ir/native/PyDecoderBuffer.cpp +++ b/src/clp_ffi_py/ir/native/PyDecoderBuffer.cpp @@ -350,8 +350,8 @@ auto PyDecoderBuffer::test_streaming(uint32_t seed) -> PyObject* { ); } -PyObjectPtr PyDecoderBuffer::m_py_type{nullptr}; -PyObjectPtr PyDecoderBuffer::m_py_incomplete_stream_error{nullptr}; +PyObjectStaticPtr PyDecoderBuffer::m_py_type{nullptr}; +PyObjectStaticPtr PyDecoderBuffer::m_py_incomplete_stream_error{nullptr}; auto PyDecoderBuffer::get_py_type() -> PyTypeObject* { return m_py_type.get(); diff --git a/src/clp_ffi_py/ir/native/PyDecoderBuffer.hpp b/src/clp_ffi_py/ir/native/PyDecoderBuffer.hpp index 98bec3cd..1de228e9 100644 --- a/src/clp_ffi_py/ir/native/PyDecoderBuffer.hpp +++ b/src/clp_ffi_py/ir/native/PyDecoderBuffer.hpp @@ -221,8 +221,8 @@ class PyDecoderBuffer { size_t m_num_decoded_message; bool m_py_buffer_protocol_enabled; - static PyObjectPtr m_py_type; - static PyObjectPtr m_py_incomplete_stream_error; + static PyObjectStaticPtr m_py_type; + static PyObjectStaticPtr m_py_incomplete_stream_error; }; } // namespace clp_ffi_py::ir::native #endif diff --git a/src/clp_ffi_py/ir/native/PyFourByteEncoder.cpp b/src/clp_ffi_py/ir/native/PyFourByteEncoder.cpp index 12110dc0..494a81a2 100644 --- a/src/clp_ffi_py/ir/native/PyFourByteEncoder.cpp +++ b/src/clp_ffi_py/ir/native/PyFourByteEncoder.cpp @@ -129,7 +129,7 @@ PyType_Spec PyFourByteEncoder_type_spec{ }; } // namespace -PyObjectPtr PyFourByteEncoder::m_py_type{nullptr}; +PyObjectStaticPtr PyFourByteEncoder::m_py_type{nullptr}; auto PyFourByteEncoder::module_level_init(PyObject* py_module) -> bool { static_assert(std::is_trivially_destructible()); diff --git a/src/clp_ffi_py/ir/native/PyFourByteEncoder.hpp b/src/clp_ffi_py/ir/native/PyFourByteEncoder.hpp index 3fa8ae60..4b848631 100644 --- a/src/clp_ffi_py/ir/native/PyFourByteEncoder.hpp +++ b/src/clp_ffi_py/ir/native/PyFourByteEncoder.hpp @@ -26,7 +26,7 @@ class PyFourByteEncoder { private: PyObject_HEAD; - static PyObjectPtr m_py_type; + static PyObjectStaticPtr m_py_type; }; } // namespace clp_ffi_py::ir::native #endif diff --git a/src/clp_ffi_py/ir/native/PyLogEvent.cpp b/src/clp_ffi_py/ir/native/PyLogEvent.cpp index 3273cc0c..3e8c8310 100644 --- a/src/clp_ffi_py/ir/native/PyLogEvent.cpp +++ b/src/clp_ffi_py/ir/native/PyLogEvent.cpp @@ -486,7 +486,7 @@ auto PyLogEvent::init( return true; } -PyObjectPtr PyLogEvent::m_py_type{nullptr}; +PyObjectStaticPtr PyLogEvent::m_py_type{nullptr}; auto PyLogEvent::get_py_type() -> PyTypeObject* { return m_py_type.get(); diff --git a/src/clp_ffi_py/ir/native/PyLogEvent.hpp b/src/clp_ffi_py/ir/native/PyLogEvent.hpp index cb49d8b9..92fb9dcd 100644 --- a/src/clp_ffi_py/ir/native/PyLogEvent.hpp +++ b/src/clp_ffi_py/ir/native/PyLogEvent.hpp @@ -144,7 +144,7 @@ class PyLogEvent { LogEvent* m_log_event; PyMetadata* m_py_metadata; - static PyObjectPtr m_py_type; + static PyObjectStaticPtr m_py_type; }; } // namespace clp_ffi_py::ir::native #endif // CLP_FFI_PY_PY_LOG_EVENT_HPP diff --git a/src/clp_ffi_py/ir/native/PyMetadata.cpp b/src/clp_ffi_py/ir/native/PyMetadata.cpp index e5607adc..1d4b5ea8 100644 --- a/src/clp_ffi_py/ir/native/PyMetadata.cpp +++ b/src/clp_ffi_py/ir/native/PyMetadata.cpp @@ -263,7 +263,7 @@ auto PyMetadata::init_py_timezone() -> bool { return true; } -PyObjectPtr PyMetadata::m_py_type{nullptr}; +PyObjectStaticPtr PyMetadata::m_py_type{nullptr}; auto PyMetadata::get_py_type() -> PyTypeObject* { return m_py_type.get(); diff --git a/src/clp_ffi_py/ir/native/PyMetadata.hpp b/src/clp_ffi_py/ir/native/PyMetadata.hpp index afcea8c7..ba31ee7f 100644 --- a/src/clp_ffi_py/ir/native/PyMetadata.hpp +++ b/src/clp_ffi_py/ir/native/PyMetadata.hpp @@ -117,7 +117,7 @@ class PyMetadata { Metadata* m_metadata; PyObject* m_py_timezone; - static PyObjectPtr m_py_type; + static PyObjectStaticPtr m_py_type; }; } // namespace clp_ffi_py::ir::native #endif // CLP_FFI_PY_PY_METADATA_HPP diff --git a/src/clp_ffi_py/ir/native/PyQuery.cpp b/src/clp_ffi_py/ir/native/PyQuery.cpp index 0d1cb304..35863d28 100644 --- a/src/clp_ffi_py/ir/native/PyQuery.cpp +++ b/src/clp_ffi_py/ir/native/PyQuery.cpp @@ -636,8 +636,8 @@ auto PyQuery::init( return true; } -PyObjectPtr PyQuery::m_py_type{nullptr}; -PyObjectPtr PyQuery::m_py_wildcard_query_type{nullptr}; +PyObjectStaticPtr PyQuery::m_py_type{nullptr}; +PyObjectStaticPtr PyQuery::m_py_wildcard_query_type{nullptr}; auto PyQuery::get_py_type() -> PyTypeObject* { return m_py_type.get(); diff --git a/src/clp_ffi_py/ir/native/PyQuery.hpp b/src/clp_ffi_py/ir/native/PyQuery.hpp index 154068e8..58ebf274 100644 --- a/src/clp_ffi_py/ir/native/PyQuery.hpp +++ b/src/clp_ffi_py/ir/native/PyQuery.hpp @@ -80,8 +80,8 @@ class PyQuery { PyObject_HEAD; Query* m_query; - static PyObjectPtr m_py_type; - static PyObjectPtr m_py_wildcard_query_type; + static PyObjectStaticPtr m_py_type; + static PyObjectStaticPtr m_py_wildcard_query_type; }; } // namespace clp_ffi_py::ir::native #endif From 6013a4f5123f39a4d7097d6dd2d857181ee16f14 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Sat, 28 Oct 2023 18:14:52 -0400 Subject: [PATCH 2/3] Update comments --- src/clp_ffi_py/PyObjectUtils.hpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/clp_ffi_py/PyObjectUtils.hpp b/src/clp_ffi_py/PyObjectUtils.hpp index c64c20bf..f41f560e 100644 --- a/src/clp_ffi_py/PyObjectUtils.hpp +++ b/src/clp_ffi_py/PyObjectUtils.hpp @@ -38,10 +38,14 @@ template using PyObjectPtr = std::unique_ptr>; /** - * A type of smart pointer that holds a static Python object raw pointer. For - * static/global variables, the destructor will be executed after the Python - * interpreter exits. Therefore, the destructor is set to empty to avoid unsafe - * memory operations. + * A smart pointer to be used for raw Python object pointers that have static + * storage duration. It holds a reference of the underlying Python object. + * Compared to PyObjectPtr, when destructed, this smart pointer does not + * decrease the reference count of the contained Python object. Since this + * pointer has static storage duration, it's possible that the Python + * interpreter exits before this pointer's destructor is called; attempting to + * decrement the reference count (maintained by the interpreter) in this + * situation would lead to undefined behaviour. * @tparam PyObjectType */ template From c870c6d3835eb63c735822e65f65da12af2dfd67 Mon Sep 17 00:00:00 2001 From: LinZhihao-723 Date: Sun, 29 Oct 2023 20:07:55 -0400 Subject: [PATCH 3/3] nullptr initilaize --- src/clp_ffi_py/Py_utils.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/clp_ffi_py/Py_utils.cpp b/src/clp_ffi_py/Py_utils.cpp index 231c9972..0a22c981 100644 --- a/src/clp_ffi_py/Py_utils.cpp +++ b/src/clp_ffi_py/Py_utils.cpp @@ -7,10 +7,10 @@ namespace clp_ffi_py { namespace { constexpr char const* const cPyFuncNameGetFormattedTimestamp{"get_formatted_timestamp"}; -PyObjectStaticPtr Py_func_get_formatted_timestamp; +PyObjectStaticPtr Py_func_get_formatted_timestamp{nullptr}; constexpr char const* const cPyFuncNameGetTimezoneFromTimezoneId{"get_timezone_from_timezone_id"}; -PyObjectStaticPtr Py_func_get_timezone_from_timezone_id; +PyObjectStaticPtr Py_func_get_timezone_from_timezone_id{nullptr}; /** * Wrapper of PyObject_CallObject.