[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
aaron at aaronballman.com
Tue Mar 12 12:15:05 PDT 2013
Certainly!
http://llvm.org/bugs/show_bug.cgi?id=15500
Thanks!
~Aaron
On Tue, Mar 12, 2013 at 2:42 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> On Mar 12, 2013, at 11:32 AM, Aaron Ballman <aaron at aaronballman.com> 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 <akyrtzi at gmail.com>
> wrote:
>
> On Mar 12, 2013, at 10:48 AM, Aaron Ballman <aaron at aaronballman.com> 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 <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
>
>
More information about the cfe-commits
mailing list