This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ClosedPublic

Authored by ZaMaZaN4iK on May 30 2017, 5:25 AM.

Diff Detail

Event Timeline

gamesh411 created this revision.May 30 2017, 5:25 AM
xazax.hun added inline comments.May 30 2017, 5:31 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
36

Use upper case letters for the beginning of member names, parameter names.

80

I'd rather eliminate this reference and push_back the result directly.

105

You could do something like T->getAs<EnumDecl>() directly without repeated cast.

gamesh411 updated this revision to Diff 107886.Jul 24 2017, 4:59 AM

Fixed the naming convention issues. Also applied the suggested modifications inside the overridden checker method.

gamesh411 marked 2 inline comments as done.Jul 24 2017, 5:11 AM
gamesh411 added inline comments.
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
105

Cannot get it with something like T->getAs<EnumDecl>() because T is a QualType, and it has an EnumDecl if it is an EnumeralType (note that it potentially has the Decl, not is the Decl). However the suggestion helped me refactor the method to make it more readable (see in the new diff).

xazax.hun accepted this revision.Jul 24 2017, 5:35 AM
xazax.hun added reviewers: dcoughlin, NoQ, a.sidorin.

Looks good to me. Did you run it on a codebase to check the results?

This revision is now accepted and ready to land.Jul 24 2017, 5:35 AM
NoQ edited edge metadata.Jul 24 2017, 11:49 AM

Yeah, this looks good. How does this compare to alpha.core.BoolConversion, should we maybe merge them?

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
85

C.getState() is the default value (if you see how generateNonFatalErrorNode() calls addTransition() which in turns substitutes nullptr with getState()), so it can be omitted.

89–90

I believe that we'd need to explain the range we're seeing for the value in this case. Probably by asking the constraint manager to dump the range of possible values through some new API call (in case of RangeConstraintManager it should be straightforward).

Because otherwise it may be unobvious from the path what values are possible, when the SVal is a symbol.

Or, even better, we could highlight, with the help of a bug reporter visitor, the places where constraints were added to the symbol (then, again, it'd be better with ranges).

Ideally:

enum { Zero=0, One, Two, Three, Four } y;

void foo(int x) {
  if (x > 3) {
    // core note: Assuming 'x' is greater than 3
    // checker note: The assigned value is assumed to be within range [4, 2147483647]
    ...
  }
  z = x;
  // intermix of 'x' and 'z' is intentional:
  // checker notes should track symbol, not variable,
  // which is what makes them great.
  if (z < 7) {
    // core note: Assuming 'z' is less than 7
    // checker note: The assigned value is assumed to be within range [4, 6]
    if (x > 4) {
      // core note: Assuming 'x' is greater than 4
      // checker note: The assigned value is assumed to be within range [5, 6]
      y = z; // warning: The value provided to the cast expression is in range [5, 6], which is not in the valid range of values for the enum
    }
  }
}

(with a special case when the range consists of one element)

120–123

I suspect that enum values often cover a whole segment, and in this case a pair of assumes for the sides of the segment (or, even better, a single assumeInclusiveRange) would be much faster.

NoQ added a comment.EditedJul 24 2017, 11:58 AM

Also, did the checker flag any intentionally underpopulated enums, eg.

enum {
  Unset = -1
} NumberOfChickens = 15;

(such code is bad, but it is not necessarily buggy - as far as i understand from the CERT rule, we're talking about "unspecified behavior" which some implementations may define and document, and then some people may rely on that)?

Are you sure this checker isn't useful for C?

NoQ accepted this revision.Jul 24 2017, 12:18 PM

These comments of mine are mostly random ideas for follow-up patches, i agree with Gabor this patch is good to land.

Aaron, could you comment on the applicability of this check to C? Thanks in advance.

aaron.ballman edited edge metadata.Jul 25 2017, 6:44 AM

Aaron, could you comment on the applicability of this check to C? Thanks in advance.

C has different rules for their enumerations in that the enumerators are all ints, but the enumeration type is either char, a signed integer type, or an unsigned integer type depending on the values of the enumerators. So this problem exists:

enum E {
  one = 1
};

void f(int i) {
  enum E e = (enum E)i;
}

int main(void) {
  f(1024);
}

If enum E is represented by a char, then the cast causes a loss of precision. However, this isn't undefined behavior in C like it is in C++.

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
30

s/2/two

35

No need for the access specifier; it defaults to private.

aaron.ballman added inline comments.Jul 25 2017, 6:44 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
48

Please do not use auto here as the type is not spelled out in the initialization. Also, you can drop the const qualifier if it's not a pointer or reference type.

75–76

Instead of enumerating over decls() and then casting, just enumerate over enumerators() and the cast isn't needed. Or, even better:

EnumValueVector DeclValues(ED->enumerator_end() - ED->enumerator_begin());
std::transform(ED->enumerator_begin(), ED->enumerator_end(), DeclValues.begin(),
                       [](const EnumConstantDecl *D) { return D->getInitVal(); });
85

Don't use auto here.

88–90

Also, diagnostics should not be full sentences or grammatically correct, so drop the capitalization and full stop.

117

You can use castAs<>() because you've already determined it's an enumeral type.

122

You can use llvm::any_of() and pass in the container.

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

I think it's reasonable to diagnose in C when converting the value would lose precision, so long as the check implements that behavior properly for C. That would suggest it belongs somewhere other than cplusplus.

gamesh411 updated this revision to Diff 109546.Aug 3 2017, 6:52 AM

Applied most of the suggested changes, thanks for all the insights!

gamesh411 marked 9 inline comments as done.EditedAug 3 2017, 7:07 AM

As for the the loss of precision problem, in the special case of char the size of char is known. However does the analysis have the necessary information in this stage to know the size of an int for example? I found bit-width specifying information in the llvm::Type class which is used in the code generation phase. It could be done by checking on a per type basis, but then again, it could possibly lead to false positives. Correct me if I am wrong.

Should the checker be still considered for default optin package?

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
89–90

As far as I know the current reporting system, and the ConstraintManager API does not allow for this degree of finesse when it comes to diagnostics. It is however a good idea worth pursuing as part of the enhancement of the aforementioned subsystems.

120–123

I have cosidered assumeInclusiveRange, however there can be enums with holes in represented values (for example the the enums in the first part of the test cases). In such case it would not be practical to call assumeInclusiveRange for all subranges.

As for the the loss of precision problem, in the special case of char the size of char is known. However does the analysis have the necessary information in this stage to know the size of an int for example? I found bit-width specifying information in the llvm::Type class which is used in the code generation phase. It could be done by checking on a per type basis, but then again, it could possibly lead to false positives. Correct me if I am wrong.

The frontend has this information available to it as well, through the ASTContext. See getTypeSize(), getTypeSizeInChars(), etc.

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
36

You can remove the newline.

75–76

I think my suggestion was a bit more efficient than your implementation, was there a reason you avoided std::transform()?

gamesh411 updated this revision to Diff 112154.EditedAug 22 2017, 5:16 AM

I have implemented the std::transform. The previous version used std::for_each because the iterator for enum declarations was not a random access iterator, but it turned out that I can solve this problem via std::distance. Thanks for sticking to your opinion on this one, because of it I could learn something new.

As for the type size checking problem: I have noticed that there is already a generic overflow checker, and that would detect the 'assigning invalid value to an enum' problem. The only advantage of doing it here is that we could probably give a more specific error diagnostic (then again this could be done in the overflow checker as well). So in my opinion that concern belongs there, and this checker should nevertheless belong in the generic optin package, because of the c-style cast is also checked in it.

gamesh411 updated this revision to Diff 112156.Aug 22 2017, 5:19 AM
gamesh411 marked an inline comment as done.
gamesh411 marked an inline comment as done.
aaron.ballman added inline comments.Aug 24 2017, 6:56 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
20

If this check is intended to conform to CERT's INT50-CPP rule, you should put a link to the wiki entry for it here as well.

73

You can use llvm::transform(ED->enumerators(), ...) instead -- the semantics are identical to std::transform(), but it takes a range instead of a pair of iterators.

Also needs an entry to www/analyzer/alpha_checks.html.

test/Analysis/enum-cast-out-of-range.cpp
2
  1. Prefer %clang_analyze_cc1
  2. Always run core checkers too. -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange
  3. Organize the run line to fit into 80 cols (if possible)
// RUN: %clang_analyze_cc1 \
// RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
// RUN:   -std=c++11 -verify %s
Szelethus added inline comments.Oct 12 2018, 5:07 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
337–340

Sort alphabetically.

ZaMaZaN4iK commandeered this revision.Oct 28 2018, 11:13 AM
ZaMaZaN4iK added a reviewer: gamesh411.
ZaMaZaN4iK added a subscriber: ZaMaZaN4iK.
ZaMaZaN4iK added inline comments.
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
20

Which wiki entry do you mean?

  1. Changed std::transform to llvm::transform
  2. Described the check in .html file
  3. Fixed RUN command for the test file
ZaMaZaN4iK marked 2 inline comments as done.Oct 28 2018, 12:23 PM

I think is good to go! Please wait on @xazax.hun or @NoQ to have the final say (it's been a while since this revision was accepted by them), but for a work-in-progress alpha checker, I like what I'm seeing.

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
20
37

You can acquire SValBuilder from ProgramState:
PS->getStateManager()->getSvalBuilder()

67

Prefer using.

Add the reference to CERT page

ZaMaZaN4iK marked 3 inline comments as done.Oct 29 2018, 3:25 AM
ZaMaZaN4iK added inline comments.
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
20

Added a note about CERT's coding standard recommendaation. Didn't add the reference to web page because web pages can become broken. I think standard and check names are quite good explanation for here

ZaMaZaN4iK marked 2 inline comments as done.Oct 29 2018, 3:25 AM
aaron.ballman added inline comments.Oct 29 2018, 4:52 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
20

That was the wiki entry I was talking about, but we do typically add links directly to coding guidelines that we implement checks for (at least, we do in clang-tidy, perhaps the static analyzer has a different policy) -- it makes it easier for folks reading the code to reference the rationale behind why the check behaves the way it does. However, what you have is also fine -- it's searchable.

test/Analysis/enum-cast-out-of-range.cpp
2

Can you add a test for bit-field assignment. e.g.,

struct S {
  some_enum E : 4;
};

int main(void) {
  struct S s;
  s.E = 9;
}

Add new test for enum bit field

ZaMaZaN4iK marked 2 inline comments as done.Oct 29 2018, 8:06 AM
ZaMaZaN4iK marked 2 inline comments as done.

Fix typedef -> using

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
37

Is there any difference? Is it critical to get SValBuilder from `CheckerContext' ?

NoQ accepted this revision.Oct 29 2018, 1:24 PM

Thanks! I like where this is going. Let's land the patch and continue developing it incrementally in trunk.

The next steps for this checker are, in my opinion:

  • Do the visitor thingy that i requested in inline-311373. I think it's a necessary thing to do, but don't jump into implementing it right away: i already have some code for this that i want to share.
  • Play nicely with typedefs. For now i believe the checker ignores them because you can't cast TypedefType to EnumType. Once this is done, it will be worth it to include the name of the enum in the warning message.
  • Optimize the code using assumeInclusiveRange. Because assume is an expensive operation, i'd like to avoid doing it O(n) times for contiguous enums in which just 2 assumes are enough (or, even better, as single assumeInclusiveRange).
  • See how this checker performs on real code, fix crashes and false positives if any.
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
37

There's only one instance of SValBuilder in existence at any particular moment of time. The same applies to BasicValueFactory, SymbolManager, MemRegionManager, ConstraintManager, StoreManager, ProgramStateManager, ...

All these objects live within ExprEngine and have the same lifetime.

ExprEngine, together with all these objects, is created from scratch for every analysis of a top-level function.

AST entities, such ast ASTContex, on the contrary, live much longer - only one is created per clang process. That is, until somebody takes ASTImporter and tries to frankenstein multiple ASTs into one :)

84–86

diagnostics should not be full sentences or grammatically correct, so drop the capitalization and full stop

No, in fact, Static Analyzer diagnostics are traditionally capitalized, unlike other warnings, so i'm afraid this will need to be changed back >.< Still no fullstop though.

Szelethus added inline comments.Oct 29 2018, 2:08 PM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
37

Then I guess no, it's not critical. ^-^

In D33672#1279305, @NoQ wrote:

Thanks! I like where this is going. Let's land the patch and continue developing it incrementally in trunk.

The next steps for this checker are, in my opinion:

  • Do the visitor thingy that i requested in inline-311373. I think it's a necessary thing to do, but don't jump into implementing it right away: i already have some code for this that i want to share.
  • Play nicely with typedefs. For now i believe the checker ignores them because you can't cast TypedefType to EnumType. Once this is done, it will be worth it to include the name of the enum in the warning message.
  • Optimize the code using assumeInclusiveRange. Because assume is an expensive operation, i'd like to avoid doing it O(n) times for contiguous enums in which just 2 assumes are enough (or, even better, as single assumeInclusiveRange).
  • See how this checker performs on real code, fix crashes and false positives if any.

Thank you for the roadmap! Honestly I am not so familiar with Clang AST and Clang Static Analyzer details (all my experience is writing some simple checkers for clang-tidy and watching some videos about Clang/Clang Static Analyzers), so I even don't understand of your words :) So don't rely on quick fixes from my side. But I think I can write something useful after couple of tries (slowly, but I can). Thank you for the help!

Thank you for the roadmap! Honestly I am not so familiar with Clang AST and Clang Static Analyzer details (all my experience is writing some simple checkers for clang-tidy and watching some videos about Clang/Clang Static Analyzers), so I even don't understand of your words :) So don't rely on quick fixes from my side. But I think I can write something useful after couple of tries (slowly, but I can). Thank you for the help!

Always happy to assist here or on cfe-dev, feel free to send a mail! If you haven't seen it yet, the Static Analyzer Developer Guide may help you expand your current knowledge greatly, especially the links on the bottom.

Which other changes and/or approvals are required for merging into trunk?

Which other changes and/or approvals are required for merging into trunk?

There's one unresolved comment from earlier and a few very minor nits that I found, but I think this is ready to land otherwise. Do you need someone to commit on your behalf? If so, then please rebase off ToT and update the patch and someone here can commit for you.

lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
84–86

This comment is still unresolved.

96

const auto *

test/Analysis/enum-cast-out-of-range.cpp
38

Formatting is off here.

ZaMaZaN4iK added inline comments.Nov 4 2018, 1:30 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Why do we need this change here? If I understand correctly, with const auto* we also need change initializer to C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>().getPointer(). But I don't understand why we need this.

ZaMaZaN4iK updated this revision to Diff 172515.Nov 4 2018, 1:56 AM
  1. Fix indentation in test file
  2. Return back capitalization for the checker description
ZaMaZaN4iK marked 3 inline comments as done.Nov 4 2018, 1:57 AM
lebedev.ri added inline comments.
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Is ValueToCastOptional a pointer, a reference, or just an actual DefinedOrUnknownSVal? I can't tell.
(sidenote: would be great to have a clang-tidy check for this.)

ZaMaZaN4iK added inline comments.Nov 4 2018, 2:10 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

ValueToCastOptional is llvm::Optional<DefinedOrUnknownSVal>

lebedev.ri added inline comments.Nov 4 2018, 2:12 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

See, all my guesses were wrong. That is why it should not be auto at all.

ZaMaZaN4iK added inline comments.Nov 4 2018, 2:18 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

I don't agree with you for this case. Honestly it's like a yet another holywar question. If we are talking only about this case - here you can see getAs<DefinedOrUnknownSVal> part of the expression. this means clearly (at least for me) that we get something like DefinedOrUnknownSVal. What we get? I just press hotkey in my favourite IDE/text editor and see that getAs returns llvm::Optional<DefinedOrUnknownSVal>. From my point of view it's clear enough here.

If we are talking more generally about question "When should we use auto at all? " - we can talk, but not here, I think :)

lebedev.ri added inline comments.Nov 4 2018, 2:23 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable comes to mind.

What we get? I just press hotkey in my favourite IDE/text editor and see that getAs returns llvm::Optional<DefinedOrUnknownSVal>

Which hotkey do i need to press to see this here, in the phabricator?

This really shouldn't be auto, if you have to explain that in the variable's name, justify it in review comments.

ZaMaZaN4iK added inline comments.Nov 4 2018, 2:28 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Ok, didn't know about such LLVM coding standard. Of course, with this information I will fix using auto here. Thank you.

ZaMaZaN4iK updated this revision to Diff 172516.EditedNov 4 2018, 2:30 AM

Fix using auto in case where it leads to worse readability

Szelethus added inline comments.Nov 4 2018, 2:35 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Actually, I disagree. In the Static Analyzer we use auto if the return type is in the name of the expression, and getAs may fail, so it returns an Optional. In the case where a nullptr may be returned to signal failure, auto * is used, so I believe that auto is appropriate here.

96

But don't change it back now, it doesn't matter a whole lot :D

aaron.ballman added inline comments.Nov 4 2018, 7:52 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Actually, I disagree. In the Static Analyzer we use auto if the return type is in the name of the expression, and getAs may fail, so it returns an Optional.

The static analyzer should be following the same coding standard as the rest of the project. This is new code, so it's not matching inappropriate styles from older code, so there's really no reason to not match the coding standard.

In the case where a nullptr may be returned to signal failure, auto * is used, so I believe that auto is appropriate here.

I disagree that auto is appropriate here. The amount of confusion about the type demonstrates why.

NoQ added inline comments.Nov 4 2018, 9:38 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

I confirm the strong local tradition of preferring auto for optional SVals in new code, and i believe it's well-justified from the overall coding guideline's point of view. Even if you guys didn't get it right from the start, the amount of Analyzer-specific experience required to understand that it's an optional is very minimal and people get used to it very quickly when they start actively working on the codebase.

On one hand, being a class that combines LLVM custom RTTI with pass-by-value semantics (i.e., it's like QualType when it comes to passing it around and like Type when it comes to casting), optionals are inevitable for representing SVal dynamic cast results.

On the other hand, SVal is one of the most common classes of objects in the Static Analyzer (like, maybe, Stmt in Clang; i think a lot of people who are interested in Static Analyzer more than in the rest of Clang learn about SVals earlier than about Stmts), and in particular SVal casts are extremely common (around 3-4 SVal::getAs<T>() casts per a path-sensitive checker, not counting castAs, ~2x more common than arithmetic operations over SVals, only ~3x less common than all sorts of dyn_cast in all checkers, including path-insensitive checkers), so it's something you get used to really quickly.

Writing Optional<DefinedOrUnknownSVal> DV = V.getAs<DefinedOrUnknownSVal>(); in a lot of checker callbacks is annoying for pretty much all checker developers from day 1. This clutters the most important parts of the checker's logic: transfer functions and the definition of the error state. Most bugs are in *that* part of the code, and those bugs are *not* due to using auto for casts. Not using auto almost doubles the amount of code you need to write to perform a simple run-time type check. With a more verbose variable name or a bit more code around it, it also causes a line break.

And it only provides information that most of the readers either already know ("SVal is a value-type, so it uses optionals"), or memorize immediately after encountering this pattern once on their first day, or barely even care (optionals are used like pointers anyway). Being finally allowed to replace it with just auto after migration to C++11 was divine.

So i'm still strongly in favor of keeping this pattern included in the list of "places where the type is already obvious from the context". This is the top-1 source of annoying boilerplate in Static Analyzer, and it's as easy to remember as it is to learn that dyn_cast<T>(S) returns a pointer (or a reference? - you need to look up the definition of S to figure this out, unlike V.getAs<T>() that is always an optional for SVals).

P.S. Though i admit that coming up with a less annoying API is also a good idea :)
P.P.S. I guess some sort of Optional<auto> would have made everybody happy. But it's not a thing unfortunately :(

aaron.ballman added inline comments.Nov 4 2018, 12:17 PM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Thank you for the explanation. However, I'm still not convinced this is supported by the coding style guideline. The point to "places where the type is already obvious from the context" has (in my estimation) only covered places where the type truly is obvious. i.e., it's either spelled out explicitly (dyn_cast<>, getAs<>, etc) or it's entirely immaterial for understanding (iterators). Whether a value is optional strikes me as highly pertinent information for code reviewers or maintainers to understand because it conveys information about the validity of a value. To me, this is just as important as the distinction between auto, auto *, and auto &, which we make the user spell out in the case of pointers and references. From what I can tell, we also spell out Optional pretty consistently elsewhere in the product.

The rationale that this is a common idiom makes the push-back understandable, but at the same time, use of auto where the type is not immediately obvious makes the static analyzer more hostile to get into, especially for people who only stray into this part of the code base periodically (which is one major motivation for having a style guide in the first place).

I also don't do a ton of work in the static analyzer, so take this feedback with a grain of salt. I certainly don't expect to change the static analyzer's decision here. However, this is also the second time this week I've run into "we don't do that in the static analyzer" and both times have been in response to common review feedback that applies elsewhere in the product and both times have consumed review time from multiple people in order to resolve.

Ultimately, a style guide is just that -- a guide, not a mandate. It may also be that deviation from the guide here is reasonable, but it does come at a cost. That said, this ship may have already sailed; churning the code to remove use of auto comes with its own costs that may not be worth paying.

NoQ added inline comments.Nov 4 2018, 10:48 PM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Like, i mean, until now i always thought of using auto for casts, including casts of value-types, as of something that the coding guideline explicitly asks for, and didn't even consider that it might be a deviation, and that's the first time i hear this concern raised. Though now that you point this out, the problem with highlighting that it's the llvm::Optional is indeed an unclear situation.

For me as a maintainer, pure auto is definitely much easier to read and review when used with getAs. For a casual reader - i don't know, i accept that it might be harder than i imagine.

As usual, i'm happy to be proven wrong and/or accept any decision on that matter. To me coding guidelines are definitely above personal preferences.

conveys information about the validity of a value

This is conveyed via the choice between getAs and castAs.

Hi!
Thanks for your reviews, although I haven't been active for some time now.
I personally do not have commit rights, so could someone else take care of it?

aaron.ballman added inline comments.Nov 5 2018, 7:48 AM
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
96

Though now that you point this out, the problem with highlighting that it's the llvm::Optional is indeed an unclear situation.

Yup!

This is conveyed via the choice between getAs and castAs.

That's a fair point, but in the rest of the code base, getAs<> returns a pointer. What started off this discussion was me asking for const auto * to clarify that it was a pointer to constant data when it really wasn't one (at least directly). So while it does convey the semantics, it still is a bit confusing.

However, I think you're right. The difference between getAs<> and castAs<> may be sufficient once you know the static analyzer a bit better.

I'll commit tomorrow, when I have time to keep an eye out for buildbots. Thanks!

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Jun 10 2019, 6:19 PM

Hey, i'm seeing a crash in this checker, would you like to look at it? It looks as if you're not being careful about dereferences/lvalue-to-rvalue-casts so it tries to compare &e to e1.

$ cat repro.c

enum E { e1 };

void foo() {
  enum E e;
  e;
}

$ clang --analyze repro.c -Xclang -analyzer-checker=alpha.cplusplus.EnumCastOutOfRange

Assertion failed: (op == BO_Add), function evalBinOp, file /Users/adergachev/llvm/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp, line 427.

Stack dump:
0.	Program arguments: /Users/adergachev/debug/bin/clang-9 -cc1 -triple x86_64-apple-macosx10.14.0 -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -analyze -disable-free -main-file-name repro.c -analyzer-store=region -analyzer-opt-analyze-nested-blocks -analyzer-checker=core -analyzer-checker=apiModeling -analyzer-checker=unix -analyzer-checker=osx -analyzer-checker=deadcode -analyzer-checker=security.insecureAPI.UncheckedReturn -analyzer-checker=security.insecureAPI.getpw -analyzer-checker=security.insecureAPI.gets -analyzer-checker=security.insecureAPI.mktemp -analyzer-checker=security.insecureAPI.mkstemp -analyzer-checker=security.insecureAPI.vfork -analyzer-checker=nullability.NullPassedToNonnull -analyzer-checker=nullability.NullReturnedFromNonnull -analyzer-output plist -w -mrelocation-model pic -pic-level 2 -mthread-model posix -mdisable-fp-elim -masm-verbose -munwind-tables -target-cpu penryn -dwarf-column-info -debugger-tuning=lldb -ggnu-pubnames -target-linker-version 510.2 -resource-dir /Users/adergachev/debug/lib/clang/9.0.0 -internal-isystem /usr/local/include -internal-isystem /Users/adergachev/debug/lib/clang/9.0.0/include -internal-externc-isystem /usr/include -fdebug-compilation-dir /Users/adergachev/test -ferror-limit 19 -fmessage-length 142 -stack-protector 1 -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fobjc-runtime=macosx-10.14.0 -fmax-type-align=16 -fdiagnostics-show-option -fcolor-diagnostics -analyzer-checker=alpha.cplusplus.EnumCastOutOfRange -o repro.plist -x c repro.c
1.	<eof> parser at end of file
2.	While analyzing stack:
	#0 Calling foo
3.	repro.c:5:3: Error evaluating statement
4.	repro.c:5:3: Error evaluating statement
0  clang-9                  0x00000001043f98cc llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  clang-9                  0x00000001043f9e89 PrintStackTraceSignalHandler(void*) + 25
2  clang-9                  0x00000001043f7bd6 llvm::sys::RunSignalHandlers() + 118
3  clang-9                  0x00000001043fd032 SignalHandler(int) + 210
4  libsystem_platform.dylib 0x00007fff63a0eb5d _sigtramp + 29
5  clang-9                  0x000000010a444d08 llvm::DenseMapInfo<llvm::codeview::GloballyHashedType>::Tombstone + 3005112
6  libsystem_c.dylib        0x00007fff638ce6a6 abort + 127
7  libsystem_c.dylib        0x00007fff6389720d basename_r + 0
8  clang-9                  0x0000000107048c06 clang::ento::SValBuilder::evalBinOp(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>, clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal, clang::QualType) + 950
9  clang-9                  0x0000000107048ef0 clang::ento::SValBuilder::evalEQ(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>, clang::ento::SVal, clang::ento::SVal) + 144
10 clang-9                  0x0000000107048f82 clang::ento::SValBuilder::evalEQ(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>, clang::ento::DefinedOrUnknownSVal, clang::ento::DefinedOrUnknownSVal) + 114
11 clang-9                  0x0000000106afe56f (anonymous namespace)::ConstraintBasedEQEvaluator::operator()(llvm::APSInt const&) + 175
12 clang-9                  0x0000000106afe3ef bool std::__1::any_of<llvm::APSInt*, (anonymous namespace)::ConstraintBasedEQEvaluator>(llvm::APSInt*, llvm::APSInt*, (anonymous namespace)::ConstraintBasedEQEvaluator) + 47
13 clang-9                  0x0000000106afdd18 bool llvm::any_of<llvm::SmallVector<llvm::APSInt, 6u>&, (anonymous namespace)::ConstraintBasedEQEvaluator>(llvm::SmallVector<llvm::APSInt, 6u>&, (anonymous namespace)::ConstraintBasedEQEvaluator) + 72
14 clang-9                  0x0000000106afdbb9 (anonymous namespace)::EnumCastOutOfRangeChecker::checkPreStmt(clang::CastExpr const*, clang::ento::CheckerContext&) const + 297
15 clang-9                  0x0000000106afda85 void clang::ento::check::PreStmt<clang::CastExpr>::_checkStmt<(anonymous namespace)::EnumCastOutOfRangeChecker>(void*, clang::Stmt const*, clang::ento::CheckerContext&) + 53
16 clang-9                  0x0000000106f128a2 clang::ento::CheckerFn<void (clang::Stmt const*, clang::ento::CheckerContext&)>::operator()(clang::Stmt const*, clang::ento::CheckerContext&) const + 66
17 clang-9                  0x0000000106f1232c (anonymous namespace)::CheckStmtContext::runChecker(clang::ento::CheckerFn<void (clang::Stmt const*, clang::ento::CheckerContext&)>, clang::ento::NodeBuilder&, clang::ento::ExplodedNode*) + 220
18 clang-9                  0x0000000106effd71 void expandGraphWithCheckers<(anonymous namespace)::CheckStmtContext>((anonymous namespace)::CheckStmtContext, clang::ento::ExplodedNodeSet&, clang::ento::ExplodedNodeSet const&) + 561
19 clang-9                  0x0000000106eff8a9 clang::ento::CheckerManager::runCheckersForStmt(bool, clang::ento::ExplodedNodeSet&, clang::ento::ExplodedNodeSet const&, clang::Stmt const*, clang::ento::ExprEngine&, bool) + 217
20 clang-9                  0x0000000106f81906 clang::ento::CheckerManager::runCheckersForPreStmt(clang::ento::ExplodedNodeSet&, clang::ento::ExplodedNodeSet const&, clang::Stmt const*, clang::ento::ExprEngine&) + 70
21 clang-9                  0x0000000106f70131 clang::ento::ExprEngine::VisitCast(clang::CastExpr const*, clang::Expr const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) + 161
22 clang-9                  0x0000000106f45224 clang::ento::ExprEngine::Visit(clang::Stmt const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) + 8084
23 clang-9                  0x0000000106f40f6e clang::ento::ExprEngine::ProcessStmt(clang::Stmt const*, clang::ento::ExplodedNode*) + 510
24 clang-9                  0x0000000106f40bf9 clang::ento::ExprEngine::processCFGElement(clang::CFGElement, clang::ento::ExplodedNode*, unsigned int, clang::ento::NodeBuilderContext*) + 201
25 clang-9                  0x0000000106f270e8 clang::ento::CoreEngine::HandlePostStmt(clang::CFGBlock const*, unsigned int, clang::ento::ExplodedNode*) + 296
26 clang-9                  0x0000000106f261b0 clang::ento::CoreEngine::dispatchWorkItem(clang::ento::ExplodedNode*, clang::ProgramPoint, clang::ento::WorkListUnit const&) + 880
27 clang-9                  0x0000000106f25ac9 clang::ento::CoreEngine::ExecuteWorkList(clang::LocationContext const*, unsigned int, llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>) + 1481
28 clang-9                  0x0000000106880b14 clang::ento::ExprEngine::ExecuteWorkList(clang::LocationContext const*, unsigned int) + 84
29 clang-9                  0x00000001068808e5 (anonymous namespace)::AnalysisConsumer::RunPathSensitiveChecks(clang::Decl*, clang::ento::ExprEngine::InliningModes, llvm::DenseSet<clang::Decl const*, llvm::DenseMapInfo<clang::Decl const*> >*) + 341
30 clang-9                  0x00000001068803f5 (anonymous namespace)::AnalysisConsumer::HandleCode(clang::Decl*, unsigned int, clang::ento::ExprEngine::InliningModes, llvm::DenseSet<clang::Decl const*, llvm::DenseMapInfo<clang::Decl const*> >*) + 501
31 clang-9                  0x000000010687108f (anonymous namespace)::AnalysisConsumer::HandleDeclsCallGraph(unsigned int) + 543
32 clang-9                  0x000000010686f998 (anonymous namespace)::AnalysisConsumer::runAnalysisOnTranslationUnit(clang::ASTContext&) + 440
33 clang-9                  0x00000001068690db (anonymous namespace)::AnalysisConsumer::HandleTranslationUnit(clang::ASTContext&) + 283
34 clang-9                  0x00000001070b086c clang::ParseAST(clang::Sema&, bool, bool) + 940
35 clang-9                  0x000000010512a6e2 clang::ASTFrontendAction::ExecuteAction() + 322
36 clang-9                  0x0000000105129cf0 clang::FrontendAction::Execute() + 112
37 clang-9                  0x000000010509b49c clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 1548
38 clang-9                  0x00000001051b092c clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 2060
39 clang-9                  0x00000001014354c1 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 1233
40 clang-9                  0x000000010142871f ExecuteCC1Tool(llvm::ArrayRef<char const*>, llvm::StringRef) + 159
41 clang-9                  0x00000001014275b9 main + 1433
42 libdyld.dylib            0x00007fff638293d5 start + 1
43 libdyld.dylib            0x0000000000000049 start + 2625465461
clang-9: error: unable to execute command: Abort trap: 6
clang-9: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 9.0.0 (https://github.com/llvm/llvm-project.git e917ff76a0f25cf6c0d3de6cceb9e84475339183)
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 6:19 PM
Herald added a subscriber: Charusso. · View Herald Transcript