[master] Wrong caching of overrides by Trigve · Pull Request #3465 · pybind/pybind11

Trigve · 2021-11-13T16:35:39Z

Description

There was a problem when the python type, which was stored in override
cache for C++ functions, was destroyed and the record wasn't removed from the
override cache. Therefor, dangling pointer was stored there. Then when the
memory was reused and new type was allocated at the given address and the
method with the same name (as previously stored in the cache) was actually
overridden in python, it would wrongly find it in the override cache for C++
functions and therefor override from python wouldn't be called.
The fix is to erase the type from the override cache when the type is destroyed.

Closes: #3369

Suggested changelog entry:

Fix caching of the C++ overrides

There was a problem when the python type, which was stored in override cache for C++ functions, was destroyed and the record wasn't removed from the override cache. Therefor, dangling pointer was stored there. Then when the memory was reused and new type was allocated at the given address and the method with the same name (as previously stored in the cache) was actually overridden in python, it would wrongly find it in the override cache for C++ functions and therefor override from python wouldn't be called. The fix is to erase the type from the override cache when the type is destroyed.

rwgk · 2021-11-13T16:39:13Z

FYI I'll be on and off a lot today but will keep an eye on this.

rwgk

Suggestions aiming to nudge things to the right places.

rwgk · 2021-11-13T17:49:36Z

        weakref((PyObject *) type, cpp_function([type](handle wr) {
            get_internals().registered_types_py.erase(type);
+
+            // Actually just `std::erase_if`, but that's only available in C++20


What do you think about something like:

erase_if(cache.begin(), cache.end(), [](const xxx& v){ return yyy; });

here and implementing erase_if in detail/common.h (bottom), using the "(2) equivalent to" code from here:

https://en.cppreference.com/w/cpp/container/vector/erase2

auto it = std::remove_if(c.begin(), c.end(), pred); auto r = std::distance(it, c.end()); c.erase(it, c.end()); return r;

Later, when we introduce PYBIND11_CPP20 we can use std::erase_if directly. (Maybe just leave something like // TODO PYBIND11_CPP20: std::erase_if for now.)

The code for the erasure was actually copied and modified from

pybind11/include/pybind11/detail/class.h

Line 218 in e450eb6

// Actually just `std::erase_if`, but that's only available in C++20

It would be better to create a helper function as you suggested, but I didn't want to add it in this PR as it isn't isn't connected to the issue. I think it would be better to add it as a separate commit.

rwgk · 2021-11-13T17:53:13Z

+    test_derived(test_derived const &Copy) = delete;
+};
+
+class py_test_derived : public test_derived {


Personal preference for clarity (in case you like that, too): class test_derived_trampoline : test_derived

Renamed to test_override_cache_helper_trampoline

rwgk · 2021-11-13T17:56:04Z

    std::vector<int> vec;
 };

+class test_derived {


At 2nd look, test_virtual_functions.* appears to be a significantly better fit, because this is closely tied to trampolines.

test_derived is a very generic name, something a bit more tailored to the purpose would be ideal (easier to pin-point and relate to what's being exerised). Maybe exercise_override_cache or test_override_cache_helper ...?

Moved and renamed to test_override_cache_helper

rwgk · 2021-11-13T18:02:38Z

@@ -0,0 +1,18 @@
+# -*- coding: utf-8 -*-


Does test_trampoline make sense here? To leave it a little bit open (for the future) but not a widely as "derived".

Renamed to test_trampoline.py

Rename the classes and files so they're no too generic. Also, better place to test the stuff is in test_virtual_functions.cpp/.py as we're basically testing the virtual functions/trampolines.

rwgk · 2021-11-13T18:39:13Z

I cannot look at the code right now, but from the comments it sounds good. If you haven't done already, could you please add a comment here: todo consolidate with xxx in class.h? Also request review from Skylion007

…

On Sat, Nov 13, 2021 at 10:12 Trigve ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/pybind11/pybind11.h <#3465 (comment)>: > @@ -1979,6 +1979,16 @@ inline std::pair<decltype(internals::registered_types_py)::iterator, bool> all_t // gets destroyed: weakref((PyObject *) type, cpp_function([type](handle wr) { get_internals().registered_types_py.erase(type); + + // Actually just `std::erase_if`, but that's only available in C++20 The code for the erasure was actually copied and modified from https://github.com/pybind/pybind11/blob/e450eb62c21c8518c1988c3b17bc5793ea347756/include/pybind11/detail/class.h#L218 It would be better to create a helper function as you suggested, but I didn't want to add it in this PR as it isn't isn't connected to the issue. I think it would be better to add it as a separate commit. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3465 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAA6P4EP245EVCQQSSLUL2TCFANCNFSM5H6ZWIQQ> . 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>.

for more information, see https://pre-commit.ci

Trigve · 2021-11-13T19:02:32Z

I cannot look at the code right now, but from the comments it sounds good. If you haven't done already, could you please add a comment here: todo consolidate with xxx in class.h? Also request review from Skylion007

How do I request a review? I've read the docs but I don't have option to add one.

rwgk · 2021-11-13T19:05:05Z

Upper right corner in the main PR view is where I'd expect it. (I can add Aaron later if it's not there for you.)

…

On Sat, Nov 13, 2021 at 11:02 Trigve ***@***.***> wrote: I cannot look at the code right now, but from the comments it sounds good. If you haven't done already, could you please add a comment here: todo consolidate with xxx in class.h? Also request review from Skylion007 How do I request a review? I've read the docs but I don't have option to add one. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3465 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAG6OFRYA3TQ46OK4NTUL2Y5FANCNFSM5H6ZWIQQ> . 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>.

rwgk · 2021-11-14T18:26:33Z

@henryiii do you want to take a look, too?

henryiii

Looks reasonable to me.

rwgk · 2021-11-15T19:21:28Z

Thanks @Trigve for the great unit tests!

…y PR pybind#3465.

Trigve requested a review from henryiii as a code owner November 13, 2021 16:35

Trigve mentioned this pull request Nov 13, 2021

[smart_holder] Wrong caching of overrides #3462

Closed

test: Pass by const ref instead of by value (clang-tidy)

3eaef47

rwgk reviewed Nov 13, 2021

View reviewed changes

test: Rename classes and move to different files

01e1efa

Rename the classes and files so they're no too generic. Also, better place to test the stuff is in test_virtual_functions.cpp/.py as we're basically testing the virtual functions/trampolines.

Trigve and others added 2 commits November 13, 2021 19:52

Add TODO for erasure code

215dcfa

[pre-commit.ci] auto fixes from pre-commit.com hooks

d2b7a67

for more information, see https://pre-commit.ci

rwgk requested a review from Skylion007 November 13, 2021 22:22

Skylion007 approved these changes Nov 14, 2021

View reviewed changes

rwgk approved these changes Nov 14, 2021

View reviewed changes

henryiii approved these changes Nov 15, 2021

View reviewed changes

Skylion007 merged commit afdc09d into pybind:master Nov 15, 2021

github-actions Bot added the needs changelog Possibly needs a changelog entry label Nov 15, 2021

rwgk added a commit to rwgk/pybind11 that referenced this pull request Nov 15, 2021

Using PYBIND11_SH_DEF for test_override_cache_helper introduced b…

8a7643f

…y PR pybind#3465.

Trigve deleted the override_cache_master branch November 16, 2021 15:07

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 21, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3465

Closed

Conversation

Trigve commented Nov 13, 2021

Description

Suggested changelog entry:

Uh oh!

rwgk commented Nov 13, 2021

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

Trigve Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

rwgk Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

Trigve Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwgk Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

Trigve Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

rwgk Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

Trigve Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

rwgk commented Nov 13, 2021 via email

Uh oh!

Trigve commented Nov 13, 2021

Uh oh!

rwgk commented Nov 13, 2021 via email

Uh oh!

rwgk commented Nov 14, 2021

Uh oh!

henryiii left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Trigve Nov 13, 2021 •

edited

Loading