Skip to content

[embedded] Fix miscompile in IRGen in offset computation of Builtin.destroyArray #73130

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

Merged

Conversation

kubamracek
Copy link
Contributor

@kubamracek kubamracek commented Apr 19, 2024

In Embedded Swift, IRGen emits the loop to destroy values in the array directly. Turns out that the GEP we use to index into the individual elements of the array's storage is using the value's size and not the stride in some cases, concretely in cases where the type is an enum with payloads. E.g. this

struct S: ~Copyable {
    deinit {
        print("S deinitialized!")
    }
}

enum Node {
    case inner(S)
    case leaf(Int)
}

ends up being represented as this type in LLVM IR:

%T1a4NodeO = type <{ [8 x i8], [1 x i8] }>

...on which GEP will not do the right thing we want (stride). Let's compute the offset directly via ptrtoint, add, inttoptr.

Will add tests, but wanted to check that this is the right approach first.

rdar://126386301

@kubamracek
Copy link
Contributor Author

@swift-ci please test

@eeckstein
Copy link
Contributor

Out of curiosity: why do we need to do something different in embedded swift for destroyArray? Can't we just use the typeinfo's destroyArray?

@eeckstein eeckstein requested a review from aschwaighofer April 19, 2024 06:15
@eeckstein
Copy link
Contributor

adding @aschwaighofer as reviewer

@eeckstein
Copy link
Contributor

Don't we have the same problem with the other array builtins, like copyArray?

Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

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

lgtm

@kubamracek
Copy link
Contributor Author

Out of curiosity: why do we need to do something different in embedded swift for destroyArray? Can't we just use the typeinfo's destroyArray?

That would emit a call to a runtime function (swift_arrayDestroy) which needs metadata and witness tables to be able to do the right destroy.

@kubamracek
Copy link
Contributor Author

It probably deserves a comment/explainer in the source code :)

@kubamracek
Copy link
Contributor Author

Added comments, applied the fix to the init/take/assign builtins too (@eeckstein you're right we needed it there too), added a testcase.

@kubamracek
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

thanks! lgtm

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.

3 participants