[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

[Argyrios Kyrtzidis]

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