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

Ability to mark a struct field as "pinned" in memory #3803

Closed
andrewrk opened this issue Nov 29, 2019 · 9 comments
Closed

Ability to mark a struct field as "pinned" in memory #3803

andrewrk opened this issue Nov 29, 2019 · 9 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Nov 29, 2019

This is @dbandstra's comment extracted into a separate issue.

I propose to use the keyword pinmem since it is 2 words smashed together lowercase, and therefore unlikely to alias anything useful.

Example code:

const std = @import("std");
const Allocator = std.mem.Allocator;

const FixedBufferAllocator = struct {
    pinmem allocator: Allocator,
    buf: []u8,
};
test "allocator pattern" {
    var bytes: [10]u8 = undefined;
    var fba = FixedBufferAllocator{
        .allocator = Allocator{
            .reallocFn = undefined, // not needed for this example
            .shrinkFn = undefined, // not needed for this example
        },
        .buf = &bytes,
    };
    const allocator = fba.allocator; // error: copy of memory-pinned field `allocator`
    const allocator = &fba.allocator; // OK
}

This would provide compile-time error protections to prevent against #591.

@fieldParentPtr would give a compile error if the field used did not have the pinmem attribute.

This would become significantly more useful with #2761 and #2765 implemented.

Another example (see related issue #172):

const S = struct {
    pinmem a: i32,
    b: *i32,
};
test "pinned field" {
    var s = S{
        .a = 1,
        .b = undefined,
    };
    s.b = &s.a;
    var s2 = s; // error: copy of memory-pinned field `a`
    var x = s.a; // error: copy of memory-pinned field `a`
    var x = (&.sa).*; // OK
}

This makes the language bigger and more complicated. One of Zig's goals is to be a small, simple language. So what justifies the change? It prevents footguns.

Accepted proposal #2414 moves the footgun of #591 to be caught by runtime safety. However this proposal further eradicates this possible mistake, by making it a compile error. From the Zen:

Runtime crashes are better than bugs.
Compile errors are better than runtime crashes.

Additional use cases:

prealloc_segment: [prealloc_item_count]T,

/// TODO copy elision / named return values so that the threads referencing *Loop
/// have the correct pointer value.
/// https://github.com/ziglang/zig/issues/2761 and https://github.com/ziglang/zig/issues/2765
pub fn init(self: *Loop) !void {

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Nov 29, 2019
@andrewrk andrewrk added this to the 0.6.0 milestone Nov 29, 2019
@ikskuh
Copy link
Contributor

ikskuh commented Nov 29, 2019

I think this is really beneficial for zig to have some kind of restriction on object locations. This prevents a whole class of errors related to "copying/moving stuff that is still referenced somewhere".

Maybe it could be useful to allow pinmem also on var declarations, but i'm not sure if this is beneficial enough to outweight the drawbacks of increased complexity.

But for struct fields its a great addition to safety

@JesseRMeyer
Copy link

This strikes me as related to Eric Lengyel's disjoint proposal for aliasing in C++.

http://terathon.com/blog/some-thoughts-about-aliasing-in-c/

@andrewrk
Copy link
Member Author

See also competing proposal #3804

@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Nov 30, 2019
@mikdusan
Copy link
Member

mikdusan commented Dec 1, 2019

IRC sparked this idea:

const FixedBufferAllocator = struct {
    allocator: &Allocator,
    buf: []u8,
};
  • still embedded storage for allocator
  • still pinned memory and FixedBufferAllocator is not copyable
  • const allocator = fba.allocator is now legal (lhs type is *Allocator)
  • const copy = fba.allocator.* is legal because explicit
  • & indicates "as pointer"
  • @typeOf(fba.allocator) == "*Allocator

@pixelherodev
Copy link
Contributor

That's basically just a different way of saying pinmem, right?

e.g. pinmem allocator: Allocator == allocator: &Allocator?

@mikdusan
Copy link
Member

mikdusan commented Dec 4, 2019

That's basically just a different way of saying pinmem, right?

e.g. pinmem allocator: Allocator == allocator: &Allocator?

1 difference. today we have this pattern which works because we explicitly ask for address of pinned memory:

    /// Implementation of io.OutStream trait for File
    pub const OutStream = struct {
        file: File,
        stream: Stream,
        ...
    };
    ...
    var file_stream = self.in_file.inStream();
    const in = &file_stream.stream;

but with field decl "as pointer" modifier we decorate the field, and now a "copy" of the field stream results in a pointer... @typeOf(file_stream.stream) == *std.io.OutStream

 /// Implementation of io.OutStream trait for File
 pub const OutStream = struct {
     file: File,
-    stream: Stream,
+    stream: &Stream,
     ...
 };
 ...
 var file_stream = self.in_file.inStream();
-const in = &file_stream.stream;
+const in = file_stream.stream;

@LemonBoy
Copy link
Contributor

What about a zero-sized type Pinned that prevents copying instances of types containing this "tag"?

For example with such a type Allocator/Random can be made non-copiable and that would solve some of the problems experienced by the users. One big advantage of this proposal is that no new syntax is needed.

@Rocknest
Copy link
Contributor

Rocknest commented Sep 20, 2020

One problem with marking 'pinmem' types with & prefix is that it blends with pointers or address of. But keyword pinmem also has problems - it is on the other side of the type. Therefore i think its better to make a builtin:

const FixedBufferAllocator = struct {
    allocator: @Pin(Allocator),
    buf: []u8,
};

That would make a field not trivially copyable (same as in #3803 (comment)). However copy by dereferencing or memcopy would still be possible.

Edit: @LemonBoy Yep im thinking the same. We can make the interface itself pinned const Allocator = @Pin(struct {...});
One downside that i see with zero-sized type as pinned struct marker is getting this information at comptime - its better to have this as a property of the struct/union itself, and not depend on some field. So i prefer pinned struct {..} or @Pin(struct {..}) and retrieve this property at comptime: @typeInfo(@Pin(struct {..})).Struct.is_pinned

@SpexGuy
Copy link
Contributor

SpexGuy commented Jan 13, 2021

Closing this in favor of #7769. Putting pinned on values would mean introducing a new CV qualifier on pointers in general. Putting it on types should be enough granularity, though we could reconsider if use cases are brought forth.

@SpexGuy SpexGuy closed this as completed Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

8 participants