Clang Importer: import all indirect fields. #6531

Fruneau · 2017-01-03T08:51:49Z

Until now, only indirect fields that didn't belong to a C union (somewhere
between the field declaration and the type in which it is indirectly
exposed) were imported in swift. This patch tries to provide an approach
that allows all those fields to be exposed in swift. However, the downside
is that we introduce new intermediate fields and types.

The main idea here is that we can simply expose the anonymous
struct/unions from which the indirect types are taken and then use swift
computed properties to access the content of those anonymous
struct/unions. As a consequence, each time we encounter an anonymous
struct or union, we actually expose it at __Anonymous_field<id> (where id
is the index of the field in the structure). At this point, we use the
existing mechanism to expose the type as
__Unnamed_<struct|union>_<fieldname>. Then, each indirect field is exposed
as a computed property.

The C object:

 typedef union foo_t {
     struct {
         int a;
         int b;
     };
     int c;
 } foo_t;

Is imported as

 struct foo_t {
   struct __Unnamed_struct___Anonymous_field1 {
     var a : Int32
     var b : Int32
   }
   var __Anonymous_field1 : foo_t.__Unnamed_struct___Anonymous_field1
   var a : Int32 {
     get {
       return __Anonymous_field1.a
     }
     set(newValue) {
       __Anonymous_field1.a = newValue
     }
   }
   var b : Int32 {
     get {
       return __Anonymous_field1.b
     }
     set(newValue) {
       __Anonymous_field1.b = newValue
     }
   }
   var c : Int32
 }

This has the advantage to work for both struct and union, even in case we
have several nested anonymous struct/unions. This does not require to know
the size and/or the offset of the fields in the structures and thus can be
properly implemented using front-end data.

slavapestov · 2017-01-03T08:57:38Z

Excellent!

jrose-apple · 2017-01-04T00:54:05Z

Thanks for looking at this, @Fruneau! The reason it is the way it is today is because we wanted to avoid having to generate names for types, which can then show up in mangling and such. I'm concerned about tying names to declaration order. On the other hand, though, it's not too different from other kinds of "binary-compatible" changes you can make in C, like replacing one struct with another one that's layout-compatible. Hm.

cc @milseman

slavapestov · 2017-01-04T05:19:42Z

@jrose-apple We could ban users from referencing the types by name. If they don't appear in public declaration signatures they shouldn't get mangled either.

slavapestov · 2017-01-04T05:20:04Z

@swift-ci Please smoke test

Fruneau · 2017-01-04T07:57:40Z

@jrose-apple Thx for the feedback. I don't really think the patch is about generating type names, since the mechanism for generating type names was already in place (it was introduced in commit 0235967). This is more about renaming anonymous fields, which has the side effect of generating more type names since those fields were previously ignored.

slavapestov · 2017-01-04T07:58:37Z

It looks like this introduces a problem with duplicate members added somewhere?

/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/test/Interpreter/SDK/GLKit.swift:17:13: error: ambiguous use of 'x'
  print("<\(v.x) \(v.y) \(v.z) \(v.w)>")
            ^
GLKit._GLKVector4:27:16: note: found this candidate
    public var x: Float { get set }
               ^
GLKit._GLKVector4:4:16: note: found this candidate
    public var x: Float { get }

Fruneau · 2017-01-04T14:27:39Z

@slavapestov It looks like the GLKit worked-around the lack of indirect field exposition by adding swift extensions to some of their structures.

I don't know what the right approach is at this point. I can see several possibilities:

have some new mechanism to avoid importing a member if we detect it is added by an extension (don't know if this is really feasible). Maybe this would look like some kind of weak symbols (weak as in __attribute__((weak)), not as ARC's weak)?
make the feature opt-in (or opt-out, but that would require patching GLKit) using per-field attribute
hide the feature behind some "has_feature" or in a specific version (but again, that will require hiding patching the swift extension of GLKit)

jrose-apple · 2017-01-04T18:05:56Z

Since the GLKit overlay is also part of this repository, we can certainly just edit it if we get sign-off from some of the GLKit folks. I am worried about existing third-party libraries having done the same thing, though.

Another option would be to change overload resolution to allow overlays to shadow imported members, or to always prefer members in the original type over members in extensions. @rudkx, @DougGregor, any thoughts on those?

slavapestov · 2017-01-04T22:25:22Z

My preference is to fix the GLKit overlay and remove the workaround there. Another option is to only import indirect fields in Swift 4 mode.

I'm worried about introducing a one-off workaround to overload resolution to handle this case, it might be more trouble than it's worth.

slavapestov · 2017-01-04T22:27:09Z

We could also give the indirect members unique names, maybe with some kind of prefix.

But again, ugh... let's try to just fix GLKit and see if there is any additional fallout. We can always change the policy later.

Fruneau · 2017-01-05T07:53:01Z

Ho!

I just didn't see the overlay was part of the repository (I tried to grep GLKVector4 which has no match in the gyb file).

Fruneau · 2017-01-05T08:43:28Z

@swift-ci Please smoke test

CodaFi · 2017-01-05T08:47:01Z

@Fruneau Unfortunately, that requires privileges.

@swift-ci please smoke test.

DougGregor

This looks fantastic, thank you! Just a few minor concerns, then I'd like to get it tested and merged.

As for the GLKit overlay, I am absolutely fine with making the changes in the implementation of the overlay when we're getting the same API out of it. The overlay here is working around Clang importer limitations; we can fix the overlay when we remove the limitations.

It is possible that there are other sources like this overlay that will break with this change. Personally, I'd like to see if that happens in the wild before we go and add a Swift 4 version check + associated trickery here.

DougGregor · 2017-01-05T17:21:22Z

lib/ClangImporter/ImportDecl.cpp

@@ -2544,6 +2659,25 @@ namespace {

        auto VD = cast<VarDecl>(member);

+        if (isa<clang::IndirectFieldDecl>(nd) || decl->isUnion()) {
+          // Don't import unconditionnaly unavailable fields that have


Typo "unconditionnaly".

DougGregor · 2017-01-05T17:33:40Z

lib/ClangImporter/ImportDecl.cpp

+          if (isUnconditionallyUnavailable) {
+            continue;
+          }
+        }


This loop looks equivalent to VD->getAttrs().isUnavailable(VD->getASTContext()).

This is not strictly equivalent since using isUnavailable() will cause the fields to be hidden for any unavailability reason (not just unconditional unavailability).

Hmm... Can you at least pull this out into a separate static function, then? It's odd to have the loop inlined here.

I actually repushed with isUnavailable() since after a few manual tests it does not seem to affect error reporting (I was worried that the error message for accessing an unimported field would be different from the one we get for an unavailable field, but it does not seem to be the case).

Okay, thanks!

Until now, only indirect fields that didn't belong to a C union (somewhere between the field declaration and the type in which it is indirectly exposed) were imported in swift. This patch tries to provide an approach that allows all those fields to be exposed in swift. However, the downside is that we introduce new intermediate fields and types, some of the fields may already have been manually defined in type extension (as seen with the GLKit overlay). The main idea here is that we can simply expose the anonymous struct/unions from which the indirect types are taken and then use swift computed properties to access the content of those anonymous struct/unions. As a consequence, each time we encounter an anonymous struct or union, we actually expose it at __Anonymous_field<id> (where id is the index of the field in the structure). At this point, we use the existing mechanism to expose the type as __Unnamed_<struct|union>_<fieldname>. Then, each indirect field is exposed as a computed property. The C object: typedef union foo_t { struct { int a; int b; }; int c; } foo_t; Is imported as struct foo_t { struct __Unnamed_struct___Anonymous_field1 { var a : Int32 var b : Int32 } var __Anonymous_field1 : foo_t.__Unnamed_struct___Anonymous_field1 var a : Int32 { get { return __Anonymous_field1.a } set(newValue) { __Anonymous_field1.a = newValue } } var b : Int32 { get { return __Anonymous_field1.b } set(newValue) { __Anonymous_field1.b = newValue } } var c : Int32 } This has the advantage to work for both struct and union, even in case we have several nested anonymous struct/unions. This does not require to know the size and/or the offset of the fields in the structures and thus can be properly implemented using front-end data. Signed-off-by: Florent Bruneau <[email protected]>

jrose-apple · 2017-01-06T17:31:10Z

@swift-ci Please smoke test

slavapestov · 2017-01-07T01:34:01Z

stdlib/public/SDK/GLKit/GLKMath.swift.gyb

@@ -58,69 +48,15 @@ def defineSubscript(Type, limit):
 % for size in [2, 3, 4]:

 extension GLKMatrix${size} {
-  public typealias _Tuple = (${ ', '.join(['Float'] * (size * size)) })


Really great to see this gunk go away.

jrose-apple · 2017-01-07T02:04:33Z

Just waiting to see if @DougGregor has any more feedback.

DougGregor · 2017-01-09T23:00:59Z

Nope, I'm happy with it now. Thanks!

slavapestov · 2017-01-11T02:19:54Z

@Fruneau Do you mind creating a new PR to add an entry to CHANGLEOG.md as well? We have several new ClangImporter features that might affect users so I'm following up with PR authors about documenting them.

Following PR swiftlang#6531 there is a position mismatch in the final loop between the position of the member in the members arrays and the position in the valueParameters array. As a consequence, a structure containing indirect fields before a computed properties (like a bitfield) caused an invalid access in the valueParameters array resulting in a crash of the compiler. This patch maintains a separate position for accessing valueParameters. A non-regression test is also added. Signed-off-by: Florent Bruneau <[email protected]>

ClangImporter: fix crash when importing type containing bitfields (regression from #6531)

jrose-apple mentioned this pull request Jan 4, 2017

[SR-2402][ClangImporter] Import nullary function macros #6530

Closed

Fruneau force-pushed the indirect-fields branch from b174d79 to 71529ba Compare January 5, 2017 08:38

Fruneau force-pushed the indirect-fields branch from 71529ba to 1636745 Compare January 5, 2017 11:32

DougGregor requested changes Jan 5, 2017

View reviewed changes

Fruneau force-pushed the indirect-fields branch from 1636745 to 68bc423 Compare January 5, 2017 21:53

slavapestov reviewed Jan 7, 2017

View reviewed changes

DougGregor approved these changes Jan 9, 2017

View reviewed changes

DougGregor merged commit 2302f59 into swiftlang:master Jan 9, 2017

Fruneau mentioned this pull request Jan 14, 2017

ClangImporter: fix crash when importing type containing bitfields (regression from #6531) #6816

Merged

slavapestov added a commit that referenced this pull request Jan 15, 2017

Merge pull request #6816 from Fruneau/fix-pr6531

7e73113

ClangImporter: fix crash when importing type containing bitfields (regression from #6531)

Clang Importer: import all indirect fields. #6531

Clang Importer: import all indirect fields. #6531

Uh oh!

Conversation

Fruneau commented Jan 3, 2017

Uh oh!

slavapestov commented Jan 3, 2017

Uh oh!

jrose-apple commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 4, 2017

Uh oh!

Fruneau commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 4, 2017

Uh oh!

Fruneau commented Jan 4, 2017

Uh oh!

jrose-apple commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 4, 2017

Uh oh!

slavapestov commented Jan 4, 2017

Uh oh!

Fruneau commented Jan 5, 2017

Uh oh!

Fruneau commented Jan 5, 2017

Uh oh!

CodaFi commented Jan 5, 2017

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

DougGregor Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

Fruneau Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

DougGregor Jan 6, 2017

Choose a reason for hiding this comment

Uh oh!

Fruneau Jan 6, 2017

Choose a reason for hiding this comment

Uh oh!

DougGregor Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jan 6, 2017

Uh oh!

slavapestov Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jan 7, 2017

Uh oh!

DougGregor commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 11, 2017

Uh oh!

Uh oh!