[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
akyrtzi at gmail.com
Tue Mar 12 11:42:59 PDT 2013
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130312/a9f38d33/attachment.html>
More information about the cfe-commits
mailing list