This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] More accurate modeling about the increment operator of the operand with type bool.
ClosedPublic

Authored by MTC on Feb 25 2018, 6:05 AM.

Details

Summary

There is a problem with analyzer that a wrong value is given when modeling the increment operator of the operand with type bool. After rL307604 is applied, a unsigned overflow may occur.

Example:

void func() {
  bool b = true;
  // unsigned overflow occur, 2 -> 0 U1b
  b++;
}

The use of an operand of type bool with the ++ operators is deprecated but valid untill C++17. And if the operand of the increment operator is of type bool, it is set to true.

This patch includes two parts:

  • If the operand of the increment operator is of type bool or type _Bool, set to true.
  • Modify BasicValueFactory::getTruthValue(), use getIntWidth() instead getTypeSize() and use unsigned instead signed.

Diff Detail

Repository
rC Clang

Event Timeline

MTC created this revision.Feb 25 2018, 6:05 AM

i see, but just in case - what about the decrement operator ?

MTC added a comment.Mar 2 2018, 2:28 AM
In D43741#1024949, @alexshap wrote:

i see, but just in case - what about the decrement operator ?

@alexshap, If I'm not wrong, decrement operation is not allowed on bool type in C++98 and C++14.

5.2.6 Increment and decrement [expr.post.incr]
The operand of postfix -- is decremented analogously to the postfix ++ operator, except that the operand
shall not be of type bool. [ Note: For prefix increment and decrement, see 5.3.2. — end note ]

NoQ added a comment.Mar 2 2018, 2:16 PM

Nice catch, thanks!

lib/StaticAnalyzer/Core/ExprEngineC.cpp
1078–1086

untill seems to be deprecated in favor of until.

1082

Doesn't isBooleanType() imply CPlusPlus? I guess we need to see if it works in Objective-C++ as well.

test/Analysis/bool.cpp
65 ↗(On Diff #135820)

dump() exposes too much internals, we try to only use it for debugging, not for tests.

Would eg. clang_analyzer_eval(b == 1) be enough?

MTC updated this revision to Diff 136901.Mar 3 2018, 3:20 AM
  • If the operand of the ++ operator is of type _Bool, also set to true.
  • Add test file _Bool-increment-decement.c.
MTC added a comment.Mar 3 2018, 3:22 AM

Thank you for your review, @NoQ!

  • isBooleanType() is used to check _Bool in C99/C11 and bool in C++. For _Bool , there is the same overflow problem.
  • In C++98/C++11/C++14, for ++bool and bool+, both sets true directly.
  • In C++, --bool and bool-- is illeagal.
  • In C99/C11 standard, there is not much information about _Bool++ and _Bool--.

From the implementation of the compiler, _Bool++ and _Bool-- are divided into the following situations.

  • _Bool b = 0; b++; // b -> 1
  • _Bool b = 1; b++; // b -> 1
  • _Bool b = 0; b--; // b -> 1
  • _Bool b = 1; b--; // b -> 0

So it's reasonable to set to true if the operand of the increment operator is of type _Bool, just my opinion.

I not familiar with Objective-C++, can you provide a appropriate test about Objective-C++ for me, thank you!

And I'm not a native speaker of English, the grammar of the comments may not be correct. If so, please correct me, thanks!

MTC marked an inline comment as done.Mar 3 2018, 3:23 AM
NoQ accepted this revision.Mar 5 2018, 2:01 PM

LGTM, thank you for the fix!

I not familiar with Objective-C++, can you provide a appropriate test about Objective-C++ for me, thank you!

Now that the explicit check and the respective code path is removed, i don't think we should explicitly test it.

And I'm not a native speaker of English

Same here, i guess.

test/Analysis/_Bool-increment-decrement.c
2–3

We don't write -analyzer-store=region these days because that's the default value and it's unlikely that anybody would ever care.

This revision is now accepted and ready to land.Mar 5 2018, 2:01 PM
MTC updated this revision to Diff 137154.Mar 6 2018, 4:25 AM

Remove the default configuration -analyzer-store=region in the test file.

MTC edited the summary of this revision. (Show Details)Mar 6 2018, 4:28 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.