[smart_holder] Wrong caching of overrides

[Trigve]

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

[rwgk]

Awesome, thanks! This looks great at first sight. I triggered the CI.

Important, so I can follow your work: please do not force push changes other than rebase-on-master. Ideally force-push immediately after a rebase-on-master, then use only non-force pushes for your work. When this PR is merged, the commits will be squashed, therefore it doesn't matter for that purpose how many commits we have here.

I still have to look in more detail, but I believe we need to turn this into a PR against master, moving the added code in tests/test_class_sh_inheritance.* to tests/test_class.* (first guess). — All fixes that are not in the added code on the smart_holder branch need to go to master first, otherwise it will become very difficult to merge the smart_holder branch into master, which I'm hoping to do very soon (needs some work fixing docs etc.).

[rwgk]

The ClangTidy failure looks very easy to fix, could you please do that?

https://github.com/pybind/pybind11/runs/4199226407?check_suite_focus=true

/__w/pybind11/pybind11/tests/test_embed/test_interpreter.cpp:54:17: error: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override,-warnings-as-errors]
    virtual int func() override { PYBIND11_OVERRIDE(int, test_derived, func); }
    ~~~~~~~~    ^
5405 warnings generated.

Edit: looks like 2 errors:

/__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:84:5: error: delete called on non-final 'pybind11_tests::class_sh_inheritance::test_derived' that has virtual functions but non-virtual destructor [clang-diagnostic-delete-non-abstract-non-virtual-dtor,-warnings-as-errors]
    delete static_cast<T *>(raw_ptr);
    ^

[rwgk]

I believe if you fix the ClangTidy errors we will have a lot less failures. All compilers except MSVC seem to be upset about the missing virtual destructor. I'll stop looking at the pile of errors. Let's just run the CI again after the two ClangTidy fixes.

[Trigve]

Awesome, thanks! This looks great at first sight. I triggered the CI.

Important, so I can follow your work: please do not force push changes other than rebase-on-master. Ideally force-push immediately after a rebase-on-master, then use only non-force pushes for your work. When this PR is merged, the commits will be squashed, therefore it doesn't matter for that purpose how many commits we have here.

I still have to look in more detail, but I believe we need to turn this into a PR against master, moving the added code in tests/test_class_sh_inheritance.* to tests/test_class.* (first guess). — All fixes that are not in the added code on the smart_holder branch need to go to master first, otherwise it will become very difficult to merge the smart_holder branch into master, which I'm hoping to do very soon (needs some work fixing docs etc.).

Ok. I've already have changes against master branch, but I the problem is, that in the smart_holder branch the tests would need to be modified (which is not a big deal).

So should I close this PR and open one against the master?

[rwgk]

Typing on my phone. Could you please just leave this PR as is for now and transfer this work with adjustments to a second new PR against master? (When we have the master PR stable we can revisit this one.)

…

On Sat, Nov 13, 2021 at 08:26 Trigve ***@***.***> wrote: Awesome, thanks! This looks great at first sight. I triggered the CI. Important, *so I can follow your work*: please do not force push changes other than rebase-on-master. Ideally force-push immediately after a rebase-on-master, then use only non-force pushes for your work. When this PR is merged, the commits will be squashed, therefore it doesn't matter for that purpose how many commits we have here. I still have to look in more detail, but I believe we need to turn this into a PR against master, moving the added code in tests/test_class_sh_inheritance.* to tests/test_class.* (first guess). — All fixes that are not in the added code on the smart_holder branch need to go to master first, otherwise it will become very difficult to merge the smart_holder branch into master, which I'm hoping to do very soon (needs some work fixing docs etc.). Ok. I've already have changes against master branch, but I the problem is, that in the smart_holder branch the tests would need to be modified (which is not a big deal). So should I close this PR and open one against the master? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3462 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAEBN5G3NG3U67JIVKLUL2GSVANCNFSM5H6Q4RZA> . 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>.

[Trigve]

Typing on my phone. Could you please just leave this PR as is for now and transfer this work with adjustments to a second new PR against master? (When we have the master PR stable we can revisit this one.)

Done as #3465

[rwgk]

Hi @Trigve, I just merged the master changes into the smart_holder branch. I believe this PR is obsolete now, but please let me know if I overlooked something.