This is an archive of the discontinued LLVM Phabricator instance.

Fix false positives for for-loop-analysis warning
ClosedPublic

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

Details

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 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?

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.

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.

Thanks, LGTM.

This revision was automatically updated to reflect the committed changes.
steven_wu marked 2 inline comments as done.