On Mar 12, 2013, at 11:32 AM, Aaron Ballman <[email protected]> wrote:
> Understandable. Since it's been broken since Jan 7, if it remains > broken another week, we can probably survive somehow. ;-) But > hopefully it stays on your radar. Could you please file a bug report, we should track this in bugzilla as well. > > Thanks! > > ~Aaron > > On Tue, Mar 12, 2013 at 2:19 PM, Argyrios Kyrtzidis <[email protected]> wrote: >> 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
