[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]

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