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

Prevent non-opaque structs from being used as behaviour parameters #3781

Merged
merged 2 commits into from
Jul 13, 2021

Conversation

ergl
Copy link
Member

@ergl ergl commented Jul 6, 2021

When an immutable object is sent across actors, the garbage collector
traces its associated object graph, using the type descriptor of the
root object. Structs don't have a type descriptor, so the GC can't trace
them.

An option is to track additional information for objects without type
descriptors, but this incurs in severe overhead for the normal case.

Another option, what this commit does, is to disallow non-opaque objects
without type descriptors to be the root of an object graph when used as
parameters to behaviours. Essentially, this means that one has to wrap
non-tag structs inside another object with a type descriptor. This also
applies to structs inside tuples, since the GC will try to trace each
element in the tuple.


Fixes #3765

@ergl ergl added do not merge This PR should not be merged at this time changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge labels Jul 6, 2021
@ergl ergl requested review from SeanTAllen and jemc July 6, 2021 20:46
@ponylang-main
Copy link
Contributor

Hi @ergl,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3781.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@ergl
Copy link
Member Author

ergl commented Jul 6, 2021

Labeled as do not merge until CodegenTraceTest.TraceStructField is fixed.

src/libponyc/verify/fun.c Outdated Show resolved Hide resolved
@ergl
Copy link
Member Author

ergl commented Jul 7, 2021

Looks like the windows failure was unrelated to this change

@ergl ergl removed the do not merge This PR should not be merged at this time label Jul 7, 2021
@ergl ergl force-pushed the ergl/fix_3765 branch from 7046e45 to 2525f9b Compare July 7, 2021 19:58
ergl added 2 commits July 7, 2021 22:18
When an immutable object is sent across actors, the garbage collector
traces its associated object graph, using the type descriptor of the
root object. Structs don't have a type descriptor, so the GC can't trace
them.

An option is to track additional information for objects without type
descriptors, but this incurs in severe overhead for the normal case.

Another option, what this commit does, is to disallow non-opaque objects
without type descriptors to be the root of an object graph when used as
parameters to behaviours. Essentially, this means that one has to wrap
non-tag structs inside another object with a type descriptor. This also
applies to structs inside tuples, since the GC will try to trace each
element in the tuple.
Verified that the modified test fails if we revert d8dfe3a
@ergl ergl force-pushed the ergl/fix_3765 branch from 2525f9b to a8d4dc1 Compare July 7, 2021 20:19
@jemc jemc merged commit 5cdb0c3 into main Jul 13, 2021
@jemc jemc deleted the ergl/fix_3765 branch July 13, 2021 18:13
github-actions bot pushed a commit that referenced this pull request Jul 13, 2021
github-actions bot pushed a commit that referenced this pull request Jul 13, 2021
@SeanTAllen SeanTAllen mentioned this pull request Sep 18, 2021
SeanTAllen added a commit that referenced this pull request Feb 5, 2022
With the merging of #3993, the changes from #3781 are no longer needed.
SeanTAllen added a commit that referenced this pull request Feb 5, 2022
…eters" (#3995)

The bug that [#3781](#3781) aka "prevent non-opaque structs from being used as behaviour parameters" was an attempt at preventing, as only partially solved by the change.

We've introduced a full fix in [#3993](#3993) that removes the need for [#3871](#3781).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault on Linux at runtime.
5 participants