Assertion failed 'cannot emit delegate call arguments for inalloca arguments' in certain cases involving lambdas

[pcc]

Bugzilla Link	28299
Version	trunk
OS	Windows XP
CC	@majnemer,@zmodem,@k15tfu,@nico,@zygoloid,@rnk,@ZequanWu

Extended Description

e.g.
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin/builds/8593/steps/compile/logs/stdio
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/10185/steps/compile/logs/stdio

Working on reproducer.

[pcc]

assigned to @ZequanWu

[pcc]

$ cat foo2.ii
class A {
A(A &);
};
class B {
A bind_state_;
};
void fn1() {
auto test_callback = {};
*test_callback;
}
$ clang -cc1 -triple i386-pc-windows-msvc19.0.0 -emit-obj -std=c++14 -x c++ foo2.ii
foo2.ii:9:3: warning: expression result unused
*test_callback;
^~~~~~~~~~~~~~
clang: ../tools/clang/lib/CodeGen/CGCall.cpp:2871: void clang::CodeGen::CodeGenFunction::EmitDelegateCallArg(clang::CodeGen::CallArgList &, const clang::VarDecl *, clang::SourceLocation): Assertion `!isInAllocaArgument(CGM.getCXXABI(), type) && "cannot emit delegate call arguments for inalloca arguments!"' failed.

[pcc]

Or even simpler:

class A {
A(A &);
};
void fn1() {
auto x = {};
*x;
}

[majnemer]

Reid, what are your thoughts here?

[rnk]

I found it easier to think about this test case:

class A { A(A &); };
typedef void (*fptr_t)(A);
fptr_t fn1() { return [](A) {}; }

MSVC doesn't accept the test case with *x, because it's not sure which conversion operator to call.

If we desugared lambdas like this, everything would fall out:

class A { A(A &); };
typedef void (*fptr_t)(A);
void fn1() {
  struct <lambda> {
    // __impl takes all parameters by reference for convenience
    void __impl(A &a) {
      ... // lambda body code
    }
    void operator()(A a) { return __impl(a); }
    static void __invoke(A a) { return __impl(a); }
  };
  return <lambda>();
}

It might be possible to keep this as a codegen-only change.

[rnk]

We could use the same techniques to fix this bug on Linux:

typedef void (*fptr_t)(int, ...);
fptr_t fn1() {
  return [](int, ...) {};
}

t.cpp:3:10: error: cannot compile this lambda conversion to variadic function yet
  return [](int, ...) {};
         ^

[zygoloid]

We could use the same techniques to fix this bug on Linux:

typedef void (*fptr_t)(int, ...);
fptr_t fn1() {
return [](int, ...) {};
}

t.cpp:3:10: error: cannot compile this lambda conversion to variadic
function yet
return [](int, ...) {};
^

I think that's a somewhat different issue -- you can't forward a varargs function by just converting parameters to pass-by-reference. I think we need to convert the ... to a va_list to support this case.

I'm also not especially happy about the idea of producing worse code for the overwhelmingly common case of a call to the lambda's operator() in order to support this corner of the language and ABI. We could try making the __impl function always_inline, but even that feels a bit hackish.

Can we (easily) determine in advance if we're in the common case where we can correctly emit the function body twice? (Eg, that there are no computed gotos in the function.)

[rnk]

We could use the same techniques to fix this bug on Linux:

typedef void (*fptr_t)(int, ...);
fptr_t fn1() {
return [](int, ...) {};
}

t.cpp:3:10: error: cannot compile this lambda conversion to variadic
function yet
return [](int, ...) {};
^

I think that's a somewhat different issue -- you can't forward a varargs
function by just converting parameters to pass-by-reference. I think we need
to convert the ... to a va_list to support this case.

The idea was that we would emit the lambda body exactly once as part of __impl, and it would use a va_list argument. The operator() and __invoke methods would do va_start and delegate to __impl.

I'm also not especially happy about the idea of producing worse code for the
overwhelmingly common case of a call to the lambda's operator() in order to
support this corner of the language and ABI. We could try making the __impl
function always_inline, but even that feels a bit hackish.

Can you elaborate on what makes it worse? One more direct call forwarding arguments? I don't plan to call any copy constructors, it would be a "delegating" call like we have for vtable thunks.

Can we (easily) determine in advance if we're in the common case where we
can correctly emit the function body twice? (Eg, that there are no computed
gotos in the function.)

Double emission is really something I'd rather avoid.

[zygoloid]

I'm also not especially happy about the idea of producing worse code for the
overwhelmingly common case of a call to the lambda's operator() in order to
support this corner of the language and ABI. We could try making the __impl
function always_inline, but even that feels a bit hackish.

Can you elaborate on what makes it worse? One more direct call forwarding
arguments? I don't plan to call any copy constructors, it would be a
"delegating" call like we have for vtable thunks.

We'd be adding an extra level of indirection to all calls to lambdas that use inalloca, where previously there was no indirection, and emitting an extra function definition for every lambda. At -O0 I expect these costs will not be negligible for lambda-heavy code.

Double emission is really something I'd rather avoid.

There would only be double emission if we actually use both the operator() and the conversion function for the lambda (or we use two different conversion functions with different calling conventions or something like that). I would expect that to be a very uncommon case.

When you say you want to avoid double emission, do you mean from a generated code point of view, or from a CodeGen complexity point of view?

[rnk]

I'll try double emission. I seem to recall that calling EmitFunctionBody on the same FD twice can lead to crashes, but maybe that's not true.

[nico]

Another instance: https://crbug.com/905310 (comment 4 has another reduction, similar to comment 1/2 here, but a bit different).

[rnk]

I think I started working on this last year, but other things took priority, so I never committed the code to diagnose this as an error instead of an assertion. I just did that in r348991.

Here's a test case to help think about this more fully:

struct Foo {
    Foo(const Foo &o);
    ~Foo();
    int x;
};
void take_cdecl(int (*fp)(int, Foo));
void take_std(int (__stdcall *fp)(int, Foo));
void take_fast(int (__fastcall *fp)(int, Foo));
int main() {
  auto l = [](int v, Foo o) {
    static int gv;
    return o.x + gv++ + v;
  };
  take_cdecl(l);
  take_std(l);
  take_fast(l);
}

While we're here, we might want to think more generally about how to implement MSVC's lambda function pointer decay extension, which makes the code above work. Notably, MSVC creates a separate gv for each decayed function pointer, so they definitely re-emit the function from scratch here. That seems like a bug, so let's not copy that behavior.

Here's what I'm thinking about doing. I think we should do the moral equivalent of the "__impl" desugaring for lambdas with "unforwardable" prototypes. An unforwardable prototype is a variadic prototype or an inalloca prototype. It's not really a desugaring, because I don't plan to change the AST. I just plan to generate a mangled name that looks as though we are emitting a static "__body" method in the lambda class. Both the operator() and the invoke helper will delegate to __body.

This approach should generalize to handle the non-default calling conventions and variadic methods as well. The __body implementation would take a va_list as the last parameter, and both operator() and __invoke would use va_start to pass in an argument list.

I think this addresses Richard's concerns about performance because this is a codegen-time decision that only affects the performance of "unforwardable" lambdas.

[rnk]

I spent yesterday and this morning working on this, but it is quite difficult, so I think I will leave it alone for the time being.

[rnk]

Dumping some thoughts after chatting with Richard about this at lunch:

• We like the idea of a "forwardable variant" GlobalDecl
• We would feed a "forwardable variant" GlobalDecl through our existing manglers similar to destructor variants today
• We'd need a new Itanium ABI mangling for forwardable variants. There, it could be named just "varags method variant" or something more specific, since it only needs to handle varargs, not inalloca. Clang would use the more general terminology internally.

[nico]

rnk made this a diag instead of an assert in r348991 (but the bug is still valid)