Page MenuHomePhabricator

[analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field
Needs ReviewPublic

Authored by Szelethus on Sun, Aug 4, 11:58 AM.

Details

Summary

Exactly what it says on the tin! Note that we're talking about interestingness in general, hence this isn't a control-dependency-tracking specific patch.

Diff Detail

Event Timeline

Szelethus created this revision.Sun, Aug 4, 11:58 AM
NoQ added a comment.Wed, Aug 7, 2:59 PM

Nice!!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
224–228

https://llvm.org/doxygen/classllvm_1_1StringLiteral.html:

In order to avoid the invocation of a global constructor, StringLiteral should only be used in a constexpr context

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–212

Ugh, so this code intentionally avoids the complexity of ExprEngine::VisitCommonDeclRefExpr() so that to work differently with references. Okay.

@xazax.hun, do you think this works correctly with lambda captures? (when the DeclRefExpr to a variable might evaluate to a field within the lambda at run-time).

208

Decl? I guess it made sense in the original code but there's no longer an obvious Decl floating around.

xazax.hun added inline comments.Wed, Aug 7, 3:17 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–212

Good point! This definitely worth a test case somewhere somehow!

I would expect DRE->getDecl() to return the captured declaration rather than the field and thus State->getSVal(State->getLValue(VD, LCtx)) to compute the wrong answer. I might be a bit rusty on this topic so of course we would need a carefully crafted test case to make sure :)

Szelethus marked an inline comment as done.
  • Make the generic messages in-class, constexpr fields.
  • Provide plenty of test cases for captured lambda variables. In our meeting we had both had a sight of relief, and some still lingering concerns about this, so maybe it deserves fixing in the long term.
Szelethus marked 4 inline comments as done.Wed, Aug 14, 12:15 PM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–212

So, we seem to be okay with this code in the context of this patch, but have voiced concerns that there probably is a testcase on which this doesn't work. What kind of FIXME message shall I put in before commiting?

Szelethus marked 2 inline comments as done.Wed, Aug 14, 12:16 PM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–212

Also, could you please detail why we think this doesn't/shouldn't work?

NoQ added inline comments.Wed, Aug 14, 1:55 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–194

Also, could you please detail why we think this doesn't/shouldn't work?

FIXME: As seen in VisitCommonDeclRefExpr, sometimes DeclRefExpr may evaluate to a FieldRegion when it refers to a declaration of a lambda capture variable. We most likely need to duplicate that logic here.