[cfe-commits] r171828 - in /cfe/trunk: lib/ARCMigrate/TransProtectedScope.cpp test/ARCMT/checking.m test/ARCMT/protected-scope.m test/ARCMT/protected-scope.m.result

[Aaron Ballman]

Have you had the chance to look into this?  It's still unresolved for
me, and causes the MSVC 11 debug builds to fail.

Thanks!

~Aaron

On Mon, Feb 18, 2013 at 10:26 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> Thanks for letting me know Aaron, I'll look into it.
>
> -Argyrios
>
> On Feb 18, 2013, at 5:59 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
>> Sorry about the incredible "lateness" of this review, but the error
>> has been bugging me for a while.
>>
>> On Mon, Jan 7, 2013 at 7:58 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> Author: akirtzidis
>>> Date: Mon Jan  7 18:58:25 2013
>>> New Revision: 171828
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=171828&view=rev
>>> Log:
>>> [arcmt] Follow-up for r171484; make sure when adding brackets enclosing case statements,
>>> that the case does not "contain" a declaration that is referenced "outside" of it,
>>> otherwise we will emit un-compilable code.
>>>
>>> Modified:
>>>    cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp
>>>    cfe/trunk/test/ARCMT/checking.m
>>>    cfe/trunk/test/ARCMT/protected-scope.m
>>>    cfe/trunk/test/ARCMT/protected-scope.m.result
>>>
>>> Modified: cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp?rev=171828&r1=171827&r2=171828&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp (original)
>>> +++ cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp Mon Jan  7 18:58:25 2013
>>> @@ -15,6 +15,7 @@
>>> #include "Transforms.h"
>>> #include "Internals.h"
>>> #include "clang/Sema/SemaDiagnostic.h"
>>> +#include "clang/AST/ASTContext.h"
>>>
>>> using namespace clang;
>>> using namespace arcmt;
>>> @@ -22,26 +23,58 @@
>>>
>>> namespace {
>>>
>>> +class LocalRefsCollector : public RecursiveASTVisitor<LocalRefsCollector> {
>>> +  SmallVectorImpl<DeclRefExpr *> &Refs;
>>> +
>>> +public:
>>> +  LocalRefsCollector(SmallVectorImpl<DeclRefExpr *> &refs)
>>> +    : Refs(refs) { }
>>> +
>>> +  bool VisitDeclRefExpr(DeclRefExpr *E) {
>>> +    if (ValueDecl *D = E->getDecl())
>>> +      if (D->getDeclContext()->getRedeclContext()->isFunctionOrMethod())
>>> +        Refs.push_back(E);
>>> +    return true;
>>> +  }
>>> +};
>>> +
>>> struct CaseInfo {
>>>   SwitchCase *SC;
>>>   SourceRange Range;
>>> -  bool FixedBypass;
>>> +  enum {
>>> +    St_Unchecked,
>>> +    St_CannotFix,
>>> +    St_Fixed
>>> +  } State;
>>>
>>> -  CaseInfo() : SC(0), FixedBypass(false) {}
>>> +  CaseInfo() : SC(0), State(St_Unchecked) {}
>>>   CaseInfo(SwitchCase *S, SourceRange Range)
>>> -    : SC(S), Range(Range), FixedBypass(false) {}
>>> +    : SC(S), Range(Range), State(St_Unchecked) {}
>>> };
>>>
>>> class CaseCollector : public RecursiveASTVisitor<CaseCollector> {
>>> +  ParentMap &PMap;
>>>   llvm::SmallVectorImpl<CaseInfo> &Cases;
>>>
>>> public:
>>> -  CaseCollector(llvm::SmallVectorImpl<CaseInfo> &Cases)
>>> -    : Cases(Cases) { }
>>> +  CaseCollector(ParentMap &PMap, llvm::SmallVectorImpl<CaseInfo> &Cases)
>>> +    : PMap(PMap), Cases(Cases) { }
>>>
>>>   bool VisitSwitchStmt(SwitchStmt *S) {
>>> -    SourceLocation NextLoc = S->getLocEnd();
>>>     SwitchCase *Curr = S->getSwitchCaseList();
>>> +    if (!Curr)
>>> +      return true;
>>> +    Stmt *Parent = getCaseParent(Curr);
>>> +    Curr = Curr->getNextSwitchCase();
>>> +    // Make sure all case statements are in the same scope.
>>> +    while (Curr) {
>>> +      if (getCaseParent(Curr) != Parent)
>>> +        return true;
>>> +      Curr = Curr->getNextSwitchCase();
>>> +    }
>>> +
>>> +    SourceLocation NextLoc = S->getLocEnd();
>>> +    Curr = S->getSwitchCaseList();
>>>     // We iterate over case statements in reverse source-order.
>>>     while (Curr) {
>>>       Cases.push_back(CaseInfo(Curr,SourceRange(Curr->getLocStart(), NextLoc)));
>>> @@ -50,69 +83,114 @@
>>>     }
>>>     return true;
>>>   }
>>> -};
>>>
>>> -} // anonymous namespace
>>> +  Stmt *getCaseParent(SwitchCase *S) {
>>> +    Stmt *Parent = PMap.getParent(S);
>>> +    while (Parent && (isa<SwitchCase>(Parent) || isa<LabelStmt>(Parent)))
>>> +      Parent = PMap.getParent(Parent);
>>> +    return Parent;
>>> +  }
>>> +};
>>>
>>> -static bool isInRange(FullSourceLoc Loc, SourceRange R) {
>>> -  return !Loc.isBeforeInTranslationUnitThan(R.getBegin()) &&
>>> -          Loc.isBeforeInTranslationUnitThan(R.getEnd());
>>> -}
>>> +class ProtectedScopeFixer {
>>> +  MigrationPass &Pass;
>>> +  SourceManager &SM;
>>> +  SmallVector<CaseInfo, 16> Cases;
>>> +  SmallVector<DeclRefExpr *, 16> LocalRefs;
>>>
>>> -static bool handleProtectedNote(const StoredDiagnostic &Diag,
>>> -                                llvm::SmallVectorImpl<CaseInfo> &Cases,
>>> -                                TransformActions &TA) {
>>> -  assert(Diag.getLevel() == DiagnosticsEngine::Note);
>>> -
>>> -  for (unsigned i = 0; i != Cases.size(); i++) {
>>> -    CaseInfo &info = Cases[i];
>>> -    if (isInRange(Diag.getLocation(), info.Range)) {
>>> -      TA.clearDiagnostic(Diag.getID(), Diag.getLocation());
>>> -      if (!info.FixedBypass) {
>>> -        TA.insertAfterToken(info.SC->getColonLoc(), " {");
>>> -        TA.insert(info.Range.getEnd(), "}\n");
>>> -        info.FixedBypass = true;
>>> +public:
>>> +  ProtectedScopeFixer(BodyContext &BodyCtx)
>>> +    : Pass(BodyCtx.getMigrationContext().Pass),
>>> +      SM(Pass.Ctx.getSourceManager()) {
>>> +
>>> +    CaseCollector(BodyCtx.getParentMap(), Cases)
>>> +        .TraverseStmt(BodyCtx.getTopStmt());
>>> +    LocalRefsCollector(LocalRefs).TraverseStmt(BodyCtx.getTopStmt());
>>> +
>>> +    SourceRange BodyRange = BodyCtx.getTopStmt()->getSourceRange();
>>> +    const CapturedDiagList &DiagList = Pass.getDiags();
>>> +    CapturedDiagList::iterator I = DiagList.begin(), E = DiagList.end();
>>> +    while (I != E) {
>>> +      if (I->getID() == diag::err_switch_into_protected_scope &&
>>> +          isInRange(I->getLocation(), BodyRange)) {
>>> +        handleProtectedScopeError(I, E);
>>> +        continue;
>>
>> The use of iterators here is problematic because
>> handleProtectedScopeError can invalidate the iterators.  This is
>> causing a failed assertion in debug build of MSVC11 where there are
>> checked iterators involved.
>>
>> From what I am seeing, handleProtectedScopeError has a Transaction
>> object, which on destruction winds up calling
>> CapturedDiagList::clearDiagnostic, and that calls erase on the list,
>> which invalidates the iterators.  Attempting to perform the comparison
>> then fires the assert.
>>
>> I'm not entirely certain of the best way to solve this issue, but I
>> figured I would mention that it causes problems.  We have a test case
>> already demonstrating the issue with test\ARCMT\protected-scope.m
>>
>> ~Aaron
>