valueflow.cpp: issue a debug message about analysis of condition expressions being disabled

[firewave]

No description provided.

[firewave]

Part of valueflow was just disabled by default without providing any feedback about it. As this message will probably happen in any invocation of the tool this shows that this change is extremely questionable.

Without this feedback it might also indicate that some issues were fixed which indeed they were not. So this is bound to wreck havoc in lots of ways.

[firewave]

Discovered by assuming that https://trac.cppcheck.net/ticket/12125 and https://trac.cppcheck.net/ticket/10949 were fixed which indeed they were not.

[firewave]

CC @danmar @orbitcowboy

[firewave]

IMO exhaustive need to be the default so somebody running this the first time does not get any warnings about this. If they have issues with the run-time then they should downgrade it to normal. Having these messages will most likely just lead to people suppressing them and never looking at them ever again since it is just too much of a hassle. It also impacts integrations into system since these messages need to be handled suppressed and will likely cause failures in that transition.

Throwing random setting of the exhaustive mode so the unit tests pass is also more than just a red flag to me.

This similarly applies to the missingInclude stuff - I dropped the ball on that and seriously need to follow-up on that. But with so much other stuff and things recently progressing extremely slow or not at all is not helping.

And with the checkersReport message appearing on any analysis it seems that we are just throwing unnecessary useless information at the user (I don't even understand how this would give the user anything useful at all). That should also be behind a command-line switch.

[orbitcowboy]

IMO exhaustive need to be the default so somebody running this the first time does not get any warnings about this. If they have issues with the run-time then they should downgrade it to normal. Having these messages will most likely just lead to people suppressing them and never looking at them ever again since it is just too much of a hassle. It also impacts integrations into system since these messages need to be handled suppressed and will likely cause failures in that transition.

Throwing random setting of the exhaustive mode so the unit tests pass is also more than just a red flag to me.

This similarly applies to the missingInclude stuff - I dropped the ball on that and seriously need to follow-up on that. But with so much other stuff and things recently progressing extremely slow or not at all is not helping.

And with the checkersReport message appearing on any analysis it seems that we are just throwing unnecessary useless information at the user (I don't even understand how this would give the user anything useful at all). That should also be behind a command-line switch.

This is my opinion about the exhaustive flag:

The exhaustive option should be switched on by default. Only if there are performance issues, users can decide to set it down to normal, while they need to be informed on the impact with an informative message that valueflow ist partly active and there are potential false negatives as a conequence.

This would improve usability and results for users that are using Cppcheck the first time. In addition, it reduces the risk that some important issues are not reportet right at the first scanning of a project.

[firewave]

Disabling it by default also completely killed the valueFlowBailoutIncompleteVar data collection in daca: http://cppcheck1.osuosl.org:8000/value_flow_bailout_incomplete_var.html.

[firewave]

I also think we should rename the check level normal to reduced so it better reflects what it actually does.

And the current exhaustive is also a bit misleading as it might indicate that we are doing more than necessary - which isn't the case as it is the mode which should be the default - full would seem more appropriate - although it might not be as we are still applying limits in that mode - so maybe that should be normal.

This just highlights that this isn't well thought through. We probably need to introduce a new option which is properly defined.

We could also include a mode which does no ValueFlow at all and call that shallow (or similar). That could be used for a two stage approach. Do a quick check with that in a CI step or your IDE (having a fast response there makes sense) and do a fuller one at a later stage. That way you could get faster result from the CI if you run that as an early step. The problem is that disabling the ValueFlow introduces false positives, but maybe those can be addressed easily.

We should also expose all the tuning parameters for the individual levels via the cppcheck.cfg. And maybe even remove the options for that from the command-line.

[danmar]

The exhaustive option should be switched on by default. Only if there are performance issues, users can decide to set it down to normal, while they need to be informed on the impact with an informative message that valueflow ist partly active and there are potential false negatives as a conequence.

Can you please let me know how they will learn about --check-level=normal when they have performance issues? Would we produce a information message in this situation?

I saw the danger that users would run into problems and don't know how to do solve it. They could get the idea that we are too slow..

Performance is a topic that is quite frequently discussed with Cppcheck customers. If we can be significantly faster than competitors its a serious advantage.

I don't have a strong opinion about which check level is default.

[danmar]

And with the checkersReport message appearing on any analysis it seems that we are just throwing unnecessary useless information at the user (I don't even understand how this would give the user anything useful at all). That should also be behind a command-line switch.

This was added for the safety certification. It's not the intention that it will be shown for normal users. But yes if you always use --enable=information then it is shown..

If you find it pointless I am not against that it's only shown if --safety option is used.

[danmar]

We could also include a mode which does no ValueFlow at all and call that shallow (or similar).

Well I fear that such mode would have a drastic number of false negatives. If it really means "no valueflow at all" then we won't find any division by zero/null pointer dereference/etc at all.

[orbitcowboy]

Can you please let me know how they will learn about --check-level=normal when they have performance issues? Would we produce a information message in this situation?

By reading the documentation

I saw the danger that users would run into problems and don't know how to do solve it. They could get the idea that we are too slow..

Performance is a topic that is quite frequently discussed with Cppcheck customers. If we can be significantly faster than competitors its a serious advantage.

I don't have a strong opinion about which check level is default.

I don't mind either, as long as there's an option to switch between the different levels. The crucial aspect from the user's perspective is understanding what to anticipate when utilizing the '--check-level=normal' parameter.

[danmar]

By reading the documentation

I fear that most users does not read the documentation much and will not learn about this solution to their problem then.

My idea with "exhaustive" was that it can be more time consuming and more targeted to i.e. a CI job where speed is not critical. Many customers wants to integrate Cppcheck in their IDE and speed is important in that situation.

[danmar]

And the current exhaustive is also a bit misleading as it might indicate that we are doing more than necessary - which isn't the case as it is the mode which should be the default - full would seem more appropriate - although it might not be as we are still applying limits in that mode - so maybe that should be normal.

The names are not written in stone we can change it. We introduced the names couple of months ago so anybody who started using our new flags already should be capable of changing their scripts again.

Would "comprehensive" feel better than "exhaustive"? Analysis is not 100% complete in all cases.

[firewave]

Sorry for the late reply but I wanted to finish up some of my other stuff before I dive into this but that didn't work out as well as I thought...

I will write a more in-depth comment later.

Leaving the user aside for now this might cause havoc for QA and development. With those messages being shown we have no idea that we are omitting things. So it might lead to more cases where it seems like something is fixed/broken but it actually isn't and you need to specify more options while doing things. We already see that with the broken data collection in daca and apparently fixed issues which triggered this.

That increases the test matrix and will make things more complex and slower in turnaround times. We also would require multiple instances of daca so we actually collect all the warning data and all the performance data as we might regress in any area with any of those configurations.

[firewave]

Disabling it by default also completely killed the valueFlowBailoutIncompleteVar data collection in daca:

Restored in #6153.

[firewave]

#6153 also revealed some things about the ValueFlow I was not (actively) aware of. We perform it for everything no matter if it is used or not. It seems if we might conditionally (and optionally) able to exclude some unused code from that it might yield a performance boost. I filed https://trac.cppcheck.net/ticket/12528 about that.

[firewave]

We are treating certain errors as critical because they cause the analysis of subsequent code to stop.

You could argue that any bailout is similar to that. Be it either the valueFlowBailoutIncompleteVar or the check level. The issue with the latter is that there is no indication of that at all unlike (most of?) our bailouts (maybe we need to raise more of the bailouts in our debug output).

[firewave]

The reduced default check level might also cause misleading results in the tests. We should default at exhaustive in the object but only set the different default when parsing the command-line.

[firewave]

We cannot release a version without at least having a message. I will adjust this PR later today so it will only show as debug for now and will file tickets about the additional points I raised.

Overall I think we should not disable this by default but let's get the message in first.

[firewave]

The reduced default check level might also cause misleading results in the tests. We should default at exhaustive in the object but only set the different default when parsing the command-line.

Addressed in #6225.