Page MenuHomePhabricator

[analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls
ClosedPublic

Authored by Charusso on Jun 27 2019, 6:45 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
NoQ added inline comments.Jul 1 2019, 12:54 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

I hope T never gets too expensive to copy. The ideal return value here is Optional<const T &> but i suspect that llvm::Optionals don't support this (while C++17 std::optionals do). Could you double-check my vague memories here?

1126 ↗(On Diff #207366)

People usually use None in such cases; i guess that's because it's slightly more explicit.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
15

Do we still need this include?

40

We can play even more nicely here by requiring namespace llvm as well (just prepend one more item to the list).

83

Let's mention the class name as well! Maybe even the fully qualified namespace.

90–92

I'd like to see what happens when State->assume() returns null. This may happen when the function is inlined and proved to return an unexpected return value. Say, if we believe that the function is always true but the code changes and it suddenly starts returning false and we inline it. Let's add such test case and make sure we behave sanely. Not sure what should the sane behavior be; the safe thing to do would be to generate sink, but we may also try to leave a note telling that something weird has happened ("note: MCParser::Error() SUDDENLY returns false" and mark the inlined stack frame as interesting).

98

I'm mildly interested, can you share a repro if you happen to still have one?

NoQ added inline comments.Jul 1 2019, 1:57 PM
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
36 ↗(On Diff #207366)

I updated D62441, you might have to rebase >.<

Charusso updated this revision to Diff 207456.Jul 1 2019, 6:18 PM
Charusso marked 11 inline comments as done.
  • Fix.
In D63915#1563049, @NoQ wrote:

Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious.

In some of my patches I have not added a description because they are so tiny changes.

I guess this checker should eventually be merged with StdCLibraryFunctionsChecker and such.

Do you have any plans for that?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

Optional<const T *> is working and used widely, I like that.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
40

Well, I have checked 4 of them, they are in the anonymous namespace, so I will leave it.

83

The class::call part would be tricky, because you need to hook what do you have in the CallDescription. It could be done with the decl-matching part, but then you have to rewrite the CallDescriptionMap interface as lookup(), key(), value(), so you could use the hooked info everywhere. It would require too much overhead for a print.

98

After the patch finishes I will make tests and try to break it again.

NoQ added inline comments.Jul 1 2019, 6:23 PM
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
83

Just use CXXMethodDecl::getParent().

Charusso updated this revision to Diff 207458.Jul 1 2019, 6:36 PM
  • I do not like Optional<const T *> anymore.
  • More simple notes.
Charusso marked 2 inline comments as done.Jul 1 2019, 6:37 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
83

Thanks, I really wanted to have a generic solution.

NoQ added a comment.Jul 2 2019, 12:36 PM

This is starting to look great, thanks!

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
97

Let's omit the word "always" here, as we know that there are exceptions from this rule. This may look bad if both Parser::Error() always returns true and Parser::Error() returns false appear in the same report.

121

LLVM coding standard is a fairly specific document: https://llvm.org/docs/CodingStandards.html . It doesn't seem to say anything about parsers.

Let's make this much softer: Parser::Error() returns false and that's it.

Also given that this note always applies to inlined calls, let's move this logic to checkEndFunction(). I.e., we emit the "false" note in checkEndFunction but we emit the "true" note in checkPostCall.

NoQ added inline comments.Jul 2 2019, 12:40 PM
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
72

auto is encouraged here.

87

Either cast<> or check for null.

117

This isn't the stack frame i was talking about, but if you move this code to checkEndFunction it will be.

Charusso updated this revision to Diff 207668.Jul 2 2019, 5:33 PM
Charusso marked 7 inline comments as done.
  • Create FunctionExitPoint diagnostics.
  • Fix.
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
121

Well, we have tons of unspoken standards like: MR, DRE, SE, RD, V.... all makes sense, but I like your idea more.

I'd love to chip in later, if you don't mind, but here is just a couple things that caught my mind that I'd like to share before falling asleep ;)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

This isn't true: the user may decide to only enable non-pathsensitive checkers.

I think the comment should rather state that these whether these checkers are enabled shouldn't be explicitly specified, unless in extreme circumstances (causes a crash in a release build?).

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
228–229 ↗(On Diff #207668)

Hmm, we use interestingness (markInteresting()) already to determine whether we should prune events or not, maybe we could (in the long term) try to make these mechanisms work in harmony.

In any case, could you please add comments about the new parameter to the class doc? :)

NoQ added inline comments.Jul 2 2019, 6:06 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
228–229 ↗(On Diff #207668)

Hmm, we use interestingness (markInteresting()) already to determine whether we should prune events or not, maybe we could (in the long term) try to make these mechanisms work in harmony.

These are two fairly different mechanisms to prevent pruning, but both seem useful. The location context is prunable as long as it's not interesting and there are no non-prunable notes emitted within it.

All basic nodes are prunable; all checker notes are non-prunable; some trackExpressionValue notes are non-prunable. This way the really important notes (such as checker notes) never get lost. And it's kinda nice and easy to understand.

NoQ added inline comments.Jul 2 2019, 6:21 PM
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
108

Something's wrong with formatting.

110

I'd still like to avoid "should". We are in no position to teach them what should their method return. It's not a matter of convention; it's a matter of correctness. We don't want people to go fix their code to return true when they see a note. We only need to point out that the return value is not what they expect.

I suggest removing this note entirely, i.e., early-return when we see an invariant break in checkPostCall, because we've already emitted a note in checkEndFunction.

146

I think it would be more precise to talk about "expected"/"actual" return value. The actual return value may be either concrete (i.e., nonloc::ConcreteInt) or symbolic (i.e., nonloc::SymbolVal).

Also, there's a relatively famous rule of a thumb for making good comments: "comments shouldn't explain what does the code do, but rather why does it do it". Instead of these comments i'd rather have one large comment at the beginning of checkEndFunction explaining what are we trying to do here.

146–158

I think you can squeeze all this code into one big isInvariantBreak(Call, ReturnV) and re-use it across both methods.

171

Something's wrong with formatting.

Charusso updated this revision to Diff 207703.Jul 2 2019, 8:21 PM
Charusso marked 10 inline comments as done.
  • Fix.
  • Refactor.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

Well, I have removed it instead. Makes no sense, you are right.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
146

Great advice, thanks you!

Charusso added inline comments.Jul 2 2019, 8:23 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
210 ↗(On Diff #207703)

It makes perfectly no sense here. Is it the mentioned "class doc"?

This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

I don't think it's a good idea -- we definitely should eventually be able to list packages with descriptions just like checkers (there actually is a FIXME in CheckerRegistry.cpp for that), but this is the next best thing that we have.

How about this:

// The APIModeling package is for checkers that model APIs and don't perform
// any diagnostics. Checkers within this package are enabled by the core or
// through checker dependencies, so one shouldn't enable/disable them by
// hand (unless they cause a crash, but that will cause dependent checkers to be
// implicitly disabled).
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

Why do we need the optional AND the pointer? How about we just return with a nullptr if we fail to find the call?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
210 ↗(On Diff #207703)

You're right. I meant to add it to NoteTags class doc, but whether a NoteTag is prunable isn't a property of NoteTag itself, but rather the infrastructure around it. Oops! I think above const NoteTag *getNoteTag() would be better.

clang/test/Analysis/return-value-guaranteed.cpp
91

Was it? I just tried it out and it doesn't seem to be the case.

NoQ added inline comments.Jul 3 2019, 11:28 AM
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

I don't think any of these are dependencies. Most of the apiModeling checkers are there to suppress infeasible paths (exactly like this one).

I think i'd prefer to leave the comment as-is. We can always update it later.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

Well, that'd be the original code.

I do not like Optional<const T *> anymore.

@Charusso, do you still plan to undo this change?

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
89–94

This can still be re-used by moving into isInvariantBreak (you can give it access to CDM by making it non-static).

96–97

This looks flipped to me, should probably say if (IsInvariantBreak) return;.

Charusso updated this revision to Diff 207851.Jul 3 2019, 11:29 AM
Charusso marked 8 inline comments as done.
  • Fix.
  • Document NoteTag.

This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?

Thanks for the reviews! They are not in the llvm namespace.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

Thanks! Copy-pasted, just that patch produce diagnostics as notes.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

Optional<> stands for optional values, that is why it is made. @NoQ tried to avoid it, but I believe if someone does not use it for an optional value, that break some kind of unspoken standard.

clang/test/Analysis/return-value-guaranteed.cpp
91

Whoops, too heavy copy-pasting.

NoQ added inline comments.Jul 3 2019, 11:32 AM
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

Let's change to don't emit any warnings then.

Charusso marked 5 inline comments as done.Jul 3 2019, 11:45 AM
Charusso added inline comments.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

I think an APIModeling could not be too much thing, most of the stuff around it is not commented out what they do. But as @Szelethus really wanted to inject that, I cannot say no to a copy-paste.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

Well, I am here at 2:1 against Optional<>, so I think it is your decision.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
96–97

It is the Optional<> checking, whether we could obtain the value. I really wanted to write !hasValue(), but no one use that, so it is some kind of unspoken standard to just ! it.

Some nits inline, note that this was just a partial review.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
15

Is DriverDiagnostic used for something?

39

Maybe this map can be const?

66

Do you need the cast here?

70

CXXMethodDecl::getQualifiedNameAsString is not doing what you want here?

Szelethus added inline comments.Jul 3 2019, 11:54 AM
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

Some are, but saying something along the lines of "most of these are enabled by default by the Driver" would've been more correct, but yea, let's leave this for another day.

Charusso updated this revision to Diff 207862.Jul 3 2019, 12:29 PM
Charusso marked 12 inline comments as done.
  • More fix.

Thanks for the reviews! The remaining question is: do we want to use Optional<> in the CallDescriptionMap::lookup()?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
97–101

Well, okay. Something like that is supposed to be correct for future developments:

// Checkers within APIModeling package are model APIs and enabled by the core    
// or through checker dependencies, so one should not enable/disable them by     
// hand (unless they cause a crash, but that will cause dependent checkers to    
// be implicitly disabled).                                                      
// They do not emit any warnings, just suppress infeasible paths.
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
15

I thought, but definitely not, good catch!

70

We want to drop the namespaces for better readability.

89–94

Well, sadly not. In both of the checks it is used inside the call, so you cannot just remove it. I really wanted to make it as simple as possible, but you know, "not simpler".

NoQ accepted this revision.Jul 3 2019, 3:34 PM

I think i like it now!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)

I'd rather leave the original code as is and think more deeply about adding support for Optional<const T &> in the future.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
229 ↗(On Diff #207862)

"Allow BugReporter to omit the note from the report if it would make the displayed bug path significantly shorter."

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
70

Yeah, i think it's important to display exactly what we match for.

We might eventually do some sort of CallDescription.explain() (and then maybe even CallDescriptionMap.explain(Call)) for that purpose.

89–94

Aha, ok, right!

96–97

Mmm. Aha. Ok. Indeed. Sry^^

I was thinking about simply err-ing towards "it's not a break" when we don't know for sure that it's a break, because in this case we have no problems with taking the branch that we want.

But your code seems to be more careful and i like it :)

155

Hmm. It is enough to set IsPrunable to false; once you do that, there's actually no need to mark the stack frame as interesting. I guess this wasn't really necessary.

This revision is now accepted and ready to land.Jul 3 2019, 3:34 PM

This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
1119 ↗(On Diff #207366)
Charusso updated this revision to Diff 207939.Jul 3 2019, 5:38 PM
Charusso marked 9 inline comments as done.
  • Done.

Thanks for the reviews!

This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great!

Yes, it is made for LLVM and tested out 4 times. Thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 5:53 PM