Page MenuHomePhabricator

[StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
ClosedPublic

Authored by danielmarjamaki on Jan 3 2017, 11:55 PM.

Details

Summary

Example code:

void f1(int x) {
  int a[20] = {0};
  if (x==25) {}
  if (a[x] == 123) {}  // <- Warning
}

If I don't enable alpha, only core, then Clang writes this misleading FP:

undef.c:5:12: warning: The left operand of '==' is a garbage value

I say it's a FP because the message is wrong. If the message correctly said "array index out of bounds" and pointed out a[x] directly, then it would be TP. This message goes away if alpha is enabled and I believe that is by intention.

Since there is a array-index-out-of-bounds check in alpha I am guessing that the UndefinedBinaryOperatorResult should not report "array index out of bounds". Therefore I remove this warning from this check.

This patch is a experimental work in progress. I would like to know if you think I should modifiy the UndefinedBinaryOperatorResult check or if I should do something in the ExprEngine? Maybe array index out of bounds should not lead to Undef SVal?

With this patch, all the existing tests succeed.

Diff Detail

Repository
rL LLVM

Event Timeline

danielmarjamaki retitled this revision from to [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds.
danielmarjamaki updated this object.
danielmarjamaki set the repository for this revision to rL LLVM.
danielmarjamaki added a subscriber: cfe-commits.

Did you experience any problems with the array out of bounds check lately? In case it was stable on large code-bases and did not give too many false positives, I think it might be worth to move that check out of alpha at the same time, so users who do not turn on alpha checks will not lose any functionality. What do you think?

Did you experience any problems with the array out of bounds check lately? In case it was stable on large code-bases and did not give too many false positives, I think it might be worth to move that check out of alpha at the same time, so users who do not turn on alpha checks will not lose any functionality. What do you think?

I don't have precise statistics. But these array-index-out-of-bounds messages are often false positives. Fixes are needed in the ExprEngine.

zaks.anna edited edge metadata.Jan 12 2017, 4:56 PM

I think it's more valuable to report a warning here even if the error message is not very precise. Marking something as in bounds when we know it's not does not feel right and could lead to inconsistent states down the road.

I am not against that the error is shown as long as it's not misleading/wrong. To avoid misleading, in my humble opinion the error message should say "array index out of bounds".

Does the code you added detects array out of bounds cases without false positives? Is it an option to just have this checkers produce a more precise error message in the specific case.

A lot of work will probably need to be done to implement a proper array out of bounds checking and no-one is working on that.

Does the code you added detects array out of bounds cases without false positives? Is it an option to just have this checkers produce a more precise error message in the specific case.

A lot of work will probably need to be done to implement a proper array out of bounds checking and no-one is working on that.

I don't know.. maybe I can avoid some false positive. Maybe if the left operand seems to be out-of-bounds and the right operand is uninitialized maybe it would be better to complain about the right operand.

It is definitely an option for me to have this checker produce more precise error messages. I believe that will solve my problems.

Making the error message more precise.

zaks.anna added inline comments.Feb 23 2017, 1:44 PM
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
76 ↗(On Diff #89540)

Please, pull this out into a sub-rutine. Thanks!

Fixed review comment. Broke out function.

danielmarjamaki marked an inline comment as done.Feb 24 2017, 3:41 AM
zaks.anna accepted this revision.Feb 24 2017, 10:26 PM

Thank you!

This revision is now accepted and ready to land.Feb 24 2017, 10:26 PM
This revision was automatically updated to reflect the committed changes.