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.

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

Jump to bottom

.byref always optimizes sink call into a bitwise memcopy if =sink is disabled #24327

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

.byref always optimizes sink call into a bitwise memcopy if =sink is disabled #24327

wants to merge 2 commits into from
+38 −5

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Oct 18, 2024 •
edited
Loading

closes #75
ref nim-lang/threading#79

Or only when =sink is disabled for byRef

Araq
Araq reviewed Oct 18, 2024
View reviewed changes
compiler/injectdestructors.nim Outdated
@@ -277,7 +277,8 @@ proc deepAliases(dest, ri: PNode): bool =
proc genSink(c: var Con; s: var Scope; dest, ri: PNode; flags: set[MoveOrCopyFlag] = {}): PNode =
if (c.inLoopCond == 0 and (isUnpackedTuple(dest) or IsDecl in flags or
(isAnalysableFieldAccess(dest, c.owner) and isFirstWrite(dest, c)))) or
isNoInit(dest) or IsReturn in flags:
isNoInit(dest) or IsReturn in flags or
tfByRef in dest.typ.flags:
Copy link
Member

Choose a reason for hiding this comment

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

I think the logic should be "it's .byref and also has its =sink disabled".

Copy link
Member Author

Choose a reason for hiding this comment

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

make sense

@ringabout ringabout changed the title .byref always optimizes sink call into a bitwise memcopy .byref always optimizes sink call into a bitwise memcopy if =sink is disabled Oct 19, 2024
@Araq
Copy link
Member

Araq commented Oct 19, 2024

This is still not correct as it can introduce leaks for code like var x = create(); x = create(). The idea here is that .byref types do not have any bitwise copies. We need to rethink this. It probably means the whole idea of =sink() .error is just wrong. =sink is fine and doesn't copy and shallow copies are prevented by .byref.

@darkestpigeon
Copy link

darkestpigeon commented Oct 21, 2024 •
edited
Loading

I usually use =copy() .error and =sink() .error for types that are used to communicate between multiple threads (perhaps some foreign) and the object needs to be pinned to a particular address. This both signals intent and prevents dumb errors (although moving an object by accident is rare). Will .byref cover this case correctly?

@Araq
Copy link
Member

Araq commented Oct 22, 2024

I think so, yes, but making =sink an .error just because you want to tie the object to a stack location is wrong regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem with the create* API of synchronization primitives
3 participants
@ringabout @Araq @darkestpigeon