Skip to content

[G-API] Fixed Coverity issues #21083

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

Merged
merged 11 commits into from
Nov 26, 2021
Merged

Conversation

OrestChura
Copy link
Contributor

@OrestChura OrestChura commented Nov 19, 2021

  • cv::detail::OpaqueKind m_kind field of VectorRef and OpaqueRef is now set to CV_UNKNOWN by default
  • added overload for saturate() function calls with the same input and output type
    • solved followed Win warnings about Fluid ConvertTo kernel with the same issue
  • sanitized resize value in ByteMemoryInStream::operator>> (std::string& str): std::string::resize() method expects std::size_t when used to get uint32_t instead
  • handled exception throws from ~GStreamingExecutor() by try-catch block (will address this issue further in the future)

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,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.4.1:20.04
build_image:Custom Win=openvino-2021.4.1
build_image:Custom Mac=openvino-2021.4.1

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

 - VectorRef&OpaqueRef m_kind = CV_UNKNOWN
 - added same-type overload for saturate()
 - sanitized resize value in ByteMemoryInStream::operator>> (std::string& str)
 - handled throws from ~GStreamingExecutor()
@@ -22,16 +22,13 @@ namespace cv { namespace gapi { namespace own {
//
//-----------------------------

template<typename DST, typename SRC>
template<typename DST, typename SRC, typename = cv::util::are_different_t<DST,SRC>>
static inline DST saturate(SRC x)
{
// only integral types please!
GAPI_DbgAssert(std::is_integral<DST>::value &&
Copy link
Contributor

Choose a reason for hiding this comment

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

can it be as static_assert ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answering question, no, it failed when I tried.

But I decided that, as I've already been using enable_if semantics, this condition should be there, too

Also, there were warnings on Windows compilers about the next saturate function overload, so I rewrote it with enable_ifs, too

@@ -928,7 +928,7 @@ IIStream& ByteMemoryInStream::operator>> (std::string& str) {
if (sz == 0u) {
str.clear();
} else {
str.resize(sz);
str.resize(std::size_t(sz));
Copy link
Contributor

Choose a reason for hiding this comment

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

static_cast by code style(?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you're right. Just really don't like to write it.
Maybe there are no such strict conventions in OCV, but you're still right :)

Copy link
Contributor

Choose a reason for hiding this comment

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

it is the place where other reviewers will pay attention, personally i dislike such writing long-symbol cast too, but it's still strong recommendation

GAPI_LOG_WARNING(NULL, message.str());
}
} else {
destruct();
Copy link
Contributor

@sivanov-work sivanov-work Nov 19, 2021

Choose a reason for hiding this comment

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

destruct throws error?

I checked code and found root cause of exception

    // Pull messages from the final queue to ensure completion
    Cmd cmd;
    while (!cv::util::holds_alternative<Stop>(cmd))
    {
        m_out_queue.pop(cmd);
    }
    GAPI_Assert(cv::util::holds_alternative<Stop>(cmd));   <----

And

  1. Assert looks redundant, because while-loop won't breaks otherwise
  2. this may be solved by more simplest way then using std::uncaught_exception
    a) separate stop() on stop() & wait_stop/shutdown(), and just call stop() in ~dtor because it looks like you should not extracts some cmd (because currently we're dropping them anyway) OR trying to wait it several cycles, then big WARNING printed
    b) do not throw Assert exception at all

From my perspective std::uncaught_exception is redundant and stirred ups and should be used in more dead-ended situations. But stop should not throw anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think your comment is highly reasonable, firstly about freeing destruction from exceptions: I'd rather dive a bit deeper & solve as many rootcauses as possible than simply cover all with try-catch.

Secondly, I like this stop() separation suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though, there are much more places where destructor can fall over.
The most obvious is assert in push() of concurrent_bounded_queue:

   if (m_capacity && m_capacity == m_data.size()) {
        // if there is a limit and it is reached, wait
        m_cond_full.wait(lock, [&](){return m_capacity > m_data.size();});
        GAPI_Assert(m_capacity > m_data.size());
    }

and maybe some others.

@dmatveev what do you think on this destructor issue?

Copy link
Contributor

@sivanov-work sivanov-work Nov 23, 2021

Choose a reason for hiding this comment

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

@OrestChura i see you point, but: from my perspective bounded_queue should not throws too because std::conditional_variable here retain its postcondition after .wait - so we do not need to debug standard c++ library and this check looks impossible according to bounded_queue design. So this throwing exception reason MUST be predictable, because this code doesn't interact with some user input or depends some OS/IO/RAM primitives setting but all behavior ensured by STD. I mean that it cannot throw sporadically and 100% determined from current code logic from 100% determined cases.
SO, there are should have been unit test for that "case" which ensure that exception must or must not to ensure guarantee that bounded_queue is consistent at all. But I'm pretty sure that it is useless here

What's about other exception: std::bad_alloc and so on - they are uncontrollable/unpredictable from us usually and/or based on user input or other system settings and SHOULD be checked in appropriate places OR ignored and falls into destructor and let program terminate in usual way, If it doesn't contradict with current policy

Example of predictable & controllable behavior:

try {
cin >> size; // user input `-1`
char* new char [size];  // usually throws std::bad_alloc on `-1` 
} catch (...)
{
cerr << "Too much size requested: " << size << std::endl;
}

Example of non predictable behavior

std::string aaa = "aaa";
foo(aaa);

foo (std::string bbb)  <---- copy may throw

The second example usually in not handled in most common applications and should be ignored

In result i assume that coverity doesn't dive into STD code and doesn't check throwing from vector push and so on and it's limited by application code only

@@ -200,7 +201,8 @@ class GStreamingExecutor final
public:
explicit GStreamingExecutor(std::unique_ptr<ade::Graph> &&g_model,
const cv::GCompileArgs &comp_args);
~GStreamingExecutor();
~GStreamingExecutor() noexcept(false); // throwing when a regular destruction fails only;
Copy link
Contributor

Choose a reason for hiding this comment

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

i thinks stop should not throw

Copy link
Contributor

@smirnov-alexey smirnov-alexey left a comment

Choose a reason for hiding this comment

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

LGTM

@sivanov-work
Copy link
Contributor

@OrestChura about destructor:
since PR called Fix Coverity issues from my opinion it would we fine if we just wrap stop() by try {}catch () {GAPI_LOG_WARNING(...); } without any further rethrowing - if it solves coverity issues
Then try to fix stop/queue in the next related PR

Copy link
Contributor

@sivanov-work sivanov-work left a comment

Choose a reason for hiding this comment

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

Please ask @anna-khakimova to review SIMD part

} catch (const std::logic_error& e) {
std::stringstream message;
message << "~GStreamingExecutor() threw exception with message '" << e.what() << "'\n";
GAPI_LOG_WARNING(NULL, message.str());
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

CV_ALWAYS_INLINE void run_convertto(DST *out, const SRC *in, const int length)
{
// manual SIMD if need rounding
GAPI_Assert(( std::is_same<SRC,float>::value ));
Copy link
Contributor

Choose a reason for hiding this comment

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

reject for double only? in this case i think we can change SFINAE from std::is_floating_point<SRC>::value to is_same<float>

Copy link
Contributor Author

@OrestChura OrestChura Nov 26, 2021

Choose a reason for hiding this comment

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

For this point, I assumed that if we utilize SFINAE there will be non-informative compile error; in the case of Assert, it will be clear why and where the problem is.
So I'd prefer leaving Assert

Copy link
Contributor

Choose a reason for hiding this comment

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

Any kind of error is better than no error, i agree

But when you spend ~30min to compile entire application and then got runtime error (or not got which is worse) about error which were managed to be found at compile time then fix it and spend 30 minutes again - it's disappointing.

So compile-time error should be found at compile time, I think. If you worried about nasty compilation message please consider static_assert(..., "Something horrible") here

Copy link
Contributor Author

@OrestChura OrestChura Nov 26, 2021

Choose a reason for hiding this comment

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

Makes sense. Made it static_assert, and added it in some other overloads too


// compute faster if no alpha no beta
if (alpha == 1 && beta == 0)
if (1 == alpha && 0 == beta)
Copy link
Contributor

Choose a reason for hiding this comment

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

because alpja & beta are floating point - i do not think it is right to compare them with straight to integer 1 & 0 is correct across using epsilon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

try {
if (state == State::READY || state == State::RUNNING)
stop();
} catch (const std::logic_error& e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

But why only logic_error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@anna-khakimova anna-khakimova left a comment

Choose a reason for hiding this comment

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

LGTM!

@alalek alalek merged commit 2deb38d into opencv:4.x Nov 26, 2021
@alalek alalek mentioned this pull request Dec 30, 2021
@alalek alalek mentioned this pull request Feb 22, 2022
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
…issues

[G-API] Fixed Coverity issues

* Fixed Coverity issues
 - VectorRef&OpaqueRef m_kind = CV_UNKNOWN
 - added same-type overload for saturate()
 - sanitized resize value in ByteMemoryInStream::operator>> (std::string& str)
 - handled throws from ~GStreamingExecutor()

* Catching exception by const ref

* Addressing Sergey's comments

* Applied enable_if semanitcs to saturate(x, round) too

* Removed uncaught_exception, made destructor noexcept back

* Split Fluid ConvertTo to multiple functions to avoid ifs; added CV_ALWAYS_INLINE

* Added FIXME to address throwings from stop()

* Fix standalone

* Addressing comments

* Guarded SIMD optimizations properly

* Removed excess parameter from simd_impl functions
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.

5 participants