r182187 - [analyzer] "Fix" ParentMap to handle non-syntactic OpaqueValueExprs.

[Matt Beaumont-Gay]

On Fri, May 17, 2013 at 7:27 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> Author: jrose
> Date: Fri May 17 21:27:09 2013
> New Revision: 182187
>
> URL: http://llvm.org/viewvc/llvm-project?rev=182187&view=rev
> Log:
> [analyzer] "Fix" ParentMap to handle non-syntactic OpaqueValueExprs.
>
> Constructs like PseudoObjectExpr, where an expression can appear more than
> once in the AST, use OpaqueValueExprs to guard against inadvertent
> re-processing of the shared expression during AST traversal. The most
> common form of this is to share expressions between the syntactic
> "as-written" form of, say, an Objective-C property access 'obj.prop', and
> the underlying "semantic" form '[obj prop]'.
>
> However, some constructs can produce OpaqueValueExprs that don't appear in
> the syntactic form at all; in these cases the ParentMap wasn't ever traversing
> the children of these expressions. This patch fixes that by checking to see
> if an OpaqueValueExpr's child has ever been traversed before. There's also a
> bit of reset logic when visiting a PseudoObjectExpr to handle the case of
> updating the ParentMap, which some external clients depend on.
>
> This still isn't exactly the right fix because we probably want the parent
> of the OpaqueValueExpr itself to be its location in the syntactic form if
> it's syntactic and the PseudoObjectExpr or BinaryConditionalOperator itself
> if it's semantic. Whe I originally wrote the code to do this, I didn't realize
> that OpaqueValueExprs themselves are shared in the AST, not just their source
> expressions. This patch doesn't change the existing behavior so as not to
> break anything inadvertently relying on it; we'll come back to this later.
>
> Modified:
>     cfe/trunk/lib/AST/ParentMap.cpp
>
> Modified: cfe/trunk/lib/AST/ParentMap.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ParentMap.cpp?rev=182187&r1=182186&r2=182187&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/ParentMap.cpp (original)
> +++ cfe/trunk/lib/AST/ParentMap.cpp Fri May 17 21:27:09 2013
> @@ -33,6 +33,11 @@ static void BuildParentMap(MapTy& M, Stm
>      assert(OVMode == OV_Transparent && "Should not appear alongside OVEs");
>      PseudoObjectExpr *POE = cast<PseudoObjectExpr>(S);
>
> +    // If we are rebuilding the map, clear out any existing state.
> +    if (M[POE->getSyntacticForm()])
> +      for (Stmt::child_range I = S->children(); I; ++I)
> +        M[*I] = 0;
> +
>      M[POE->getSyntacticForm()] = S;
>      BuildParentMap(M, POE->getSyntacticForm(), OV_Transparent);
>
> @@ -62,13 +67,19 @@ static void BuildParentMap(MapTy& M, Stm
>
>      break;
>    }
> -  case Stmt::OpaqueValueExprClass:
> -    if (OVMode == OV_Transparent) {
> -      OpaqueValueExpr *OVE = cast<OpaqueValueExpr>(S);
> +  case Stmt::OpaqueValueExprClass: {
> +    // FIXME: This isn't correct; it assumes that multiple OpaqueValueExprs
> +    // share a single source expression, but in the AST a single
> +    // OpaqueValueExpr is shared among multiple parent expressions.
> +    // The right thing to do is to give the OpaqueValueExpr its syntactic
> +    // parent, then not reassign that when traversing the semantic expressions.
> +    OpaqueValueExpr *OVE = cast<OpaqueValueExpr>(S);
> +    if (OVMode == OV_Transparent || !M[OVE->getSourceExpr()]) {

Some unicode garbage snuck in here...

>        M[OVE->getSourceExpr()] = S;
>        BuildParentMap(M, OVE->getSourceExpr(), OV_Transparent);
>      }
>      break;
> +  }
>    default:
>      for (Stmt::child_range I = S->children(); I; ++I) {
>        if (*I) {
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits