Page MenuHomePhabricator

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

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

Diff Detail

Repository
rL LLVM

Event Timeline

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

Nice!!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
225–226 ↗(On Diff #213261)

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–194 ↗(On Diff #213261)

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

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.Aug 7 2019, 3:17 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–194 ↗(On Diff #213261)

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.Aug 14 2019, 12:15 PM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–194 ↗(On Diff #213261)

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.Aug 14 2019, 12:16 PM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–194 ↗(On Diff #213261)

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

NoQ added inline comments.Aug 14 2019, 1:55 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
192–194 ↗(On Diff #215195)

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.

Szelethus updated this revision to Diff 216283.EditedAug 20 2019, 4:22 PM
  • Add the FIXME
  • Cascade test changes from D65575
NoQ accepted this revision.Aug 21 2019, 10:53 AM

LGTM!

This revision is now accepted and ready to land.Aug 21 2019, 10:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 3:08 PM