Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-11299: [Python] Fix invalid-offsetof warnings #9274

Closed
wants to merge 2 commits into from

Conversation

cyb70289
Copy link
Contributor

@cyb70289 cyb70289 commented Jan 20, 2021

Use unique_ptr to hold FunctionOptions derived classes instances to fix
invalid-offsetof python build warnings.

~/arrow/python/build/temp.linux-x86_64-3.6/_compute.cpp:26034:146:
warning: offsetof within non-standard-layout type ‘__pyx_obj_7pyarrow_8_compute__CastOptions’ is undefined [-Winvalid-offsetof]
x_type_7pyarrow_8_compute__CastOptions.tp_weaklistoffset = offsetof(struct __pyx_obj_7pyarrow_8_compute__CastOptions, __pyx_base.__pyx_base.weakref);

@github-actions
Copy link

@cyb70289
Copy link
Contributor Author

cyb70289 commented Jan 20, 2021

All debug build failed due to this change. FunctionOptions is supposed to be polymorphic, but looks not necessary.
Would like to hear comments before moving on.
@bkietz @pitrou

@bkietz
Copy link
Member

bkietz commented Jan 20, 2021

@cyb70289 arrow::dataset::Expression::Call::options requires that virtual destructor since a shared_ptr<FunctionOptions> is used to store subclasses of FunctionOptions. Removing this virtual destructor results in a resource leak since the derived classes' (implicit) destructors are not being called, leaking data members such as arrow::compute::SetLookupOptions::value_set which are not destroyed.

@bkietz
Copy link
Member

bkietz commented Jan 20, 2021

I think the best way to resolve this issue is to avoid direct instances of derived classes of FunctionOptions in Cython, since Cython inappropriately applies offsetof. For example, note that the warning is emitted for MinMaxOptions but not for SetLookupOptions (where the options are held by unique_ptr rather than being a direct member).

I'd recommend the following approach: rewrite _compute.pyx::FunctionOptions to hold a shared_ptr[CFunctionOptions], and let the cdef classes acquire a non owning pointer to the appropriate FunctionOptions subclass. For example:

_compute.pxd

cdef class FunctionOptions(_Weakrefable):
    cdef:
        shared_ptr[CFunctionOptions] options

    cdef const CFunctionOptions* get_options(self) except NULL

_compute.pyx

cdef class _CastOptions(FunctionOptions):
    cdef:
        const CCastOptions* cast_options

    __slots__ = ()  # avoid mistakingly creating attributes

    cdef const CFunctionOptions* get_options(self) except NULL:
        return self.cast_options

@cyb70289
Copy link
Contributor Author

@cyb70289 arrow::dataset::Expression::Call::options requires that virtual destructor since a shared_ptr<FunctionOptions> is used to store subclasses of FunctionOptions. Removing this virtual destructor results in a resource leak since the derived classes' (implicit) destructors are not being called, leaking data members such as arrow::compute::SetLookupOptions::value_set which are not destroyed.

Thanks @bkietz , I missed this point.
Will update according to your comment.

Use unique_ptr to hold FunctionOptions derived classes instances to fix
`invalid-offsetof` python build warnings.

~/arrow/python/build/temp.linux-x86_64-3.6/_compute.cpp:26034:146:
warning: offsetof within non-standard-layout type ‘__pyx_obj_7pyarrow_8_compute__CastOptions’ is undefined [-Winvalid-offsetof]
 x_type_7pyarrow_8_compute__CastOptions.tp_weaklistoffset = offsetof(struct __pyx_obj_7pyarrow_8_compute__CastOptions, __pyx_base.__pyx_base.__weakref__);
@cyb70289 cyb70289 changed the title ARROW-11299: [C++][Python] Fix invalid-offsetof warnings ARROW-11299: [Python] Fix invalid-offsetof warnings Jan 22, 2021
python/pyarrow/_compute.pyx Outdated Show resolved Hide resolved
Copy link
Member

@bkietz bkietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@bkietz bkietz closed this in b44a4ad Jan 22, 2021
@cyb70289 cyb70289 deleted the offsetof branch January 23, 2021 01:34
kszucs pushed a commit that referenced this pull request Jan 25, 2021
Use unique_ptr to hold FunctionOptions derived classes instances to fix
`invalid-offsetof` python build warnings.

~/arrow/python/build/temp.linux-x86_64-3.6/_compute.cpp:26034:146:
warning: offsetof within non-standard-layout type ‘__pyx_obj_7pyarrow_8_compute__CastOptions’ is undefined [-Winvalid-offsetof]
 x_type_7pyarrow_8_compute__CastOptions.tp_weaklistoffset = offsetof(struct __pyx_obj_7pyarrow_8_compute__CastOptions, __pyx_base.__pyx_base.__weakref__);

Closes #9274 from cyb70289/offsetof

Authored-by: Yibo Cai <yibo.cai@arm.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
Use unique_ptr to hold FunctionOptions derived classes instances to fix
`invalid-offsetof` python build warnings.

~/arrow/python/build/temp.linux-x86_64-3.6/_compute.cpp:26034:146:
warning: offsetof within non-standard-layout type ‘__pyx_obj_7pyarrow_8_compute__CastOptions’ is undefined [-Winvalid-offsetof]
 x_type_7pyarrow_8_compute__CastOptions.tp_weaklistoffset = offsetof(struct __pyx_obj_7pyarrow_8_compute__CastOptions, __pyx_base.__pyx_base.__weakref__);

Closes apache#9274 from cyb70289/offsetof

Authored-by: Yibo Cai <yibo.cai@arm.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
Use unique_ptr to hold FunctionOptions derived classes instances to fix
`invalid-offsetof` python build warnings.

~/arrow/python/build/temp.linux-x86_64-3.6/_compute.cpp:26034:146:
warning: offsetof within non-standard-layout type ‘__pyx_obj_7pyarrow_8_compute__CastOptions’ is undefined [-Winvalid-offsetof]
 x_type_7pyarrow_8_compute__CastOptions.tp_weaklistoffset = offsetof(struct __pyx_obj_7pyarrow_8_compute__CastOptions, __pyx_base.__pyx_base.__weakref__);

Closes apache#9274 from cyb70289/offsetof

Authored-by: Yibo Cai <yibo.cai@arm.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants