[PATCH] D83788: Removed unused variable in clang

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

No problem, thanks for this!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83788/new/

https://reviews.llvm.org/D83788



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

[PATCH] D83788: Removed unused variable in clang

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

By the way, I tried to accept this diff and leave the following inline comment 
on `SemaExpr.cpp:15799`:

> I guess this assert was never capable of being hit previously, since the 
> `FixItHint::isNull` would always return true? I wonder if we'll now see some 
> genuine asserts for `ConversionFixItGenerator::isNull` thanks to this change.

However when I attempt to do either, Phabricator displays a dialog indicating I 
don't have permission to "modify" the diff. Not sure what's going on there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83788/new/

https://reviews.llvm.org/D83788



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

[PATCH] D83788: Removed unused variable in clang

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

This LGTM! Looks like the last time this variable was touched was in 2010 as 
part of a mechanical renaming, 
https://github.com/llvm/llvm-project/commit/a771f46c82d7. At that point it was 
used, as a parameter to the static function `MakeObjCStringLiteralFixItHint`. 
That static function was later renamed on Dec 17 2013 
https://github.com/llvm/llvm-project/commit/bd714e9bb12, to 
`ConversionToObjCStringLiteralCheck`, and on the following day Dec 18 2013 in 
https://github.com/llvm/llvm-project/commit/283bf895066 the signature was 
changed to no longer take a `FixitHint` parameter. The author must not have 
removed this variable at that point because technically it is still referenced, 
but as you point out, it's never mutated in a way that gives the hint a value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83788/new/

https://reviews.llvm.org/D83788



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

[PATCH] D82986: [Coroutines] Fix test breakage in D82928

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

Thanks for the fix!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82986/new/

https://reviews.llvm.org/D82986



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

[PATCH] D82928: [Coroutines] Fix code coverage for coroutine

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

Excellent, thanks for this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82928/new/

https://reviews.llvm.org/D82928



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

[PATCH] D82332: [Coroutines] Handle dependent promise types for final_suspend non-throw check

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good to me!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82332/new/

https://reviews.llvm.org/D82332



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

[PATCH] D82029: [Coroutines] Ensure co_await promise.final_suspend() does not throw

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

Sweet! Thanks for the reviews/responses, LGTM :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82029/new/

https://reviews.llvm.org/D82029



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

[PATCH] D81885: [Coroutines] Return false on error of buildSuspends

[Brian Gesiak via Phabricator via cfe-commits]

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

I don't have a preference as to whether `Sema::ActOnCoroutineBodyStart` returns 
`true` or `false` to indicate failure, although I think returning `true` for 
failures is a bit of a pre-existing convention, such as here, where `true` is 
returned for an invalid declaration: 
https://github.com/llvm/llvm-project/blob/84167a8d58e8af79625abcffdf2c860d556955e6/clang/lib/Sema/SemaDecl.cpp#L1572

However I don't think this change is a good one because it removes test cases 
for what should be a an "NFC" change -- that is, changing the invalid return 
value from `true` to `false` shouldn't impact the behavior of the compiler in a 
way that necessitates test cases to be removed.




Comment at: clang/test/SemaCXX/coroutines.cpp:106
 double bad_promise_type_2(int) { // expected-error {{no member named 
'initial_suspend'}}
-  co_yield 0; // expected-error {{no member named 'yield_value' in 
'std::experimental::coroutine_traits::promise_type'}}
+  co_yield 0;
 }

This test ought not change -- we do expect an error here. `co_yield` can only 
be used if the promise type defines `yield_value`.



Comment at: clang/test/SemaCXX/coroutines.cpp:479
   co_await transform_awaitable{};
-  // expected-error@-1 {{no member named 'await_ready'}}
 }

Same as my comment above: this is a valuable diagnostic, why remove the test 
for it? Same goes for the rest of the test changes in this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81885/new/

https://reviews.llvm.org/D81885



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

[PATCH] D82029: [Coroutines] Ensure co_await promise.final_suspend() does not throw

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a reviewer: junparser.
modocache added a subscriber: junparser.
modocache added a comment.

Excellent, thank you! The test failures on the diff appear to be legitimate, 
they reproduce for me when I apply this patch to my local checkout and run 
`ninja check-clang`. Could you take a look?

This LGTM otherwise! Also adding @junparser in case they have any thoughts, if 
not I'll accept after the test failures are addressed, Thanks!




Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:1051
+FT = S.ResolveExceptionSpec(Loc.isInvalid() ? E->getBeginLoc() : Loc, FT);
+  }
   if (!FT)

Small nit-pick: Omit the `{` and `}` here, as that's the convention in the 
surrounding lines of this file and in the LLVM project in general.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82029/new/

https://reviews.llvm.org/D82029



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

[PATCH] D70555: [coroutines] Don't build promise init with no args

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG627e01feb718: [coroutines] Dont build promise init 
with no args (authored by modocache).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70555/new/

https://reviews.llvm.org/D70555

Files:
  clang/lib/Sema/SemaCoroutine.cpp


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -502,8 +502,9 @@
 return nullptr;
 
   auto *ScopeInfo = getCurFunction();
-  // Build a list of arguments, based on the coroutine functions arguments,
-  // that will be passed to the promise type's constructor.
+
+  // Build a list of arguments, based on the coroutine function's arguments,
+  // that if present will be passed to the promise type's constructor.
   llvm::SmallVector CtorArgExprs;
 
   // Add implicit object parameter.
@@ -519,6 +520,7 @@
 }
   }
 
+  // Add the coroutine function's parameters.
   auto  = ScopeInfo->CoroutineParameterMoves;
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
@@ -540,28 +542,33 @@
 CtorArgExprs.push_back(RefExpr.get());
   }
 
-  // Create an initialization sequence for the promise type using the
-  // constructor arguments, wrapped in a parenthesized list expression.
-  Expr *PLE = ParenListExpr::Create(Context, FD->getLocation(),
-CtorArgExprs, FD->getLocation());
-  InitializedEntity Entity = InitializedEntity::InitializeVariable(VD);
-  InitializationKind Kind = InitializationKind::CreateForInit(
-  VD->getLocation(), /*DirectInit=*/true, PLE);
-  InitializationSequence InitSeq(*this, Entity, Kind, CtorArgExprs,
- /*TopLevelOfInitList=*/false,
- /*TreatUnavailableAsInvalid=*/false);
-
-  // Attempt to initialize the promise type with the arguments.
-  // If that fails, fall back to the promise type's default constructor.
-  if (InitSeq) {
-ExprResult Result = InitSeq.Perform(*this, Entity, Kind, CtorArgExprs);
-if (Result.isInvalid()) {
-  VD->setInvalidDecl();
-} else if (Result.get()) {
-  VD->setInit(MaybeCreateExprWithCleanups(Result.get()));
-  VD->setInitStyle(VarDecl::CallInit);
-  CheckCompleteVariableDeclaration(VD);
-}
+  // If we have a non-zero number of constructor arguments, try to use them.
+  // Otherwise, fall back to the promise type's default constructor.
+  if (!CtorArgExprs.empty()) {
+// Create an initialization sequence for the promise type using the
+// constructor arguments, wrapped in a parenthesized list expression.
+Expr *PLE = ParenListExpr::Create(Context, FD->getLocation(),
+  CtorArgExprs, FD->getLocation());
+InitializedEntity Entity = InitializedEntity::InitializeVariable(VD);
+InitializationKind Kind = InitializationKind::CreateForInit(
+VD->getLocation(), /*DirectInit=*/true, PLE);
+InitializationSequence InitSeq(*this, Entity, Kind, CtorArgExprs,
+   /*TopLevelOfInitList=*/false,
+   /*TreatUnavailableAsInvalid=*/false);
+
+// Attempt to initialize the promise type with the arguments.
+// If that fails, fall back to the promise type's default constructor.
+if (InitSeq) {
+  ExprResult Result = InitSeq.Perform(*this, Entity, Kind, CtorArgExprs);
+  if (Result.isInvalid()) {
+VD->setInvalidDecl();
+  } else if (Result.get()) {
+VD->setInit(MaybeCreateExprWithCleanups(Result.get()));
+VD->setInitStyle(VarDecl::CallInit);
+CheckCompleteVariableDeclaration(VD);
+  }
+} else
+  ActOnUninitializedDecl(VD);
   } else
 ActOnUninitializedDecl(VD);
 


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -502,8 +502,9 @@
 return nullptr;
 
   auto *ScopeInfo = getCurFunction();
-  // Build a list of arguments, based on the coroutine functions arguments,
-  // that will be passed to the promise type's constructor.
+
+  // Build a list of arguments, based on the coroutine function's arguments,
+  // that if present will be passed to the promise type's constructor.
   llvm::SmallVector CtorArgExprs;
 
   // Add implicit object parameter.
@@ -519,6 +520,7 @@
 }
   }
 
+  // Add the coroutine function's parameters.
   auto  = ScopeInfo->CoroutineParameterMoves;
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
@@ -540,28 +542,33 @@
 CtorArgExprs.push_back(RefExpr.get());
   }
 
-  // Create an initialization sequence for the promise type using the
-  // constructor arguments, wrapped in a parenthesized list expression.
-  Expr *PLE = 

[PATCH] D70555: [coroutines] Don't build promise init with no args

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Of course, your approval is very welcome!  I'll go ahead and land this today, 
thanks for the review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70555/new/

https://reviews.llvm.org/D70555



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

[PATCH] D75579: Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime-registration

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Awesome, thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75579/new/

https://reviews.llvm.org/D75579



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

[PATCH] D75579: Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime-registration

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

I may be wrong, but I believe this breaks `check-llvm` when run with the 
following configuration: `cmake 
-DLLVM_BINUTILS_INCDIR=/path/to/binutils/include`:

  Failing Tests (6):
  LLVM :: tools/gold/X86/common_thinlto.ll
  LLVM :: tools/gold/X86/emit-llvm.ll
  LLVM :: tools/gold/X86/pr25907.ll
  LLVM :: tools/gold/X86/relax-relocs.ll
  LLVM :: tools/gold/X86/relocation-model-pic.ll
  LLVM :: tools/gold/X86/strip_names.ll

When bisecting for those specific test failures, I found this commit. The 
issue, I believe, is with how those tests pass options to the LLVMgold plugin. 
For what it's worth, my binutils reports version 2.27.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75579/new/

https://reviews.llvm.org/D75579



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

[PATCH] D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`

[Brian Gesiak via Phabricator via cfe-commits]

modocache abandoned this revision.
modocache added a comment.

Awesome, thanks!

> Alternatively, I considered modifying the `clang::OverloadCandidate` 
> constructor to initialize `IsSurrogate` with a false value. I feel doing so 
> would be safer, but I stuck with what appears to be the convention: 
> initializing OverloadCandidate members are the site of use.

Looks like the patch does exactly this, which is nice.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75542/new/

https://reviews.llvm.org/D75542



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

[PATCH] D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Friendly ping! `OverloadCandidate` has uninitialized members and so can cause 
UB if used incorrectly in another part of the compiler, so I feel this is a 
fairly straightforward patch: prevent UB by initializing all members. But I'd 
like some review to confirm. @rsmith? @aaron.ballman?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75542/new/

https://reviews.llvm.org/D75542



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

[PATCH] D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`

[Brian Gesiak via Phabricator via cfe-commits]

modocache marked an inline comment as done.
modocache added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:7371
 return;
   }
 

@rsmith I'd definitely appreciate any pointers here -- if initializing 
`IsSurrogate` with a value as I do in this patch is the kind of change that 
doesn't "need" an explicit test, then great. But if a test is in order, I'd be 
happy to write one, but I'm not certain this `if` statement (and the one whose 
body statement I modify above) is being tested. Even commenting it out entirely 
still has `check-clang` passing for me. I'd greatly appreciate advice on how to 
exercise this code path in a Clang regression test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75542/new/

https://reviews.llvm.org/D75542



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

[PATCH] D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: rsmith, RKSimon, aaron.ballman, wenlei.
Herald added a project: clang.

A closed-source C++ codebase I help maintain began hitting a Clang ICE
with a stack trace that referenced
`clang::OverloadCandidate::getNumParams`:
https://gist.github.com/modocache/84eac9c519796644139471dd06ef4628

Specifically, Clang was querying `getNumParams`, the implementation of
which checks the `IsSurrogate` member:

  unsigned getNumParams() const {
if (IsSurrogate) {
  QualType STy = Surrogate->getConversionType();
  // ...
}
// ...
  }

However, the `OverloadCandidate` instance had not initialized that member
with a value, triggering UB. In the case of the stack trace, `IsSurrogate`
was evaluated as if it were `true`, and the access into uninitialized
`Surrogate` caused the crash.

Every other use of `clang::OverloadCandidate` initializes `IsSurrogate`
with a value, so I do so in this patch in order to avoid triggering UB
in the case of my closed-source codebase. I believe the potential for UB
was introduced in this commit from January 9:
https://github.com/llvm/llvm-project/commit/25195541349b1d6dfc03bf7511483110bda69b29

Alternatively, I considered modifying the `clang::OverloadCandidate`
constructor to initialize `IsSurrogate` with a `false` value. I feel doing
so would be safer, but I stuck with what appears to be the convention:
initializing `OverloadCandidate` members are the site of use.

Unfortunately I don't have a small example I can share of how
`getNumParams` can be called in an invalid state. The closed-source C++
code that triggers the UB looks something like this and only occurs when
using GCC's libc++:

  class MyClass {
std::pair myPair;
// ...
void myMemberFunctionoFoo(std::pair p) {}
void myMemberFunctionBar() {
  std::bind(::myMemberFunctionFoo, this, myPair);
}
  };

I tried reverse-engineering a test for this patch by commenting out the
if statements added in the commit that I think introduced the UB,
https://github.com/llvm/llvm-project/commit/25195541349b1d6dfc03bf7511483110bda69b29.
But in fact I found that removing these if statements entirely did not
cause any tests in check-clang to fail, so I'm concerned the code may be
untested at the moment.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75542

Files:
  clang/lib/Sema/SemaOverload.cpp


Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6978,6 +6978,7 @@
 OverloadCandidate  = CandidateSet.addCandidate();
 Candidate.FoundDecl = FoundDecl;
 Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.IsSurrogate = false;
 Candidate.Viable = false;
 Candidate.FailureKind = ovl_fail_explicit;
 return;
@@ -7363,6 +7364,7 @@
 OverloadCandidate  = CandidateSet.addCandidate();
 Candidate.FoundDecl = FoundDecl;
 Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.IsSurrogate = false;
 Candidate.Viable = false;
 Candidate.FailureKind = ovl_fail_explicit;
 return;


Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6978,6 +6978,7 @@
 OverloadCandidate  = CandidateSet.addCandidate();
 Candidate.FoundDecl = FoundDecl;
 Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.IsSurrogate = false;
 Candidate.Viable = false;
 Candidate.FailureKind = ovl_fail_explicit;
 return;
@@ -7363,6 +7364,7 @@
 OverloadCandidate  = CandidateSet.addCandidate();
 Candidate.FoundDecl = FoundDecl;
 Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.IsSurrogate = false;
 Candidate.Viable = false;
 Candidate.FailureKind = ovl_fail_explicit;
 return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG048239e46e49: [Coroutines][6/6] Clang schedules new passes 
(authored by modocache).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp

Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,57 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template  struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template  struct coroutine_traits {
+  struct promise_type {
+awaitable initial_suspend() { return {}; }
+awaitable final_suspend() { return {}; }
+void return_void() {}
+T get_return_object() { return T(); }
+void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+void foo() { co_return; }
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 +49,10 @@
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Coroutines.h"
+#include "llvm/Transforms/Coroutines/CoroCleanup.h"
+#include "llvm/Transforms/Coroutines/CoroEarly.h"
+#include "llvm/Transforms/Coroutines/CoroElide.h"
+#include "llvm/Transforms/Coroutines/CoroSplit.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/LowerTypeTests.h"
@@ -957,6 +961,22 @@
   }
 }
 
+static void addCoroutinePassesAtO0(ModulePassManager ,
+   const LangOptions ,
+   const CodeGenOptions ) {
+  if (!LangOpts.Coroutines)
+return;
+
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
+
+  CGSCCPassManager CGPM(CodeGenOpts.DebugPassManager);
+  CGPM.addPass(CoroSplitPass());
+  CGPM.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
+
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
+}
+
 static void addSanitizersAtO0(ModulePassManager ,
   const Triple ,
   const LangOptions ,
@@ -1076,6 +1096,7 @@
   PTO.LoopInterleaving = CodeGenOpts.UnrollLoops;
   PTO.LoopVectorization = CodeGenOpts.VectorizeLoop;
   PTO.SLPVectorization = CodeGenOpts.VectorizeSLP;
+  PTO.Coroutines = LangOpts.Coroutines;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI;
@@ -1279,6 +1300,7 @@
 }
 
 if (CodeGenOpts.OptimizationLevel == 0) {
+  addCoroutinePassesAtO0(MPM, LangOpts, CodeGenOpts);
   addSanitizersAtO0(MPM, TargetTriple, LangOpts, CodeGenOpts);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73835: [IRBuilder] Virtualize IRBuilder

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: llvm/include/llvm/IR/IRBuilder.h:137
+
+  Value *Insert(Value *V, const Twine& = "") const {
+if (Instruction *I = dyn_cast(V))

I experienced a tiny regression after applying this patch, and I think this 
line might be the reason why. I tried addressing the issue in D74754, please 
give that a review when you get a chance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73835/new/

https://reviews.llvm.org/D73835



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

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 245054.
modocache removed a reviewer: wenlei.
modocache removed subscribers: wenlei, hiraditya.
modocache removed a project: LLVM.
modocache added a comment.

Rebase on top of the latest version of D71902 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp

Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,57 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template  struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template  struct coroutine_traits {
+  struct promise_type {
+awaitable initial_suspend() { return {}; }
+awaitable final_suspend() { return {}; }
+void return_void() {}
+T get_return_object() { return T(); }
+void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+void foo() { co_return; }
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 +49,10 @@
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Coroutines.h"
+#include "llvm/Transforms/Coroutines/CoroCleanup.h"
+#include "llvm/Transforms/Coroutines/CoroEarly.h"
+#include "llvm/Transforms/Coroutines/CoroElide.h"
+#include "llvm/Transforms/Coroutines/CoroSplit.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/LowerTypeTests.h"
@@ -957,6 +961,22 @@
   }
 }
 
+static void addCoroutinePassesAtO0(ModulePassManager ,
+   const LangOptions ,
+   const CodeGenOptions ) {
+  if (!LangOpts.Coroutines)
+return;
+
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
+
+  CGSCCPassManager CGPM(CodeGenOpts.DebugPassManager);
+  CGPM.addPass(CoroSplitPass());
+  CGPM.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
+
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
+}
+
 static void addSanitizersAtO0(ModulePassManager ,
   const Triple ,
   const LangOptions ,
@@ -1076,6 +1096,7 @@
   PTO.LoopInterleaving = CodeGenOpts.UnrollLoops;
   PTO.LoopVectorization = CodeGenOpts.VectorizeLoop;
   PTO.SLPVectorization = CodeGenOpts.VectorizeSLP;
+  PTO.Coroutines = LangOpts.Coroutines;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI;
@@ -1279,6 +1300,7 @@
 }
 
 if (CodeGenOpts.OptimizationLevel == 0) {
+  addCoroutinePassesAtO0(MPM, LangOpts, CodeGenOpts);
   addSanitizersAtO0(MPM, TargetTriple, LangOpts, CodeGenOpts);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 244566.
modocache added a comment.

Clean up coroutine intrinsics as part of the ThinLTO pre-link pipeline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -242,6 +242,7 @@
   SLPVectorization = RunSLPVectorization;
   LoopUnrolling = true;
   ForgetAllSCEVInLoopUnroll = ForgetSCEVInLoopUnroll;
+  Coroutines = false;
   LicmMssaOptCap = SetLicmMssaOptCap;
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
 }
@@ -721,6 +722,8 @@
   EarlyFPM.addPass(SROA());
   EarlyFPM.addPass(EarlyCSEPass());
   EarlyFPM.addPass(LowerExpectIntrinsicPass());
+  if (PTO.Coroutines)
+EarlyFPM.addPass(CoroEarlyPass());
   if (Level == OptimizationLevel::O3)
 EarlyFPM.addPass(CallSiteSplittingPass());
 
@@ -844,6 +847,11 @@
 
   MainCGPipeline.addPass(AttributorCGSCCPass());
 
+  if (PTO.Coroutines) {
+MainCGPipeline.addPass(CoroSplitPass());
+MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  }
+
   // Now deduce any function attributes based in the current code.
   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
 
@@ -1046,6 +1054,9 @@
   // inserting redundancies into the program. This even includes SimplifyCFG.
   OptimizePM.addPass(SpeculateAroundPHIsPass());
 
+  if (PTO.Coroutines)
+OptimizePM.addPass(CoroCleanupPass());
+
   for (auto  : OptimizerLastEPCallbacks)
 C(OptimizePM, Level);
 
@@ -1129,6 +1140,12 @@
   // Reduce the size of the IR as much as possible.
   MPM.addPass(GlobalOptPass());
 
+  // Module simplification splits coroutines, but does not fully clean up
+  // coroutine intrinsics. To ensure ThinLTO optimization passes don't trip up
+  // on these, we schedule the cleanup here.
+  if (PTO.Coroutines)
+MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
+
   return MPM;
 }
 
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -92,6 +92,12 @@
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
+  /// value is false. Frontends such as Clang may enable this conditionally. For
+  /// example, Clang enables this option if the flags `-std=c++2a` or above, or
+  /// `-fcoroutines-ts`, have been specified.
+  bool Coroutines;
+
   /// Tuning option to cap the number of calls to retrive clobbering accesses in
   /// MemorySSA, in LICM.
   unsigned LicmMssaOptCap;
Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,57 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template  struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template  struct coroutine_traits {
+  struct promise_type {
+awaitable initial_suspend() 

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 239437.
modocache added a comment.

Rebase past D72547 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -239,6 +239,7 @@
   SLPVectorization = RunSLPVectorization;
   LoopUnrolling = true;
   ForgetAllSCEVInLoopUnroll = ForgetSCEVInLoopUnroll;
+  Coroutines = false;
   LicmMssaOptCap = SetLicmMssaOptCap;
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
 }
@@ -718,6 +719,8 @@
   EarlyFPM.addPass(SROA());
   EarlyFPM.addPass(EarlyCSEPass());
   EarlyFPM.addPass(LowerExpectIntrinsicPass());
+  if (PTO.Coroutines)
+EarlyFPM.addPass(CoroEarlyPass());
   if (Level == OptimizationLevel::O3)
 EarlyFPM.addPass(CallSiteSplittingPass());
 
@@ -832,6 +835,11 @@
 IP.HotCallSiteThreshold = 0;
   MainCGPipeline.addPass(InlinerPass(IP));
 
+  if (PTO.Coroutines) {
+MainCGPipeline.addPass(CoroSplitPass());
+MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  }
+
   // Now deduce any function attributes based in the current code.
   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
 
@@ -1026,6 +1034,9 @@
   // inserting redundancies into the program. This even includes SimplifyCFG.
   OptimizePM.addPass(SpeculateAroundPHIsPass());
 
+  if (PTO.Coroutines)
+OptimizePM.addPass(CoroCleanupPass());
+
   for (auto  : OptimizerLastEPCallbacks)
 C(OptimizePM, Level);
 
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -92,6 +92,12 @@
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
+  /// value is false. Frontends such as Clang may enable this conditionally. For
+  /// example, Clang enables this option if the flags `-std=c++2a` or above, or
+  /// `-fcoroutines-ts`, have been specified.
+  bool Coroutines;
+
   /// Tuning option to cap the number of calls to retrive clobbering accesses in
   /// MemorySSA, in LICM.
   unsigned LicmMssaOptCap;
Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,57 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template  struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template  struct coroutine_traits {
+  struct promise_type {
+awaitable initial_suspend() { return {}; }
+awaitable final_suspend() { return {}; }
+void return_void() {}
+T get_return_object() { return T(); }
+void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+void foo() { co_return; }
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ 

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 236967.
modocache added a comment.

Initialize PipelineTuningOptions properly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -233,6 +233,7 @@
   SLPVectorization = RunSLPVectorization;
   LoopUnrolling = true;
   ForgetAllSCEVInLoopUnroll = ForgetSCEVInLoopUnroll;
+  Coroutines = false;
   LicmMssaOptCap = SetLicmMssaOptCap;
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
 }
@@ -711,6 +712,8 @@
   EarlyFPM.addPass(SROA());
   EarlyFPM.addPass(EarlyCSEPass());
   EarlyFPM.addPass(LowerExpectIntrinsicPass());
+  if (PTO.Coroutines)
+EarlyFPM.addPass(CoroEarlyPass());
   if (Level == O3)
 EarlyFPM.addPass(CallSiteSplittingPass());
 
@@ -825,6 +828,11 @@
 IP.HotCallSiteThreshold = 0;
   MainCGPipeline.addPass(InlinerPass(IP));
 
+  if (PTO.Coroutines) {
+MainCGPipeline.addPass(CoroSplitPass());
+MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  }
+
   // Now deduce any function attributes based in the current code.
   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
 
@@ -1020,6 +1028,9 @@
   // inserting redundancies into the program. This even includes SimplifyCFG.
   OptimizePM.addPass(SpeculateAroundPHIsPass());
 
+  if (PTO.Coroutines)
+OptimizePM.addPass(CoroCleanupPass());
+
   for (auto  : OptimizerLastEPCallbacks)
 C(OptimizePM, Level);
 
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -92,6 +92,12 @@
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
+  /// value is false. Frontends such as Clang may enable this conditionally. For
+  /// example, Clang enables this option if the flags `-std=c++2a` or above, or
+  /// `-fcoroutines-ts`, have been specified.
+  bool Coroutines;
+
   /// Tuning option to cap the number of calls to retrive clobbering accesses in
   /// MemorySSA, in LICM.
   unsigned LicmMssaOptCap;
Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,57 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template  struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template  struct coroutine_traits {
+  struct promise_type {
+awaitable initial_suspend() { return {}; }
+awaitable final_suspend() { return {}; }
+void return_void() {}
+T get_return_object() { return T(); }
+void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+void foo() { co_return; }
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 236889.
modocache added a comment.

Update tests -- we now re-run the SCC pass, but don't insert the coroutine 
funclets into the SCC, so we no longer see the funclets in the output being 
tested here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -711,6 +711,8 @@
   EarlyFPM.addPass(SROA());
   EarlyFPM.addPass(EarlyCSEPass());
   EarlyFPM.addPass(LowerExpectIntrinsicPass());
+  if (PTO.Coroutines)
+EarlyFPM.addPass(CoroEarlyPass());
   if (Level == O3)
 EarlyFPM.addPass(CallSiteSplittingPass());
 
@@ -825,6 +827,11 @@
 IP.HotCallSiteThreshold = 0;
   MainCGPipeline.addPass(InlinerPass(IP));
 
+  if (PTO.Coroutines) {
+MainCGPipeline.addPass(CoroSplitPass());
+MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  }
+
   // Now deduce any function attributes based in the current code.
   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
 
@@ -1020,6 +1027,9 @@
   // inserting redundancies into the program. This even includes SimplifyCFG.
   OptimizePM.addPass(SpeculateAroundPHIsPass());
 
+  if (PTO.Coroutines)
+OptimizePM.addPass(CoroCleanupPass());
+
   for (auto  : OptimizerLastEPCallbacks)
 C(OptimizePM, Level);
 
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -92,6 +92,12 @@
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
+  /// value is false. Frontends such as Clang may enable this conditionally. For
+  /// example, Clang enables this option if the flags `-std=c++2a` or above, or
+  /// `-fcoroutines-ts`, have been specified.
+  bool Coroutines;
+
   /// Tuning option to cap the number of calls to retrive clobbering accesses in
   /// MemorySSA, in LICM.
   unsigned LicmMssaOptCap;
Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,57 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template  struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template  struct coroutine_traits {
+  struct promise_type {
+awaitable initial_suspend() { return {}; }
+awaitable final_suspend() { return {}; }
+void return_void() {}
+T get_return_object() { return T(); }
+void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+void foo() { co_return; }
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 +49,10 @@
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Coroutines.h"
+#include 

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 236245.
modocache removed a subscriber: wenlei.
modocache added a comment.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

Thanks for the reviews. Based on my latest revision of D71899 
, the coro-split diff, this patch now no 
longer manually enqueues coro-split twice. In addition, it uses the PassBuilder 
registration callbacks to enqueue coroutine passes in appropriate spots in the 
pipeline. coro-split now restarts the entire `MainCGPipeline` that's built as 
part of module optimization. As discussed, the coroutine passes are enqueued at 
`-O0`, and they're not enqueued when `-disable-llvm-passes` is specified.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -711,6 +711,8 @@
   EarlyFPM.addPass(SROA());
   EarlyFPM.addPass(EarlyCSEPass());
   EarlyFPM.addPass(LowerExpectIntrinsicPass());
+  if (PTO.Coroutines)
+EarlyFPM.addPass(CoroEarlyPass());
   if (Level == O3)
 EarlyFPM.addPass(CallSiteSplittingPass());
 
@@ -825,6 +827,11 @@
 IP.HotCallSiteThreshold = 0;
   MainCGPipeline.addPass(InlinerPass(IP));
 
+  if (PTO.Coroutines) {
+MainCGPipeline.addPass(CoroSplitPass());
+MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  }
+
   // Now deduce any function attributes based in the current code.
   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
 
@@ -1020,6 +1027,9 @@
   // inserting redundancies into the program. This even includes SimplifyCFG.
   OptimizePM.addPass(SpeculateAroundPHIsPass());
 
+  if (PTO.Coroutines)
+OptimizePM.addPass(CoroCleanupPass());
+
   for (auto  : OptimizerLastEPCallbacks)
 C(OptimizePM, Level);
 
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -92,6 +92,12 @@
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
+  /// value is false. Frontends such as Clang may enable this conditionally. For
+  /// example, Clang enables this option if the flags `-std=c++2a` or above, or
+  /// `-fcoroutines-ts`, have been specified.
+  bool Coroutines;
+
   /// Tuning option to cap the number of calls to retrive clobbering accesses in
   /// MemorySSA, in LICM.
   unsigned LicmMssaOptCap;
Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,70 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s --check-prefix=CHECK-O1
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+// CHECK-O1: Starting llvm::Module pass manager run.
+// CHECK-O1: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+// CHECK-O1: Starting CGSCC pass manager run.
+// CHECK-O1: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK-O1: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK-O1: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'. Above -O0, these are inlined
+// and removed.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov, _Z3foov.resume, _Z3foov.destroy, _Z3foov.cleanup)
+// CHECK: Finished CGSCC pass manager run.
+// CHECK-O1: Starting CGSCC pass manager run.
+// CHECK-O1: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK-O1: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK-O1: Finished CGSCC 

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

I'm currently working on ensuring that CGSCC optimizations are rerun to 
optimize coroutine funclets -- the primary feedback I received on this and on 
D71899  -- but I just realized I didn't 
respond to one comment on this set of reviews, from @junparser:

> There is another issue we should consider: clang is crashed when compile 
> coroutine with -disable-llvm-passes and output an object file.

It's always been the case, since the coroutine intrinsics and passes were first 
added to LLVM, that attempting to codegen without first running coroutine 
passes would cause a crash during instruction selection. So `clang -Xclang 
-disable-llvm-passes -c` has always crashed Clang during LLVM ISel, as it does 
in this example that uses Clang 9.0.0 and the legacy pass manager: 
https://godbolt.org/z/Mj2R5G

Personally I'm of the opinion that this is less than ideal... I may be wrong, 
but I don't think there are very many other C++ features that *require* Clang 
to run LLVM passes (perhaps the `always_inline` attribute requires LLVM passes 
to be run for correctness? I'm not sure). So I would like to see this 
eventually addressed somehow.

> Is it reasonable to run coroutine passes even -disable-llvm-passes is enabled?

My personal opinion is that this would not be reasonable. The option 
`-disable-llvm-passes` should, from my point of view, prevent any and all LLVM 
passes from being run. I also frequently make use of this option when debugging 
the LLVM IR being output for C++ coroutines code, so if `-disable-llvm-passes` 
didn't disable coroutines passes, I'd need another option that did 
(`-disable-llvm-passes-no-really-even-coroutine-passes-them-too` ).

All this being said, considering this behavior has existed in the legacy PM 
since day one, I think we should start a separate discussion on if/how to 
change that behavior. I'm working on an update for these patches to address 
funclet optimization, but the update will not change the fact that coroutine 
passes are not run when `-disable-llvm-passes` is specified. I think that's an 
orthogonal issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903



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

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache planned changes to this revision.
modocache marked 2 inline comments as done and an inline comment as not done.
modocache added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1227
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));

junparser wrote:
> modocache wrote:
> > junparser wrote:
> > > Since coro elision depends on other optimization pass(inline and so on)  
> > > implicitly,  how can we adjust the pipeline to achieve this.
> > One option would be to use the new pass manager's registration callbacks, 
> > like `PassBuilder::registerPipelineStartEPCallback` or 
> > `PassBuilder::registerOptimizerLastEPCallback`. These work similarly to the 
> > old pass manager's `PassManagerBuilder::addExtension`. That's something 
> > that I think would be good to improve in a follow-up patch, but let me know 
> > if you'd rather see it in this one.
> yes,  please. It should be done in this patch sets. 
Will do!



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1228
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));

wenlei wrote:
> Manually scheduling the 2nd coro-split passes in the same CGSCC pipeline 
> would make the resume/suspend funclet ineligible for the bulk of CSGSS opts, 
> given the split point is relatively early. The implication would be 
> discouraging the use of coroutine in performance critical path. I would love 
> to see this being addressed before we claim coroutine is ready for new PM.
> 
> As commented in the 2nd patch, we may not need the devirt trick used with 
> legacy PM, instead for new PM, we could try to leverage `CGSCCUpdateResult` 
> without involving artificial indirect call and devirt (or follow how 
> outlining is handled by new PM).
> I would love to see this being addressed before we claim coroutine is ready 
> for new PM.

I apologize, since I didn't intend to make such a claim. In 
http://lists.llvm.org/pipermail/llvm-dev/2019-December/137835.html I explained 
that these 6 patches were focused on lowering coroutine intrinsics. My goal was 
to resolve https://bugs.llvm.org/show_bug.cgi?id=42867, so that C++ coroutines 
code didn't trigger a fatal error in ISel.

That being said, I'm happy to make changes here. I'll send updates for D71899 
and this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903



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

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache marked an inline comment as done.
modocache added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1227
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));

junparser wrote:
> Since coro elision depends on other optimization pass(inline and so on)  
> implicitly,  how can we adjust the pipeline to achieve this.
One option would be to use the new pass manager's registration callbacks, like 
`PassBuilder::registerPipelineStartEPCallback` or 
`PassBuilder::registerOptimizerLastEPCallback`. These work similarly to the old 
pass manager's `PassManagerBuilder::addExtension`. That's something that I 
think would be good to improve in a follow-up patch, but let me know if you'd 
rather see it in this one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71903/new/

https://reviews.llvm.org/D71903



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

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, lewissbaker, chandlerc, junparser.
Herald added subscribers: cfe-commits, EricWF.
Herald added a project: clang.

Depends on https://reviews.llvm.org/D71902.

The last in a series of six patches that ports the LLVM coroutines
passes to the new pass manager infrastructure.

This patch has Clang schedule the new coroutines passes when the
`-fexperimental-new-pass-manager` option is used. With this and the
previous 5 patches, Clang is capable of building and successfully
running the test suite of large coroutines projects such as
https://github.com/lewissbaker/cppcoro with
`ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER=On`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71903

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp


Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,18 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null 
\
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts 
\
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null 
\
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts 
\
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+// CHECK: Running pass:{{.*}}CoroSplitPass
+// CHECK: Running pass:{{.*}}CoroElidePass
+// CHECK: Running pass:{{.*}}CoroSplitPass
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+void foo() {}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 +49,10 @@
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Coroutines.h"
+#include "llvm/Transforms/Coroutines/CoroCleanup.h"
+#include "llvm/Transforms/Coroutines/CoroEarly.h"
+#include "llvm/Transforms/Coroutines/CoroElide.h"
+#include "llvm/Transforms/Coroutines/CoroSplit.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
@@ -1133,6 +1137,16 @@
   if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))
 MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
+  // For IR that makes use of coroutines intrinsics, coroutine passes must
+  // be run, even at -O0.
+  if (LangOpts.Coroutines) {
+MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
+MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
+  }
+
   // Lastly, add semantically necessary passes for LTO.
   if (IsLTO || IsThinLTO) {
 MPM.addPass(CanonicalizeAliasesPass());
@@ -1206,6 +1220,15 @@
   MPM.addPass(InstrProfiling(*Options, false));
 });
 
+  if (LangOpts.Coroutines)
+PB.registerPipelineStartEPCallback([](ModulePassManager ) {
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
+});
+
   if (IsThinLTO) {
 MPM = PB.buildThinLTOPreLinkDefaultPipeline(
 Level, CodeGenOpts.DebugPassManager);


Index: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp
@@ -0,0 +1,18 @@
+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: 

[PATCH] D71709: Give frontend dump flags consistent names (*-dump instead of dump-*)

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a subscriber: mehdi_amini.
modocache added a comment.

Ah, I'm sorry I wasn't clear -- instead of changing a lot of tests to use the 
new names exclusively, my suggestion was to change one or two tests to use the 
new canonical name, and have the remaining tests keep using the alias. That 
would keep the patch small, but still exercise both the new option name 
`-tokens-dump` as well as the old alias `-dump-tokens`.

That being said, this change also works for me! I'd be happy to have this 
landed. But I want to make sure others have input as well. @mehdi_amini  
commented on this patch on the LLVM Discord, for example:

> This patch seems weird to me: with this logic we would shuffle around every 
> possible options (`-fdump-vtable-layouts` / `-fvtable-layouts-dump`, 
> `-disable-O0-optnone` / `-O0-optnone-disable`, ...) ?

To which I responded:

> I'm of the opinion -- and maybe this would be a radical change -- that all 
> "dump" options should either use it as a prefix (`-fdump-vtable-layouts`) or 
> as a suffix (`-fvtable-layouts-dump`). I'm not an expert though, so I don't 
> know what would be the polite way to change option names without disrupting 
> end users of LLVM. I thought the patch was good because it adds aliases, so 
> people who are used to `-dump-tokens` or `-ast-dump` could still use those 
> option names.
> 
> So I think my answer to your question is "no, we shouldn't provide aliases 
> with all permutations of words in options." My take is "for options that dump 
> some sort of intermediate output, dump should appear in a standard place in 
> the option name. Whether it's at the beginning (`-dump-ast`, `-dump-tokens`) 
> or at the end (`-ast-dump`, `-tokens-dump`) doesn't matter to me, per se, as 
> long as it's consistent." And, my second opinion: "Personally I'd love to 
> change all of these option names right now, but that might be disruptive, I 
> don't know. If it isn't, then let's change them. If it is, then using aliases 
> seems like a good migratory plan."

I'd like to wait and give others the chance to review this patch, so I'll wait 
a bit to accept it. It would help me personally day to day to have this change 
landed, because I can never remember the order of options like `-tokens-dump` 
and `-dump-ast`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71709/new/

https://reviews.llvm.org/D71709



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

[PATCH] D71731: [Format] fix dereference of pointers in co_yeld and co_return statements

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Nice! Thanks for this. This looks good to me, but I'll defer to the other 
reviewers you specified, since I think they're more familiar with clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71731/new/

https://reviews.llvm.org/D71731



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

[PATCH] D71709: Give frontend dump flags consistent names (*-dump instead of dump-*)

[Brian Gesiak via Phabricator via cfe-commits]

modocache requested changes to this revision.
modocache added subscribers: jroelofs, echristo.
modocache added a comment.
This revision now requires changes to proceed.

Grepping for "dump-tokens", I can see one regression test that exercises this 
option: clang/test/Lexer/dollar-idents.c. "dump-coverage-mapping" also shows up 
in a few regression tests. One way to exercise the new option names is to 
modify these tests to use both the new and old option names. I think this would 
test that they can be used interchangeably.

As for the unit tests in llvm/unittests or clang/unittests, I agree, I don't 
think this diff requires any new tests in there.

I'll request changes on this diff because some `switch` statements in Clang 
need to be updated to handle the new option names. Also I'll suggest some 
additional reviewers: @jroelofs (who suggested we create aliases for these), 
and @echristo (because these options were last modified over 7 years ago and 
Eric has a long tenure in this codebase).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71709/new/

https://reviews.llvm.org/D71709



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

[PATCH] D71542: [coroutines][PR41909] don't build dependent coroutine statements if the coroutine still has a dependent promise type

[Brian Gesiak via Phabricator via cfe-commits]

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

Great minds think alike! This looks like the patch I sent in November, D70579 
.  I only just committed it two days ago in 
rG376cf43 
, but I 
ought to have updated the bug report. Thanks for pointing this out!

Unfortunately we duplicated some work here, and so I'm not sure this patch is 
needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71542/new/

https://reviews.llvm.org/D71542



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

[PATCH] D70579: [coroutines][PR41909] Generalize fix from D62550

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thanks for the review! Much appreciated :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70579/new/

https://reviews.llvm.org/D70579



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

[PATCH] D70579: [coroutines][PR41909] Generalize fix from D62550

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG376cf43729c8: [coroutines][PR41909] Generalize fix from 
D62550 (authored by modocache).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70579/new/

https://reviews.llvm.org/D70579

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/coroutines.cpp


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -731,6 +731,12 @@
   [](auto ) -> coro {
 co_await 24;
   }("argument");
+  [](auto ) ->coro { // expected-warning {{expression 
result unused}}
+[]() -> coro {
+  co_await 36;
+};
+co_await 48;
+  };
 }
 template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in 
instantiation of function template specialization 
'ok_generic_lambda_coawait_PR41909' requested here}}
 
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -7190,8 +7190,12 @@
   // that may fail.
   ScopeInfo->setNeedsCoroutineSuspends(false);
 
-  // The new CoroutinePromise object needs to be built and put into the current
-  // FunctionScopeInfo before any transformations or rebuilding occurs.
+  // We re-build the coroutine promise object (and the coroutine parameters its
+  // type and constructor depend on) based on the types used in our current
+  // function. We must do so, and set it on the current FunctionScopeInfo,
+  // before attempting to transform the other parts of the coroutine body
+  // statement, such as the implicit suspend statements (because those
+  // statements reference the FunctionScopeInfo::CoroutinePromise).
   if (!SemaRef.buildCoroutineParameterMoves(FD->getLocation()))
 return StmtError();
   auto *Promise = SemaRef.buildCoroutinePromise(FD->getLocation());
@@ -7200,8 +7204,9 @@
   getDerived().transformedLocalDecl(S->getPromiseDecl(), {Promise});
   ScopeInfo->CoroutinePromise = Promise;
 
-  // Transform the implicit coroutine statements we built during the initial
-  // parse.
+  // Transform the implicit coroutine statements constructed using dependent
+  // types during the previous parse: initial and final suspensions, the return
+  // object, and others. We also transform the coroutine function's body.
   StmtResult InitSuspend = getDerived().TransformStmt(S->getInitSuspendStmt());
   if (InitSuspend.isInvalid())
 return StmtError();
@@ -7228,17 +7233,13 @@
 return StmtError();
   Builder.ReturnValue = Res.get();
 
+  // If during the previous parse the coroutine still had a dependent promise
+  // statement, we may need to build some implicit coroutine statements
+  // (such as exception and fallthrough handlers) for the first time.
   if (S->hasDependentPromiseType()) {
-// PR41909: We may find a generic coroutine lambda definition within a
-// template function that is being instantiated. In this case, the lambda
-// will have a dependent promise type, until it is used in an expression
-// that creates an instantiation with a non-dependent promise type. We
-// should not assert or build coroutine dependent statements for such a
-// generic lambda.
-auto *MD = dyn_cast_or_null(FD);
-if (!MD || !MD->getParent()->isGenericLambda()) {
-  assert(!Promise->getType()->isDependentType() &&
- "the promise type must no longer be dependent");
+// We can only build these statements, however, if the current promise type
+// is not dependent.
+if (!Promise->getType()->isDependentType()) {
   assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
  !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
  "these nodes should not have been built yet");


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -731,6 +731,12 @@
   [](auto ) -> coro {
 co_await 24;
   }("argument");
+  [](auto ) ->coro { // expected-warning {{expression result unused}}
+[]() -> coro {
+  co_await 36;
+};
+co_await 48;
+  };
 }
 template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in instantiation of function template specialization 'ok_generic_lambda_coawait_PR41909' requested here}}
 
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -7190,8 +7190,12 @@
   // that may fail.
   ScopeInfo->setNeedsCoroutineSuspends(false);
 
-  // The new CoroutinePromise object needs to be built and put into the current
-  // FunctionScopeInfo before any transformations or rebuilding occurs.
+  // We re-build 

[PATCH] D70219: Make `-fmodule-file==` apply to .pcm file compilations

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

This seems like a outright improvement, but as mentioned above it would be nice 
to get @rsmith's take on it since I'm not an expert. Two nitpicks, though:

1. Could you please run clang-format on the lines you added/modified?
2. Could you apply these changes on top of the llvm-project monorepo? You can 
do so by using the "download raw diff" link on this page, and then running `git 
apply --directory=clang -p 0 D70219.diff` from within the monorepo. I tried 
just now to confirm the tests pass with this change (they do!), and that 
command was able to apply the patch cleanly for me.




Comment at: lib/Frontend/FrontendAction.cpp:568
+ASTDiags, CI.getFileSystemOpts(), CI.getCodeGenOpts().DebugTypeExtRefs,
+false, None, CaptureDiagsKind::None, false, false, 
CI.getHeaderSearchOpts().PrebuiltModuleFiles);
 if (!AST)

For example I think this line would change if clang-format were to be run on it.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70219/new/

https://reviews.llvm.org/D70219



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

[PATCH] D70579: [coroutines][PR41909] Generalize fix from D62550

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

@GorNishanov, @rsmith, friendly ping! @rsmith this patch addresses your 
requests from https://reviews.llvm.org/D62550.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70579/new/

https://reviews.llvm.org/D70579



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

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:1761
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;

I didn't notice this during review, but `tok_kw_co_yield` is a typo. I'm 
replacing this with `tok::kw_co_yield` in the commit I'm making for this patch, 
as that spelling allows the build and tests to pass.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69180/new/

https://reviews.llvm.org/D69180



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

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8682d29a2877: [Format] Add format check for coroutine 
keywords with negative numbers (authored by modocache).

Changed prior to commit:
  https://reviews.llvm.org/D69180?vs=225672=231596#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69180/new/

https://reviews.llvm.org/D69180

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6990,6 +6990,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1829,7 +1829,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok::kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6990,6 +6990,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1829,7 +1829,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok::kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thanks again for the patch @junparser! And sorry the review took so long!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69022/new/

https://reviews.llvm.org/D69022



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

[PATCH] D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0b3d1d1348da: [coroutines] Remove assert on 
CoroutineParameterMoves in Sema… (authored by modocache).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69022/new/

https://reviews.llvm.org/D69022

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutines.cpp


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function 
cannot be a coroutine: 'std::experimental::coroutine_traits' has no 
member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int 
promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be 
a coroutine: 'experimental::coroutine_traits::promise_type' 
(aka 'int') is not a class}}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1527,8 +1527,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits' has no member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be a coroutine: 'experimental::coroutine_traits::promise_type' (aka 'int') is not a class}}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1527,8 +1527,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

LGTM! Let me know if you'd like me to commit this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69180/new/

https://reviews.llvm.org/D69180



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

[PATCH] D62035: [AST] const-ify ObjC inherited class search

[Brian Gesiak via Phabricator via cfe-commits]

modocache abandoned this revision.
modocache added a comment.

I'm not super interested in this patch anymore, someone else feel free to work 
on this! :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62035/new/

https://reviews.llvm.org/D62035



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

[PATCH] D59765: [Lex] Warn about invisible Hangul whitespace

[Brian Gesiak via Phabricator via cfe-commits]

modocache abandoned this revision.
modocache added a comment.

I'm not super interested in this patch anymore, someone else feel free to work 
on this! :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59765/new/

https://reviews.llvm.org/D59765



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

[PATCH] D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! Please let me know if you'd like me to commit this change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69022/new/

https://reviews.llvm.org/D69022



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

[PATCH] D70579: [coroutines][PR41909] Generalize fix from D62550

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, rsmith, lewissbaker.
Herald added a subscriber: EricWF.
Herald added a project: clang.

In https://reviews.llvm.org/D62550 @rsmith pointed out that there are
many situations in which a coroutine body statement may be
transformed/rebuilt as part of a template instantiation, and my naive
check whether the coroutine was a generic lambda was insufficient.

This is indeed true, as I've learned by reading more of the
TreeTransform code. Most transformations are written in a way that
doesn't assume the resulting types are not dependent types. So the
assertion in 'TransformCoroutineBodyStmt', that the promise type must no
longer be dependent, is out of place.

This patch removes the assertion, spruces up some code comments, and
adds a test that would have failed with my naive check from
https://reviews.llvm.org/D62550.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70579

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/coroutines.cpp


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -726,6 +726,12 @@
   [](auto ) -> coro {
 co_await 24;
   }("argument");
+  [](auto ) ->coro { // expected-warning {{expression 
result unused}}
+[]() -> coro {
+  co_await 36;
+};
+co_await 48;
+  };
 }
 template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in 
instantiation of function template specialization 
'ok_generic_lambda_coawait_PR41909' requested here}}
 
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -7205,8 +7205,12 @@
   // that may fail.
   ScopeInfo->setNeedsCoroutineSuspends(false);
 
-  // The new CoroutinePromise object needs to be built and put into the current
-  // FunctionScopeInfo before any transformations or rebuilding occurs.
+  // We re-build the coroutine promise object (and the coroutine parameters its
+  // type and constructor depend on) based on the types used in our current
+  // function. We must do so, and set it on the current FunctionScopeInfo,
+  // before attempting to transform the other parts of the coroutine body
+  // statement, such as the implicit suspend statements (because those
+  // statements reference the FunctionScopeInfo::CoroutinePromise).
   if (!SemaRef.buildCoroutineParameterMoves(FD->getLocation()))
 return StmtError();
   auto *Promise = SemaRef.buildCoroutinePromise(FD->getLocation());
@@ -7215,8 +7219,9 @@
   getDerived().transformedLocalDecl(S->getPromiseDecl(), {Promise});
   ScopeInfo->CoroutinePromise = Promise;
 
-  // Transform the implicit coroutine statements we built during the initial
-  // parse.
+  // Transform the implicit coroutine statements constructed using dependent
+  // types during the previous parse: initial and final suspensions, the return
+  // object, and others. We also transform the coroutine function's body.
   StmtResult InitSuspend = getDerived().TransformStmt(S->getInitSuspendStmt());
   if (InitSuspend.isInvalid())
 return StmtError();
@@ -7243,17 +7248,13 @@
 return StmtError();
   Builder.ReturnValue = Res.get();
 
+  // If during the previous parse the coroutine still had a dependent promise
+  // statement, we may need to build some implicit coroutine statements
+  // (such as exception and fallthrough handlers) for the first time.
   if (S->hasDependentPromiseType()) {
-// PR41909: We may find a generic coroutine lambda definition within a
-// template function that is being instantiated. In this case, the lambda
-// will have a dependent promise type, until it is used in an expression
-// that creates an instantiation with a non-dependent promise type. We
-// should not assert or build coroutine dependent statements for such a
-// generic lambda.
-auto *MD = dyn_cast_or_null(FD);
-if (!MD || !MD->getParent()->isGenericLambda()) {
-  assert(!Promise->getType()->isDependentType() &&
- "the promise type must no longer be dependent");
+// We can only build these statements, however, if the current promise type
+// is not dependent.
+if (!Promise->getType()->isDependentType()) {
   assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
  !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
  "these nodes should not have been built yet");


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -726,6 +726,12 @@
   [](auto ) -> coro {
 co_await 24;
   }("argument");
+  [](auto ) ->coro { // expected-warning {{expression result unused}}
+[]() -> coro {
+  co_await 36;
+};
+ 

[PATCH] D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

[Brian Gesiak via Phabricator via cfe-commits]

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

Sorry for the slow response here, @junparser!

The test case you came up with here is great! I can see the issue is that 
`ScopeInfo->CoroutineParameterMoves` are built up when Clang parses the first 
`co_await a`, but are not cleared when `lookupPromiseType` results in an error. 
As a result, when Clang hits the second `co_await a`, it's in a state that the 
current code didn't anticipate. Your test case does a great job demonstrating 
this. Your fix for the problem also looks good to me! My only suggestion is to 
make the test case just a little clearer, as I'll explain...

(Also, in the future could you please upload your patches with full context? 
You can read https://llvm.org/docs/Phabricator.html for more details. I think 
the section explaining the web interface might be relevant to you, where it 
suggests `git show HEAD -U99 > mypatch.patch`. The reason I ask is because 
on Phabricator I can see what lines you're proposing should be added, but not 
the surrounding source lines, which made this more difficult to review.)




Comment at: test/SemaCXX/coroutines.cpp:93
+  co_await a;
+}
+

This is a great test case, thanks for coming up with it! However, I think it 
could be a little clearer, though: right now the `int a` variable is shadowing 
the `awaitable a` that's declared further up in this file. At first, I wasn't 
sure whether the shadowing had something to do with the test, but in fact it 
doesn't. This test case demonstrates the issue but without the confusion, I 
think:

```
int no_promise_type_multiple_awaits(int) { // expected-error {{this function 
cannot be a coroutine: 'std::experimental::coroutine_traits' has no 
member named 'promise_type'}}
  co_await a;
  co_await a;
}
```


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69022/new/

https://reviews.llvm.org/D69022



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

[PATCH] D70555: [coroutines] Don't build promise init with no args

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, rsmith, lewissbaker.
Herald added subscribers: cfe-commits, EricWF.
Herald added a project: clang.
modocache edited the summary of this revision.

In the case of a coroutine that takes no arguments,
`Sema::buildCoroutinePromise` constructs a list-initialization
(`clang::InitializationKind::InitKind::IK_DirectList`) of the
promise variable, using a list of empty arguments. So, if one were to
dump the promise `VarDecl` immediately after `Sema::ActOnCoroutineBodyStart`
calls `checkCoroutineContext`, for a coroutine function that takes no
arguments, they'd see the following:

  VarDecl 0xb514490  col:3 __promise '' callinit
  `-ParenListExpr 0xb514510  'NULL TYPE'

But after this patch, the `ParenListExpr` is no longer constructed, and
the promise variable uses default initialization
(`clang::InitializationKind::InitKind::IK_Default`):

  VarDecl 0x63100012dae0  col:3 __promise ''

As far as I know, there's no case in which list-initialization with no
arguments differs from default initialization, but if I'm wrong please
let me know (and I'll add a test case that demonstrates the change --
but as-is I can't think of a functional test case for this). I think both
comply with the wording of C++20 `[dcl.fct.def.coroutine]p5`:

> _promise-constructor-arguments_ is determined as follows: overload
>  resolution is performed on a promise constructor call created by
>  assembling an argument list with lvalues `p1 ... pn`. If a viable
>  constructor is found (12.4.2), then _promise-constructor-arguments_
>  is `(p1, ... , pn)`, otherwise _promise-constructor-arguments_ is
>  empty.

Still, I think this patch is an improvement regardless, because it
reduces the size of the AST.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70555

Files:
  clang/lib/Sema/SemaCoroutine.cpp


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -502,8 +502,9 @@
 return nullptr;
 
   auto *ScopeInfo = getCurFunction();
-  // Build a list of arguments, based on the coroutine functions arguments,
-  // that will be passed to the promise type's constructor.
+
+  // Build a list of arguments, based on the coroutine function's arguments,
+  // that if present will be passed to the promise type's constructor.
   llvm::SmallVector CtorArgExprs;
 
   // Add implicit object parameter.
@@ -519,6 +520,7 @@
 }
   }
 
+  // Add the coroutine function's parameters.
   auto  = ScopeInfo->CoroutineParameterMoves;
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
@@ -540,28 +542,33 @@
 CtorArgExprs.push_back(RefExpr.get());
   }
 
-  // Create an initialization sequence for the promise type using the
-  // constructor arguments, wrapped in a parenthesized list expression.
-  Expr *PLE = ParenListExpr::Create(Context, FD->getLocation(),
-CtorArgExprs, FD->getLocation());
-  InitializedEntity Entity = InitializedEntity::InitializeVariable(VD);
-  InitializationKind Kind = InitializationKind::CreateForInit(
-  VD->getLocation(), /*DirectInit=*/true, PLE);
-  InitializationSequence InitSeq(*this, Entity, Kind, CtorArgExprs,
- /*TopLevelOfInitList=*/false,
- /*TreatUnavailableAsInvalid=*/false);
-
-  // Attempt to initialize the promise type with the arguments.
-  // If that fails, fall back to the promise type's default constructor.
-  if (InitSeq) {
-ExprResult Result = InitSeq.Perform(*this, Entity, Kind, CtorArgExprs);
-if (Result.isInvalid()) {
-  VD->setInvalidDecl();
-} else if (Result.get()) {
-  VD->setInit(MaybeCreateExprWithCleanups(Result.get()));
-  VD->setInitStyle(VarDecl::CallInit);
-  CheckCompleteVariableDeclaration(VD);
-}
+  // If we have a non-zero number of constructor arguments, try to use them.
+  // Otherwise, fall back to the promise type's default constructor.
+  if (!CtorArgExprs.empty()) {
+// Create an initialization sequence for the promise type using the
+// constructor arguments, wrapped in a parenthesized list expression.
+Expr *PLE = ParenListExpr::Create(Context, FD->getLocation(),
+  CtorArgExprs, FD->getLocation());
+InitializedEntity Entity = InitializedEntity::InitializeVariable(VD);
+InitializationKind Kind = InitializationKind::CreateForInit(
+VD->getLocation(), /*DirectInit=*/true, PLE);
+InitializationSequence InitSeq(*this, Entity, Kind, CtorArgExprs,
+   /*TopLevelOfInitList=*/false,
+   /*TreatUnavailableAsInvalid=*/false);
+
+// Attempt to initialize the promise type with the arguments.
+// If that fails, fall back to the promise type's default constructor.
+if (InitSeq) {
+ 

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 227808.
modocache added a comment.

Rebasing onto the monorepo. @rsmith, I confirmed the test cases that this diff 
adds still fail on trunk, and that the Clang source changes made in this diff 
fix the test case failures. Tomorrow I plan on poking around to see if I can 
reproduce similar issues in the C++20 modules implementation. But in the 
meantime, how do you feel about this patch? You suggested a change to 
`~FindExistingResult` but also that it'd be difficult to test/verify such a 
change. Is that change still something you're looking for before accepting this 
diff?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58920/new/

https://reviews.llvm.org/D58920

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/pr39287-2/a.h
  clang/test/Modules/Inputs/pr39287-2/module.modulemap
  clang/test/Modules/Inputs/pr39287/a.h
  clang/test/Modules/Inputs/pr39287/module.modulemap
  clang/test/Modules/pr39287-2.cpp
  clang/test/Modules/pr39287.cpp

Index: clang/test/Modules/pr39287.cpp
===
--- /dev/null
+++ clang/test/Modules/pr39287.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/pr39287 %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: clang/test/Modules/pr39287-2.cpp
===
--- /dev/null
+++ clang/test/Modules/pr39287-2.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/pr39287-2 %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: clang/test/Modules/Inputs/pr39287/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: clang/test/Modules/Inputs/pr39287/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: clang/test/Modules/Inputs/pr39287-2/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287-2/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: clang/test/Modules/Inputs/pr39287-2/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287-2/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -47,6 +47,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/IdentifierResolver.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
@@ -3195,6 +3196,9 @@
 // Add the declaration to its redeclaration context so later merging
 // lookups will find it.
 MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true);
+if (isa(New) && Name.getAsString() == "std")
+  if (!Reader.getSema()->StdNamespace)
+Reader.getSema()->StdNamespace = New;
   }
 }
 
@@ -3358,6 +3362,14 @@
   return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
 TypedefNameForLinkage);
 }
+if (isa(D) && D->getName() == "std") {
+  auto StdPtr = Reader.getSema()->StdNamespace;
+  if (StdPtr.isValid() && !StdPtr.isOffset())
+if (auto *Std = cast_or_null(StdPtr.get(nullptr)))
+  if (isSameEntity(Std, D))
+return FindExistingResult(Reader, D, Std, AnonymousDeclNumber,
+  TypedefNameForLinkage);
+}
   } else {
 // Not in a mergeable context.
 return FindExistingResult(Reader);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69144: [Format] Add format check for throwing negative numbers

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f704320b058: [Format] Add format check for throwing 
negative numbers (authored by modocache).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69144/new/

https://reviews.llvm.org/D69144

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1757,7 +1757,7 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1757,7 +1757,7 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51741: [coro]Pass rvalue reference for named local variable to return_value

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a subscriber: lewissbaker.
modocache added a comment.

> Is that maybe intentional, and is the code not intended to compile?

It looks like it should work to me, but maybe @lewissbaker or @GorNishanov can 
answer definitively.




Comment at: cfe/trunk/lib/Sema/SemaCoroutine.cpp:846
+  if (E) {
+auto NRVOCandidate = this->getCopyElisionCandidate(E->getType(), E, 
CES_AsIfByStdMove);
+if (NRVOCandidate) {

aaronpuchert wrote:
> Why not `CES_Strict` like in `Sema::BuildReturnStmt`? With `CES_Strict` the 
> test still works, and we can also return references.
So this fixes your test case? If so it sounds good to me. I'll make this change 
or you can feel free to if you get around to it first.



Comment at: cfe/trunk/lib/Sema/SemaCoroutine.cpp:849
+  InitializedEntity Entity =
+  InitializedEntity::InitializeResult(Loc, E->getType(), 
NRVOCandidate);
+  ExprResult MoveResult = this->PerformMoveOrCopyInitialization(

aaronpuchert wrote:
> The last parameter has type `bool`, and because we're in `if 
> (NRVOCandidate)`, that will always be true. Wouldn't it be more 
> straightforward to just pass `true` into the function?
Makes sense! I can send a patch to do this, or feel free to commit one yourself 
if you get to it first.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51741/new/

https://reviews.llvm.org/D51741



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

[PATCH] D65044: [Format] Add option to enable coroutines keywords

[Brian Gesiak via Phabricator via cfe-commits]

modocache abandoned this revision.
modocache added a comment.

I work on a C++17 codebase with coroutines enabled, but yes 
just formatting the codebase as if it were C++20 seems fine. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65044/new/

https://reviews.llvm.org/D65044



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache abandoned this revision.
modocache added a comment.

Oh, thank you! Yes, I had been meaning to abandon this and my other patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043



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

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thank you!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



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

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368675: [CodeGen] Disable UBSan for coroutine functions 
(authored by modocache, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44672?vs=214370=214809#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672

Files:
  cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
  cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp


Index: cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
+++ cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// This test merely verifies that emitting the object file does not cause a
+// crash when the LLVM coroutines passes are run.
+// RUN: %clang_cc1 -emit-obj -std=c++2a -fsanitize=null %s -o %t.o
+
+namespace std::experimental {
+template  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  coroutine_handle() = default;
+  static coroutine_handle from_address(void *) noexcept;
+};
+}
+
+struct suspend_always {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct task {
+  struct promise_type {
+task get_return_object() { return task(); }
+suspend_always initial_suspend() { return {}; }
+suspend_always final_suspend() { return {}; }
+void return_void() {}
+void unhandled_exception() {}
+  };
+};
+
+struct awaitable {
+  task await() { (void)co_await *this; }
+  bool await_ready() { return false; }
+  bool await_suspend(std::experimental::coroutine_handle<> awaiter) { return 
false; }
+  bool await_resume() { return false; }
+};
+
+int main() {
+  awaitable a;
+  a.await();
+}
Index: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
@@ -732,6 +732,15 @@
   SanOpts.Mask &= ~SanitizerKind::CFIUnrelatedCast;
   }
 
+  // Ignore null checks in coroutine functions since the coroutines passes
+  // are not aware of how to move the extra UBSan instructions across the split
+  // coroutine boundaries.
+  if (D && SanOpts.has(SanitizerKind::Null))
+if (const auto *FD = dyn_cast(D))
+  if (FD->getBody() &&
+  FD->getBody()->getStmtClass() == Stmt::CoroutineBodyStmtClass)
+SanOpts.Mask &= ~SanitizerKind::Null;
+
   // Apply xray attributes to the function (as a string, for now)
   if (D) {
 if (const auto *XRayAttr = D->getAttr()) {


Index: cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
+++ cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// This test merely verifies that emitting the object file does not cause a
+// crash when the LLVM coroutines passes are run.
+// RUN: %clang_cc1 -emit-obj -std=c++2a -fsanitize=null %s -o %t.o
+
+namespace std::experimental {
+template  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  coroutine_handle() = default;
+  static coroutine_handle from_address(void *) noexcept;
+};
+}
+
+struct suspend_always {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct task {
+  struct promise_type {
+task get_return_object() { return task(); }
+suspend_always initial_suspend() { return {}; }
+suspend_always final_suspend() { return {}; }
+void return_void() {}
+void unhandled_exception() {}
+  };
+};
+
+struct awaitable {
+  task await() { (void)co_await *this; }
+  bool await_ready() { return false; }
+  bool await_suspend(std::experimental::coroutine_handle<> awaiter) { return false; }
+  bool await_resume() { return false; }
+};
+
+int main() {
+  awaitable a;
+  a.await();
+}
Index: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
@@ -732,6 +732,15 @@
   SanOpts.Mask &= 

[PATCH] D65149: [Format] Add test demonstrating PR42722

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:5184
+   "c++;\n"
+   "d++\n"
+   "  });\n"

MyDeveloperDay wrote:
> modocache wrote:
> > This is a passing test that demonstrates that the indentation of `c++` and 
> > `d++` here is wacky.
> in what way do you think its incorrect? change the test to show what it 
> should be...I think its better to commit a fix and a test at the same time 
> then its easier for the reviewer and others to understand what you are 
> driving at.
The linked bug report, https://bugs.llvm.org/show_bug.cgi?id=42722, describes 
what I think the indentation of these two lines should be. I apologize, I 
should have included the actual code snippet in the commit message here. I am 
curious if you think the indentation in that bug report is desirable, though, 
so please let me know!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65149/new/

https://reviews.llvm.org/D65149



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

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 214370.
modocache added a comment.

Thanks for the review, @vsk! Sorry it took me so long to update this diff.

In the mailing list discussion, 
http://lists.llvm.org/pipermail/llvm-dev/2018-March/121925.html, you mentioned 
that I should use an allow-list of known good sanitizer options, rather than a 
deny-list of known bad ones (`-fsanitize=null`, in this case). However, at this 
time I'm only aware of problems with `-fsanitize=null`, so I don't know what 
else to exclude. In other words, an allow-list of known good sanitizers would, 
at this point, be every other sanitizer pass. So I figured for now, I'll just 
deny `-fsanitize=null`. Does that work for you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGenCXX/ubsan-coroutines.cpp


Index: clang/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// This test merely verifies that emitting the object file does not cause a
+// crash when the LLVM coroutines passes are run.
+// RUN: %clang_cc1 -emit-obj -std=c++2a -fsanitize=null %s -o %t.o
+
+namespace std::experimental {
+template  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  coroutine_handle() = default;
+  static coroutine_handle from_address(void *) noexcept;
+};
+}
+
+struct suspend_always {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct task {
+  struct promise_type {
+task get_return_object() { return task(); }
+suspend_always initial_suspend() { return {}; }
+suspend_always final_suspend() { return {}; }
+void return_void() {}
+void unhandled_exception() {}
+  };
+};
+
+struct awaitable {
+  task await() { (void)co_await *this; }
+  bool await_ready() { return false; }
+  bool await_suspend(std::experimental::coroutine_handle<> awaiter) { return 
false; }
+  bool await_resume() { return false; }
+};
+
+int main() {
+  awaitable a;
+  a.await();
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -732,6 +732,15 @@
   SanOpts.Mask &= ~SanitizerKind::CFIUnrelatedCast;
   }
 
+  // Ignore null checks in coroutine functions since the coroutines passes
+  // are not aware of how to move the extra UBSan instructions across the split
+  // coroutine boundaries.
+  if (D && SanOpts.has(SanitizerKind::Null))
+if (const auto *FD = dyn_cast(D))
+  if (FD->getBody() &&
+  FD->getBody()->getStmtClass() == Stmt::CoroutineBodyStmtClass)
+SanOpts.Mask &= ~SanitizerKind::Null;
+
   // Apply xray attributes to the function (as a string, for now)
   if (D) {
 if (const auto *XRayAttr = D->getAttr()) {


Index: clang/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// This test merely verifies that emitting the object file does not cause a
+// crash when the LLVM coroutines passes are run.
+// RUN: %clang_cc1 -emit-obj -std=c++2a -fsanitize=null %s -o %t.o
+
+namespace std::experimental {
+template  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  coroutine_handle() = default;
+  static coroutine_handle from_address(void *) noexcept;
+};
+}
+
+struct suspend_always {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct task {
+  struct promise_type {
+task get_return_object() { return task(); }
+suspend_always initial_suspend() { return {}; }
+suspend_always final_suspend() { return {}; }
+void return_void() {}
+void unhandled_exception() {}
+  };
+};
+
+struct awaitable {
+  task await() { (void)co_await *this; }
+  bool await_ready() { return false; }
+  bool await_suspend(std::experimental::coroutine_handle<> awaiter) { return false; }
+  bool await_resume() { return false; }
+};
+
+int main() {
+  

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:13780
+  // In C++2a, it is interpreted as a prefix increment on 'i'.
+  verifyFormat("co_yield++ i;");
+  verifyFormat("co_yield ++i;", Cpp2a);

Quuxplusone wrote:
> My comment https://reviews.llvm.org/D65043#inline-582865 still stands. Please 
> just write
> 
> verifyFormat("f(co_yield - 1);");
> verifyFormat("f(co_yield -1);", Cpp2a);
> 
Unfortunately I think there's a separate issue here, that `co_yield -1` is 
formatted as `co_yield - 1` no matter which C++ standard is used. I'd like to 
address that in a separate commit, if that's alright. For now, I removed the 
test verifying the C++17 behavior, and instead only test the correct C++20 
formatting of `co_yield ++it`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 214168.
modocache marked 3 inline comments as done.
modocache added a comment.

Thanks for the review, @Quuxplusone! I addressed your test comments, but the 
'co_yield' test is something I think I'll need to deal with separately.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -3803,10 +3803,13 @@
   "if (aa.aa(\n"
   "aa) == 5) {\n"
   "}");
+
+  FormatStyle Cpp2a = getLLVMStyle();
+  Cpp2a.Standard = FormatStyle::LS_Cpp2a;
   verifyFormat(
   "if (a(\n"
   "aa) <=> 5) {\n"
-  "}");
+  "}", Cpp2a);
   // Even explicit parentheses stress the precedence enough to make the
   // additional break unnecessary.
   verifyFormat("if ((a +\n"
@@ -3829,7 +3832,7 @@
   verifyFormat("if (a +\n"
"aa <=>\n"
"5) {\n"
-   "}");
+   "}", Cpp2a);
 
   FormatStyle OnePerLine = getLLVMStyle();
   OnePerLine.BinPackParameters = false;
@@ -11839,8 +11842,14 @@
   Style.Standard = FormatStyle::LS_Auto;
   CHECK_PARSE("Standard: Cpp03", Standard, FormatStyle::LS_Cpp03);
   CHECK_PARSE("Standard: Cpp11", Standard, FormatStyle::LS_Cpp11);
+  CHECK_PARSE("Standard: Cpp14", Standard, FormatStyle::LS_Cpp14);
+  CHECK_PARSE("Standard: Cpp17", Standard, FormatStyle::LS_Cpp17);
+  CHECK_PARSE("Standard: Cpp2a", Standard, FormatStyle::LS_Cpp2a);
   CHECK_PARSE("Standard: C++03", Standard, FormatStyle::LS_Cpp03);
   CHECK_PARSE("Standard: C++11", Standard, FormatStyle::LS_Cpp11);
+  CHECK_PARSE("Standard: C++14", Standard, FormatStyle::LS_Cpp14);
+  CHECK_PARSE("Standard: C++17", Standard, FormatStyle::LS_Cpp17);
+  CHECK_PARSE("Standard: C++2a", Standard, FormatStyle::LS_Cpp2a);
   CHECK_PARSE("Standard: Auto", Standard, FormatStyle::LS_Auto);
 
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
@@ -13770,6 +13779,18 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
+TEST_F(FormatTest, Coroutines) {
+  FormatStyle Cpp2a = getLLVMStyle();
+  Cpp2a.Standard = FormatStyle::LS_Cpp2a;
+
+  // 'co_yield' is treated as an identifier in standards below C++2a, and so
+  // the increment is interpreted as a postfix on that identifier.
+  // In C++2a, it is interpreted as a prefix increment on 'it'.
+  verifyFormat("f(co_yield ++it);", Cpp2a);
+
+  verifyFormat("co_await []() -> g { co_return; }();", Cpp2a);
+}
+
 TEST_F(FormatTest, FileAndCode) {
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.cc", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2862,7 +2862,7 @@
 (Style.Language == FormatStyle::LK_Proto && Left.is(TT_DictLiteral)))
   return !Style.Cpp11BracedListStyle;
 return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-   (Style.Standard < FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+   (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }
   if (Right.isOneOf(tok::arrow, tok::arrowstar, tok::periodstar) ||
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
@@ -2881,7 +2881,7 @@
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
   if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
 return (Left.is(TT_TemplateOpener) &&
-Style.Standard < FormatStyle::LS_Cpp11) ||
+Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
   TT_TemplateOpener)) ||
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -71,6 +71,12 @@
 IO.enumCase(Value, "C++03", FormatStyle::LS_Cpp03);
 IO.enumCase(Value, "Cpp11", FormatStyle::LS_Cpp11);
 IO.enumCase(Value, "C++11", FormatStyle::LS_Cpp11);
+IO.enumCase(Value, "Cpp14", 

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Friendly ping! @sammccall is this what you had in mind?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 213360.
modocache added a comment.

Thanks for the reviews, @sammccall, @Quuxplusone, and @MyDeveloperDay. I added 
C++14 and C++17 options. In an earlier comment I mentioned splitting this work 
up into a series of commits, but it ended up being a smaller set of changes 
than I thought, so I'll just update this diff with all of the changes. What do 
you all think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -3794,10 +3794,13 @@
   "if (aa.aa(\n"
   "aa) == 5) {\n"
   "}");
+
+  FormatStyle Cpp2a = getLLVMStyle();
+  Cpp2a.Standard = FormatStyle::LS_Cpp2a;
   verifyFormat(
   "if (a(\n"
   "aa) <=> 5) {\n"
-  "}");
+  "}", Cpp2a);
   // Even explicit parentheses stress the precedence enough to make the
   // additional break unnecessary.
   verifyFormat("if ((a +\n"
@@ -3820,7 +3823,7 @@
   verifyFormat("if (a +\n"
"aa <=>\n"
"5) {\n"
-   "}");
+   "}", Cpp2a);
 
   FormatStyle OnePerLine = getLLVMStyle();
   OnePerLine.BinPackParameters = false;
@@ -11830,8 +11833,14 @@
   Style.Standard = FormatStyle::LS_Auto;
   CHECK_PARSE("Standard: Cpp03", Standard, FormatStyle::LS_Cpp03);
   CHECK_PARSE("Standard: Cpp11", Standard, FormatStyle::LS_Cpp11);
+  CHECK_PARSE("Standard: Cpp14", Standard, FormatStyle::LS_Cpp14);
+  CHECK_PARSE("Standard: Cpp17", Standard, FormatStyle::LS_Cpp17);
+  CHECK_PARSE("Standard: Cpp2a", Standard, FormatStyle::LS_Cpp2a);
   CHECK_PARSE("Standard: C++03", Standard, FormatStyle::LS_Cpp03);
   CHECK_PARSE("Standard: C++11", Standard, FormatStyle::LS_Cpp11);
+  CHECK_PARSE("Standard: C++14", Standard, FormatStyle::LS_Cpp14);
+  CHECK_PARSE("Standard: C++17", Standard, FormatStyle::LS_Cpp17);
+  CHECK_PARSE("Standard: C++2a", Standard, FormatStyle::LS_Cpp2a);
   CHECK_PARSE("Standard: Auto", Standard, FormatStyle::LS_Auto);
 
   Style.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
@@ -13761,6 +13770,19 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
+TEST_F(FormatTest, Coroutines) {
+  FormatStyle Cpp2a = getLLVMStyle();
+  Cpp2a.Standard = FormatStyle::LS_Cpp2a;
+
+  // 'co_yield' is treated as an identifier in standards below C++2a, and so
+  // the increment is interpreted as a postfix on that identifier.
+  // In C++2a, it is interpreted as a prefix increment on 'i'.
+  verifyFormat("co_yield++ i;");
+  verifyFormat("co_yield ++i;", Cpp2a);
+
+  verifyFormat("co_await []() { co_return; }();", Cpp2a);
+}
+
 TEST_F(FormatTest, FileAndCode) {
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.cc", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2860,7 +2860,7 @@
 (Style.Language == FormatStyle::LK_Proto && Left.is(TT_DictLiteral)))
   return !Style.Cpp11BracedListStyle;
 return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-   (Style.Standard < FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+   (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }
   if (Right.isOneOf(tok::arrow, tok::arrowstar, tok::periodstar) ||
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
@@ -2879,7 +2879,7 @@
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
   if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
 return (Left.is(TT_TemplateOpener) &&
-Style.Standard < FormatStyle::LS_Cpp11) ||
+Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
   TT_TemplateOpener)) ||
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -71,6 +71,12 @@
 IO.enumCase(Value, "C++03", 

[PATCH] D65149: [Format] Add test demonstrating PR42722

[Brian Gesiak via Phabricator via cfe-commits]

modocache marked an inline comment as done.
modocache added a comment.

Sure thing! Just to be clear: this test doesn't fail, it passes. My intention 
was to commit this, then commit a patch that improved the indentation behavior, 
which would also include a change to the test that demonstrated the new 
behavior. But, as per your suggestion, I'll wait for the fix before committing 
this. Thanks!




Comment at: clang/unittests/Format/FormatTest.cpp:5184
+   "c++;\n"
+   "d++\n"
+   "  });\n"

This is a passing test that demonstrates that the indentation of `c++` and 
`d++` here is wacky.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65149/new/

https://reviews.llvm.org/D65149



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache marked 2 inline comments as done.
modocache added inline comments.



Comment at: clang/lib/Format/Format.cpp:2373
   LangOpts.CPlusPlus17 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
-  LangOpts.CPlusPlus2a = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
+  LangOpts.CPlusPlus2a = Style.Standard == FormatStyle::LS_Cpp20 ? 1 : 0;
   LangOpts.LineComment = 1;

Quuxplusone wrote:
> Incidentally, these four lines seem like a great place to use `Style.Standard 
> >= FormatStyle::LS_CppWhatever` (with a cast if necessary), unless that's 
> against some style rule.
This has already been updated to do so in D65183.



Comment at: clang/unittests/Format/FormatTest.cpp:13815
+  verifyFormat("co_yield++ i;");
+  verifyFormat("co_yield ++i;", Cpp20);
+

Quuxplusone wrote:
> If you're going to test C++11's behavior here, please use `co_yield - 1;` or 
> something else that might reasonably appear in a C++11 program. `co_yield++ 
> i;` is not valid C++ (unless `i` is a macro, I guess).
`i` would be an iterator in this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache marked 3 inline comments as done.
modocache added a comment.

> It sounds like currently they're very different, and you're proposing to make 
> them basically the same. I think that's a good thing.

I 100% agree with this statement, and thank you @sammccall for the suggestion 
to move in this direction.

> The obvious problem here is that this may well break existing projects that 
> use Cpp11 and C++14/17 features. I think we can punt on this by making 
> Cpp11/14 still enable 14/17 in LangOpts for now (so we don't change behavior, 
> just give people a better way to specify their intent) and change it later if 
> needed.

I like this idea. `LS_Cpp14` and `LS_Cpp17` are new options that we'll 
introduce and so clang-format should interpret them very precisely, whereas 
`LS_Cpp11` (for now) will remain a little ambiguous. Hopefully some thorough 
documentation will prevent some user confusion here.

The codebase I'm applying formatting to uses `-std=c++17` and `-fcoroutines-ts` 
to compile, but specifies `LS_Cpp11` in its .clang-format file -- as you 
mentioned, the intent was to simply use "new" C++ formatting, for whatever 
clang-format decided "new" to mean. I'd like to reason "out loud" about what 
changes we can make to clang-format, and what my codebase's migration path 
would look like should we make those changes. Based on the discussion above I'm 
thinking we make these commits/changes to clang-format itself:

1. Add the `enum LanguageStandard` @sammccall suggested above verbatim, thereby 
allowing users to specify an arbitrary standard such as `LS_Cpp17`. To split up 
the work for easier code review, we might want to consider just updating the 
.clang-format parsing to recognize `"Cpp17"`, but internally still have 
clang-format treat it as `LS_Cpp11`. This is because step (2) is a bit of a 
doozy.
2. Audit the existing uses of `FormatStyle::LS_Cpp{03,11,Auto}` in the 
clang-format codebase and modify them to instead check for the appropriate 
standard. We might want this work to land as several small commits.
  1. For example, formatting that should only be applied to C++17 should be 
modified, and instead of checking for `LS_Cpp11`, it should check for `LS_Cpp17 
|| LS_Cpp11` (`LS_Cpp11` is still included in the check because we're using 
that to mean "new"). Commits with changes like this can land without any impact 
to existing users.
  2. Formatting that should only be applied to C++20 should be modified to 
check for `LS_Cpp20` //instead of// `LS_Cpp11`. These commits //will// change 
end behavior for users. In my codebase, for example, we would probably want to 
update our .clang-format, from `"Cpp11"` to `"Cpp20"`, before we update to a 
clang-format that includes these commits, because otherwise our `co_yield` 
keywords will begin to get formatted incorrectly.
3. Change .clang-format parsing to recognize the new enum cases like `"Cpp17"` 
as `LS_Cpp17`, not `LS_Cpp11` (see item 1).
4. Commit the `CoroutinesTS` option from D65044 
. My codebase would switch from `LS_Cpp20` to 
`LS_Cpp17` and `CoroutinesTS` and almost no formatting should change.

If the above series of patches sounds reasonable, I wouldn't mind working on 
some or all of them, so let me know!




Comment at: clang/docs/ClangFormatStyleOptions.rst:2227
+  * ``LS_Cpp20`` (in configuration: ``Cpp20``)
+Use features of C++20 and C++2a (e.g.: treating ``co_yield`` as a keyword,
+not an identifier, so ``co_yield++ i`` is formatted as ``co_yield ++i``).

Quuxplusone wrote:
> C++2a //will// be C++20, barring any radically unforeseen events. So saying 
> "C++20 and C++2a" is redundant. Personally I would follow GCC/Clang's lead 
> and say "C++2a" until the standard is actually out.
Thank you, I'll do that. I wasn't sure about the naming conventions. While 
we're talking about this, mind if I ask: why did "C++1z" use "z" whereas 
"C++2a" use "a"? Once C++20 is actually out, is the next version "C++2b", or 
does "C++2a" start referring to C++23? Sorry for the rookie questions.



Comment at: clang/include/clang/Format/Format.h:1878
 LS_Cpp11,
+/// Use features of C++20 and C++2a (e.g.: treating ``co_yield`` as a
+/// keyword, not an identifier, so ``co_yield++ i`` is formatted as

Quuxplusone wrote:
> Again, C++2a is likely a synonym for C++20.
> Three lines earlier, you might want to change "C++1z" to "C++17" (and grep 
> the codebase for other instances of "1z").
Cool, will do, thank you!



Comment at: clang/unittests/Format/FormatTest.cpp:3721
+  "aa) <= >\n"
+  "5) {\n"
   "}");

Quuxplusone wrote:
> This doesn't seem to test what you set out to test, does it? `(x) <= > 5` 
> isn't a valid C++ expression anyway. Maybe what you want to test here is that 
> clang-format is willing to reformat pre-C++2a 

[PATCH] D65183: [Format] Make it easy to add new format::FormatStyle::LanguageStandard. NFCI

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

LGTM, but I don't actively work in this codebase so I really can't say. I'll 
wait to hear from some other more active clang-format reviewers.




Comment at: lib/Format/TokenAnnotator.cpp:2862
 return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-   (Style.Standard != FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+   (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }

Just a note: I don't know what the original intent of 
https://github.com/llvm/llvm-project/commit/dd978ae0e11 was, but in D65043 I 
modified this condition to be `Style.Standard == FormatStyle::LS_Cpp03 || 
Style.Standard == FormatStyle::LS_Auto`, because I believe that mirrors the 
current behavior exactly. With this change, a user that specified a standard of 
`FormatStyle::LS_Auto` will experience a change in behavior.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65183/new/

https://reviews.llvm.org/D65183



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

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Friendly ping! I'm wondering, from the clang-format maintainers' point of view, 
whether a diff like this and https://reviews.llvm.org/D65044 make sense to add? 
I do think that it is useful for users to specify whether they wish to use 
C++11 or C++20 constructs, but I'm not an expert.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043



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

[PATCH] D65149: [Format] Add test demonstrating PR42722

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: ank, klimek, acoomans.
Herald added a project: clang.

https://bugs.llvm.org/show_bug.cgi?id=42722 describes what I believe to
be a bug in lambda formatting. If it is indeed a bug, I'd like to commit
this test that reliably reproduces it. I'll try in the coming days to
then fix the behavior and update this test to demonstrate the correct
behavior (but if anyone fixes it before I do, this test will help them
too, I think).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65149

Files:
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5173,6 +5173,17 @@
"   \"\");\n"
"}",
getLLVMStyleWithColumns(30));
+  verifyFormat("testPR42722() {\n"
+   "  int c = 19;\n"
+   "  int d = 32;\n"
+   "  Executor ex;\n"
+   "  ex.addFuture([]() {\n"
+   "  throw std::runtime_error(\"oops\");\n"
+   "}).then([](Result &) {\n"
+   "c++;\n"
+   "d++\n"
+   "  });\n"
+   "}");
 }
 
 TEST_F(FormatTest, BreaksAccordingToOperatorPrecedence) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5173,6 +5173,17 @@
"   \"\");\n"
"}",
getLLVMStyleWithColumns(30));
+  verifyFormat("testPR42722() {\n"
+   "  int c = 19;\n"
+   "  int d = 32;\n"
+   "  Executor ex;\n"
+   "  ex.addFuture([]() {\n"
+   "  throw std::runtime_error(\"oops\");\n"
+   "}).then([](Result &) {\n"
+   "c++;\n"
+   "d++\n"
+   "  });\n"
+   "}");
 }
 
 TEST_F(FormatTest, BreaksAccordingToOperatorPrecedence) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65044: [Format] Add option to enable coroutines keywords

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: rsmith, sammccall, Typz, klimek.
Herald added a subscriber: EricWF.
Herald added a project: clang.

In a previous change, https://reviews.llvm.org/D65043, I allowed users to
explicitly opt-in to formatting according to the C++20 standard. Should
a user choose to do so, the formatter would treat coroutine keywords
such as `co_yield` as keywords, whereas under the C++11 standard they
would be treated as identifiers.

However, this excludes a use case in which, for example, a user's codebase
is written using the C++17 standard, but with an explicit opt-in to
`-fcoroutines-ts`. This commit adds an option to clang-format to allow
formatting under an arbitrary standard, but with `co_yield` and friends
treated as keywords.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65044

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13810,13 +13810,17 @@
 TEST_F(FormatTest, Coroutines) {
   FormatStyle Cpp20 = getLLVMStyle();
   Cpp20.Standard = FormatStyle::LS_Cpp20;
+  FormatStyle Coro = getLLVMStyle();
+  Coro.CoroutinesTS = true;
 
   verifyFormat("co_yield++ i;");
   verifyFormat("co_yield ++i;", Cpp20);
+  verifyFormat("co_yield ++i;", Coro);
 
   verifyFormat("co_await[]() { co_return; }\n"
"();");
   verifyFormat("co_await []() { co_return; }();", Cpp20);
+  verifyFormat("co_await []() { co_return; }();", Coro);
 }
 
 } // end namespace
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -434,6 +434,7 @@
 IO.mapOptional("ConstructorInitializerIndentWidth",
Style.ConstructorInitializerIndentWidth);
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
+IO.mapOptional("CoroutinesTS", Style.CoroutinesTS);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
@@ -701,6 +702,7 @@
   LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.ConstructorInitializerIndentWidth = 4;
   LLVMStyle.ContinuationIndentWidth = 4;
+  LLVMStyle.CoroutinesTS = false;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.ExperimentalAutoDetectBinPacking = false;
@@ -2378,6 +2380,8 @@
   LangOpts.ObjC = 1;
   LangOpts.MicrosoftExt = 1;// To get kw___try, kw___finally.
   LangOpts.DeclSpecKeyword = 1; // To get __declspec.
+  if (!LangOpts.CPlusPlus2a)
+LangOpts.Coroutines = Style.CoroutinesTS;
   return LangOpts;
 }
 
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -,6 +,21 @@
   /// \endcode
   unsigned ContinuationIndentWidth;
 
+  /// If ``true``, C++ Coroutines TS keywords will be treated as keywords, not
+  /// identifiers, regardless of the language standard specified for formatting.
+  /// If ``false``, their formatting depends on the standard selected.
+  /// \code
+  ///true, C++11:
+  ///co_yield ++i;
+  ///
+  ///false, C++11:
+  ///co_yield++ i;
+  ///
+  ///both true and false, C++20:
+  ///co_yield ++i;
+  /// \endcode
+  bool CoroutinesTS;
+
   /// If ``true``, format braced lists as best suited for C++11 braced
   /// lists.
   ///
@@ -1949,6 +1964,7 @@
ConstructorInitializerIndentWidth ==
R.ConstructorInitializerIndentWidth &&
ContinuationIndentWidth == R.ContinuationIndentWidth &&
+   CoroutinesTS == R.CoroutinesTS &&
Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
DerivePointerAlignment == R.DerivePointerAlignment &&
DisableFormat == R.DisableFormat &&
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -1315,6 +1315,22 @@
longFunction( // Again a long comment
  arg);
 
+**CoroutinesTS** (``bool``)
+  If ``true``, C++ Coroutines TS keywords will be treated as keywords, not
+  identifiers, regardless of the language standard specified for formatting.
+  If ``false``, their formatting depends on the standard selected.
+
+  .. code-block:: c++
+
+ true, C++11:
+ co_yield ++i;
+
+ false, C++11:
+ co_yield++ i;
+
+ both true and false, C++20:
+ co_yield ++i;
+
 

[PATCH] D65043: [Format] Add C++20 standard to style options

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: rsmith, sammccall, Typz, klimek.
Herald added a project: clang.

When C++ coroutines were adopted as part of the C++20 standard, a change
was committed in https://github.com/llvm/llvm-project/commit/10ab78e854f:
coroutine keywords such as `co_yield` went from being gated on the
presence of the `-fcoroutines-ts` flag, to instead being gated on
`-std=c++2a`. This resulted, perhaps unexpectedly to some users, in a
change in how coroutine keywords were formatted. Because libclangFormat
has only 3 options for formatting according to a language standard --
C++03, C++11, or "auto" -- and because it enabled C++20 keywords for all
settings aside from C++03, users who specified a standard of C++11 in
their style options would have their C++ formatted as if `co_yield` were
a keyword:

- Before, C++03: `co_yield ++i` would be formatted as `co_yield++ i`
- Before, C++11: `co_yield ++i` would be formatted as `co_yield++ i`
- After, C++03: `co_yield ++i` would be formatted as `co_yield++ i`
- After, C++11: `co_yield ++i` would be formatted as `co_yield ++i`

Although the "after" examples above appear like correct formatting
choices to those who are used to seeing coroutine keywords, I would
argue that they aren't technically correct, because a user may define a
variable in C++11 named `co_yield`, and they could increment that
variable by typing `co_yield++`. In this case, clang-format would change
the formatting, despite the user never opting-in to treating `co_yield`
as a keyword.

(There are other examples of clang-format suddenly formatting C++11 code
according to C++20 standards differently as a result of changes like
https://github.com/llvm/llvm-project/commit/10ab78e854f, and I've included
them as tests in this commit, but I won't go into detail explaining them
here.)

To give users the option of formatting according to the C++11 standard,
without the use of coroutines, I've added a style option for the C++20
standard (and, similarly to how the C++11 standard enables C++14, 17,
and 1z, I've written the documentation to indicate using the C++20
option enables any future C++2a standards).

In a future commit, I add a boolean style option to enable coroutines,
so that users may specify they wish to format according to the C++11
standard, but with coroutine keywords enabled.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65043

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -3713,10 +3713,20 @@
   "if (aa.aa(\n"
   "aa) == 5) {\n"
   "}");
+
+  // Only in C++20 and above is <=> treated as as operator.
   verifyFormat(
   "if (a(\n"
-  "aa) <=> 5) {\n"
+  "aa) <= >\n"
+  "5) {\n"
   "}");
+  FormatStyle Cpp20 = getLLVMStyle();
+  Cpp20.Standard = FormatStyle::LS_Cpp20;
+  verifyFormat(
+  "if (a(\n"
+  "aa) <=> 5) {\n"
+  "}", Cpp20);
+
   // Even explicit parentheses stress the precedence enough to make the
   // additional break unnecessary.
   verifyFormat("if ((a +\n"
@@ -3736,10 +3746,15 @@
"aa ==\n"
"5) {\n"
"}");
+  // Only in C++20 and above is <=> treated as as operator.
+  verifyFormat("if (a +\n"
+   "aa <=\n"
+   "> 5) {\n"
+   "}");
   verifyFormat("if (a +\n"
"aa <=>\n"
"5) {\n"
-   "}");
+   "}", Cpp20);
 
   FormatStyle OnePerLine = getLLVMStyle();
   OnePerLine.BinPackParameters = false;
@@ -13792,6 +13807,18 @@
   verifyFormat("STACK_OF(int*)* a;", Macros);
 }
 
+TEST_F(FormatTest, Coroutines) {
+  FormatStyle Cpp20 = getLLVMStyle();
+  Cpp20.Standard = FormatStyle::LS_Cpp20;
+
+  verifyFormat("co_yield++ i;");
+  verifyFormat("co_yield ++i;", Cpp20);
+
+  verifyFormat("co_await[]() { co_return; }\n"
+   "();");
+  verifyFormat("co_await []() { co_return; }();", Cpp20);
+}
+
 } // end namespace
 } // end namespace format
 } // end 

[PATCH] D62550: [coroutines][PR41909] Don't build dependent coroutine statements for generic lambda

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: cfe/trunk/lib/Sema/TreeTransform.h:7173
+auto *MD = dyn_cast_or_null(FD);
+if (!MD || !MD->getParent()->isGenericLambda()) {
+  assert(!Promise->getType()->isDependentType() &&

rsmith wrote:
> This assert doesn't seem correct to me; there's no reason to assume we have a 
> generic lambda here. (A non-generic lambda inside a generic lambda, whose 
> parameter types are dependent on the generic lambda's parameters, would hit 
> exactly the same issue, for instance.)
> 
> Generally we should write the template instantiation code so it could be used 
> to substitute into the definition of a function template to produce another 
> function template, even if we happen to never call it that way right now 
> (though deduction guide processing gets pretty close). The right thing to do 
> here is to either substitute into the representation we already formed 
> (building a dependent representation if it's still dependent and a 
> non-dependent representation otherwise) or to rebuild the coroutine body from 
> scratch (again, creating a dependent representation if the result is still 
> dependent).
Sorry I missed this! Indeed you're right, the check here is weak, and even with 
this patch the following coroutines code crashes in the same way: 
https://godbolt.org/z/ixLpCq

I'll send a follow-up patch that attempts to implement one of the approaches 
you describe. Thanks for the guidance!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62550/new/

https://reviews.llvm.org/D62550



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

[PATCH] D62550: [coroutines][PR41909] Don't build dependent coroutine statements for generic lambda

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Great, thanks for the review!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62550/new/

https://reviews.llvm.org/D62550



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

[PATCH] D62550: [coroutines][PR41909] Don't build dependent coroutine statements for generic lambda

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362348: [coroutines][PR41909] Dont build dependent 
coroutine statements for generic… (authored by modocache, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62550/new/

https://reviews.llvm.org/D62550

Files:
  cfe/trunk/lib/Sema/TreeTransform.h
  cfe/trunk/test/SemaCXX/coroutines.cpp


Index: cfe/trunk/test/SemaCXX/coroutines.cpp
===
--- cfe/trunk/test/SemaCXX/coroutines.cpp
+++ cfe/trunk/test/SemaCXX/coroutines.cpp
@@ -720,6 +720,16 @@
   co_await 42;
 }
 
+template void ok_generic_lambda_coawait_PR41909() {
+  [](auto& arg) -> coro { // expected-warning {{expression 
result unused}}
+co_await 12;
+  };
+  [](auto ) -> coro {
+co_await 24;
+  }("argument");
+}
+template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in 
instantiation of function template specialization 
'ok_generic_lambda_coawait_PR41909' requested here}}
+
 template<> struct std::experimental::coroutine_traits
 { using promise_type = promise; };
 
Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -7163,13 +7163,22 @@
   Builder.ReturnValue = Res.get();
 
   if (S->hasDependentPromiseType()) {
-assert(!Promise->getType()->isDependentType() &&
-   "the promise type must no longer be dependent");
-assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
-   !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
-   "these nodes should not have been built yet");
-if (!Builder.buildDependentStatements())
-  return StmtError();
+// PR41909: We may find a generic coroutine lambda definition within a
+// template function that is being instantiated. In this case, the lambda
+// will have a dependent promise type, until it is used in an expression
+// that creates an instantiation with a non-dependent promise type. We
+// should not assert or build coroutine dependent statements for such a
+// generic lambda.
+auto *MD = dyn_cast_or_null(FD);
+if (!MD || !MD->getParent()->isGenericLambda()) {
+  assert(!Promise->getType()->isDependentType() &&
+ "the promise type must no longer be dependent");
+  assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
+ !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
+ "these nodes should not have been built yet");
+  if (!Builder.buildDependentStatements())
+return StmtError();
+}
   } else {
 if (auto *OnFallthrough = S->getFallthroughHandler()) {
   StmtResult Res = getDerived().TransformStmt(OnFallthrough);


Index: cfe/trunk/test/SemaCXX/coroutines.cpp
===
--- cfe/trunk/test/SemaCXX/coroutines.cpp
+++ cfe/trunk/test/SemaCXX/coroutines.cpp
@@ -720,6 +720,16 @@
   co_await 42;
 }
 
+template void ok_generic_lambda_coawait_PR41909() {
+  [](auto& arg) -> coro { // expected-warning {{expression result unused}}
+co_await 12;
+  };
+  [](auto ) -> coro {
+co_await 24;
+  }("argument");
+}
+template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in instantiation of function template specialization 'ok_generic_lambda_coawait_PR41909' requested here}}
+
 template<> struct std::experimental::coroutine_traits
 { using promise_type = promise; };
 
Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -7163,13 +7163,22 @@
   Builder.ReturnValue = Res.get();
 
   if (S->hasDependentPromiseType()) {
-assert(!Promise->getType()->isDependentType() &&
-   "the promise type must no longer be dependent");
-assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
-   !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
-   "these nodes should not have been built yet");
-if (!Builder.buildDependentStatements())
-  return StmtError();
+// PR41909: We may find a generic coroutine lambda definition within a
+// template function that is being instantiated. In this case, the lambda
+// will have a dependent promise type, until it is used in an expression
+// that creates an instantiation with a non-dependent promise type. We
+// should not assert or build coroutine dependent statements for such a
+// generic lambda.
+auto *MD = dyn_cast_or_null(FD);
+if (!MD || !MD->getParent()->isGenericLambda()) {
+  assert(!Promise->getType()->isDependentType() &&
+ "the promise type must no longer be dependent");
+ 

[PATCH] D62550: [coroutines][PR41909] Don't build dependent coroutine statements for generic lambda

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, EricWF, lewissbaker, tks2103.
Herald added a project: clang.

https://bugs.llvm.org/show_bug.cgi?id=41909 describes an issue in which
a generic lambda that takes a dependent argument `auto set` causes the
template instantiation machinery for coroutine body statements to crash
with an ICE. The issue is two-fold:

1. The paths taken by the template instantiator contain several asserts that 
the coroutine promise must not have a dependent type.
2. The template instantiator unconditionally builds corotuine statements that 
depend on the promise type, which cannot be dependent.

To work around the issue, prevent the template instantiator from building
dependent coroutine statements if the coroutine promise type is dependent.
Since we only expect this to occur in the case of a generic lambda, limit
the workaround behavior to just that case.


Repository:
  rC Clang

https://reviews.llvm.org/D62550

Files:
  lib/Sema/TreeTransform.h
  test/SemaCXX/coroutines.cpp


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -720,6 +720,16 @@
   co_await 42;
 }
 
+template void ok_generic_lambda_coawait_PR41909() {
+  [](auto& arg) -> coro { // expected-warning {{expression 
result unused}}
+co_await 12;
+  };
+  [](auto ) -> coro {
+co_await 24;
+  }("argument");
+}
+template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in 
instantiation of function template specialization 
'ok_generic_lambda_coawait_PR41909' requested here}}
+
 template<> struct std::experimental::coroutine_traits
 { using promise_type = promise; };
 
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -7155,13 +7155,22 @@
   Builder.ReturnValue = Res.get();
 
   if (S->hasDependentPromiseType()) {
-assert(!Promise->getType()->isDependentType() &&
-   "the promise type must no longer be dependent");
-assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
-   !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
-   "these nodes should not have been built yet");
-if (!Builder.buildDependentStatements())
-  return StmtError();
+// PR41909: We may find a generic coroutine lambda definition within a
+// template function that is being instantiated. In this case, the lambda
+// will have a dependent promise type, until it is used in an expression
+// that creates an instantiation with a non-dependent promise type. We
+// should not assert or build coroutine dependent statements for such a
+// generic lambda.
+auto *MD = dyn_cast_or_null(FD);
+if (!MD || !MD->getParent()->isGenericLambda()) {
+  assert(!Promise->getType()->isDependentType() &&
+ "the promise type must no longer be dependent");
+  assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
+ !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
+ "these nodes should not have been built yet");
+  if (!Builder.buildDependentStatements())
+return StmtError();
+}
   } else {
 if (auto *OnFallthrough = S->getFallthroughHandler()) {
   StmtResult Res = getDerived().TransformStmt(OnFallthrough);


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -720,6 +720,16 @@
   co_await 42;
 }
 
+template void ok_generic_lambda_coawait_PR41909() {
+  [](auto& arg) -> coro { // expected-warning {{expression result unused}}
+co_await 12;
+  };
+  [](auto ) -> coro {
+co_await 24;
+  }("argument");
+}
+template void ok_generic_lambda_coawait_PR41909(); // expected-note {{in instantiation of function template specialization 'ok_generic_lambda_coawait_PR41909' requested here}}
+
 template<> struct std::experimental::coroutine_traits
 { using promise_type = promise; };
 
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -7155,13 +7155,22 @@
   Builder.ReturnValue = Res.get();
 
   if (S->hasDependentPromiseType()) {
-assert(!Promise->getType()->isDependentType() &&
-   "the promise type must no longer be dependent");
-assert(!S->getFallthroughHandler() && !S->getExceptionHandler() &&
-   !S->getReturnStmtOnAllocFailure() && !S->getDeallocate() &&
-   "these nodes should not have been built yet");
-if (!Builder.buildDependentStatements())
-  return StmtError();
+// PR41909: We may find a generic coroutine lambda definition within a
+// template function that is being instantiated. In this case, the lambda
+// will have a dependent promise type, 

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

@rsmith, what do you think of the patch as-is?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58920/new/

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 200518.
modocache added a comment.

Hmm... alright, I'm not really sure how I could implement a test that fails 
without this, but I added a check in the FindExistingResult destructor.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58920/new/

https://reviews.llvm.org/D58920

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
  test/Modules/Inputs/mod-virtual-destructor-bug/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
  test/Modules/mod-virtual-destructor-bug-two.cpp
  test/Modules/mod-virtual-destructor-bug.cpp

Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -47,6 +47,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/IdentifierResolver.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
@@ -3252,6 +3253,9 @@
 // Add the declaration to its redeclaration context so later merging
 // lookups will find it.
 MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true);
+if (isa(New) && Name.getAsString() == "std")
+  if (!Reader.getSema()->StdNamespace)
+Reader.getSema()->StdNamespace = New;
   }
 }
 
@@ -3415,6 +3419,14 @@
   return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
 TypedefNameForLinkage);
 }
+if (isa(D) && D->getName() == "std") {
+  auto StdPtr = Reader.getSema()->StdNamespace;
+  if (StdPtr.isValid() && !StdPtr.isOffset())
+if (auto *Std = cast_or_null(StdPtr.get(nullptr)))
+  if (isSameEntity(Std, D))
+return FindExistingResult(Reader, D, Std, AnonymousDeclNumber,
+  TypedefNameForLinkage);
+}
   } else {
 // Not in a mergeable context.
 return FindExistingResult(Reader);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62035: [AST] const-ify ObjC inherited class search

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added a reviewer: rjmccall.
Herald added a project: clang.

When writing an AST matcher to find inherited Objective-C classes, I
noticed the returned `ObjCInterfaceDecl*` was mutable. It doesn't seem
like it needs to be mutable, so this patch makes it const.

Patch by Adam Ernst.


Repository:
  rC Clang

https://reviews.llvm.org/D62035

Files:
  include/clang/AST/DeclObjC.h
  lib/AST/DeclObjC.cpp
  lib/Sema/SemaDeclObjC.cpp


Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -2685,7 +2685,7 @@
   assert (IDecl && "CheckProtocolMethodDefs - IDecl is null");
 
   ObjCInterfaceDecl *Super = IDecl->getSuperClass();
-  ObjCInterfaceDecl *NSIDecl = nullptr;
+  const ObjCInterfaceDecl *NSIDecl = nullptr;
 
   // If this protocol is marked 
'objc_protocol_requires_explicit_implementation'
   // then we should check if any class in the super class hierarchy also
Index: lib/AST/DeclObjC.cpp
===
--- lib/AST/DeclObjC.cpp
+++ lib/AST/DeclObjC.cpp
@@ -646,10 +646,10 @@
 }
 
 /// lookupInheritedClass - This method returns ObjCInterfaceDecl * of the super
-/// class whose name is passed as argument. If it is not one of the super 
classes
-/// the it returns NULL.
-ObjCInterfaceDecl *ObjCInterfaceDecl::lookupInheritedClass(
-const IdentifierInfo*ICName) {
+/// class whose name is passed as argument. If it is not one of the super
+/// classes then it returns NULL.
+const ObjCInterfaceDecl *
+ObjCInterfaceDecl::lookupInheritedClass(const IdentifierInfo *ICName) const {
   // FIXME: Should make sure no callers ever do this.
   if (!hasDefinition())
 return nullptr;
@@ -657,7 +657,7 @@
   if (data().ExternallyCompleted)
 LoadExternalDefinition();
 
-  ObjCInterfaceDecl* ClassDecl = this;
+  auto *ClassDecl = this;
   while (ClassDecl != nullptr) {
 if (ClassDecl->getIdentifier() == ICName)
   return ClassDecl;
Index: include/clang/AST/DeclObjC.h
===
--- include/clang/AST/DeclObjC.h
+++ include/clang/AST/DeclObjC.h
@@ -1852,7 +1852,8 @@
 return lookupMethod(Sel, false/*isInstance*/);
   }
 
-  ObjCInterfaceDecl *lookupInheritedClass(const IdentifierInfo *ICName);
+  const ObjCInterfaceDecl *
+  lookupInheritedClass(const IdentifierInfo *ICName) const;
 
   /// Lookup a method in the classes implementation hierarchy.
   ObjCMethodDecl *lookupPrivateMethod(const Selector ,


Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -2685,7 +2685,7 @@
   assert (IDecl && "CheckProtocolMethodDefs - IDecl is null");
 
   ObjCInterfaceDecl *Super = IDecl->getSuperClass();
-  ObjCInterfaceDecl *NSIDecl = nullptr;
+  const ObjCInterfaceDecl *NSIDecl = nullptr;
 
   // If this protocol is marked 'objc_protocol_requires_explicit_implementation'
   // then we should check if any class in the super class hierarchy also
Index: lib/AST/DeclObjC.cpp
===
--- lib/AST/DeclObjC.cpp
+++ lib/AST/DeclObjC.cpp
@@ -646,10 +646,10 @@
 }
 
 /// lookupInheritedClass - This method returns ObjCInterfaceDecl * of the super
-/// class whose name is passed as argument. If it is not one of the super classes
-/// the it returns NULL.
-ObjCInterfaceDecl *ObjCInterfaceDecl::lookupInheritedClass(
-const IdentifierInfo*ICName) {
+/// class whose name is passed as argument. If it is not one of the super
+/// classes then it returns NULL.
+const ObjCInterfaceDecl *
+ObjCInterfaceDecl::lookupInheritedClass(const IdentifierInfo *ICName) const {
   // FIXME: Should make sure no callers ever do this.
   if (!hasDefinition())
 return nullptr;
@@ -657,7 +657,7 @@
   if (data().ExternallyCompleted)
 LoadExternalDefinition();
 
-  ObjCInterfaceDecl* ClassDecl = this;
+  auto *ClassDecl = this;
   while (ClassDecl != nullptr) {
 if (ClassDecl->getIdentifier() == ICName)
   return ClassDecl;
Index: include/clang/AST/DeclObjC.h
===
--- include/clang/AST/DeclObjC.h
+++ include/clang/AST/DeclObjC.h
@@ -1852,7 +1852,8 @@
 return lookupMethod(Sel, false/*isInstance*/);
   }
 
-  ObjCInterfaceDecl *lookupInheritedClass(const IdentifierInfo *ICName);
+  const ObjCInterfaceDecl *
+  lookupInheritedClass(const IdentifierInfo *ICName) const;
 
   /// Lookup a method in the classes implementation hierarchy.
   ObjCMethodDecl *lookupPrivateMethod(const Selector ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 199709.
modocache added a comment.

Oops, sent the patch from the wrong repository.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58920/new/

https://reviews.llvm.org/D58920

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
  test/Modules/Inputs/mod-virtual-destructor-bug/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
  test/Modules/mod-virtual-destructor-bug-two.cpp
  test/Modules/mod-virtual-destructor-bug.cpp


Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -47,6 +47,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/IdentifierResolver.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
@@ -3401,6 +3402,14 @@
   return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
 TypedefNameForLinkage);
 }
+if (isa(D) && D->getName() == "std") {
+  auto StdPtr = Reader.getSema()->StdNamespace;
+  if (StdPtr.isValid() && !StdPtr.isOffset())
+if (auto *Std = cast_or_null(StdPtr.get(nullptr)))
+  if (isSameEntity(Std, D))
+return FindExistingResult(Reader, D, Std, AnonymousDeclNumber,
+  TypedefNameForLinkage);
+}
   } else {
 // Not in a mergeable context.
 return FindExistingResult(Reader);


Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 199708.
modocache added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a 
little while to figure out why, even using the `LazyDeclPtr` directly, I was 
still triggering deserialization. It turns out `dump()` causes deserialization 
too -- whoops!)

> You should also change `FindExistingResult::~FindExistingResult` to update 
> `Sema::StdNamespace` to point to your newly-deserialized namespace if you 
> didn't find a prior declaration of it, so that `Sema::getStdNamespace()` 
> finds the deserialized namespace.

I haven't done this yet. I'm trying to think of a test case that would fail if 
this were not done properly -- or would there not be one?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58920/new/

https://reviews.llvm.org/D58920

Files:
  lib/Target/X86/X86ISelLowering.cpp
  test/CodeGen/X86/vector-reduce-mul-widen.ll
  test/CodeGen/X86/vector-reduce-mul.ll

Index: test/CodeGen/X86/vector-reduce-mul.ll
===
--- test/CodeGen/X86/vector-reduce-mul.ll
+++ test/CodeGen/X86/vector-reduce-mul.ll
@@ -2465,36 +2465,37 @@
 ; AVX512BW-NEXT:vpandq %zmm3, %zmm0, %zmm0
 ; AVX512BW-NEXT:vpackuswb %zmm2, %zmm0, %zmm0
 ; AVX512BW-NEXT:vextracti128 $1, %ymm0, %xmm1
-; AVX512BW-NEXT:vpunpckhbw {{.*#+}} zmm2 = zmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15,24,24,25,25,26,26,27,27,28,28,29,29,30,30,31,31,40,40,41,41,42,42,43,43,44,44,45,45,46,46,47,47,56,56,57,57,58,58,59,59,60,60,61,61,62,62,63,63]
-; AVX512BW-NEXT:vpunpckhbw {{.*#+}} zmm4 = zmm1[8],zmm0[8],zmm1[9],zmm0[9],zmm1[10],zmm0[10],zmm1[11],zmm0[11],zmm1[12],zmm0[12],zmm1[13],zmm0[13],zmm1[14],zmm0[14],zmm1[15],zmm0[15],zmm1[24],zmm0[24],zmm1[25],zmm0[25],zmm1[26],zmm0[26],zmm1[27],zmm0[27],zmm1[28],zmm0[28],zmm1[29],zmm0[29],zmm1[30],zmm0[30],zmm1[31],zmm0[31],zmm1[40],zmm0[40],zmm1[41],zmm0[41],zmm1[42],zmm0[42],zmm1[43],zmm0[43],zmm1[44],zmm0[44],zmm1[45],zmm0[45],zmm1[46],zmm0[46],zmm1[47],zmm0[47],zmm1[56],zmm0[56],zmm1[57],zmm0[57],zmm1[58],zmm0[58],zmm1[59],zmm0[59],zmm1[60],zmm0[60],zmm1[61],zmm0[61],zmm1[62],zmm0[62],zmm1[63],zmm0[63]
-; AVX512BW-NEXT:vpmullw %zmm4, %zmm2, %zmm2
-; AVX512BW-NEXT:vpandq %zmm3, %zmm2, %zmm2
-; AVX512BW-NEXT:vpunpcklbw {{.*#+}} zmm0 = zmm0[0,0,1,1,2,2,3,3,4,4,5,5,6,6,7,7,16,16,17,17,18,18,19,19,20,20,21,21,22,22,23,23,32,32,33,33,34,34,35,35,36,36,37,37,38,38,39,39,48,48,49,49,50,50,51,51,52,52,53,53,54,54,55,55]
-; AVX512BW-NEXT:vpunpcklbw {{.*#+}} zmm1 = zmm1[0],zmm0[0],zmm1[1],zmm0[1],zmm1[2],zmm0[2],zmm1[3],zmm0[3],zmm1[4],zmm0[4],zmm1[5],zmm0[5],zmm1[6],zmm0[6],zmm1[7],zmm0[7],zmm1[16],zmm0[16],zmm1[17],zmm0[17],zmm1[18],zmm0[18],zmm1[19],zmm0[19],zmm1[20],zmm0[20],zmm1[21],zmm0[21],zmm1[22],zmm0[22],zmm1[23],zmm0[23],zmm1[32],zmm0[32],zmm1[33],zmm0[33],zmm1[34],zmm0[34],zmm1[35],zmm0[35],zmm1[36],zmm0[36],zmm1[37],zmm0[37],zmm1[38],zmm0[38],zmm1[39],zmm0[39],zmm1[48],zmm0[48],zmm1[49],zmm0[49],zmm1[50],zmm0[50],zmm1[51],zmm0[51],zmm1[52],zmm0[52],zmm1[53],zmm0[53],zmm1[54],zmm0[54],zmm1[55],zmm0[55]
-; AVX512BW-NEXT:vpmullw %zmm1, %zmm0, %zmm0
-; AVX512BW-NEXT:vpandq %zmm3, %zmm0, %zmm0
-; AVX512BW-NEXT:vpackuswb %zmm2, %zmm0, %zmm0
-; AVX512BW-NEXT:vpunpckhbw {{.*#+}} zmm1 = zmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15,24,24,25,25,26,26,27,27,28,28,29,29,30,30,31,31,40,40,41,41,42,42,43,43,44,44,45,45,46,46,47,47,56,56,57,57,58,58,59,59,60,60,61,61,62,62,63,63]
-; AVX512BW-NEXT:vpunpcklbw {{.*#+}} zmm0 = zmm0[0,0,1,1,2,2,3,3,4,4,5,5,6,6,7,7,16,16,17,17,18,18,19,19,20,20,21,21,22,22,23,23,32,32,33,33,34,34,35,35,36,36,37,37,38,38,39,39,48,48,49,49,50,50,51,51,52,52,53,53,54,54,55,55]
-; AVX512BW-NEXT:vpmullw %zmm1, %zmm0, %zmm0
-; AVX512BW-NEXT:vpandq %zmm3, %zmm0, %zmm0
+; AVX512BW-NEXT:vpunpckhbw {{.*#+}} xmm2 = xmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15]
+; AVX512BW-NEXT:vpunpckhbw {{.*#+}} xmm3 = xmm1[8],xmm0[8],xmm1[9],xmm0[9],xmm1[10],xmm0[10],xmm1[11],xmm0[11],xmm1[12],xmm0[12],xmm1[13],xmm0[13],xmm1[14],xmm0[14],xmm1[15],xmm0[15]
+; AVX512BW-NEXT:vpmullw %xmm3, %xmm2, %xmm2
+; AVX512BW-NEXT:vmovdqa {{.*#+}} xmm3 = [255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255]
+; AVX512BW-NEXT:vpand %xmm3, %xmm2, %xmm2
+; AVX512BW-NEXT:vpmovzxbw {{.*#+}} xmm0 = xmm0[0],zero,xmm0[1],zero,xmm0[2],zero,xmm0[3],zero,xmm0[4],zero,xmm0[5],zero,xmm0[6],zero,xmm0[7],zero
+; AVX512BW-NEXT:vpmovzxbw {{.*#+}} xmm1 = xmm1[0],zero,xmm1[1],zero,xmm1[2],zero,xmm1[3],zero,xmm1[4],zero,xmm1[5],zero,xmm1[6],zero,xmm1[7],zero
+; AVX512BW-NEXT:vpmullw %xmm1, %xmm0, %xmm0
+; AVX512BW-NEXT:vpand %xmm3, %xmm0, %xmm0
+; AVX512BW-NEXT:vpackuswb %xmm2, %xmm0, %xmm0
+; AVX512BW-NEXT:vpunpckhbw {{.*#+}} xmm1 = 

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thanks @rsmith for the guidance here! I appreciate it very much. One snag I ran 
into after following your suggestion, though, is that when I modify 
`ASTDeclReader::findExisting` to return Sema's existing implicit std namespace, 
I run into an assertion later on, when the decl chain is being marked as 
incomplete. That is, the following patch (debug output included):

  diff --git a/lib/Serialization/ASTReaderDecl.cpp 
b/lib/Serialization/ASTReaderDecl.cpp
  index 32bd82d077..9d447952e1 100644
  --- a/lib/Serialization/ASTReaderDecl.cpp
  +++ b/lib/Serialization/ASTReaderDecl.cpp
  @@ -47,6 +47,7 @@
   #include "clang/Basic/SourceLocation.h"
   #include "clang/Basic/Specifiers.h"
   #include "clang/Sema/IdentifierResolver.h"
  +#include "clang/Sema/Sema.h"
   #include "clang/Serialization/ASTBitCodes.h"
   #include "clang/Serialization/ASTReader.h"
   #include "clang/Serialization/ContinuousRangeMap.h"
  @@ -3401,6 +3402,22 @@ ASTDeclReader::FindExistingResult 
ASTDeclReader::findExisting(NamedDecl *D) {
 return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
   TypedefNameForLinkage);
   }
  +if (isa(D) && D->getName() == "std") {
  +  llvm::outs() << "Found std namespace: ";
  +  D->dump();
  +  llvm::outs() << "Merging into Sema std namespace: ";
  +  Sema *S = Reader.getSema();
  +  S->getStdNamespace()->dump();
  +  NamedDecl *Existing =
  +  getDeclForMerging(S->getStdNamespace(), TypedefNameForLinkage);
  +  llvm::outs() << "Found Existing: ";
  +  Existing->dump();
  +  if (isSameEntity(Existing, D)) {
  +llvm::outs() << "isSameEntity is true\n";
  +return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
  +  TypedefNameForLinkage);
  +  }
  +}
 } else {
   // Not in a mergeable context.
   return FindExistingResult(Reader);

Results in the expected output, along with an unexpected assert:

  Found std namespace: NamespaceDecl 0x55a391530910 
 col:11 imported in a.h std
  Merging into Sema std namespace: NamespaceDecl 0x55a391501ec8 <>  implicit std
  Found Existing: NamespaceDecl 0x55a391501ec8 <>  
implicit std
  isSameEntity is true
  clang-9: ../include/llvm/ADT/PointerUnion.h:135: T llvm::PointerUnion::get() const [with T = clang::LazyGenerationalUpdatePtr; 
PT1 = llvm::PointerUnion; PT2 = 
clang::LazyGenerationalUpdatePtr]: Assertion `is() && 
"Invalid accessor called"' failed.
  Stack dump:
  0.  Program arguments: 
/home/modocache/Source/llvm/git/dev/llvm/build/bin/clang-9 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-file-name 
mod-virtual-destructor-bug.cpp -mrelocation-model static -mthread-model posix 
-mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases 
-munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info 
-debugger-tuning=gdb -resource-dir 
/home/modocache/Source/llvm/git/dev/llvm/build/lib/clang/9.0.0 -I 
mod-virtual-destructor-bug -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/backward 
-internal-isystem /usr/local/include -internal-isystem 
/home/modocache/Source/llvm/git/dev/llvm/build/lib/clang/9.0.0/include 
-internal-externc-isystem /usr/include/x86_64-linux-gnu 
-internal-externc-isystem /include -internal-externc-isystem /usr/include 
-std=c++17 -fdeprecated-macro -fdebug-compilation-dir 
/home/modocache/Source/tmp/mod -ferror-limit 19 -fmessage-length 195 -fmodules 
-fimplicit-module-maps -fmodules-cache-path=mod-virtual-destructor-cache 
-fmodules-validate-system-headers -fobjc-runtime=gcc -fcxx-exceptions 
-fexceptions -fdiagnostics-show-option -fcolor-diagnostics -o 
/tmp/mod-virtual-destructor-bug-0da92f.o -x c++ mod-virtual-destructor-bug.cpp 
-faddrsig
  1.  mod-virtual-destructor-bug.cpp:10:17: current parser token 'class'
   #0 0x55a388bfaaa5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Unix/Signals.inc:494:22
   #1 0x55a388bfab38 PrintStackTraceSignalHandler(void*) 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Unix/Signals.inc:558:1
   #2 0x55a388bf8b32 llvm::sys::RunSignalHandlers() 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Signals.cpp:68:20
   #3 0x55a388bfa4fb SignalHandler(int) 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Unix/Signals.inc:357:1
   #4 0x7f909ab86dd0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12dd0)
   #5 0x7f909a249077 raise 
/build/glibc-B9XfQf/glibc-2.28/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
   #6 

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Brian Gesiak via Phabricator via cfe-commits]

modocache closed this revision.
modocache added a comment.

Committed in rL357010 . Apologies, I forgot 
to include the differential revision in the commit message so this diff wasn't 
closed automatically as a result. I'll comment on rL357010 
 with the missing information.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140



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

[PATCH] D59765: [Lex] Warn about invisible Hangul whitespace

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 192092.
modocache added a comment.

Remove unneeded change to test identifier 'xx'.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59765/new/

https://reviews.llvm.org/D59765

Files:
  lib/Lex/Lexer.cpp
  test/Lexer/unicode.c


Index: test/Lexer/unicode.c
===
--- test/Lexer/unicode.c
+++ test/Lexer/unicode.c
@@ -39,10 +39,12 @@
  // expected-warning@-1 {{treating Unicode character  
as identifier character rather than as ';' symbol}}
 int v＝［＝］（auto）｛return～x；｝（）; // expected-warning 12{{treating Unicode 
character}}
 
-int ⁠xx‍;
+int ⁠xx‍ㅤﾠ;
 // expected-warning@-1 {{identifier contains Unicode character  that 
is invisible in some environments}}
 // expected-warning@-2 {{identifier contains Unicode character  that 
is invisible in some environments}}
 // expected-warning@-3 {{identifier contains Unicode character  that 
is invisible in some environments}}
+// expected-warning@-4 {{identifier contains Unicode character  that 
is invisible in some environments}}
+// expected-warning@-5 {{identifier contains Unicode character  that 
is invisible in some environments}}
 int foo​bar = 0; // expected-warning {{identifier contains Unicode character 
 that is invisible in some environments}}
 int x = foobar; // expected-error {{undeclared identifier}}
 
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -1528,6 +1528,7 @@
 {U'\u2227', '^'}, // LOGICAL AND
 {U'\u2236', ':'}, // RATIO
 {U'\u223c', '~'}, // TILDE OPERATOR
+{U'\u3164', 0},   // HANGUL FILLER
 {U'\ua789', ':'}, // MODIFIER LETTER COLON
 {U'\ufeff', 0},   // ZERO WIDTH NO-BREAK SPACE
 {U'\uff01', '!'}, // FULLWIDTH EXCLAMATION MARK
@@ -1558,6 +1559,7 @@
 {U'\uff5c', '|'}, // FULLWIDTH VERTICAL LINE
 {U'\uff5d', '}'}, // FULLWIDTH RIGHT CURLY BRACKET
 {U'\uff5e', '~'}, // FULLWIDTH TILDE
+{U'\uffa0', 0},   // HALFWIDTH HANGUL FILLER
 {0, 0}
   };
   auto Homoglyph =


Index: test/Lexer/unicode.c
===
--- test/Lexer/unicode.c
+++ test/Lexer/unicode.c
@@ -39,10 +39,12 @@
  // expected-warning@-1 {{treating Unicode character  as identifier character rather than as ';' symbol}}
 int v＝［＝］（auto）｛return～x；｝（）; // expected-warning 12{{treating Unicode character}}
 
-int ⁠xx‍;
+int ⁠xx‍ㅤﾠ;
 // expected-warning@-1 {{identifier contains Unicode character  that is invisible in some environments}}
 // expected-warning@-2 {{identifier contains Unicode character  that is invisible in some environments}}
 // expected-warning@-3 {{identifier contains Unicode character  that is invisible in some environments}}
+// expected-warning@-4 {{identifier contains Unicode character  that is invisible in some environments}}
+// expected-warning@-5 {{identifier contains Unicode character  that is invisible in some environments}}
 int foo​bar = 0; // expected-warning {{identifier contains Unicode character  that is invisible in some environments}}
 int x = foobar; // expected-error {{undeclared identifier}}
 
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -1528,6 +1528,7 @@
 {U'\u2227', '^'}, // LOGICAL AND
 {U'\u2236', ':'}, // RATIO
 {U'\u223c', '~'}, // TILDE OPERATOR
+{U'\u3164', 0},   // HANGUL FILLER
 {U'\ua789', ':'}, // MODIFIER LETTER COLON
 {U'\ufeff', 0},   // ZERO WIDTH NO-BREAK SPACE
 {U'\uff01', '!'}, // FULLWIDTH EXCLAMATION MARK
@@ -1558,6 +1559,7 @@
 {U'\uff5c', '|'}, // FULLWIDTH VERTICAL LINE
 {U'\uff5d', '}'}, // FULLWIDTH RIGHT CURLY BRACKET
 {U'\uff5e', '~'}, // FULLWIDTH TILDE
+{U'\uffa0', 0},   // HALFWIDTH HANGUL FILLER
 {0, 0}
   };
   auto Homoglyph =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59765: [Lex] Warn about invisible Hangul whitespace

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: chandlerc, rsmith.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

On Twitter @LunarLambda pointed out that Clang allows Hangul whitespace Unicode
characters in identifiers, which allows users to write very confusing
programs: https://twitter.com/LunarLambda/status/1110097030423240705

Clang warns about similar whitespace Unicode characters. Add the Hangul
half-width and full-width whitespace characters to the set that Clang
warns about.

N.B.: Clang warns about Japanese space character ``, but in a
different way, because that character is not a valid identifier
character according to the C++11 standard. So Clang emits a warning that
it will treat the Japanese `` as whitespace. This is different
from the Korean Hangul whitespace character, which is a valid identifier
character according to the C++11 standard. For this reason, Clang warns
the character will be treated as an identifier character, not as a
whitecpace character -- so in sum, Clang's behavior is slightly
different for the Japanese whitespace character compared to the Korean
Hangul one.


Repository:
  rC Clang

https://reviews.llvm.org/D59765

Files:
  lib/Lex/Lexer.cpp
  test/Lexer/unicode.c


Index: test/Lexer/unicode.c
===
--- test/Lexer/unicode.c
+++ test/Lexer/unicode.c
@@ -39,10 +39,12 @@
  // expected-warning@-1 {{treating Unicode character  
as identifier character rather than as ';' symbol}}
 int v＝［＝］（auto）｛return～x；｝（）; // expected-warning 12{{treating Unicode 
character}}
 
-int ⁠xx‍;
+int ⁠xx‍xㅤﾠ;
 // expected-warning@-1 {{identifier contains Unicode character  that 
is invisible in some environments}}
 // expected-warning@-2 {{identifier contains Unicode character  that 
is invisible in some environments}}
 // expected-warning@-3 {{identifier contains Unicode character  that 
is invisible in some environments}}
+// expected-warning@-4 {{identifier contains Unicode character  that 
is invisible in some environments}}
+// expected-warning@-5 {{identifier contains Unicode character  that 
is invisible in some environments}}
 int foo​bar = 0; // expected-warning {{identifier contains Unicode character 
 that is invisible in some environments}}
 int x = foobar; // expected-error {{undeclared identifier}}
 
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -1528,6 +1528,7 @@
 {U'\u2227', '^'}, // LOGICAL AND
 {U'\u2236', ':'}, // RATIO
 {U'\u223c', '~'}, // TILDE OPERATOR
+{U'\u3164', 0},   // HANGUL FILLER
 {U'\ua789', ':'}, // MODIFIER LETTER COLON
 {U'\ufeff', 0},   // ZERO WIDTH NO-BREAK SPACE
 {U'\uff01', '!'}, // FULLWIDTH EXCLAMATION MARK
@@ -1558,6 +1559,7 @@
 {U'\uff5c', '|'}, // FULLWIDTH VERTICAL LINE
 {U'\uff5d', '}'}, // FULLWIDTH RIGHT CURLY BRACKET
 {U'\uff5e', '~'}, // FULLWIDTH TILDE
+{U'\uffa0', 0},   // HALFWIDTH HANGUL FILLER
 {0, 0}
   };
   auto Homoglyph =


Index: test/Lexer/unicode.c
===
--- test/Lexer/unicode.c
+++ test/Lexer/unicode.c
@@ -39,10 +39,12 @@
  // expected-warning@-1 {{treating Unicode character  as identifier character rather than as ';' symbol}}
 int v＝［＝］（auto）｛return～x；｝（）; // expected-warning 12{{treating Unicode character}}
 
-int ⁠xx‍;
+int ⁠xx‍xㅤﾠ;
 // expected-warning@-1 {{identifier contains Unicode character  that is invisible in some environments}}
 // expected-warning@-2 {{identifier contains Unicode character  that is invisible in some environments}}
 // expected-warning@-3 {{identifier contains Unicode character  that is invisible in some environments}}
+// expected-warning@-4 {{identifier contains Unicode character  that is invisible in some environments}}
+// expected-warning@-5 {{identifier contains Unicode character  that is invisible in some environments}}
 int foo​bar = 0; // expected-warning {{identifier contains Unicode character  that is invisible in some environments}}
 int x = foobar; // expected-error {{undeclared identifier}}
 
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -1528,6 +1528,7 @@
 {U'\u2227', '^'}, // LOGICAL AND
 {U'\u2236', ':'}, // RATIO
 {U'\u223c', '~'}, // TILDE OPERATOR
+{U'\u3164', 0},   // HANGUL FILLER
 {U'\ua789', ':'}, // MODIFIER LETTER COLON
 {U'\ufeff', 0},   // ZERO WIDTH NO-BREAK SPACE
 {U'\uff01', '!'}, // FULLWIDTH EXCLAMATION MARK
@@ -1558,6 +1559,7 @@
 {U'\uff5c', '|'}, // FULLWIDTH VERTICAL LINE
 {U'\uff5d', '}'}, // FULLWIDTH RIGHT CURLY BRACKET
 {U'\uff5e', '~'}, // FULLWIDTH TILDE
+{U'\uffa0', 0},   // HALFWIDTH HANGUL FILLER
 {0, 0}
   };
   auto Homoglyph =
___
cfe-commits 

[PATCH] D59752: Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thank you for the review!




Comment at: test/SemaCXX/exceptions.cpp:25
+  int ref = k;
+}
 int j = i; // expected-error {{use of undeclared identifier 'i'}}

riccibruno wrote:
> I am wondering if there is already a test which checks systematically that a 
> declaration which shadows another declaration is valid/invalid. I am not 
> seeing such a systematic test, but it might be a nice addition (though that 
> it clearly out of scope for this patch)
That's a good point. I was very surprised when I learned that my original patch 
had broken the lookup behavior described in 
https://bugs.llvm.org/show_bug.cgi?id=41171 but still passed all the tests in 
`check-clang`. I'll try to follow up this patch with some more comprehensive 
tests.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59752/new/

https://reviews.llvm.org/D59752



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

[PATCH] D59752: Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356865: Un-revert [coroutines][PR40978] Emit error for 
co_yield within catch block (authored by modocache, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59752?vs=192045=192050#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59752/new/

https://reviews.llvm.org/D59752

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp
  test/SemaCXX/exceptions.cpp

Index: test/SemaCXX/exceptions.cpp
===
--- test/SemaCXX/exceptions.cpp
+++ test/SemaCXX/exceptions.cpp
@@ -7,6 +7,7 @@
 struct Abstract { virtual void f() = 0; }; // expected-note {{unimplemented pure virtual method 'f'}}
 
 void trys() {
+  int k = 42;
   try {
   } catch(int i) { // expected-note {{previous definition}}
 int j = i;
@@ -18,6 +19,10 @@
   } catch(A ) { // expected-error {{cannot catch reference to incomplete type 'A'}}
   } catch(Abstract) { // expected-error {{variable type 'Abstract' is an abstract class}}
   } catch(...) {
+int ref = k;
+{
+  int ref = k;
+}
 int j = i; // expected-error {{use of undeclared identifier 'i'}}
   }
 
Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -314,13 +314,23 @@
   }
 };
 
+namespace std { class type_info; }
+
 void unevaluated() {
-  decltype(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  sizeof(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  typeid(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  decltype(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
-  sizeof(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
-  typeid(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
+  decltype(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+// expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+  // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+  // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+  // expected-warning@-2 {{expression result unused}}
+  decltype(co_yield 1); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+// expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+  // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+  // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+  // expected-warning@-2 {{expression result unused}}
 }
 
 // [expr.await]p2: "An await-expression shall not appear in a default argument."
@@ -328,6 +338,47 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+[]() -> void { co_await a; }(); // OK
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_nested_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+try {
+  co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  []() -> void { co_await a; }(); // OK
+} catch (...) {
+  co_return 123;
+}
+  }
+}
+
+void await_in_lambda_in_catch_coroutine() {
+  try {
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_yield 1; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used 

[PATCH] D59752: Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: lib/Parse/ParseStmt.cpp:2293
   // FIXME: Possible draft standard bug: attribute-specifier should be allowed?
   StmtResult Block(ParseCompoundStatement());
   if (Block.isInvalid())

riccibruno wrote:
> Just to make sure I understood the problem correctly, the issue was that you 
> passed `ScopeFlags` to `ParseCompoundStatement()`. This override the default 
> flags which are `Scope::DeclScope | Scope::CompoundStmtScope`. In particular 
> now `ParseCompoundStatement()` was done as if in the controlling scope of an 
> if statement. Now as per [basic.scope.block]/p4:
> 
> > Names declared in the init-statement, the for-range-declaration, and in the 
> > condition of if, while, for, and switch statements are local to the if, 
> > while, for, or switch statement (including the controlled statement), and 
> > shall not be redeclared in a subsequent condition of that statement nor in 
> > the outermost block (or, for the if statement, any of the outermost blocks) 
> > of the controlled statement; see 9.4.
> 
> This caused the declaration in the compound statement to be detected as 
> erroneous. Indeed the following worked just fine. 
> 
> ```
> void f() {
> try {}
> catch (...) {
> int i;
> {
> {
> int i;
> }
> }
> }
> }
> ```
> 
> Does this make sense or I am completely off base here ?
> 
That's exactly my understanding, yes! In fact thank you for the clear 
explanation.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59752/new/

https://reviews.llvm.org/D59752



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

[PATCH] D59752: Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, tks2103, rsmith.
Herald added subscribers: jdoerfert, EricWF.
Herald added a project: clang.

https://reviews.llvm.org/D59076 added a new coroutine error that
prevented users from using 'co_await' or 'co_yield' within a exception
handler. However, it was reverted in https://reviews.llvm.org/rC356774
because it caused a regression in nested scopes in C++ catch statements,
as documented by https://bugs.llvm.org/show_bug.cgi?id=41171.

The issue was due to an incorrect use of a `clang::ParseScope`. To fix:

1. Add a regression test for catch statement parsing that mimics the bug report 
from https://bugs.llvm.org/show_bug.cgi?id=41171.
2. Re-apply the coroutines error patch from https://reviews.llvm.org/D59076, 
but this time with the correct ParseScope behavior.


Repository:
  rC Clang

https://reviews.llvm.org/D59752

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp
  test/SemaCXX/exceptions.cpp

Index: test/SemaCXX/exceptions.cpp
===
--- test/SemaCXX/exceptions.cpp
+++ test/SemaCXX/exceptions.cpp
@@ -7,6 +7,7 @@
 struct Abstract { virtual void f() = 0; }; // expected-note {{unimplemented pure virtual method 'f'}}
 
 void trys() {
+  int k = 42;
   try {
   } catch(int i) { // expected-note {{previous definition}}
 int j = i;
@@ -18,6 +19,10 @@
   } catch(A ) { // expected-error {{cannot catch reference to incomplete type 'A'}}
   } catch(Abstract) { // expected-error {{variable type 'Abstract' is an abstract class}}
   } catch(...) {
+int ref = k;
+{
+  int ref = k;
+}
 int j = i; // expected-error {{use of undeclared identifier 'i'}}
   }
 
Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -314,13 +314,23 @@
   }
 };
 
+namespace std { class type_info; }
+
 void unevaluated() {
-  decltype(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  sizeof(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  typeid(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  decltype(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
-  sizeof(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
-  typeid(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
+  decltype(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+// expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+  // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+  // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+  // expected-warning@-2 {{expression result unused}}
+  decltype(co_yield 1); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+// expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+  // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+  // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+  // expected-warning@-2 {{expression result unused}}
 }
 
 // [expr.await]p2: "An await-expression shall not appear in a default argument."
@@ -328,6 +338,47 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+[]() -> void { co_await a; }(); // OK
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_nested_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+try {
+  co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  []() -> void { co_await a; }(); // OK
+} catch (...) {
+  co_return 123;
+}
+  }
+}
+
+void 

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Reverted in rC356296 . I'll rework this 
change along with a test confirming https://bugs.llvm.org/show_bug.cgi?id=41171 
doesn't occur. Apologies!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Sorry for the late response, I'm looking now. Should I revert this for now?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

I'm pushing a revert. Sorry for the trouble!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Great, thanks for the reviews, everyone!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356296: [coroutines][PR40978] Emit error for co_yield within 
catch block (authored by modocache, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59076?vs=190740=190890#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: lib/Sema/Scope.cpp
===
--- lib/Sema/Scope.cpp
+++ lib/Sema/Scope.cpp
@@ -166,7 +166,9 @@
   {SEHExceptScope, "SEHExceptScope"},
   {SEHFilterScope, "SEHFilterScope"},
   {CompoundStmtScope, "CompoundStmtScope"},
-  {ClassInheritanceScope, "ClassInheritanceScope"}};
+  {ClassInheritanceScope, "ClassInheritanceScope"},
+  {CatchScope, "CatchScope"},
+  };
 
   for (auto Info : FlagInfo) {
 if (Flags & Info.first) {
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema , SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
@@ -668,12 +655,57 @@
   return true;
 }
 
+// Recursively walks up the scope hierarchy until either a 'catch' or a function
+// scope is found, whichever comes first.
+static bool isWithinCatchScope(Scope *S) {
+  // 'co_await' and 'co_yield' keywords are disallowed within catch blocks, but
+  // lambdas that use 'co_await' are allowed. The loop below ends when a
+  // function scope is found in order to ensure the following behavior:
+  //
+  // void foo() {  // <- function scope
+  //   try {   //
+  // co_await x;   // <- 'co_await' is OK within a function scope
+  //   } catch {   // <- catch scope
+  // co_await x;   // <- 'co_await' is not OK within a catch scope
+  // []() {// <- function scope
+  //   co_await x; // <- 'co_await' is OK within a function scope
+  // }();
+  //   }
+  // }
+  while (S && !(S->getFlags() & Scope::FnScope)) {
+if (S->getFlags() & Scope::CatchScope)
+  return true;
+S = S->getParent();
+  }
+  return false;
+}
+
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static void checkSuspensionContext(Sema , SourceLocation Loc,
+   StringRef Keyword) {
+  // First emphasis of [expr.await]p2: must be a potentially evaluated context.
+  // That is, 'co_await' and 'co_yield' cannot appear in subexpressions of
+  // \c sizeof.
+  if (S.isUnevaluatedContext())
+S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
+
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (isWithinCatchScope(S.getCurScope()))
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
+}
+
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
   if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
 CorrectDelayedTyposInExpr(E);
 return ExprError();
   }
 
+  checkSuspensionContext(*this, Loc, "co_await");
+
   if (E->getType()->isPlaceholderType()) {
 ExprResult R = CheckPlaceholderExpr(E);
 if (R.isInvalid()) return ExprError();
@@ -771,6 +803,8 @@
 return ExprError();
   }
 
+  checkSuspensionContext(*this, Loc, "co_yield");
+
   // Build yield_value call.
   ExprResult 

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

I realize this isn't the correct solution, but would any would-be reviewers 
like to comment on the problem? Whether it's here or on the Bugzilla report 
https://bugs.llvm.org/show_bug.cgi?id=39287, as a newcomer to Clang modules I 
could use some help understanding whether this sort of behavior is expected, or 
if there are known workarounds. Any and all comments appreciated!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58920/new/

https://reviews.llvm.org/D58920



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 190740.
modocache added a comment.

OK, I've responded to all your review comments -- thank you! Please give this 
another look when you get a chance. I would like to emit note diagnostics 
pointing to the catch blocks, but I've left that as a FIXME for now.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -314,13 +314,23 @@
   }
 };
 
+namespace std { class type_info; }
+
 void unevaluated() {
-  decltype(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  sizeof(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  typeid(co_await a); // expected-error {{cannot be used in an unevaluated context}}
-  decltype(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
-  sizeof(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
-  typeid(co_yield a); // expected-error {{cannot be used in an unevaluated context}}
+  decltype(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+// expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+  // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_await a); // expected-error {{'co_await' cannot be used in an unevaluated context}}
+  // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+  // expected-warning@-2 {{expression result unused}}
+  decltype(co_yield 1); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+// expected-warning@-1 {{declaration does not declare anything}}
+  sizeof(co_yield 2); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+  // expected-error@-1 {{invalid application of 'sizeof' to an incomplete type 'void'}}
+  typeid(co_yield 3); // expected-error {{'co_yield' cannot be used in an unevaluated context}}
+  // expected-warning@-1 {{expression with side effects has no effect in an unevaluated context}}
+  // expected-warning@-2 {{expression result unused}}
 }
 
 // [expr.await]p2: "An await-expression shall not appear in a default argument."
@@ -328,6 +338,47 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+[]() -> void { co_await a; }(); // OK
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_nested_in_catch_coroutine() {
+  try {
+  } catch (...) { // FIXME: Emit a note diagnostic pointing out the try handler on this line.
+try {
+  co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  []() -> void { co_await a; }(); // OK
+} catch (...) {
+  co_return 123;
+}
+  }
+}
+
+void await_in_lambda_in_catch_coroutine() {
+  try {
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_yield 1; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema , SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within 

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 190727.
modocache added a comment.

Remove unused function parameter.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -328,6 +328,47 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try { // Expect a note diagnostic here.
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_nested_in_catch_coroutine() {
+  try { // Expect a note diagnostic here.
+  } catch (...) {
+try {
+  co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  []() -> void { co_await a; }(); // OK
+} catch (...) {
+  co_return 123;
+}
+  }
+}
+
+void await_in_lambda_in_catch_coroutine() {
+  try {
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_yield a; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema , SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
@@ -668,8 +655,58 @@
   return true;
 }
 
+// Recursively walks up the scope hierarchy until either a 'catch' or a function
+// scope is found, whichever comes first.
+static bool isWithinCatchScope(Scope *S) {
+  // 'co_await' and 'co_yield' keywords are disallowed within catch blocks, but
+  // lambdas that use 'co_await' are allowed. The loop below ends when a
+  // function scope is found in order to ensure the following behavior:
+  //
+  // void foo() {  // <- function scope
+  //   try {   //
+  // co_await x;   // <- 'co_await' is OK within a function scope
+  //   } catch {   // <- catch scope
+  // co_await x;   // <- 'co_await' is not OK within a catch scope
+  // []() {// <- function scope
+  //   co_await x; // <- 'co_await' is OK within a function scope
+  // }();
+  //   }
+  // }
+  while (S && !(S->getFlags() & Scope::FnScope)) {
+if (S->getFlags() & Scope::CatchScope)
+  return true;
+S = S->getParent();
+  }
+  return false;
+}
+
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static bool isValidSuspensionContext(Sema , SourceLocation Loc,
+ StringRef Keyword) {
+  // First emphasis of [expr.await]p2: must be a potentially evaluated context.
+  // That is, 'co_await' and 'co_yield' 

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 190718.
modocache added a comment.

Thank you for the reviews! This revision fixes the nested try/catch behavior. I 
still need to modify this such that semantic analysis continues for the rest of 
the function body.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -328,6 +328,47 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try { // Expect a note diagnostic here.
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_nested_in_catch_coroutine() {
+  try { // Expect a note diagnostic here.
+  } catch (...) {
+try {
+  co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  []() -> void { co_await a; }(); // OK
+} catch (...) {
+  co_return 123;
+}
+  }
+}
+
+void await_in_lambda_in_catch_coroutine() {
+  try {
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_yield a; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema , SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
@@ -668,8 +655,59 @@
   return true;
 }
 
+// Recursively walks up the scope hierarchy until either a 'catch' or a function
+// scope is found, whichever comes first.
+static bool isWithinCatchScope(Scope *S) {
+  // 'co_await' and 'co_yield' keywords are disallowed within catch blocks, but
+  // lambdas that use 'co_await' are allowed. The loop below ends when a
+  // function scope is found in order to ensure the following behavior:
+  //
+  // void foo() {  // <- function scope
+  //   try {   //
+  // co_await x;   // <- 'co_await' is OK within a function scope
+  //   } catch {   // <- catch scope
+  // co_await x;   // <- 'co_await' is not OK within a catch scope
+  // []() {// <- function scope
+  //   co_await x; // <- 'co_await' is OK within a function scope
+  // }();
+  //   }
+  // }
+  while (S && !(S->getFlags() & Scope::FnScope)) {
+if (S->getFlags() & Scope::CatchScope)
+  return true;
+S = S->getParent();
+  }
+  return false;
+}
+
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static bool isValidSuspensionContext(Sema , SourceLocation Loc,
+

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache marked an inline comment as done.
modocache added inline comments.



Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception 
handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;

modocache wrote:
> lewissbaker wrote:
> > modocache wrote:
> > > EricWF wrote:
> > > > We can still build a valid AST after encountering this error, no?
> > > > 
> > > > 
> > > I believe so. Just to be clear: you'd like me to continue building the 
> > > AST even after emitting this error diagnostic? My understanding is that 
> > > most of this file bails soon after any error is encountered (correct me 
> > > if that's wrong). I'm happy to change that, but I wonder if it'd be 
> > > better to do that in a separate diff...?
> > Do the scope flags reset when a lambda occurs within a catch-block?
> > 
> > eg. The following should still be valid.
> > ```
> > try { ... }
> > catch (...) {
> >   []() -> task { co_await foo(); }();
> > }
> > ```
> I believe they're reset, the example you posted compiles fine with this 
> patch. I'll add a test case specifically for this and confirm exactly where 
> the scope flags are reset or ignored in the lambda case.
When the parser encounters a lambda, it takes the path 
`Parser::ParseLambdaExpression -> Parser::ParseLambdaExpressionAfterIntroducer 
-> Parser::ParseCompoundStatementBody`, which creates an inner scope for the 
body of the lambda. This inner scope does not have the `Scope::CatchScope` 
flag, so it doesn't result in the error.

Although, your question did make me realize that this compiles just fine, even 
with this patch:

```
void example() {
  try {
throw;
  } catch (...) {
try {
  co_await a;
}
  }
}
```

But I believe this above case should also be treated as an error, right?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 189763.
modocache added a comment.

Add test case for executing a lambda using co_yield within a catch block.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -328,6 +328,34 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void await_in_lambda_in_catch_coroutine() {
+  try {
+  } catch (...) {
+[]() -> void { co_await a; }(); // OK
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_yield a; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema , SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
@@ -668,8 +655,34 @@
   return true;
 }
 
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static bool isValidSuspensionContext(Sema , SourceLocation Loc,
+ StringRef Keyword,
+ bool IsImplicit = false) {
+  // First emphasis of [expr.await]p2: must be a potentially evaluated context.
+  // That is, 'co_await' and 'co_yield' cannot appear in subexpressions of
+  // \c sizeof.
+  if (S.isUnevaluatedContext()) {
+S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
+return false;
+  }
+
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
+return false;
+  }
+
+  return true;
+}
+
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_await") ||
+  !isValidSuspensionContext(*this, Loc, "co_await")) {
 CorrectDelayedTyposInExpr(E);
 return ExprError();
   }
@@ -689,7 +702,7 @@
 ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *E,
 UnresolvedLookupExpr *Lookup) {
   auto *FSI = checkCoroutineContext(*this, Loc, "co_await");
-  if (!FSI)
+  if (!FSI || !isValidSuspensionContext(*this, Loc, "co_await"))
 return ExprError();
 
   if (E->getType()->isPlaceholderType()) {
@@ -766,7 +779,8 @@
 }
 
 ExprResult Sema::ActOnCoyieldExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!ActOnCoroutineBodyStart(S, Loc, "co_yield")) {
+  if 

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: lib/Sema/SemaCoroutine.cpp:197-200
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }

lewissbaker wrote:
> Does removing this check now mean that we're not checking that `co_return` 
> statements don't appear in unevaluated contexts?
> 
> Or is that already handled elsewhere by the fact that `co_return` statements 
> are not expressions and are therefore detected earlier as a grammar violation 
> when parsing `sizeof()` expression?
That's right! If one were to attempt to use a `co_return` within a 
subexpression of `sizeof`, they'd see `error: expected expression` before 
they'd ever have seen this error message. So I believe there's no need to 
perform this check for `co_return`, and I believe that's why the revisions to 
the standard included in the Coroutines TS don't make any special mention of 
disallowing `co_return` in those contexts.



Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception 
handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;

lewissbaker wrote:
> modocache wrote:
> > EricWF wrote:
> > > We can still build a valid AST after encountering this error, no?
> > > 
> > > 
> > I believe so. Just to be clear: you'd like me to continue building the AST 
> > even after emitting this error diagnostic? My understanding is that most of 
> > this file bails soon after any error is encountered (correct me if that's 
> > wrong). I'm happy to change that, but I wonder if it'd be better to do that 
> > in a separate diff...?
> Do the scope flags reset when a lambda occurs within a catch-block?
> 
> eg. The following should still be valid.
> ```
> try { ... }
> catch (...) {
>   []() -> task { co_await foo(); }();
> }
> ```
I believe they're reset, the example you posted compiles fine with this patch. 
I'll add a test case specifically for this and confirm exactly where the scope 
flags are reset or ignored in the lambda case.



Comment at: test/SemaCXX/coroutines.cpp:299-311
   // FIXME: The spec says this is ill-formed.
   void operator=(CtorDtor&) {
 co_yield 0; // expected-error {{'co_yield' cannot be used in a copy 
assignment operator}}
   }
   void operator=(CtorDtor const &) {
 co_yield 0; // expected-error {{'co_yield' cannot be used in a copy 
assignment operator}}
   }

lewissbaker wrote:
> Not related to this diff, but...
> 
> I don't think that these should be ill-formed.
> 
> According to N4775 there are only exclusions added for [class.ctor] and 
> [class.dtor].
> I can't see anything in the spec that says that assignment special member 
> functions cannot be coroutines.
That's a great point. Could you create a Bugzilla for this work? And maybe we 
can get @GorNishanov's opinion?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.



Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception 
handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;

EricWF wrote:
> We can still build a valid AST after encountering this error, no?
> 
> 
I believe so. Just to be clear: you'd like me to continue building the AST even 
after emitting this error diagnostic? My understanding is that most of this 
file bails soon after any error is encountered (correct me if that's wrong). 
I'm happy to change that, but I wonder if it'd be better to do that in a 
separate diff...?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, tks2103, rsmith.
Herald added subscribers: jdoerfert, EricWF.
Herald added a project: clang.

As reported in https://bugs.llvm.org/show_bug.cgi?id=40978, it's an
error to use the `co_yield` or `co_await` keywords outside of a valid
"suspension context" as defined by [expr.await]p2 of
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4775.pdf.

Whether or not the current scope was in a function-try-block's
(https://en.cppreference.com/w/cpp/language/function-try-block) handler
could be determined using scope flag `Scope::FnTryCatchScope`. No
such flag existed for a simple C++ catch statement, so this commit adds
one.


Repository:
  rC Clang

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -328,6 +328,27 @@
 // not allowed. A user may not understand that this is "outside a function."
 void default_argument(int arg = co_await 0) {} // expected-error {{'co_await' cannot be used outside a function}}
 
+void await_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_await a; // expected-error {{'co_await' cannot be used in the handler of a try block}}
+  }
+}
+
+void yield_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_yield a; // expected-error {{'co_yield' cannot be used in the handler of a try block}}
+  }
+}
+
+void return_in_catch_coroutine() {
+  try {
+  } catch (...) {
+co_return 123; // OK
+  }
+}
+
 constexpr auto constexpr_deduced_return_coroutine() {
   co_yield 0; // expected-error {{'co_yield' cannot be used in a constexpr function}}
   // expected-error@-1 {{'co_yield' cannot be used in a function with a deduced return type}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema , SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
@@ -668,8 +655,34 @@
   return true;
 }
 
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static bool isValidSuspensionContext(Sema , SourceLocation Loc,
+ StringRef Keyword,
+ bool IsImplicit = false) {
+  // First emphasis of [expr.await]p2: must be a potentially evaluated context.
+  // That is, 'co_await' and 'co_yield' cannot appear in subexpressions of
+  // \c sizeof.
+  if (S.isUnevaluatedContext()) {
+S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
+return false;
+  }
+
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
+return false;
+  }
+
+  return true;
+}
+
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
-  if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
+  if (!ActOnCoroutineBodyStart(S, Loc, "co_await") ||
+  !isValidSuspensionContext(*this, Loc, "co_await")) {
 CorrectDelayedTyposInExpr(E);
 return ExprError();
   }
@@ -689,7 +702,7 @@
 ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *E,
 UnresolvedLookupExpr *Lookup) {
   auto *FSI