[cxx-interop] Fix linker errors when using std-vector.

[zoecarver]

Adds tests for using std-vector and some other interesting types.

This patch fixes four mis conceptions that the compiler was previously making:

1. Implicit destructors have no side effects. (Yes, this means we were not cleaning up some objects.)
2. Implicit destructors have bodies. (Technically they do, but the body doesn't include CallExprs that they make when lowered to IR.)
3. Functions other than methods can be uninstantiated templates.
4. Uninstantiated templates may have executable code. (I.e., we can never take the fast path.)

[zoecarver]

CC @plotfi

[zoecarver]

@guitard0g please verify this fixes the issues using std::functional and then add a test for that similar to my std::vector test.

[zoecarver]

@swift-ci please smoke test.

[plotfi]

@zoecarver Does this drop the need for PR #39551 ?

[plotfi]

I believe this is where I was encountering the operator() for the std::functional that wasn't getting added to the stack.

[plotfi]

Ah I see, so this gets to the dtors from the VarDecl's type rather than the ctors it already visited?

[zoecarver]

Yes. The ctors aren't really important here; they weren't causing problems. We might need to destroy something that we never constructed. Think about it like this: what might cause us to call a destructor? Well, if we have an instance of a C++ type with a dtor, we will have to call its dtor. If that C++ type has bases/fields, same thing. To fix these issues, I look at all the places we might call a dtor, and make we tell clang to generate that symbol.

[plotfi]

Good approach. Can you add this as context in a comment on this VarDecl visitor?

[zoecarver]

@zoecarver Does this drop the need for PR #39551 ?

Yes, I think so.

I'm sorry I wasn't able to review that patch more thoroughly, but I think this patch is a better solution. In that patch you are essentially walking the whole AST tree. It happens that your constructors lead you to the right place, but that is not guaranteed. I think many of these tests cases will fail with your patch, and it won't help the following use case: func test(v: std.vector) { ... } where we need to invoke the destructor on v, but we never call the constructor. This is why it's important that we look at the destructor for all VarDecls (including parameters).

[zoecarver]

@swift-ci please smoke test.

[zoecarver]

@swift-ci please smoke test.

[plotfi]

@zoecarver Does this drop the need for PR #39551 ?

Yes, I think so.

I'm sorry I wasn't able to review that patch more thoroughly, but I think this patch is a better solution. In that patch you are essentially walking the whole AST tree. It happens that your constructors lead you to the right place, but that is not guaranteed. I think many of these tests cases will fail with your patch, and it won't help the following use case: func test(v: std.vector) { ... } where we need to invoke the destructor on v, but we never call the constructor. This is why it's important that we look at the destructor for all VarDecls (including parameters).

Sounds good. I agree I like this patch a lot more, was going about tracking the Decls by walking from the ctors the entire depth is not great. I tested your patch out and it works too; will take a look at default template params for some of the other containers.

[plotfi]

@zoecarver Does this drop the need for PR #39551 ?

Yes, I think so.

I'm sorry I wasn't able to review that patch more thoroughly, but I think this patch is a better solution. In that patch you are essentially walking the whole AST tree. It happens that your constructors lead you to the right place, but that is not guaranteed. I think many of these tests cases will fail with your patch, and it won't help the following use case: func test(v: std.vector) { ... } where we need to invoke the destructor on v, but we never call the constructor. This is why it's important that we look at the destructor for all VarDecls (including parameters).

No worries BTW! :-)

[zoecarver]

@swift-ci please smoke test.

[zoecarver]

@swift-ci please smoke test Linux.