Skip to content

[IRGen] [Builtin] Update zeroInitializer to support addresses. #32591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

zoecarver
Copy link
Contributor

@zoecarver zoecarver commented Jun 28, 2020

Adds support for addresses in Builtin.zeroInitializer. If an address is passed as the first (and only) argument, it will be zero'd out with a memset.

This will "fix" the C++ default constructor for address types.

The successor to #32530. Refs #32402.

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Jun 28, 2020
@zoecarver zoecarver requested review from jckarter and gribozavr June 28, 2020 17:56
Adds support for addresses in `Builtin.zeroInitializer`. If an address is passed as the first (and only) argument, it will be zero'd out with a memset.

This will "fix" the C++ default constructor for address types.
@zoecarver zoecarver force-pushed the cxx/memset-addr-init branch from e69550f to 6d86c17 Compare June 28, 2020 17:57
@gribozavr
Copy link
Contributor

gribozavr commented Jun 28, 2020

This will "fix" the C++ default constructor for address types.

I agree that this extension to zeroInitializer looks like a natural extension -- but could you elaborate about how it relates to C++ support? We can't always implement a no-arg constructor for a C++ type as a memset to zero -- we can only do that when that's equivalent to running constructors of subobjects in that type.

@zoecarver
Copy link
Contributor Author

but could you elaborate about how it relates to C++ support?

Here's an example:

struct Foo {
  int x = 42;
  Foo(Foo const&) { }
};
import Foo

public func test() {
  let f = Foo()
}
%2 = alloc_stack $Foo
%3 = builtin "zeroInitializer"<Foo>(%2 : $*Foo) : $()

Two things here. First, to directly address your question, (as seen above) we use zeroInitializer with an address when synthesizing the default constructor of an address only type. Second, you'll see that what we currently do creates a miscompile. I'd like to fix this issue but, I think that's a larger change that requires a few other PRs to land first. More on this issue below.

We can't always implement a no-arg constructor for a C++ type as a memset to zero

This is a good point. I should have put this in the summary but, I only intend for this to be a temporary solution. Right now we do the wrong thing. This is a smaller (and hopefully easier to commit) patch that makes it so other patches won't crash. I think we should always invoke the clang-generated constructor, that will fix this issue (and probably others that we don't know about). Once #30630 lands, I can implement that.

@gribozavr
Copy link
Contributor

I agree that this extension to zeroInitializer looks like a natural extension

Thinking more about it, maybe it is not that natural after all. For loadable types zeroInitializer is a pure function that returns a fresh value. But for address-only types it is a function with side-effects. These two cases look quite different to me. The code in lib/SIL/IR/ValueOwnership.cpp, for example, might care about the new semantics, although I don't exactly understand what it does.

I should have put this in the summary but, I only intend for this to be a temporary solution.

Given that the temporary solution requires introducing extra complexity in zeroInitializer that won't be needed in the long run, I'd recommend to either undo it when the real fix lands, or maybe better -- to ignore the incorrect behavior in no-arg initializers until then.

@zoecarver
Copy link
Contributor Author

or maybe better -- to ignore the incorrect behavior in no-arg initializers until then.

Yeah, that's not a bad idea. Just don't emit an initializer. For all the cases that this constructor is "correct" it would still have the same behavior as initializing a stack object in C++.

@gribozavr
Copy link
Contributor

Yeah, that's not a bad idea. Just don't emit an initializer. For all the cases that this constructor is "correct" it would still have the same behavior as initializing a stack object in C++.

Given that C++ interop is an experimental feature that I hope nobody uses yet, what you're describing sounds like an acceptable temporary state until we get a final fix.

@zoecarver
Copy link
Contributor Author

Given that C++ interop is an experimental feature that I hope nobody uses yet, what you're describing sounds like an acceptable temporary state until we get a final fix.

OK. I'm going to close this, then. I'll make another patch to simply remove the zeroInitializer call for non-trivially-copyable types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants