Skip to content
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

Perform value operations opaquely on ABI-inaccessible types #16615

Merged
merged 3 commits into from May 15, 2018

Conversation

rjmccall
Copy link
Member

@rjmccall rjmccall commented May 14, 2018

Track whether types are fixed-ABI and ABI-accessible at the SIL level.

Fixed ABI means that we can do value operations on the type without any metadata: value-allocations, copies, and destroys. It's currently equivalent to being fixed-size, but (1) being fixed-size isn't useful by itself at the SIL level and (2) you can imagine resilience or generics micro-optimizations where there's like an attribute that tells us the size of a type without actually telling us how to copy it. All types are fixed-ABI except:

  • layout-unconstrained generic types,
  • resilient value types, and
  • value types which contain a subobject of such a type (except within indirect enum cases).

ABI-accessible means that it's legal to perform value operations on the type. We cannot perform value operations on a type T if it's not fixed-ABI and we can't construct metadata for it in the current file. The latter will be true if T is private to a different file (in non-WMO builds) or internal to a different module because the metadata-accessor symbol will be private or hidden, respectively. Importantly, in these cases, it always illegal to use T directly in the current file, which means that SIL generated from that source code won't contain any direct value operations on T. However, we may be able use some type C that's itself permitted to use T and which is accessible to the current file. If C contains subobjects of type T, value operations on C will have to perform value operations on T recursively. As long as C is itself ABI-accessible, this can always be done provided we don't naively attempt to recursively expand a value operation on C into value operations on its subobjects.

(Note that we cannot simply treat C as an opaque type because we may need to perform operations on it that would not be legal on a totally opaque type, such as directly accessing a known-stored property. It is tempting to try to define that away by saying that we never use direct property access on such types and instead go through property accessors. However, this would require us to guarantee the emission of property accessors for properties, which we'd like to avoid. Also, it's not actually possible to define this problem completely, because the current file might define a type C2 which contains C as a subobject. The same recursive trouble applies to C2 as would apply to C, but we must be able to directly access the properties of C2 in order to define its constructors and accessors. Any model which can support directly accessing the properties of C2 can equally support it for C as well.)

The SIL optimizer currently never tries to expand value operations on objects in memory. (It does sometimes expand value operations on scalars, and this may be a problem for the opaque-values effort.) However, IRGen currently recursively expands value operations on non-resilient types. That is fixed as the final commit in this PR.

This recursive-expansion rule is not the only concern when dealing with ABI-inaccessible types. Most importantly, code must not be copied from other modules (e.g. by deserializing a shared function or inlining some other function) if:

  • it performs value operations on ABI-inaccessible types or
  • it requires type metadata for a type for which we cannot construct metadata in the current file.

The SIL verification that I've added here is definitely incomplete.

Fixes rdar://39763787 / SR-7549.

@rjmccall
Copy link
Member Author

@swift-ci Please test.

@rjmccall
Copy link
Member Author

@slavapestov @jrose-apple A review would be appreciated.

@xedin
Copy link
Member

xedin commented May 14, 2018

ABI-accessible means that we can perform value operations at all.

@rjmccall Did you mean "cannot"?

@rjmccall
Copy link
Member Author

@xedin No, that would be ABI-inaccessible. :)

@rjmccall
Copy link
Member Author

@jrose-apple I'm just asking for a review on the bit in SIL.cpp, although of course I won't complain about other comments.

@xedin
Copy link
Member

xedin commented May 14, 2018

@rjmccall Yeah, I figured :) It just that sentance reads like there is something missing...

@rjmccall
Copy link
Member Author

@xedin Good point. I've rewritten that entire paragraph; let me know if it's still unclear.

@xedin
Copy link
Member

xedin commented May 15, 2018

@rjmccall It's definitely much clearer now, thank you!

return false;

// Check whether the declaration is inaccessible from the current context.
switch (getDeclLinkage(decl)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a minor observation, but I feel that the FormalLinkage enum adds no value. It's only used in a couple of other places, and we should look at AST's access instead.

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 1840eb688b923dfb4ea2a360d8af855531f57f64

@rjmccall
Copy link
Member Author

A couple of sneaky i64s.

@rjmccall
Copy link
Member Author

@swift-ci Please test.

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - 1840eb688b923dfb4ea2a360d8af855531f57f64

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 1840eb688b923dfb4ea2a360d8af855531f57f64

@rjmccall rjmccall requested a review from jrose-apple May 15, 2018 01:54
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this looks reasonable to me. A few comments I noticed while reading through the whole thing, but nothing serious.



/// Is a lowered SIL type trivial? That is, are copies ultimately more
/// than just bit-copies, and it does it take any work to destroy?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: The second sentence describes conditions for being non-trivial, not trivial.

/// (such as copies, destroys, constructions, and projections) without
/// metadata?
///
/// Note that a type can be frozen (non-resilient) without being fixed-ABI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't quite settled on the use of "frozen" for structs yet, so it's probably better not to use this. It's also possible to have a non-frozen type that is nonetheless fixed-ABI: an @objc enum.

private enum PrivateEnum {
case first(First?)
case second(Second?)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding an internal test as well, since that gets its own case in your switch and will behave differently in the multi-file vs. multi-module tests.

Fixed-ABI means that we can do value operations on the type without
any metadata: value-allocations, copies, and destroys.  It's currently
equivalent to being fixed-size, but (1) being fixed-size isn't useful
by itself at the SIL level and (2) you can imagine resilience or generics
micro-optimizations where there's like an attribute that tells us the
size of a type without actually telling us how to copy it.  All types
are fixed-ABI except:
  - layout-unconstrained generic types,
  - resilient value types, and
  - value types which contain a subobject of such a type (except within
    indirect enum cases).

ABI-accessible means that we can perform value operations at all.
We might not be able to if the type is not fixed-ABI and it is private
to a different file (in non-WMO builds) or internal to a different
module, because in such cases we will not be able to access its metadata.
In general, we can't use such types `T` directly, but we may be able to
use types `C` that contain such types as subobjects.  Furthermore, we
may be reasonably exposed to SIL that performs operations that treat `C`
as non-opaque, e.g. if `C` is frozen (as it will be by default for
modules in Swift 5).  We can still achieve correctness in these cases
as long as we don't either:
  - inline code that contains value operations on `T` or
  - attempt to recursively expand a value operation on `T` into value
    operations on its subobjects.

The SIL optimizer currently never tries to expand value operations on
objects in memory.  However, IRGen always recursively expands value
operations on frozen types; that will be fixed in a follow-up patch.

The SIL verification that I've added here is definitely incomplete.
@rjmccall
Copy link
Member Author

@swift-ci Please smoke test.

@rjmccall
Copy link
Member Author

Rebased and incorporated Jordan's feedback.

huonw added a commit to huonw/swift that referenced this pull request May 23, 2018
This work-around is no longer needed now that the full fix landed in
apple#16615. The argument is left with a warning
to help with migration between compilers with the work-around and compilers with
the full fix (see also rdar://problem/40502379).

Fixes rdar://problem/40476573.
huonw added a commit to huonw/swift that referenced this pull request May 24, 2018
This work-around is no longer needed now that the full fix landed in
apple#16615. The argument is left with a warning
to help with migration between compilers with the work-around and compilers with
the full fix (see also rdar://problem/40502379).

Fixes rdar://problem/40476573.
kitasuke pushed a commit to kitasuke/swift that referenced this pull request Jun 9, 2018
This work-around is no longer needed now that the full fix landed in
apple#16615. The argument is left with a warning
to help with migration between compilers with the work-around and compilers with
the full fix (see also rdar://problem/40502379).

Fixes rdar://problem/40476573.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants