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

[Matt Beaumont-Gay]

On Fri, May 17, 2013 at 9:52 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> On May 17, 2013, at 19:45 , Matt Beaumont-Gay <matthewbg at google.com> wrote:
>
>> 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...
>
> Sorry about that! I just looked and Clang actually gave me a warning ("treating Unicode character as whitespace"). Unicode character support continues to haunt me even now.

I noticed because of that warning and -Werror :)