This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Jun 27 2019, 6:45 PM
Charusso planned changes to this revision.Jun 27 2019, 6:48 PM
Charusso marked 2 inline comments as done.

Heavily WIP, see inline.

clang/test/Analysis/return-value-guaranteed.cpp
37 ↗(On Diff #206983)

It is blind.

82 ↗(On Diff #206983)

It makes no reports.

Charusso updated this revision to Diff 207133.Jun 28 2019, 1:20 PM
Charusso edited the summary of this revision. (Show Details)
  • Make it match the class name. (Whoops!)
  • Reorder the tuple (swap the return value with the call name).
  • Remove the Projects option.
  • Remove unnecessary input validation.
Charusso updated this revision to Diff 207135.Jun 28 2019, 1:26 PM
  • Fix the note as the class name is optional.
NoQ added a comment.Jun 28 2019, 3:13 PM

Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious. Even though i know what's going on, nobody else does, so it's good to explain that this is for modeling certain weird LLVM-specific APIs that intentionally always return true.

I guess this checker should eventually be merged with StdCLibraryFunctionsChecker and such. I wonder if it's trivial to convert it to use CallDescriptions and then extend it trivially to C++ functions. See also D54149. This is one more reason why i don't want to reinvent APINotes: i'm responsible for one more re-inventing. I'll see if i can generalize upon all of these.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
31–37 ↗(On Diff #207135)

Let's use CallDescriptionMap<bool> (it hasn't landed yet but it's almost there, D62441).

It should save quite some code; feel free to introduce a convenient constructor if it turns out to be hard to produce an initializer list.

81 ↗(On Diff #207135)

Please make the note prunable, so that it didn't bring in the nested stack frame if it appears in a nested stack frame (cf. D61817) - you might need to add a new flag to CheckerContext::getNoteTag() to make this happen.

89 ↗(On Diff #207135)

"returns"

98 ↗(On Diff #207135)

castAs if you're sure.

Generally it's either evaluated conservatively (so the return value is a conjured symbol) or inlined (and in this case returning an undefined value would result in a fatal warning), so return values shouldn't ever be undefined.

The only way to obtain an undefined return value is by binding it in evalCall(), but even then, i'd rather issue a warning immediately.

clang/test/Analysis/return-value-guaranteed.cpp
20 ↗(On Diff #207135)

core.builtin.NoReturnFunctions reacts on this function. You can easily see it in the explodedgraph dump, as the last sink node in test_calls::parseFoo is tagged with that checker. You'll have to pick a different name for testing purposes.

NoQ added inline comments.Jun 28 2019, 4:28 PM
clang/test/Analysis/return-value-guaranteed.cpp
20 ↗(On Diff #207135)

(yes, this is why your tests aren't working)
(see also D63965)

Charusso updated this revision to Diff 207366.Jul 1 2019, 10:19 AM
Charusso marked 8 inline comments as done.
Charusso edited the summary of this revision. (Show Details)
  • Revert the Calls option. It turned out too difficult to create. (May it useless, as we have tons of similar checkers without any option.)
  • Halve the test. We do not want to catch only a call, we have to catch the class as well to prevent error()s like NoReturnFunctionChecker produce.
  • Prunable notes added to CheckerContext.
  • CallDescriptionMap::lookup() now Optional.
Charusso added inline comments.Jul 1 2019, 10:19 AM
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
98 ↗(On Diff #207135)

Sometimes it failed with live tests in the wild.

clang/test/Analysis/return-value-guaranteed.cpp
20 ↗(On Diff #207135)

I was not sure why do you mention an unrelated patch, but I realized as the previous graph-rewriter patches, it has my stuff in the summary. That would be the last thing I considered. It is a nice catch, thanks!

Charusso retitled this revision from [analyzer][WIP] ReturnValueChecker: Model the guaranteed boolean return value of function calls to [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls.
NoQ added inline comments.Jul 1 2019, 12:54 PM
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
52 ↗(On Diff #207366)

Nope. NoReturnFunctionChecker only reacts on C functions.

98 ↗(On Diff #207135)

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

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
14 ↗(On Diff #207366)

Do we still need this include?

39 ↗(On Diff #207366)

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

82 ↗(On Diff #207366)

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

89–91 ↗(On Diff #207366)

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).

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
39 ↗(On Diff #207366)

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

82 ↗(On Diff #207366)

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 ↗(On Diff #207135)

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
82 ↗(On Diff #207366)

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
82 ↗(On Diff #207366)

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
96 ↗(On Diff #207458)

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.

120 ↗(On Diff #207458)

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
71 ↗(On Diff #207458)

auto is encouraged here.

86 ↗(On Diff #207458)

Either cast<> or check for null.

116 ↗(On Diff #207458)

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
120 ↗(On Diff #207458)

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
98–100 ↗(On Diff #207668)

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
107 ↗(On Diff #207668)

Something's wrong with formatting.

109 ↗(On Diff #207668)

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.

145 ↗(On Diff #207668)

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.

145–157 ↗(On Diff #207668)

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

170 ↗(On Diff #207668)

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
98–100 ↗(On Diff #207668)

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

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
145 ↗(On Diff #207668)

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
98–100 ↗(On Diff #207668)

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
90 ↗(On Diff #207703)

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
98–100 ↗(On Diff #207668)

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
88–93 ↗(On Diff #207703)

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

95–96 ↗(On Diff #207703)

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
98–100 ↗(On Diff #207668)

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
90 ↗(On Diff #207703)

Whoops, too heavy copy-pasting.

NoQ added inline comments.Jul 3 2019, 11:32 AM
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
98–100 ↗(On Diff #207668)

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
98–100 ↗(On Diff #207668)

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
95–96 ↗(On Diff #207703)

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
14 ↗(On Diff #207851)

Is DriverDiagnostic used for something?

38 ↗(On Diff #207851)

Maybe this map can be const?

65 ↗(On Diff #207851)

Do you need the cast here?

69 ↗(On Diff #207851)

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
98–100 ↗(On Diff #207668)

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
98–100 ↗(On Diff #207668)

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
88–93 ↗(On Diff #207703)

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".

14 ↗(On Diff #207851)

I thought, but definitely not, good catch!

69 ↗(On Diff #207851)

We want to drop the namespaces for better readability.

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
88–93 ↗(On Diff #207703)

Aha, ok, right!

95–96 ↗(On Diff #207703)

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 :)

69 ↗(On Diff #207851)

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.

154 ↗(On Diff #207862)

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