On Mar 12, 2013, at 10:48 AM, Aaron Ballman <[email protected]> wrote:
> Have you had the chance to look into this? It's still unresolved for > me, and causes the MSVC 11 debug builds to fail. No sorry. This week is pretty busy not sure I can look into it this week. -Argyrios > > Thanks! > > ~Aaron > > On Mon, Feb 18, 2013 at 10:26 PM, Argyrios Kyrtzidis <[email protected]> > wrote: >> Thanks for letting me know Aaron, I'll look into it. >> >> -Argyrios >> >> On Feb 18, 2013, at 5:59 PM, Aaron Ballman <[email protected]> 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 <[email protected]> >>> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
