Page MenuHomePhabricator

[analyzer] Add more tests for ArrayBoundCheckerV2

Authored by steakhal on Aug 31 2020, 4:30 AM.



According to a Bugzilla ticket, ArrayBoundCheckerV2 produces a false-positive report.
This patch adds a test demonstrating the current flawed behavior.
Also adds several similar test cases just to be on the safe side.

Diff Detail

Event Timeline

steakhal created this revision.Aug 31 2020, 4:30 AM
steakhal requested review of this revision.Aug 31 2020, 4:30 AM
steakhal edited the summary of this revision. (Show Details)
martong added inline comments.Sep 1 2020, 1:23 AM

Hmm, this seems to be quite redundant with the size_t tests. Why is it not enough to have test for one unsigned type?
Are you trying to check for overflow errors? Then I'd expect to have indexes around UINT_MAX and so on.

Same comment applies to the tests with the signed types.

steakhal added inline comments.Sep 1 2020, 1:52 AM

In the current implementation - and in any implementation of the checker logic will have to deal with integral-promotion during the simplification of the array indexer expression and the given extent.
All of these can have different signess and bitwidth which makes the implementation quite tricky.

In fact, this resulted in the bug, which this patch-stack aims to fix.
I'm gonna highlight the related parts in the refactoring patch if you think it helps.

martong accepted this revision.Sep 1 2020, 2:04 AM
martong added inline comments.

Okay, makes sense.

It's just very painful to have code repetitions, even in the test code. In gtest unittests we can have tests with type parameters to avoid this kind of repetition. But I guess, there is no way to get rid of this repetition in lit tests.

This revision is now accepted and ready to land.Sep 1 2020, 2:04 AM
steakhal added inline comments.Sep 1 2020, 2:47 AM

You can imagine duplicating all of this several times, since the constant in the subscript expression could also have different types, such as: unsigned, long, unsigned long, etc. Potentially uncovering bugs.
However, I was reluctant to add those as well :|

I could introduce a macro, to expand these - but IMO macros in tests ehh. probably reduces readability.

martong added inline comments.Sep 1 2020, 2:56 AM

Macros, ehh.

Could it work if we instantiated a test function template with the types as parameters? AFAIK, we analyze all instantiations as top level. The problem seems to be with the -verify and matching the expected-warning for each instantiation...