Re: [PATCH] D20349: Fix a clang bug in lambda capture of 'this'

2016-06-26 Thread Faisal Vali via cfe-commits 
faisalv resigned from this revision.
faisalv removed a reviewer: faisalv.
faisalv added a comment.

Should be fixed by: http://reviews.llvm.org/D19783.

Thanks again for your feedback and all the time you put into this =)


http://reviews.llvm.org/D20349



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20349: Fix a clang bug in lambda capture of 'this'

2016-05-18 Thread Faisal Vali via cfe-commits 
faisalv added a comment.

That feedback would be greatly appreciated - thanks Taewook!
Faisal Vali


http://reviews.llvm.org/D20349



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20349: Fix a clang bug in lambda capture of 'this'

2016-05-18 Thread Taewook Oh via cfe-commits 
twoh added a comment.

Thank you for your comments. @faisalv, it is great that you already submitted a 
patch. Let me see if your patch resolves the issue I have. Thanks!


http://reviews.llvm.org/D20349



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20349: Fix a clang bug in lambda capture of 'this'

2016-05-18 Thread Faisal Vali via cfe-commits 
Thanks for the work here - but this bug is a duplicate of this:
https://llvm.org/bugs/show_bug.cgi?id=27507, which unearthed some
other subtle bugs with the cv capture of *this by copy.
I've submitted a patch for review here: http://reviews.llvm.org/D19783
that addresses both issues - about 2-3 weeks ago -  given the
nastiness of this bug, the fact that it was introduced by one of my
patches and that there is now a second report of the bug - and that I
feel reasonably good about the fix - I'll commit this patch before the
weekend, unless Richard raises an issue.


Faisal Vali



On Tue, May 17, 2016 at 11:23 PM, Taewook Oh  wrote:
> twoh created this revision.
> twoh added reviewers: faisalv, rsmith.
> twoh added a subscriber: cfe-commits.
>
> (This is a fix for Bug 27797 https://llvm.org/bugs/show_bug.cgi?id=27797).
>
> Currently when RebuildLambdaScopeInfo() function in lib/Sema/SemaDecl.cpp 
> observes lambda capturing 'this', it calls Sema::getCurrentThisType() to get 
> the type of 'this'. However, clang (revision 269789) crashes with an 
> assertion failure inside Sema::getCurrentThisType() if template instantiation 
> creates nested lambdas. Inside, Sema::getCurrentThisType(), there is an 
> assertion saying that getCurLambda() never returns nullptr, but nest lambdas 
> created by template instantiation makes getCurLambda() returns nullptr.
>
> Actually, even without the assertion failure, calling 
> Sema::getCurrentThisType() from RebuildLambdaScopeInfo() seems wrong. When 
> there are nested lambdas, what is required from Sema::getCurrentThisType() is 
> a type of 'this' for nesting lambda, while what is supposed to be returned 
> from Sema::getCurrentThisType() is a type of 'this' for nested lambda.
>
> This patch addresses this issue and makes RebuildLambdaScopeInfo() compute 
> the correct 'this' type.
>
> http://reviews.llvm.org/D20349
>
> Files:
>   lib/Sema/SemaDecl.cpp
>
> Index: lib/Sema/SemaDecl.cpp
> ===
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -11109,8 +11109,16 @@
>CaptureType, /*Expr*/ nullptr);
>
>  } else if (C.capturesThis()) {
> +  QualType ThisTy = CallOperator->getThisType(S.Context);
> +  QualType BaseTy = ThisTy->getPointeeType();
> +  if (C.getCaptureKind() == LCK_StarThis &&
> +  CallOperator->isConst() &&
> +  !BaseTy.isConstQualified()) {
> +BaseTy.addConst();
> +ThisTy = S.Context.getPointerType(BaseTy);
> +  }
>LSI->addThisCapture(/*Nested*/ false, C.getLocation(),
> -  S.getCurrentThisType(), /*Expr*/ nullptr,
> +  ThisTy, /*Expr*/ nullptr,
>C.getCaptureKind() == LCK_StarThis);
>  } else {
>LSI->addVLATypeCapture(C.getLocation(), I->getType());
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20349: Fix a clang bug in lambda capture of 'this'

2016-05-17 Thread David Blaikie via cfe-commits 
The patch'll need a test case (in clang/tests) - I've not looked at the
change/have much opinion there, just suggesting adding a test at minimum so
when someone does come to review it it's more complete/closer to/ready to
commit.

On Tue, May 17, 2016 at 9:23 PM, Taewook Oh via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> twoh created this revision.
> twoh added reviewers: faisalv, rsmith.
> twoh added a subscriber: cfe-commits.
>
> (This is a fix for Bug 27797 https://llvm.org/bugs/show_bug.cgi?id=27797).
>
> Currently when RebuildLambdaScopeInfo() function in lib/Sema/SemaDecl.cpp
> observes lambda capturing 'this', it calls Sema::getCurrentThisType() to
> get the type of 'this'. However, clang (revision 269789) crashes with an
> assertion failure inside Sema::getCurrentThisType() if template
> instantiation creates nested lambdas. Inside, Sema::getCurrentThisType(),
> there is an assertion saying that getCurLambda() never returns nullptr, but
> nest lambdas created by template instantiation makes getCurLambda() returns
> nullptr.
>
> Actually, even without the assertion failure, calling
> Sema::getCurrentThisType() from RebuildLambdaScopeInfo() seems wrong. When
> there are nested lambdas, what is required from Sema::getCurrentThisType()
> is a type of 'this' for nesting lambda, while what is supposed to be
> returned from Sema::getCurrentThisType() is a type of 'this' for nested
> lambda.
>
> This patch addresses this issue and makes RebuildLambdaScopeInfo() compute
> the correct 'this' type.
>
> http://reviews.llvm.org/D20349
>
> Files:
>   lib/Sema/SemaDecl.cpp
>
> Index: lib/Sema/SemaDecl.cpp
> ===
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -11109,8 +11109,16 @@
>CaptureType, /*Expr*/ nullptr);
>
>  } else if (C.capturesThis()) {
> +  QualType ThisTy = CallOperator->getThisType(S.Context);
> +  QualType BaseTy = ThisTy->getPointeeType();
> +  if (C.getCaptureKind() == LCK_StarThis &&
> +  CallOperator->isConst() &&
> +  !BaseTy.isConstQualified()) {
> +BaseTy.addConst();
> +ThisTy = S.Context.getPointerType(BaseTy);
> +  }
>LSI->addThisCapture(/*Nested*/ false, C.getLocation(),
> -  S.getCurrentThisType(), /*Expr*/ nullptr,
> +  ThisTy, /*Expr*/ nullptr,
>C.getCaptureKind() == LCK_StarThis);
>  } else {
>LSI->addVLATypeCapture(C.getLocation(), I->getType());
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D20349: Fix a clang bug in lambda capture of 'this'

2016-05-17 Thread Taewook Oh via cfe-commits 
twoh created this revision.
twoh added reviewers: faisalv, rsmith.
twoh added a subscriber: cfe-commits.

(This is a fix for Bug 27797 https://llvm.org/bugs/show_bug.cgi?id=27797).

Currently when RebuildLambdaScopeInfo() function in lib/Sema/SemaDecl.cpp 
observes lambda capturing 'this', it calls Sema::getCurrentThisType() to get 
the type of 'this'. However, clang (revision 269789) crashes with an assertion 
failure inside Sema::getCurrentThisType() if template instantiation creates 
nested lambdas. Inside, Sema::getCurrentThisType(), there is an assertion 
saying that getCurLambda() never returns nullptr, but nest lambdas created by 
template instantiation makes getCurLambda() returns nullptr. 

Actually, even without the assertion failure, calling 
Sema::getCurrentThisType() from RebuildLambdaScopeInfo() seems wrong. When 
there are nested lambdas, what is required from Sema::getCurrentThisType() is a 
type of 'this' for nesting lambda, while what is supposed to be returned from 
Sema::getCurrentThisType() is a type of 'this' for nested lambda.

This patch addresses this issue and makes RebuildLambdaScopeInfo() compute the 
correct 'this' type. 

http://reviews.llvm.org/D20349

Files:
  lib/Sema/SemaDecl.cpp

Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11109,8 +11109,16 @@
   CaptureType, /*Expr*/ nullptr);
 
 } else if (C.capturesThis()) {
+  QualType ThisTy = CallOperator->getThisType(S.Context);
+  QualType BaseTy = ThisTy->getPointeeType();
+  if (C.getCaptureKind() == LCK_StarThis &&
+  CallOperator->isConst() &&
+  !BaseTy.isConstQualified()) {
+BaseTy.addConst();
+ThisTy = S.Context.getPointerType(BaseTy);
+  }
   LSI->addThisCapture(/*Nested*/ false, C.getLocation(),
-  S.getCurrentThisType(), /*Expr*/ nullptr,
+  ThisTy, /*Expr*/ nullptr,
   C.getCaptureKind() == LCK_StarThis);
 } else {
   LSI->addVLATypeCapture(C.getLocation(), I->getType());


Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11109,8 +11109,16 @@
   CaptureType, /*Expr*/ nullptr);
 
 } else if (C.capturesThis()) {
+  QualType ThisTy = CallOperator->getThisType(S.Context);
+  QualType BaseTy = ThisTy->getPointeeType();
+  if (C.getCaptureKind() == LCK_StarThis &&
+  CallOperator->isConst() &&
+  !BaseTy.isConstQualified()) {
+BaseTy.addConst();
+ThisTy = S.Context.getPointerType(BaseTy);
+  }
   LSI->addThisCapture(/*Nested*/ false, C.getLocation(),
-  S.getCurrentThisType(), /*Expr*/ nullptr,
+  ThisTy, /*Expr*/ nullptr,
   C.getCaptureKind() == LCK_StarThis);
 } else {
   LSI->addVLATypeCapture(C.getLocation(), I->getType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits