[coroutines] clang fails to run trivial ABI destructors when a suspended coroutine is destroyed

[jacobsa]

Here's a small program containing a coroutine (Foo) that starts to evaluate a function call expression but then destroys itself before actually making the function call:

#include <coroutine>
#include <cstdlib>
#include <iostream>

struct DestroySelfTag {};

// An eager coroutine result type that supports awaiting DestroySelf to
// cause the promise to destroy the coroutine frame.
struct MyTask {
  struct promise_type {
    MyTask get_return_object() { return {}; }
    std::suspend_never initial_suspend() { return {}; }
    std::suspend_always final_suspend() noexcept { return {}; }
    void unhandled_exception();
    void return_void();

    auto await_transform(DestroySelfTag) {
      struct Awaiter {
        bool await_ready() { return false; }
        void await_suspend(const std::coroutine_handle<> h) { h.destroy(); }

        int await_resume() {
          // We should never resume; we destroyed ourselves just after
          // suspending.
          std::abort();
        }
      };

      return Awaiter{};
    }
  };
};

// A trivial ABI type that lets us know when it's created and destroyed.
struct HasDestructor {
  HasDestructor() { std::cout << "created:   " << this << "\n"; }
  ~HasDestructor() { std::cout << "destroyed: " << this << "\n"; }
};

// A function that we have a call expression for below in Foo. This just
// needs to accept the right types -- it should never actually be called.
void AcceptArgs(HasDestructor, int, HasDestructor) { std::abort(); }

// A coroutine that creates a HasDestructor object then destroys itself
// before doing anything with it.
MyTask Foo() {
  AcceptArgs(HasDestructor(), co_await DestroySelfTag(), HasDestructor());
  std::abort();
}

int main() {
  Foo();
  return 0;
}

When compiled with -std=c++20 -O2 -fno-exceptions (Compiler Explorer), the program works as expected. It creates one HasDestructor object, then destroys it again when std::coroutine_handle::destroy is called:

created:   0x7ffd04aa2b6a
destroyed: 0x7ffd04aa2b6a

However, the program is miscompiled when we put the [[clang::trivial_abi]] attribute on HasDestructor (Compiler Explorer). In this case we fail to run the destructor, creating but then not ever destroying the HasDestructor object:

created:   0x7fff8f4da6b2

It doesn't seem like this should happen. There isn't extremely rigorous documentation on the semantics of [[clang::trivial_abi]], but what does exist says "the convention is that the callee will destroy the object before returning". That makes sense, but there is no function call here. AcceptArgs is never actually called. Instead this should be treated the same as the following, which does work correctly:

MyTask Foo() {
  HasDestructor(), co_await DestroySelfTag();
  std::abort();
}

In a real codebase this bug could cause resources to be leaked when a coroutine is stopped early, e.g. due to cancellation.

[ilya-biryukov]

It does seem like a bug, the presence of [[clang::trivial_abi]] should not inhibit destructor calls here.
Similar example with exceptions does work: https://godbolt.org/z/j1Gqo9dax.

@usx95 this seems similar to the bug you fixed where destructors did not run. Could you take a look at this one?

[llvmbot]

@llvm/issue-subscribers-coroutines

Author: Aaron Jacobs (jacobsa)

Here's a small program containing a coroutine (`Foo`) that starts to evaluate a function call expression but then destroys itself before actually making the function call:

#include <coroutine>
#include <cstdlib>
#include <iostream>

struct DestroySelfTag {};

// An eager coroutine result type that supports awaiting DestroySelf to
// cause the promise to destroy the coroutine frame.
struct MyTask {
  struct promise_type {
    MyTask get_return_object() { return {}; }
    std::suspend_never initial_suspend() { return {}; }
    std::suspend_always final_suspend() noexcept { return {}; }
    void unhandled_exception();
    void return_void();

    auto await_transform(DestroySelfTag) {
      struct Awaiter {
        bool await_ready() { return false; }
        void await_suspend(const std::coroutine_handle<> h) { h.destroy(); }

        int await_resume() {
          // We should never resume; we destroyed ourselves just after
          // suspending.
          std::abort();
        }
      };

      return Awaiter{};
    }
  };
};

// A trivial ABI type that lets us know when it's created and destroyed.
struct HasDestructor {
  HasDestructor() { std::cout << "created:   " << this << "\n"; }
  ~HasDestructor() { std::cout << "destroyed: " << this << "\n"; }
};

// A function that we have a call expression for below in Foo. This just
// needs to accept the right types -- it should never actually be called.
void AcceptArgs(HasDestructor, int, HasDestructor) { std::abort(); }

// A coroutine that creates a HasDestructor object then destroys itself
// before doing anything with it.
MyTask Foo() {
  AcceptArgs(HasDestructor(), co_await DestroySelfTag(), HasDestructor());
  std::abort();
}

int main() {
  Foo();
  return 0;
}

When compiled with -std=c++20 -O2 -fno-exceptions (Compiler Explorer), the program works as expected. It creates one HasDestructor object, then destroys it again when std::coroutine_handle::destroy is called:

created:   0x7ffd04aa2b6a
destroyed: 0x7ffd04aa2b6a

However, the program is miscompiled when we put the [[clang::trivial_abi]] attribute on HasDestructor (Compiler Explorer). In this case we fail to run the destructor, creating but then not ever destroying the HasDestructor object:

created:   0x7fff8f4da6b2

It doesn't seem like this should happen. There isn't extremely rigorous documentation on the semantics of [[clang::trivial_abi]], but what does exist says "the convention is that the callee will destroy the object before returning". That makes sense, but there is no function call here. AcceptArgs is never actually called. Instead this should be treated the same as the following, which does work correctly:

MyTask Foo() {
  HasDestructor(), co_await DestroySelfTag();
  std::abort();
}

In a real codebase this bug could cause resources to be leaked when a coroutine is stopped early, e.g. due to cancellation.

[usx95]

Reproduces with early exit in statement expression and without std::abort. So this is very likely an edge case of #63818.
The fact that it works with exceptions is reassuring. I will take a look at it.

void AcceptArgs(HasDestructor, int, HasDestructor) {}

void Foo() {
    AcceptArgs(HasDestructor(), ({return; 0;}), HasDestructor());
}

compiler explorer

[rnk]

The fact that it works with exceptions is reassuring.

I agree, that is reassuring. I was going to say that's the case we need to check. There are a few places like this where clang has a scheme to push EH-only cleanups, and then disable or pop them off the stack after the call transferring ownership occurs, which seems related.

[usx95]

Sent out fix: #88751

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Aaron Jacobs (jacobsa)

Here's a small program containing a coroutine (`Foo`) that starts to evaluate a function call expression but then destroys itself before actually making the function call:

#include <coroutine>
#include <cstdlib>
#include <iostream>

struct DestroySelfTag {};

// An eager coroutine result type that supports awaiting DestroySelf to
// cause the promise to destroy the coroutine frame.
struct MyTask {
  struct promise_type {
    MyTask get_return_object() { return {}; }
    std::suspend_never initial_suspend() { return {}; }
    std::suspend_always final_suspend() noexcept { return {}; }
    void unhandled_exception();
    void return_void();

    auto await_transform(DestroySelfTag) {
      struct Awaiter {
        bool await_ready() { return false; }
        void await_suspend(const std::coroutine_handle<> h) { h.destroy(); }

        int await_resume() {
          // We should never resume; we destroyed ourselves just after
          // suspending.
          std::abort();
        }
      };

      return Awaiter{};
    }
  };
};

// A trivial ABI type that lets us know when it's created and destroyed.
struct HasDestructor {
  HasDestructor() { std::cout << "created:   " << this << "\n"; }
  ~HasDestructor() { std::cout << "destroyed: " << this << "\n"; }
};

// A function that we have a call expression for below in Foo. This just
// needs to accept the right types -- it should never actually be called.
void AcceptArgs(HasDestructor, int, HasDestructor) { std::abort(); }

// A coroutine that creates a HasDestructor object then destroys itself
// before doing anything with it.
MyTask Foo() {
  AcceptArgs(HasDestructor(), co_await DestroySelfTag(), HasDestructor());
  std::abort();
}

int main() {
  Foo();
  return 0;
}

When compiled with -std=c++20 -O2 -fno-exceptions (Compiler Explorer), the program works as expected. It creates one HasDestructor object, then destroys it again when std::coroutine_handle::destroy is called:

created:   0x7ffd04aa2b6a
destroyed: 0x7ffd04aa2b6a

However, the program is miscompiled when we put the [[clang::trivial_abi]] attribute on HasDestructor (Compiler Explorer). In this case we fail to run the destructor, creating but then not ever destroying the HasDestructor object:

created:   0x7fff8f4da6b2

It doesn't seem like this should happen. There isn't extremely rigorous documentation on the semantics of [[clang::trivial_abi]], but what does exist says "the convention is that the callee will destroy the object before returning". That makes sense, but there is no function call here. AcceptArgs is never actually called. Instead this should be treated the same as the following, which does work correctly:

MyTask Foo() {
  HasDestructor(), co_await DestroySelfTag();
  std::abort();
}

In a real codebase this bug could cause resources to be leaked when a coroutine is stopped early, e.g. due to cancellation.

[usx95]

(the primary commit for stmt-expr cleanups was reverted)

[Alcaro]

For those wondering, the revert is #88884 (which also reverts #63818 )

[usx95]

Updated PR: #89154