[Clang] Skip past code generation for unevaluated lambdas

[zyn0217]

This is taken from #122423 (comment) as suggested by Corentin.

Fixes #82926

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

This is taken from #122423 (comment) as suggested by Corentin.

Fixes #82926

---

Full diff: https://github.com/llvm/llvm-project/pull/124572.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+18-2)
• (added) clang/test/CodeGenCXX/unevaluated-lambdas.cpp (+37)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f75c726e2751cf..7bb33d6b44d4ee 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -966,6 +966,7 @@ Bug Fixes to C++ Support
   constraints are applied. (#GH122134)
 - Fixed canonicalization of pack indexing types - Clang did not always recognized identical pack indexing. (#GH123033)
 - Fixed a nested lambda substitution issue for constraint evaluation. (#GH123441)
+- Fixed some crashes involving unevaluated lambdas during code generation. (#GH82926)
 
 
 Bug Fixes to AST Handling
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 6a2331e59477a2..6a55dbd98b0b4d 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5297,8 +5297,24 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
     savedContext.pop();
   }
 
-  DeclGroupRef DG(Function);
-  Consumer.HandleTopLevelDecl(DG);
+  // We never need to emit the code for a lambda in unevaluated context.
+  // We also can't mangle a lambda in the require clause of a function template
+  // during constraint checking as the MSI ABI would need to mangle the (not yet
+  // specialized) enclosing declaration
+  // FIXME: Should we try to skip this for non-lambda functions too?
+  bool ShouldSkipCG = [&] {
+    auto *RD = dyn_cast<CXXRecordDecl>(Function->getParent());
+    if (!RD || !RD->isLambda())
+      return false;
+
+    return llvm::any_of(ExprEvalContexts, [](auto &Context) {
+      return Context.isUnevaluated() || Context.isImmediateFunctionContext();
+    });
+  }();
+  if (!ShouldSkipCG) {
+    DeclGroupRef DG(Function);
+    Consumer.HandleTopLevelDecl(DG);
+  }
 
   // This class may have local implicit instantiations that need to be
   // instantiation within this scope.
diff --git a/clang/test/CodeGenCXX/unevaluated-lambdas.cpp b/clang/test/CodeGenCXX/unevaluated-lambdas.cpp
new file mode 100644
index 00000000000000..5bdf8294dc1fc3
--- /dev/null
+++ b/clang/test/CodeGenCXX/unevaluated-lambdas.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -std=c++2b -emit-llvm %s -o - | FileCheck %s -dump-input=always
+
+namespace GH82926 {
+
+template<class Tp>
+using simd_vector = Tp;
+
+template<class VecT>
+using simd_vector_underlying_type_t
+    = decltype([]<class Tp>(simd_vector<Tp>) {}(VecT {}), 1);
+
+template<class VecT>
+void temp() {
+  // CHECK: call void @_ZZN7GH829264tempIcEEvvENKUliE_clEi
+  [](simd_vector_underlying_type_t<VecT>) {}(42);
+}
+
+void call() {
+  temp<simd_vector<char>>(); 
+}
+
+} // namespace GH82926
+
+namespace GH111058 {
+
+// FIXME: This still crashes because the unevaluated lambda as an argument
+// is also supposed to skipping codegen in Sema::InstantiateFunctionDefinition().
+// auto eat(auto) {}
+
+void foo() {
+	// [] -> decltype(eat([] {})) {};
+
+  // CHECK: call void @"_ZZN8GH1110583fooEvENK3$_0clEv"
+  [] -> decltype([](auto){}(1)) {}();
+}
+
+} // namespace GH111058

[cor3ntin]

Fundamentally the issue is that we emit codegen decl for all function instantiations (and presumably that gets ignored for decls that are not ODR-used).
But in the process, we try to produce a mangling, which we cannot do for some unevaluated lambdas whose mangling depends on the outerscope that is not yet instantiated.

I'm not sure this is the best fix but neither @zyn0217 nor myself can come up with a better idea.

[erichkeane]

I would expect that we should instead be marking this function not isUsed (or isReferenced? ) to determine whether this should be emitted, and I would expect that to get caught in our existing infrastructure for that.

[cor3ntin]

I would expect that we should instead be marking this function not isUsed (or isReferenced? ) to determine whether this should be emitted, and I would expect that to get caught in our existing infrastructure for that.

And we do. But mangling happens before we make that decision
cf stacktrace here #82926 (comment)

[erichkeane]

I would expect that we should instead be marking this function not isUsed (or isReferenced? ) to determine whether this should be emitted, and I would expect that to get caught in our existing infrastructure for that.

And we do. But mangling happens before we make that decision cf stacktrace here #82926 (comment)

Which call in EmitGlobal is the one that calls it? I would expect that either of the two (not hte annotations one for obvious reasons) should be able to filter it THERE instead. Do we perhaps just mangle 'too early'?

[cor3ntin]

Which call in EmitGlobal is the one that calls it? I would expect that either of the two (not hte annotations one for obvious reasons) should be able to filter it THERE instead. Do we perhaps just mangle 'too early'?

We use the mangle name as key to decide whether something was already used haha

  StringRef MangledName = getMangledName(GD);
  if (GetGlobalValue(MangledName) != nullptr) {
    // The value has already been used and should therefore be emitted.
    addDeferredDeclToEmit(GD);
  } else if (MustBeEmitted(Global)) {
    // The value must be emitted, but cannot be emitted eagerly.
    assert(!MayBeEmittedEagerly(Global));
    addDeferredDeclToEmit(GD);
  } else {
    // Otherwise, remember that we saw a deferred decl with this name.  The
    // first use of the mangled name will cause it to move into
    // DeferredDeclsToEmit.
    DeferredDecls[MangledName] = GD;
  }

But really i don't think we should even get there from an unevaluated context

[erichkeane]

Which call in EmitGlobal is the one that calls it? I would expect that either of the two (not hte annotations one for obvious reasons) should be able to filter it THERE instead. Do we perhaps just mangle 'too early'?

We use the mangle name as key to decide whether something was already used haha

  StringRef MangledName = getMangledName(GD);
  if (GetGlobalValue(MangledName) != nullptr) {
    // The value has already been used and should therefore be emitted.
    addDeferredDeclToEmit(GD);
  } else if (MustBeEmitted(Global)) {
    // The value must be emitted, but cannot be emitted eagerly.
    assert(!MayBeEmittedEagerly(Global));
    addDeferredDeclToEmit(GD);
  } else {
    // Otherwise, remember that we saw a deferred decl with this name.  The
    // first use of the mangled name will cause it to move into
    // DeferredDeclsToEmit.
    DeferredDecls[MangledName] = GD;
  }

But really i don't think we should even get there from an unevaluated context

Yeah, I agree that we shouldn't get to that point on the decision. That said, I THINK the decision should be made in CodeGen, not Sema? Perhaps @efriedma-quic has comments?

[efriedma-quic]

See also discussion on #117845. I think it makes some sense to add a way for CodeGen to query whether a given declaration is impossible to emit. I suggested in that discussion that it could be a kind of linkage.

I'd prefer not to mess with the way DeferredDecls works for decls that aren't impossible to emit, if possible.

I don't think "odr-used" is a useful concept here; I think it's possible for an decl to be odr-used even if there isn't a mangling.

Alternatively, we could just invent a mangling here, like we did for #117845.

[cor3ntin]

@efriedma-quic So we would track that a function that was declared in an unevaluated context has "unevaluated linkage" (or really, a bit that said it was declared in unevaluated context), and skip them in EmitTopLevelDecl like we do for immediate functions?

Seems reasonable (and I prefer that to inventing mangling, in think)

[efriedma-quic]

Looking a bit more, I'm not sure I buy the premise that we don't need to mangle lambdas written in unevaluated contexts. Consider the following related example:

template<class Tp>
using simd_vector = Tp;

template<class VecT>
using simd_vector_underlying_type_t
    = decltype([]<class Tp>(simd_vector<Tp>) { return 3; });

template<class VecT>
auto temp() {
  return [](simd_vector_underlying_type_t<VecT> x) { return x(10); };
}

int call() {
  return temp<simd_vector<char>>()({}); 
}

Both gcc and MSVC seem perfectly happy with this.

[shafik]

Fundamentally the issue is that we emit codegen decl for all function instantiations (and presumably that gets ignored for decls that are not ODR-used). But in the process, we try to produce a mangling, which we cannot do for some unevaluated lambdas whose mangling depends on the outerscope that is not yet instantiated.

I'm not sure this is the best fix but neither @zyn0217 nor myself can come up with a better idea.

Some of these details need to end up in the summary before this is landed.