[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 4 inline comments as done.
gamesh411 added a comment.

In D153954#4456713 , @shafik wrote:

> I did not look at this in detail but I don't think this approach is correct. 
> I fixed this for constant evaluation the other day and you can find the 
> details here: D130058 
>
> The test suite I wrote should be sufficient for you to validate your approach 
> against.

@shafik, @donat.nagy  Thanks for looking at this patch.
I have checked the linked improvements in Sema, and this initial modification 
would only lead to a ClangSA implementation of the same error-detection logic.
Due to symbolic execution, the set of potentially detectable errors is bigger, 
but this is maybe not the right place for this checker.

My goal is to improve the checker and eventually bring it out of alpha.

I see one main problem with this checker:

It does not support enums with fixed underlying type, namely `std::byte`, which 
is implemented like this:

  enum class byte: unsigned char {};

As `std::byte` has no enumerators, the checker would say any otherwise allowed 
non-default initialization of a std::byte instance is bad, i.e.:

  std::byte b {42}; // the checker gives a warning for this

Aside from this, in the `optin` package, there is a place for the current state 
of the checker (namely that only the value mentioned in the enumerator list are 
OK).
I will create a separate patch to move it out of alpha and into `optin` package.




Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:42
+class RangeBasedValueMatchEvaluator : ValueMatchEvaluator {
+  llvm::APSInt Min, Max;
 

steakhal wrote:
> I can see `llvm::APSInt` used a few places. Consider `using namespace llvm;`
Again, good point, I just abandoned the direction where many APSInt mentions 
are introduced, and the original only has 2 mentions of APSInt.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:65-69
+  if (TrueState && !FalseState) {
+Pred = C.addTransition(TrueState, Pred);
+  } else if (FalseState && !TrueState) {
+Pred = C.addTransition(FalseState, Pred);
+  }

steakhal wrote:
> Why do you fold the "Pred" ExplodedNode?
> I'd say, you probably didn't want to use `addTransition` here.
> Why don't you assume the subsequent assumptions and transition only once?
> 
> Anyway, I think it's better to have the `addTransition` closer to the outer 
> scope (where the error reporting is done), so that we can easily see how many 
> ways we branch off, etc.
Thanks! This is a very good point; I just changed the general direction of the 
checker, and this implementation is no longer touched by the change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153954/new/

https://reviews.llvm.org/D153954

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 543413.
gamesh411 edited the summary of this revision.
gamesh411 added a comment.

The checker now retains the original detection logic, but only whitelists empty
enums.

As a future step the checker is moved into the optin package.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153954/new/

https://reviews.llvm.org/D153954

Files:
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/test/Analysis/enum-cast-out-of-range.cpp


Index: clang/test/Analysis/enum-cast-out-of-range.cpp
===
--- clang/test/Analysis/enum-cast-out-of-range.cpp
+++ clang/test/Analysis/enum-cast-out-of-range.cpp
@@ -198,3 +198,20 @@
   s.E = static_cast(4); // OK.
   s.E = static_cast(5); // expected-warning {{The 
value provided to the cast expression is not in the valid range of values for 
the enum}}
 }
+
+
+enum class empty_unfixed {};
+
+enum class empty_fixed: char {};
+
+enum class empty_fixed_unsigned: unsigned char {};
+
+void ignore_unused(...);
+
+void empty_enums_init_with_zero_should_not_warn() {
+  empty_unfixed eu = static_cast(0); //should always be OK to 
zero initialize any enum
+  empty_fixed ef = static_cast(0);
+  empty_fixed_unsigned efu = static_cast(0);
+
+  ignore_unused(eu, ef, efu);
+}
Index: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -129,6 +129,14 @@
   const EnumDecl *ED = T->castAs()->getDecl();
 
   EnumValueVector DeclValues = getDeclValuesForEnum(ED);
+
+  // If the declarator list is empty, bail out.
+  // Every initialization an enum with a fixed underlying type but without any
+  // enumerators would produce a warning if we were to continue at this point.
+  // The most notable example is std::byte in the C++17 standard library.
+  if (DeclValues.size() == 0)
+return;
+
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));


Index: clang/test/Analysis/enum-cast-out-of-range.cpp
===
--- clang/test/Analysis/enum-cast-out-of-range.cpp
+++ clang/test/Analysis/enum-cast-out-of-range.cpp
@@ -198,3 +198,20 @@
   s.E = static_cast(4); // OK.
   s.E = static_cast(5); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
 }
+
+
+enum class empty_unfixed {};
+
+enum class empty_fixed: char {};
+
+enum class empty_fixed_unsigned: unsigned char {};
+
+void ignore_unused(...);
+
+void empty_enums_init_with_zero_should_not_warn() {
+  empty_unfixed eu = static_cast(0); //should always be OK to zero initialize any enum
+  empty_fixed ef = static_cast(0);
+  empty_fixed_unsigned efu = static_cast(0);
+
+  ignore_unused(eu, ef, efu);
+}
Index: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -129,6 +129,14 @@
   const EnumDecl *ED = T->castAs()->getDecl();
 
   EnumValueVector DeclValues = getDeclValuesForEnum(ED);
+
+  // If the declarator list is empty, bail out.
+  // Every initialization an enum with a fixed underlying type but without any
+  // enumerators would produce a warning if we were to continue at this point.
+  // The most notable example is std::byte in the C++17 standard library.
+  if (DeclValues.size() == 0)
+return;
+
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D153954#4480296 , @steakhal wrote:

> [...] I'd not recommend moving the actual implementation anywhere.

I agree, I mostly spoke figuratively, suggesting that this checker could end up 
in the optin group when it's eventually brought out of alpha.




Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:44
+// Suppress unused warnings
+[](...){}(unscoped, scoped);
+  }

steakhal wrote:
> `void sink(...);` would have achieved the same and I'd say it's less complex.
> `sink(unscoped, scoped);`
Or just `(void)unscoped; (void)scoped;` if we're bikeshedding this test...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153954/new/

https://reviews.llvm.org/D153954

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D153954#4480103 , @donat.nagy 
wrote:

> I think the old "report when the value stored in an enum type is not equal to 
> one of the enumerators" behavior would be useful as an opt-in checker that 
> could be adopted by some projects; while the new "relaxed" version is too 
> bland to be really useful. I'd suggest keeping the old behavior in the 
> general case, eliminating the "obvious" false positives like `std::byte` 
> (don't report types that have no enumerators) and moving this checker towards 
> the optin group.

I would agree that after ignoring the obvious cases (like no enumerators), a 
checker like this could be useful as an "optin" checker, however, I'd not 
recommend moving the actual implementation anywhere.
You can create "virtual" checkers like it's done in many cases, eg. 
`MismatchedDeallocatorChecker`.

FYI I haven't checked the actual implementation thoroughly, because I had 
higher-level remarks inline.




Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:42
+class RangeBasedValueMatchEvaluator : ValueMatchEvaluator {
+  llvm::APSInt Min, Max;
 

I can see `llvm::APSInt` used a few places. Consider `using namespace llvm;`



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:65-69
+  if (TrueState && !FalseState) {
+Pred = C.addTransition(TrueState, Pred);
+  } else if (FalseState && !TrueState) {
+Pred = C.addTransition(FalseState, Pred);
+  }

Why do you fold the "Pred" ExplodedNode?
I'd say, you probably didn't want to use `addTransition` here.
Why don't you assume the subsequent assumptions and transition only once?

Anyway, I think it's better to have the `addTransition` closer to the outer 
scope (where the error reporting is done), so that we can easily see how many 
ways we branch off, etc.



Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:44
+// Suppress unused warnings
+[](...){}(unscoped, scoped);
+  }

`void sink(...);` would have achieved the same and I'd say it's less complex.
`sink(unscoped, scoped);`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153954/new/

https://reviews.llvm.org/D153954

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

I think the old "report when the value stored in an enum type is not equal to 
one of the enumerators" behavior would be useful as an opt-in checker that 
could be adopted by some projects; while the new "relaxed" version is too bland 
to be really useful. I'd suggest keeping the old behavior in the general case, 
eliminating the "obvious" false positives like `std::byte` (don't report types 
that have no enumerators) and moving this checker towards the optin group.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153954/new/

https://reviews.llvm.org/D153954

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I did not look at this in detail but I don't think this approach is correct. I 
fixed this for constant evaluation the other day and you can find the details 
here: D130058 

The test suite I wrote should be sufficient for you to validate your approach 
against.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153954/new/

https://reviews.llvm.org/D153954

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits