Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 15, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,14 +1133,18 @@ createValueConstructor(ClangImporter::Implementation &Impl,

// To keep DI happy, initialize stored properties before computed.
for (unsigned pass = 0; pass < 2; pass++) {
unsigned paramPos = 0;

for (unsigned i = 0, e = members.size(); i < e; i++) {
auto var = members[i];

if (var->hasClangNode() && isa<clang::IndirectFieldDecl>(var->getClangDecl()))
continue;

if (var->hasStorage() == (pass != 0))
if (var->hasStorage() == (pass != 0)) {
paramPos++;
continue;
}

// Construct left-hand side.
Expr *lhs = new (context) DeclRefExpr(selfDecl, DeclNameLoc(),
Expand All @@ -1149,12 +1153,14 @@ createValueConstructor(ClangImporter::Implementation &Impl,
/*Implicit=*/true);

// Construct right-hand side.
auto rhs = new (context) DeclRefExpr(valueParameters[i], DeclNameLoc(),
auto rhs = new (context) DeclRefExpr(valueParameters[paramPos],
DeclNameLoc(),
/*Implicit=*/true);

// Add assignment.
stmts.push_back(new (context) AssignExpr(lhs, SourceLoc(), rhs,
/*Implicit=*/true));
paramPos++;
}
}

Expand Down
16 changes: 16 additions & 0 deletions test/ClangImporter/Inputs/custom-modules/IndirectFields.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
struct StructWithIndirectField {
union {
int a;
int b;
};
int c;
int d : 3; /* Imported as a computed property */
};

union UnionWithIndirectField {
struct {
int a;
int b;
};
int c;
};
4 changes: 4 additions & 0 deletions test/ClangImporter/Inputs/custom-modules/module.map
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,3 +150,7 @@ module MacrosRedefA {
module MacrosRedefB {
header "MacrosRedefB.h"
}

module IndirectFields {
header "IndirectFields.h"
}
19 changes: 19 additions & 0 deletions test/ClangImporter/indirect_fields.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: %target-swift-frontend -typecheck -sdk "" -I %t -I %S/Inputs/custom-modules %s -verify

import IndirectFields

func build_struct(a: Int32, c: Int32, d: Int32) -> StructWithIndirectField {
return StructWithIndirectField(__Anonymous_field0: .init(a: a), c: c, d: d)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is making me think we're doing this wrong too; we'd really want something like StructWithIndirectField(a: a, c: c, d: d), right? But that seems much harder (and combinatoric, ultimately).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clearly, the best solution would be to have an init() per argument combination, but this can rapidly become a mess. Another approach, would be to simply use anonymous parameters in the initializer for fields corresponding to anonymous struct or union.

struct StructWithIndirectField {
    init(_ __Anonymous_field0: __Unnamed_union___Anonymous_field0, c: Int32, d: Int32)
}

StructWithIndirectField(.init(a: a), c: c, d: d)

This would also avoid exposing the generated field name.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not bad, but having a mix of named and nameless arguments is weird. I suppose we could try to trust that most C cases like this will put the transparent struct/union at the front or back of the structure?

@milseman, @DougGregor, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like the lesser of evils, unless there's any better ideas here.

}

func build_struct(b: Int32, c: Int32, d: Int32) -> StructWithIndirectField {
return StructWithIndirectField(__Anonymous_field0: .init(b: b), c: c, d: d)
}

func build_union(a: Int32, b: Int32) -> UnionWithIndirectField {
return UnionWithIndirectField(__Anonymous_field0: .init(a: a, b: b))
}

func build_union(c: Int32) -> UnionWithIndirectField {
return UnionWithIndirectField(c: c)
}