This is an archive of the discontinued LLVM Phabricator instance.

Differential D17627

Fix false positives for for-loop-analysis warning
ClosedPublic

Authored by steven_wu on Feb 25 2016, 5:08 PM.

Details

Reviewers
rtrieu
rjmccall
thakis
doug.gregor
Commits
rG92910f69a06e: Fix false positives for for-loop-analysis warning
rC263087: Fix false positives for for-loop-analysis warning
rL263087: Fix false positives for for-loop-analysis warning
Summary

For PseudoObjectExpr, the DeclMatcher need to search only all the semantics
but also need to search pass OpaqueValueExpr for all potential uses for the Decl.

Diff Detail

Event Timeline

steven_wu updated this revision to Diff 49125.Feb 25 2016, 5:08 PM
steven_wu retitled this revision from to Fix false positives for for-loop-analysis warning.
steven_wu updated this object.
steven_wu added reviewers: rtrieu, thakis.
steven_wu added a subscriber: cfe-commits.
rtrieu edited edge metadata.Feb 25 2016, 9:25 PM

This seems rather strange. Usually, a OpaqueValueExpr will hold an expression that is held elsewhere in the AST. However, in this case, it appears that all the references to --i are all behind OpaqueValueExpr's, so they aren't processed.

`-CStyleCastExpr 0x5675d30 <line:11:5, col:19> 'void' <ToVoid>
  `-PseudoObjectExpr 0x5675d00 <col:11, col:19> 'id':'id'
    |-ObjCSubscriptRefExpr 0x5675c90 <col:11, col:19> '<pseudo-object type>' lvalue Kind=ArraySubscript GetterForArray="(null)" SetterForArray="(null)
    | |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    | |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    | `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    |   `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |     `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    |   `-DeclRefExpr 0x5675b80 <co        `-CStyleCastExpr 0x5675d30 <line:11:5, col:19> 'void' <ToVoid>
  `-PseudoObjectExpr 0x5675d00 <col:11, col:19> 'id':'id'
    |-ObjCSubscriptRefExpr 0x5675c90 <col:11, col:19> '<pseudo-object type>' lvalue Kind=ArraySubscript GetterForArray="(null)" SetterForArray="(null)
    | |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    | |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    | `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    |   `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |     `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    |-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    | `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |   `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    `-ObjCMessageExpr 0x5675cc8 <col:11> 'id':'id' selector=objectAtIndexedSubscript:
      |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
      | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
      |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
      `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
        `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
          `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'l:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    |-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    | `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |   `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    `-ObjCMessageExpr 0x5675cc8 <col:11> 'id':'id' selector=objectAtIndexedSubscript:
      |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
      | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
      |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
      `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
        `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
          `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'

Typically, we skip OpaqueValueExpr so that we don't process the same expression multiple times. I don't know enough about Objective C to know whether this is the intended pattern or an oversight. Instead of visiting all the sub-expressions of OpaqueValueExpr's, try to make a tighter visitor to avoid repeating work. I think checking the sub-expressions of ObjCSubscriptRefExpr, and looking through the OpaqueValueExpr's there would be the best way for this.

steven_wu added reviewers: rjmccall, doug.gregor.Feb 26 2016, 11:05 AM
steven_wu added subscribers: doug.gregor, rjmccall.

Looking through the subscript sounds fine but I would like to know if this is indeed the only case that is being ignored because of OpaqueValueExpr and if everything is behind OpaqueValueExpr is a bug in building AST.

John, Doug, any opinion on this?

xazax.hun added a subscriber: xazax.hun.Feb 29 2016, 12:18 AM
steven_wu updated this revision to Diff 49549.Mar 1 2016, 2:12 PM
steven_wu edited edge metadata.

I did some more digging of the issue. It seems ObjCSubscriptRefExpr always
rebuilds and hinds both base and key behind OpaqueValueExpr. See
ObjCSubscriptOpBuilder::rebuildAndCaptureObject so it looks the AST generated
is intentional. If that is the case, Richard's suggestion is the best fix I
can think about. Update the patch to reflect the suggestion.

steven_wu added a comment.Mar 9 2016, 9:50 AM

ping

rjmccall edited edge metadata.Mar 9 2016, 11:19 AM

The right fix is probably at the PseudoObjectExpr level; you should probably be looking at the semantic expressions instead of the syntactic.

steven_wu updated this revision to Diff 50186.Mar 9 2016, 12:52 PM
steven_wu edited edge metadata.

Update the patch to address John's feedback. We shouldn't even checking
ObjCSubscript but looking at the Semantics for PseudoObjectExpr only.

steven_wu updated this object.Mar 9 2016, 12:53 PM
rjmccall added inline comments.Mar 9 2016, 3:15 PM
lib/Sema/SemaStmt.cpp
1448

"Look past the OVE into the expression it binds."

1449

I believe this is never null.

steven_wu updated this revision to Diff 50212.Mar 9 2016, 3:51 PM
steven_wu updated this object.

Update according to feedback. I agree that OVE should never be null as semantics
of PseudoObjectExpr.

rjmccall added a comment.Mar 9 2016, 6:05 PM

Thanks, LGTM.

Closed by commit rL263087: Fix false positives for for-loop-analysis warning (authored by steven_wu). · Explain WhyMar 9 2016, 6:07 PM
This revision was automatically updated to reflect the committed changes.
steven_wu marked 2 inline comments as done.

Revision Contents

PathSize
lib/
Sema/
12 lines
test/
SemaObjC/
15 lines

Diff 49549

lib/Sema/SemaStmt.cpp

Show First 20 Lines • Show All 1,434 Lines • ▼ Show 20 Lines public:
} }
void VisitDeclRefExpr(DeclRefExpr *E) { void VisitDeclRefExpr(DeclRefExpr *E) {
if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl())) if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
if (Decls.count(VD)) if (Decls.count(VD))
FoundDecl = true; FoundDecl = true;
} }
void VisitObjCSubscriptRefExpr(ObjCSubscriptRefExpr *E) {
// ObjCSubscriptRefExpr always converts its subexpressions behind OVE.
// Look pass OVE in case it has the only reference to Decl.
if (auto *OVE = dyn_cast<OpaqueValueExpr>(E->getKeyExpr())) {
if (Expr *Key = OVE->getSourceExpr())
Visit(Key);
rjmccallUnsubmitted
Done
ReplyInline Actions

"Look past the OVE into the expression it binds."

rjmccall: "Look past the OVE into the expression it binds."
} else
rjmccallUnsubmitted
Done
ReplyInline Actions

I believe this is never null.

rjmccall: I believe this is never null.
Visit(E->getKeyExpr());
Visit(E->getBaseExpr());
}
bool FoundDeclInUse() { return FoundDecl; } bool FoundDeclInUse() { return FoundDecl; }
}; // end class DeclMatcher }; // end class DeclMatcher
void CheckForLoopConditionalStatement(Sema &S, Expr *Second, void CheckForLoopConditionalStatement(Sema &S, Expr *Second,
Expr *Third, Stmt *Body) { Expr *Third, Stmt *Body) {
// Condition is empty // Condition is empty
if (!Second) return; if (!Second) return;
▲ Show 20 Lines • Show All 2,513 Lines • Show Last 20 Lines

test/SemaObjC/warn-loop-analysis.m

  • This file was added.
// RUN: %clang_cc1 -fsyntax-only -Wloop-analysis -verify %s
// expected-no-diagnostics
@interface MyArray
- (id)objectAtIndexedSubscript:(unsigned int)idx;
@end
// Do not warn on objc classes has objectAtIndexedSubscript method.
MyArray *test;
void foo()
{
unsigned int i;
for (i = 42; i > 0;) // No warnings here
(void)test[--i];
}