[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

No worries, always happy to help! :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

@Szelethus, firstly, thank you for landing this change. I really appreciate it! 
  Secondly, apologies for misspelling your name earlier.  And lastly, thank you 
for your patient explanation of how to get the diffs updated correctly in a 
Phabricator review.  I think my mistake was that I made the change and the 
updates updates in a private branch, and not directly off master, and then 
duplicated the changes on master.  For one of those sets of changes, I amended 
the first commit with the second round of changes, and I think I confused 
myself by doing that.  In any case, lesson learned, and thank you again for 
your coaching!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I seem to have been able to put this together by fetching the individual diffs 
and squashing them this time :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369760: [analyzer] Avoid unnecessary enum range check on 
LValueToRValue casts (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66014?vs=215433&id=216842#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014

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


Index: cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -91,6 +91,22 @@
 
 void EnumCastOutOfRangeChecker::checkPreStmt(const CastExpr *CE,
  CheckerContext &C) const {
+
+  // Only perform enum range check on casts where such checks are valid.  For
+  // all other cast kinds (where enum range checks are unnecessary or invalid),
+  // just return immediately.  TODO: The set of casts whitelisted for enum
+  // range checking may be incomplete.  Better to add a missing cast kind to
+  // enable a missing check than to generate false negatives and have to remove
+  // those later.
+  switch (CE->getCastKind()) {
+  case CK_IntegralCast:
+break;
+
+  default:
+return;
+break;
+  }
+
   // Get the value of the expression to cast.
   const llvm::Optional ValueToCast =
   C.getSVal(CE->getSubExpr()).getAs();
Index: cfe/trunk/test/Analysis/enum-cast-out-of-range.c
===
--- cfe/trunk/test/Analysis/enum-cast-out-of-range.c
+++ cfe/trunk/test/Analysis/enum-cast-out-of-range.c
@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
+// RUN:   -verify %s
+
+enum En_t {
+  En_0 = -4,
+  En_1,
+  En_2 = 1,
+  En_3,
+  En_4 = 4
+};
+
+void unscopedUnspecifiedCStyle() {
+  enum En_t Below = (enum En_t)(-5);// expected-warning {{not in the valid 
range}}
+  enum En_t NegVal1 = (enum En_t)(-4);  // OK.
+  enum En_t NegVal2 = (enum En_t)(-3);  // OK.
+  enum En_t InRange1 = (enum En_t)(-2); // expected-warning {{not in the valid 
range}}
+  enum En_t InRange2 = (enum En_t)(-1); // expected-warning {{not in the valid 
range}}
+  enum En_t InRange3 = (enum En_t)(0);  // expected-warning {{not in the valid 
range}}
+  enum En_t PosVal1 = (enum En_t)(1);   // OK.
+  enum En_t PosVal2 = (enum En_t)(2);   // OK.
+  enum En_t InRange4 = (enum En_t)(3);  // expected-warning {{not in the valid 
range}}
+  enum En_t PosVal3 = (enum En_t)(4);   // OK.
+  enum En_t Above = (enum En_t)(5); // expected-warning {{not in the valid 
range}}
+}
+
+enum En_t unused;
+void unusedExpr() {
+  // Following line is not something that EnumCastOutOfRangeChecker should
+  // evaluate.  Checker should either ignore this line or process it without
+  // producing any warnings.  However, compilation will (and should) still
+  // generate a warning having nothing to do with this checker.
+  unused; // expected-warning {{expression result unused}}
+}
Index: cfe/trunk/test/Analysis/enum-cast-out-of-range.cpp
===
--- cfe/trunk/test/Analysis/enum-cast-out-of-range.cpp
+++ cfe/trunk/test/Analysis/enum-cast-out-of-range.cpp
@@ -150,7 +150,15 @@
   scoped_specified_t InvalidAfterRangeEnd = (scoped_specified_t)(5); // 
expected-warning {{The value provided to the cast expression is not in the 
valid range of values for the enum}}
 }
 
-void rangeContstrained1(int input) {
+unscoped_unspecified_t unused;
+void unusedExpr() {
+  // following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
+  // or process it without producing any warnings.  However, compilation will 
(and should) still generate a warning having
+  // nothing to do with this checker.
+  unused; // expected-warning {{expression result unused}}
+}
+
+void rangeConstrained1(int input) {
   if (input > -5 && input < 5)
 auto value = static_cast(input); // OK. Being 
conservative, this is a possibly good value.
 }


Index: cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -91,6 +91,22 @@
 
 void EnumCastOutOfRangeChecker::checkPreStmt(const CastExpr *CE,
  CheckerContext &C) const {
+
+  // Only perform enum range check on cas

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

What I meant is that the diff has to be made against the master branch or I 
won't be able to apply it locally.  Say your repo is structured like this:

  * dcba2 (HEAD -> my_fix)
  * dcba1
  * abcd4 (master)
  * abcd3
  * abcd2
  * abcd1

Then the diff has to be made with `git diff master -U9 > my_fix.diff` and 
uploaded here, or with the use of arcanist (which I still find incredibly 
inconvenient). If you only upload the latest commit, I'd be missing `dcba1`.

If you have a lot of changes you want to submit, like this:

  * hjkd2 (enable_feature_by_default)
  * hjkd1
  * haha1 (my_feature)
  * dcba2 (HEAD -> my_fix)
  * dcba1
  * abcd4 (master)

You should create a revision for each branch, diff them  against each other 
(`git checkout my_feature && git diff my_fix -U9 > my_feature.diff`), and 
mark them as dependencies of each other on the right hand panel.

It is kinda inconvenient, I'll admit :^)


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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Kristoff, if you wouldn't mind, since you offered earlier, please go ahead and 
commit this change as-is, since it was accepted.  I ran into some non-technical 
issues with my follow-up changes and I'm going to be unavailable for several 
weeks.  To mitigate risk and work for my team, I'd like to submit the newer 
changes separately (and will reference this review in that changeset when I do, 
of course), after I return to work.

And-- thank you for your help and feedback!  I appreciated this upstreaming 
process (especially for what seemed like a fairly small/simple chnage).  I 
expect there will be many more.


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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

You seem to have diffed against your latest local commit, rather than against 
trunk (or master, if you use the monorepo). Phabricator isn't smart enough to 
put two and two together, and only displays the uploaded diff (though one has 
to admit, its doing a damn good job at that at least!).

Could you please upload the version diffed against master (`git diff master 
-U99`)?


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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 215433.
chrish_ericsson_atx added a comment.

Follow-up on reviewer feedback.  Changed from blacklisting LValueToRValue to 
whitelisting IntegralCast.  This was a good call -- additional testing with 
different cast kinds showed that the assertion tripped for other casts besides 
LValueToRValue, e.g., FloatToIntegral.  I couldn't see any casts other than 
Integral where the enum check seemed appropriate.  Testing with only 
IntegralCast enabled gave expected (correct) results.

Also reformatted the new regtest file per reviewer comments.


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

https://reviews.llvm.org/D66014

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


Index: clang/test/Analysis/enum-cast-out-of-range.c
===
--- clang/test/Analysis/enum-cast-out-of-range.c
+++ clang/test/Analysis/enum-cast-out-of-range.c
@@ -2,33 +2,34 @@
 // RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
 // RUN:   -verify %s
 
-enum unscoped_unspecified_t {
-  unscoped_unspecified_0 = -4,
-  unscoped_unspecified_1,
-  unscoped_unspecified_2 = 1,
-  unscoped_unspecified_3,
-  unscoped_unspecified_4 = 4
+enum En_t {
+  En_0 = -4,
+  En_1,
+  En_2 = 1,
+  En_3,
+  En_4 = 4
 };
 
 void unscopedUnspecifiedCStyle() {
-  enum unscoped_unspecified_t InvalidBeforeRangeBegin = (enum 
unscoped_unspecified_t)(-5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidNegativeValue1 = (enum 
unscoped_unspecified_t)(-4); // OK.
-  enum unscoped_unspecified_t ValidNegativeValue2 = (enum 
unscoped_unspecified_t)(-3); // OK.
-  enum unscoped_unspecified_t InvalidInsideRange1 = (enum 
unscoped_unspecified_t)(-2); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t InvalidInsideRange2 = (enum 
unscoped_unspecified_t)(-1); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t InvalidInsideRange3 = (enum 
unscoped_unspecified_t)(0); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidPositiveValue1 = (enum 
unscoped_unspecified_t)(1); // OK.
-  enum unscoped_unspecified_t ValidPositiveValue2 = (enum 
unscoped_unspecified_t)(2); // OK.
-  enum unscoped_unspecified_t InvalidInsideRange4 = (enum 
unscoped_unspecified_t)(3); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidPositiveValue3 = (enum 
unscoped_unspecified_t)(4); // OK.
-  enum unscoped_unspecified_t InvalidAfterRangeEnd = (enum 
unscoped_unspecified_t)(5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum En_t Below= (enum En_t)(-5); // expected-warning {{not in the valid 
range}}
+  enum En_t NegVal1  = (enum En_t)(-4); // OK.
+  enum En_t NegVal2  = (enum En_t)(-3); // OK.
+  enum En_t InRange1 = (enum En_t)(-2); // expected-warning {{not in the valid 
range}}
+  enum En_t InRange2 = (enum En_t)(-1); // expected-warning {{not in the valid 
range}}
+  enum En_t InRange3 = (enum En_t)(0);  // expected-warning {{not in the valid 
range}}
+  enum En_t PosVal1  = (enum En_t)(1);  // OK.
+  enum En_t PosVal2  = (enum En_t)(2);  // OK.
+  enum En_t InRange4 = (enum En_t)(3);  // expected-warning {{not in the valid 
range}}
+  enum En_t PosVal3  = (enum En_t)(4);  // OK.
+  enum En_t Above= (enum En_t)(5);  // expected-warning {{not in the valid 
range}}
 }
 
-enum unscoped_unspecified_t unused;
+enum En_t unused;
 void unusedExpr() {
-// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
-// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
-// nothing to do with this checker.
+// Following line is not something that EnumCastOutOfRangeChecker should
+// evaluate.  Checker should either ignore this line or process it without
+// producing any warnings.  However, compilation will (and should) still
+// generate a warning having nothing to do with this checker.
 unused; // expected-warning {{expression result unused}}
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -91,10 +91,21 @@
 
 void EnumCastOutOfRangeChecker

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Oh, there is no need for a new differential, you can update this one by 
clicking on 'update diff' in the right panel.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Yup, you can upload it and the green checkmark will stay. If it doesn't, I'll 
accept again.

I think there was one case when I added like 800 extra LOCs to a patch and 
phabricator automatically marked it as "needs reviews", but I never came across 
this after that, even when I completely rewrote the entire thing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 5 inline comments as done.
chrish_ericsson_atx added a comment.

In D66014#1627858 , @Szelethus wrote:

> LGTM, thanks! Do you need someone to commit this on your behalf? Also, could 
> you please make the comments capitalized, terminated, and fitting in 80 
> columns?


I have updated the comments and line formatting as you recommended.  Given that 
this change is already "Accepted", can I (should I) upload new differential for 
this change, or should this be delivered as-is, and I'll upload the new diffs 
as a new/separate change?

And yes, I will need someone to commit on my behalf.  I'm too new to have 
commit privs. :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

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

gamesh411 accepted this revision.
gamesh411 added a comment.

@chrish_ericsson_atx Thanks for fixing this! Your help is much appreciated :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

LGTM, thanks! Do you need someone to commit this on your behalf? Also, could 
you please make the comments capitalized, terminated, and fitting in 80 columns?




Comment at: clang/test/Analysis/enum-cast-out-of-range.c:14-24
+  enum unscoped_unspecified_t InvalidBeforeRangeBegin = (enum 
unscoped_unspecified_t)(-5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t ValidNegativeValue1 = (enum 
unscoped_unspecified_t)(-4); // OK.
+  enum unscoped_unspecified_t ValidNegativeValue2 = (enum 
unscoped_unspecified_t)(-3); // OK.
+  enum unscoped_unspecified_t InvalidInsideRange1 = (enum 
unscoped_unspecified_t)(-2); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t InvalidInsideRange2 = (enum 
unscoped_unspecified_t)(-1); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t InvalidInsideRange3 = (enum 
unscoped_unspecified_t)(0); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t ValidPositiveValue1 = (enum 
unscoped_unspecified_t)(1); // OK.

Note that you don't need to add the entire warning message here, and since its 
all the same, we could shorten it. Could you please format these lines like 
this?:

```lang=c++
  enum unscoped_unspecified_t InvalidAfterRangeEnd = (enum 
unscoped_unspecified_t)(5);
  // expected-warning@-1{{not in the valid range of values for the enum}}
```

If by some miracle the entire warning message fits in 80 columns, go for it! :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Thanks!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

chrish_ericsson_atx wrote:
> NoQ wrote:
> > chrish_ericsson_atx wrote:
> > > NoQ wrote:
> > > > chrish_ericsson_atx wrote:
> > > > > NoQ wrote:
> > > > > > If you look at the list of cast kinds, you'll be shocked to see 
> > > > > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > > > > definitely stands out (because it's the only one that has very 
> > > > > > little to do with actual casting), i'd still be much more 
> > > > > > comfortable if we *whitelist* the casts that we check, rather than 
> > > > > > blacklist them.
> > > > > > 
> > > > > > Can you take a look at the list and find which casts are we 
> > > > > > actually talking about and hardcode them here?
> > > > > I'm happy to cross-check a list; however, I'm not aware of the list 
> > > > > you are referring to.  Would you mind providing a bit more detail?
> > > > See **[[ 
> > > > https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
> > > >  | include/clang/AST/OperationKinds.def ]]**.
> > > Ah.   Okay -- That is a list I've seen before, of course...  :)   
> > > 
> > > Hmm...  regarding whitelisting versus blacklisting...  In this case, it 
> > > seems to me that switching to whitelisting casts that we know we should 
> > > be checking increases the risk that we would introduce a new bug -- 
> > > specifically that we'd accidentally leave out cast type that should have 
> > > been included, which would disable a legitimate check (that some users 
> > > might already be relying on).  By contrast, blacklisting the one cast 
> > > type we know should *not* be checked adds zero new risk and still fixes 
> > > this bug.
> > > 
> > > The sense of risk for me comes partly from being fairly new to working on 
> > > clang AST code -- this is my first change.   It strikes me that if I were 
> > > to feel confident about having the "correct" whitelist, I would have to 
> > > understand every cast type fairly deeply.  Given that I see several cast 
> > > kinds in that list that I know nothing about, I'm concerned with the 
> > > level of effort required, and that I still won't be able to fully 
> > > mitigate the risk.
> > > 
> > > Can you help me understand your rationale for preferring a whitelist in 
> > > this case?  Or, do you feel I've overstated the risk here?  I'm not 
> > > emotionally invested in being proven correct-- just want to learn!  :)
> > > 
> > Generally, if you're not sure, try to not to warn. False positives are 
> > worse than false negatives. It is natural to start with a small set of 
> > cases in which you're sure, and then slowly cover more and more cases as 
> > you investigate further. Leave a comment if you're unsure if you covered 
> > all cases, so that people knew that the code is potentially incomplete and 
> > it's a good place to look for potential improvements.
> > 
> > When you're actively developing the checker, you should rely on your own 
> > experiments with the real-world code that you plan to analyze. When the 
> > checker is still in alpha, it's fine for it to be more aggressive than 
> > necessary, because "you're still experimenting with it", but this will need 
> > to be addressed when the checker is moved out of alpha.
> > 
> > In particular, you have the following tricks at your disposal:
> > - Take a large codebase, run your checker on it (you'll need to do this 
> > regularly anyway) and include the cast kind in the warning text. This will 
> > give you a nice list of casts that you want to support (and probably also a 
> > list of casts that you //don't// want to support).
> > - If you want to be notified of false negatives by your power-users, 
> > instead of a whitelist put an assertion (immediately before emitting the 
> > report) that the cast kind is one of the known cast kinds. This is a bit 
> > evil, of course, but it only affects the users that run analysis with 
> > assertions on, which means custom-built clang, which means power-users that 
> > actively want to report these bugs.
> > - If you don't know what code does a specific AST node kind represent, as a 
> > last resort you can always assert that this kind of node never gets 
> > constructed and see which clang tests fail.
> > - If you want to have some protection against newly introduced cast kinds, 
> > make a `switch (getCastKind())` and don't add a `default:` case into it. 
> > This way anybody who introduces a new cast kind would get a compiler 
> > warning ("unhandled case in switch") and will be forced to take a look at 
> > how this code should

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked an inline comment as done.
chrish_ericsson_atx added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> chrish_ericsson_atx wrote:
> > NoQ wrote:
> > > chrish_ericsson_atx wrote:
> > > > NoQ wrote:
> > > > > If you look at the list of cast kinds, you'll be shocked to see 
> > > > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > > > definitely stands out (because it's the only one that has very little 
> > > > > to do with actual casting), i'd still be much more comfortable if we 
> > > > > *whitelist* the casts that we check, rather than blacklist them.
> > > > > 
> > > > > Can you take a look at the list and find which casts are we actually 
> > > > > talking about and hardcode them here?
> > > > I'm happy to cross-check a list; however, I'm not aware of the list you 
> > > > are referring to.  Would you mind providing a bit more detail?
> > > See **[[ 
> > > https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
> > >  | include/clang/AST/OperationKinds.def ]]**.
> > Ah.   Okay -- That is a list I've seen before, of course...  :)   
> > 
> > Hmm...  regarding whitelisting versus blacklisting...  In this case, it 
> > seems to me that switching to whitelisting casts that we know we should be 
> > checking increases the risk that we would introduce a new bug -- 
> > specifically that we'd accidentally leave out cast type that should have 
> > been included, which would disable a legitimate check (that some users 
> > might already be relying on).  By contrast, blacklisting the one cast type 
> > we know should *not* be checked adds zero new risk and still fixes this bug.
> > 
> > The sense of risk for me comes partly from being fairly new to working on 
> > clang AST code -- this is my first change.   It strikes me that if I were 
> > to feel confident about having the "correct" whitelist, I would have to 
> > understand every cast type fairly deeply.  Given that I see several cast 
> > kinds in that list that I know nothing about, I'm concerned with the level 
> > of effort required, and that I still won't be able to fully mitigate the 
> > risk.
> > 
> > Can you help me understand your rationale for preferring a whitelist in 
> > this case?  Or, do you feel I've overstated the risk here?  I'm not 
> > emotionally invested in being proven correct-- just want to learn!  :)
> > 
> Generally, if you're not sure, try to not to warn. False positives are worse 
> than false negatives. It is natural to start with a small set of cases in 
> which you're sure, and then slowly cover more and more cases as you 
> investigate further. Leave a comment if you're unsure if you covered all 
> cases, so that people knew that the code is potentially incomplete and it's a 
> good place to look for potential improvements.
> 
> When you're actively developing the checker, you should rely on your own 
> experiments with the real-world code that you plan to analyze. When the 
> checker is still in alpha, it's fine for it to be more aggressive than 
> necessary, because "you're still experimenting with it", but this will need 
> to be addressed when the checker is moved out of alpha.
> 
> In particular, you have the following tricks at your disposal:
> - Take a large codebase, run your checker on it (you'll need to do this 
> regularly anyway) and include the cast kind in the warning text. This will 
> give you a nice list of casts that you want to support (and probably also a 
> list of casts that you //don't// want to support).
> - If you want to be notified of false negatives by your power-users, instead 
> of a whitelist put an assertion (immediately before emitting the report) that 
> the cast kind is one of the known cast kinds. This is a bit evil, of course, 
> but it only affects the users that run analysis with assertions on, which 
> means custom-built clang, which means power-users that actively want to 
> report these bugs.
> - If you don't know what code does a specific AST node kind represent, as a 
> last resort you can always assert that this kind of node never gets 
> constructed and see which clang tests fail.
> - If you want to have some protection against newly introduced cast kinds, 
> make a `switch (getCastKind())` and don't add a `default:` case into it. This 
> way anybody who introduces a new cast kind would get a compiler warning 
> ("unhandled case in switch") and will be forced to take a look at how this 
> code should handle the new cast kind.
Okay.  Very fair points.  I've been focused too much on released production 
code lately-- the mantra there is to be a surgical as possible, and minimize 
risk of any changes 

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

chrish_ericsson_atx wrote:
> NoQ wrote:
> > chrish_ericsson_atx wrote:
> > > NoQ wrote:
> > > > If you look at the list of cast kinds, you'll be shocked to see 
> > > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > > definitely stands out (because it's the only one that has very little 
> > > > to do with actual casting), i'd still be much more comfortable if we 
> > > > *whitelist* the casts that we check, rather than blacklist them.
> > > > 
> > > > Can you take a look at the list and find which casts are we actually 
> > > > talking about and hardcode them here?
> > > I'm happy to cross-check a list; however, I'm not aware of the list you 
> > > are referring to.  Would you mind providing a bit more detail?
> > See **[[ 
> > https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
> >  | include/clang/AST/OperationKinds.def ]]**.
> Ah.   Okay -- That is a list I've seen before, of course...  :)   
> 
> Hmm...  regarding whitelisting versus blacklisting...  In this case, it seems 
> to me that switching to whitelisting casts that we know we should be checking 
> increases the risk that we would introduce a new bug -- specifically that 
> we'd accidentally leave out cast type that should have been included, which 
> would disable a legitimate check (that some users might already be relying 
> on).  By contrast, blacklisting the one cast type we know should *not* be 
> checked adds zero new risk and still fixes this bug.
> 
> The sense of risk for me comes partly from being fairly new to working on 
> clang AST code -- this is my first change.   It strikes me that if I were to 
> feel confident about having the "correct" whitelist, I would have to 
> understand every cast type fairly deeply.  Given that I see several cast 
> kinds in that list that I know nothing about, I'm concerned with the level of 
> effort required, and that I still won't be able to fully mitigate the risk.
> 
> Can you help me understand your rationale for preferring a whitelist in this 
> case?  Or, do you feel I've overstated the risk here?  I'm not emotionally 
> invested in being proven correct-- just want to learn!  :)
> 
Generally, if you're not sure, try to not to warn. False positives are worse 
than false negatives. It is natural to start with a small set of cases in which 
you're sure, and then slowly cover more and more cases as you investigate 
further. Leave a comment if you're unsure if you covered all cases, so that 
people knew that the code is potentially incomplete and it's a good place to 
look for potential improvements.

When you're actively developing the checker, you should rely on your own 
experiments with the real-world code that you plan to analyze. When the checker 
is still in alpha, it's fine for it to be more aggressive than necessary, 
because "you're still experimenting with it", but this will need to be 
addressed when the checker is moved out of alpha.

In particular, you have the following tricks at your disposal:
- Take a large codebase, run your checker on it (you'll need to do this 
regularly anyway) and include the cast kind in the warning text. This will give 
you a nice list of casts that you want to support (and probably also a list of 
casts that you //don't// want to support).
- If you want to be notified of false negatives by your power-users, instead of 
a whitelist put an assertion (immediately before emitting the report) that the 
cast kind is one of the known cast kinds. This is a bit evil, of course, but it 
only affects the users that run analysis with assertions on, which means 
custom-built clang, which means power-users that actively want to report these 
bugs.
- If you don't know what code does a specific AST node kind represent, as a 
last resort you can always assert that this kind of node never gets constructed 
and see which clang tests fail.
- If you want to have some protection against newly introduced cast kinds, make 
a `switch (getCastKind())` and don't add a `default:` case into it. This way 
anybody who introduces a new cast kind would get a compiler warning ("unhandled 
case in switch") and will be forced to take a look at how this code should 
handle the new cast kind.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked an inline comment as done.
chrish_ericsson_atx added a comment.

In D66014#1625733 , @Szelethus wrote:

> Oh, btw, thank you for working on this!


Glad to help!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> chrish_ericsson_atx wrote:
> > NoQ wrote:
> > > If you look at the list of cast kinds, you'll be shocked to see 
> > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > definitely stands out (because it's the only one that has very little to 
> > > do with actual casting), i'd still be much more comfortable if we 
> > > *whitelist* the casts that we check, rather than blacklist them.
> > > 
> > > Can you take a look at the list and find which casts are we actually 
> > > talking about and hardcode them here?
> > I'm happy to cross-check a list; however, I'm not aware of the list you are 
> > referring to.  Would you mind providing a bit more detail?
> See **[[ 
> https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
>  | include/clang/AST/OperationKinds.def ]]**.
Ah.   Okay -- That is a list I've seen before, of course...  :)   

Hmm...  regarding whitelisting versus blacklisting...  In this case, it seems 
to me that switching to whitelisting casts that we know we should be checking 
increases the risk that we would introduce a new bug -- specifically that we'd 
accidentally leave out cast type that should have been included, which would 
disable a legitimate check (that some users might already be relying on).  By 
contrast, blacklisting the one cast type we know should *not* be checked adds 
zero new risk and still fixes this bug.

The sense of risk for me comes partly from being fairly new to working on clang 
AST code -- this is my first change.   It strikes me that if I were to feel 
confident about having the "correct" whitelist, I would have to understand 
every cast type fairly deeply.  Given that I see several cast kinds in that 
list that I know nothing about, I'm concerned with the level of effort 
required, and that I still won't be able to fully mitigate the risk.

Can you help me understand your rationale for preferring a whitelist in this 
case?  Or, do you feel I've overstated the risk here?  I'm not emotionally 
invested in being proven correct-- just want to learn!  :)



Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Oh, btw, thank you for working on this!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

NoQ wrote:
> chrish_ericsson_atx wrote:
> > Szelethus wrote:
> > > So this is where the assertion comes from, and will eventually lead to 
> > > `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this 
> > > will fire on line 427:
> > > ```
> > > assert(op == BO_Add);
> > > ```
> > > Seems like this happens because `unused`'s value in your testcase will be 
> > > retrieved as a `Loc`, while the values in the enum are (correctly) 
> > > `NonLoc`, and `SValBuilder::evalBinOp` thinks this is some sort of 
> > > pointer arithmetic (`5 + ptr` etc).
> > > 
> > > How about instead of checking for LValueToRValue cast, we check whether 
> > > `ValueToCast` is `Loc`, and bail out if so? That sounds like a more 
> > > general solution, but I didn't sit atop of this for hours.
> > Is this the only case where ValueToCast will be Loc?   I was unsure about 
> > the implications of Loc vs. NonLoc in this code, and I didn't want to risk 
> > breaking a check that should have been valid.  That's why I opted for 
> > bailing on an LValueToRValue cast at a higher level-- that seemed much 
> > safer to me.  I certainly might be missing something, but I couldn't find 
> > any evidence that an LValueToRValue cast should ever need to be checked for 
> > enum range errors.   I will certainly defer to your judgement, but I'd like 
> > to have a better understanding of why looking for ValueToCast == Loc would 
> > actually be more correct.
> > How about instead of checking for `LValueToRValue` cast, we check whether 
> > `ValueToCast` is `Loc`, and bail out if so?
> 
> In this case it probably doesn't matter too much, but generally i always 
> recommend against this sort of stuff: if possible, consult the AST. A `Loc` 
> may represent either a glvalue or a pointer-typed prvalue, and there's no way 
> to tell the two apart by only looking at that `Loc`. In particular, both 
> unary operators `*` and `&` are modeled as //no-op//. I've been a lot of 
> incorrect code that tried to dereference a `Loc` too many or too few times 
> because the code had misjudged whether it has already obtained the prvalue it 
> was looking for. But it's easy to discriminate between the two when you look 
> at the AST.
> 
> The Static Analyzer is a converter from ASTs to ExplodedGraphs. When in 
> doubt, consult the AST.
Notes taken, thanks! :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

chrish_ericsson_atx wrote:
> NoQ wrote:
> > If you look at the list of cast kinds, you'll be shocked to see 
> > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > definitely stands out (because it's the only one that has very little to do 
> > with actual casting), i'd still be much more comfortable if we *whitelist* 
> > the casts that we check, rather than blacklist them.
> > 
> > Can you take a look at the list and find which casts are we actually 
> > talking about and hardcode them here?
> I'm happy to cross-check a list; however, I'm not aware of the list you are 
> referring to.  Would you mind providing a bit more detail?
See **[[ 
https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
 | include/clang/AST/OperationKinds.def ]]**.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

chrish_ericsson_atx wrote:
> Szelethus wrote:
> > So this is where the assertion comes from, and will eventually lead to 
> > `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this 
> > will fire on line 427:
> > ```
> > assert(op == BO_Add);
> > ```
> > Seems like this happens because `unused`'s value in your testcase will be 
> > retrieved as a `Loc`, while the values in the enum are (correctly) 
> > `NonLoc`, and `SValBuilder::evalBinOp` thinks this is some sort of pointer 
> > arithmetic (`5 + ptr` etc).
> > 
> > How about instead of checking for LValueToRValue cast, we check whether 
> > `ValueToCast` is `Loc`, and bail out if so? That sounds like a more general 
> > solution, but I didn't sit atop of this for hours.
> Is this the only case where ValueToCast will be Loc?   I was unsure about the 
> implications of Loc vs. NonLoc in this code, and I didn't want to risk 
> breaking a check that should have been valid.  That's why I opted for bailing 
> on an LValueToRValue cast at a higher level-- that seemed much safer to me.  
> I certainly might be missing something, but I couldn't find any evidence that 
> an LValueToRValue cast should ever need to be checked for enum range errors.  
>  I will certainly defer to your judgement, but I'd like to have a better 
> understanding of why looking for ValueToCast == Loc would actually be more 
> correct.
> How about instead of checking for `LValueToRValue` cast, we check whether 
> `ValueToCast` is `Loc`, and bail out if so?

In this case it probably doesn't matter too much, but generally i always 
recommend against this sort of stuff: if possible, consult the AST. A `Loc` may 
represent either a glvalue or a pointer-typed prvalue, and there's no way to 
tell the two apart by only looking at that `Loc`. In particular, both unary 
operators `*` and `&` are modeled as //no-op//. I've been a lot of incorrect 
code that tried to dereference a `Loc` too many or too few times because the 
code had misjudged whether it has already obtained the prvalue it was looking 
for. But it's easy to discriminate between the two when you look at the AST.

The Static Analyzer is a converter from ASTs to ExplodedGraphs. When in doubt, 
consult the AST.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 3 inline comments as done.
chrish_ericsson_atx added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> If you look at the list of cast kinds, you'll be shocked to see ridiculously 
> weird cornercases. Even though lvalue-to-rvalue cast definitely stands out 
> (because it's the only one that has very little to do with actual casting), 
> i'd still be much more comfortable if we *whitelist* the casts that we check, 
> rather than blacklist them.
> 
> Can you take a look at the list and find which casts are we actually talking 
> about and hardcode them here?
I'm happy to cross-check a list; however, I'm not aware of the list you are 
referring to.  Would you mind providing a bit more detail?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

Szelethus wrote:
> So this is where the assertion comes from, and will eventually lead to 
> `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this will 
> fire on line 427:
> ```
> assert(op == BO_Add);
> ```
> Seems like this happens because `unused`'s value in your testcase will be 
> retrieved as a `Loc`, while the values in the enum are (correctly) `NonLoc`, 
> and `SValBuilder::evalBinOp` thinks this is some sort of pointer arithmetic 
> (`5 + ptr` etc).
> 
> How about instead of checking for LValueToRValue cast, we check whether 
> `ValueToCast` is `Loc`, and bail out if so? That sounds like a more general 
> solution, but I didn't sit atop of this for hours.
Is this the only case where ValueToCast will be Loc?   I was unsure about the 
implications of Loc vs. NonLoc in this code, and I didn't want to risk breaking 
a check that should have been valid.  That's why I opted for bailing on an 
LValueToRValue cast at a higher level-- that seemed much safer to me.  I 
certainly might be missing something, but I couldn't find any evidence that an 
LValueToRValue cast should ever need to be checked for enum range errors.   I 
will certainly defer to your judgement, but I'd like to have a better 
understanding of why looking for ValueToCast == Loc would actually be more 
correct.



Comment at: clang/test/Analysis/enum-cast-out-of-range.c:27-34
+enum unscoped_unspecified_t unused;
+void unusedExpr() {
+// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
+// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
+// nothing to do with this checker.
+unused; // expected-warning {{expression result unused}}
+}

NoQ wrote:
> I guess this covers D33672#1537287!
> 
> That said, there's one thing about this test that i don't understand. Why 
> does this AST contain an implicit lvalue-to-rvalue cast at all? This looks 
> like a (most likely otherwise harmless) bug in the AST; if i understand 
> correctly, this should be a freestanding `DeclRefExpr`.
It sure looks like D33672 is the same bug, so yes, I bet it will fix that one.  
 This fix also addresses https://bugs.llvm.org/show_bug.cgi?id=41388.  (Forgive 
me if there's a way to directly link to bugs.llvm.org with this markup.)

Not sure about whether the AST should have represented this node as a 
DeclRefExpr-- that certainly makes some sense, but the LValueToRValue cast 
isn't really wrong either.   At the end of the day, this statement is useless, 
so I'm not sure it matters (much) how the AST represents it.  Representing it 
as LValueToRValue cast wouldn't cause side effects, would it? (E.g.,  cause IR 
to contain explicit dereferencing code?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

If you look at the list of cast kinds, you'll be shocked to see ridiculously 
weird cornercases. Even though lvalue-to-rvalue cast definitely stands out 
(because it's the only one that has very little to do with actual casting), i'd 
still be much more comfortable if we *whitelist* the casts that we check, 
rather than blacklist them.

Can you take a look at the list and find which casts are we actually talking 
about and hardcode them here?



Comment at: clang/test/Analysis/enum-cast-out-of-range.c:27-34
+enum unscoped_unspecified_t unused;
+void unusedExpr() {
+// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
+// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
+// nothing to do with this checker.
+unused; // expected-warning {{expression result unused}}
+}

I guess this covers D33672#1537287!

That said, there's one thing about this test that i don't understand. Why does 
this AST contain an implicit lvalue-to-rvalue cast at all? This looks like a 
(most likely otherwise harmless) bug in the AST; if i understand correctly, 
this should be a freestanding `DeclRefExpr`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added reviewers: gamesh411, NoQ.
Szelethus added subscribers: gamesh411, NoQ.
Szelethus added a comment.

+ @gamesh411 as he took over this checker before I commited it on his behalf, 
+@NoQ because he is far more knowledgeable about this part of the analyzer.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

So this is where the assertion comes from, and will eventually lead to 
`SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this will 
fire on line 427:
```
assert(op == BO_Add);
```
Seems like this happens because `unused`'s value in your testcase will be 
retrieved as a `Loc`, while the values in the enum are (correctly) `NonLoc`, 
and `SValBuilder::evalBinOp` thinks this is some sort of pointer arithmetic (`5 
+ ptr` etc).

How about instead of checking for LValueToRValue cast, we check whether 
`ValueToCast` is `Loc`, and bail out if so? That sounds like a more general 
solution, but I didn't sit atop of this for hours.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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