Skip to content

Fix #20594 u/mat refcounts with exceptions#20602

Merged
opencv-pushbot merged 2 commits intoopencv:3.4from
diablodale:fix20594-refcounts-wiith-exceptions
Aug 25, 2021
Merged

Fix #20594 u/mat refcounts with exceptions#20602
opencv-pushbot merged 2 commits intoopencv:3.4from
diablodale:fix20594-refcounts-wiith-exceptions

Conversation

@diablodale
Copy link
Copy Markdown
Contributor

@diablodale diablodale commented Aug 24, 2021 •
edited by alalek
Loading

This is to fix #20594 by adding some exception handling in critical refcount areas of U/Mat. It does not fix them all everywhere, details in the issue. I can also squash into a single commit; left two separate commits so you can run the test case before/after the fix.

Tested on 3.x. If 4.x doesn't diverge too much should work there also. If you are not clear on how to merge into 4.x, please involve me.

Added test case to catch regression when OpenCL allocator is used. Can't think of a way to make more test cases as I need to cause allocators to throw exceptions.

Can we end work on 3.x? Not having C++11 is significantly preventing progress for reliable resource management/ownership with exceptions, allocators, and opencl.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
force_builders=Custom,Linux AVX2,Linux OpenCL
build_image:Custom=ubuntu:18.04
buildworker:Custom=linux-5
test_opencl:Custom=ON

build_image:Linux AVX2=ubuntu:18.04
buildworker:Linux AVX2=linux-3
test_opencl:Linux AVX2=ON

allow_multiple_commits=1

@diablodale diablodale changed the title Fix20594 u/mat refcounts with exceptions Fix #20594 u/mat refcounts with exceptions Aug 24, 2021
@asmorkalov asmorkalov requested a review from alalek August 25, 2021 07:21
alalek
alalek reviewed Aug 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for contribution!

Please take a look on comments below.

We don't have test for changes in Kernel implementation, right?

modules/core/test/test_umat.cpp Outdated
EXPECT_EQ(umat1.u->refcount, 0);

// reset state
umat1.u->handle = original_handle;
Copy link
Copy Markdown
Member

@alalek alalek Aug 25, 2021

Choose a reason for hiding this comment

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

ASSERT_EQ(NULL, umat1.u->handle) ?

Copy link
Copy Markdown
Contributor Author

@diablodale diablodale Aug 25, 2021

Choose a reason for hiding this comment

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

ok

modules/core/test/test_umat.cpp Outdated
}

UMat umat1(10, 10, CV_8UC1);
EXPECT_EQ(umat1.u->refcount, 0);
Copy link
Copy Markdown
Member

@alalek alalek Aug 25, 2021

Choose a reason for hiding this comment

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

_EQ checks in Google tests have this order of arguments: (expected, actual).
So they should be swapped here and below.

Copy link
Copy Markdown
Contributor Author

@diablodale diablodale Aug 25, 2021

Choose a reason for hiding this comment

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

ok

modules/core/src/ocl.cpp
Comment on lines +2788 to +2794
}
catch(...)
{
// limited by legacy before C++11
exceptionOccurred = true;
Copy link
Copy Markdown
Member

@alalek alalek Aug 25, 2021

Choose a reason for hiding this comment

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

It makes sense to dump exception message through logging.
See oclCleanupCallback below.

Copy link
Copy Markdown
Contributor Author

@diablodale diablodale Aug 25, 2021

Choose a reason for hiding this comment

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

ok

@diablodale
Copy link
Copy Markdown
Contributor Author

diablodale commented Aug 25, 2021

We don't have test for changes in Kernel implementation, right?

Right. I have not (yet?) thought of a way to cause exceptions in very specific locations, at very specific times, and catch both the good/bad state in test cases for all the changes.

  • void cv::ocl::Kernel::Impl::cleanupUMats is async and difficult for me to consistantly catch outcomes.
    Additionally, causing the assert at the exact time needed is difficult. I would need a way for all apis to work correctly until
    this async call and then deterministically cause deallocate() to fail. Neither the core OpenCL apis or OpenCV wrapper
    infrastructure has such a very specific simulation feature.
  • Similar with Mat::getUMat and the constructors. I need hooks in OpenCV apis or underlying OpenCL apis to simulate an
    expected failure/exception.

💡 I just thought of an idea for a future feature. There could be a class TestAllocator : public MatAllocator that returns dummy/test data and has flags/hooks that enable dev/testing simulated states/errors like this. Useful also with
fuzz testing where this TestAllocator could return random values and test if the calling code correctly handles erratic behavior.

@opencv-pushbot opencv-pushbot merged commit df83459 into opencv:3.4 Aug 25, 2021
@alalek alalek mentioned this pull request Aug 28, 2021
Merged
@alalek alalek mentioned this pull request Oct 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

Assignees

@alalek alalek

Labels

bug category: core category: ocl category: t-api cv::UMat related issues

Projects

None yet

Milestone

3.4.16

Development

Successfully merging this pull request may close these issues.

3 participants

@diablodale @alalek @opencv-pushbot