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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130312/105b0575/attachment.html>