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

proposal: x/sys: add unsafe mmap #56123

Open
piotr-sneller opened this issue Oct 10, 2022 · 18 comments · May be fixed by golang/sys#197
Open

proposal: x/sys: add unsafe mmap #56123

piotr-sneller opened this issue Oct 10, 2022 · 18 comments · May be fixed by golang/sys#197

Comments

@piotr-sneller
Copy link

The current implementation of the Mmap wrappers for the Unix-derived OSes does not dispatch directly to the relevant syscall but relies on the mmapper layer. The mmapper adds address range validation and returns EINVAL for the ranges not matching the content of the mapping registry. While it might be arguably correct for most of the use cases, it also prevents partial Munmaps and similar valid parts of the pretty rich mmap semantics. I suggest that the existing implementation should be left as-is, but undisturbed access to the underlying OS primitives should also be provided. The scope of the extension would be minimal, as the low-level part already is there -- the package should just export more entry points. The naming details are irrelevant to me, so I leave them unspecified: I will be equally happy with MmapUnsafe as with MmapRaw.

The only suggestion I'd make is the return type: the functions should operate with uintptr, as their WinAPI VirtualAlloc() counterparts do. Wrapping the uintptr in a slice just to unwrap it in the follow-up step to recover the uintptr adds no value. I'll make the slice when I'm done.

@gopherbot gopherbot added this to the Proposal milestone Oct 10, 2022
@ianlancetaylor
Copy link
Contributor

Can you write down the exact new API that you propose adding? Thanks.

@ncruces
Copy link
Contributor

ncruces commented May 27, 2024

I need this in github.com/ncruces/go-sqlite3.

I'm currently go:linknaming mmap (I know, I'm sorry!) because I need unix.MAP_FIXED (which is unusable with unix.Mmap as is). This is understandably discouraged, and on a path to being curtailed in #67401.

I know my package is pretty much irrelevant, but exposing the autogenerated bits that allow mmap to be called from a bunch of different Unixes would help. For example, I have no chance to test z/OS, but IBM is incentivized to add a portable version to x/sys/unix.

Hence, answering @ianlancetaylor, I'd like to propose x/sys/unix exports:

func MmapPtr(addr uintptr, length uintptr, prot int, flag int, fd int, pos int64) (ret uintptr, err error)
func MunmapPtr(addr uintptr, length uintptr) (err error)

And maybe, for the platforms that support it:

func MremapPtr(oldaddr uintptr, oldsize uintptr, newaddr uintptr, newsize uintptr, flags int) (ret uintptr, err error)

These match the portability layer already in x/sys/unix.

@database64128
Copy link
Contributor

Related discussion: #58625

@rsc
Copy link
Contributor

rsc commented May 29, 2024

It seems like the pointers should at least be pointers. What about:

func MmapPtr(addr unsafe.Pointer, length uintptr, prot int, flag int, fd int, pos int64) (ret unsafe.Pointer, err error)
func MunmapPtr(addr unsafe.Pointer, length uintptr) (err error)
func MremapPtr(oldaddr unsafe.Pointer, oldsize uintptr, newaddr unsafe.Pointer, newsize uintptr, flags int) (ret unsafe.Pointer, err error)

?

@ncruces
Copy link
Contributor

ncruces commented May 29, 2024

I don't know that I'm qualified to answer that.

I'll just say this is not Go managed memory, and as said above x/sys/windows.VirtualAlloc uses uintptr.

That was my reasoning, but I'm happy to be overridden by someone more knowledgeable. 🙂

@aclements
Copy link
Member

In general, the unix syscall packages try to be as type-safe as possible, so using unsafe.Pointer would force code to talk about these pointers as pointers, and also to use unsafe to get at these unsafe APIs. I don't think it's a strong requirement, but I think it's nice to have. The Windows syscall packages tend to follow the Windows C APIs more literally and are much more lax about safely wrapping them, so I wouldn't put much weight on VirtualAlloc's signature.

@ncruces
Copy link
Contributor

ncruces commented May 30, 2024

If this is a likely approve, I don't minding submitting a CL myself.
Whatever is easiest for the Go team.

@rsc rsc changed the title proposal: x/sys: Add the "unsafe" mmap() family of functions proposal: x/sys: add unsafe mmap May 30, 2024
@rsc
Copy link
Contributor

rsc commented May 30, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 5, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add:

func MmapPtr(addr unsafe.Pointer, length uintptr, prot int, flag int, fd int, pos int64) (ret unsafe.Pointer, err error)
func MunmapPtr(addr unsafe.Pointer, length uintptr) (err error)
func MremapPtr(oldaddr unsafe.Pointer, oldsize uintptr, newaddr unsafe.Pointer, newsize uintptr, flags int) (ret unsafe.Pointer, err error)

@rsc
Copy link
Contributor

rsc commented Jun 12, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add:

func MmapPtr(addr unsafe.Pointer, length uintptr, prot int, flag int, fd int, pos int64) (ret unsafe.Pointer, err error)
func MunmapPtr(addr unsafe.Pointer, length uintptr) (err error)
func MremapPtr(oldaddr unsafe.Pointer, oldsize uintptr, newaddr unsafe.Pointer, newsize uintptr, flags int) (ret unsafe.Pointer, err error)

@ncruces
Copy link
Contributor

ncruces commented Jun 13, 2024

While trying to implement this a few nits came up.
Sorry to bring these up late in the process.


For unix.MmapPtr we're following a parameter order consistent with mmap syscall:

func MmapPtr(addr unsafe.Pointer, length uintptr, prot int, flag int, fd int, pos int64) (ret unsafe.Pointer, err error)

But unix.Mmap follows a different order (fd and offset come first):

func Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error)

Should we be consistent with the syscall, or with Go? Both work for me.

I also see no reason to use pos instead of offset, and flag instead of flags.

Final nit, should length be uintptr or int?


For unix.MremapPtr we're not following an order consistent with the Linux mremap syscall, but rather with the NetBSD syscall:

func MremapPtr(oldaddr unsafe.Pointer, oldsize uintptr, newaddr unsafe.Pointer, newsize uintptr, flags int) (ret unsafe.Pointer, err error)

Is this intentional?

I'd also use camel case for the arguments, but that's less important.

@ncruces ncruces linked a pull request Jun 13, 2024 that will close this issue
@ncruces
Copy link
Contributor

ncruces commented Jun 13, 2024

If it helps I've tested golang/sys#197 with my package and it works fine: ncruces/go-sqlite3@main...mmap

ncruces added a commit to ncruces/go-sys that referenced this issue Jun 13, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/592415 mentions this issue: x/sys: add unsafe mmap, munmap, mremap

@ianlancetaylor
Copy link
Contributor

Since MmapPtr is intended to exactly match the system call, I think we should keep the conventional argument ordering, which I believe matches the current proposal.

The length argument should be uintptr as that matches the C size_t. It's OK that it's not the same as the Mmap method.

MremapPtr is a more complicated case. It looks like Linux and NetBSD disagree on the argument order for the C function. Darwin, OpenBSD, DragonFLY, AIX, and Solaris apparently don't have mremap. FreeBSD matches NetBSD.

I think that the NetBSD argument order makes more sense, so let's stick with that. Fortunately the compiler will catch any confusion.

Thanks for raising these issues.

@ncruces
Copy link
Contributor

ncruces commented Jun 13, 2024

Thanks @ianlancetaylor. I can add tests after the proposal is accepted.
PR was mostly meant to flesh out these API nits.

@rsc
Copy link
Contributor

rsc commented Jun 20, 2024

It does seem like if you are editing code to update from unix.Mmap to unix.MmapPtr you should not have to permute the existing arguments. That suggest we should change the MmapPtr list. The current Mmap is:

func Mmap(fd int, offset int64, length int, prot int, flags int) (data []byte, err error)

We need to add addr, which maybe makes sense next to length, and length should be a uintptr now (arguably), so

func MmapPtr(fd int, offset int64, addr unsafe.Pointer, length int uintptr, prot int, flags int) (data []byte unsafe.Pointer, err error)

is the minimal change that will make sense if you are editing.

Clearly I just copied the system call, but unix.Mmap and unix.MmapPtr will be next to each other in docs and should be consistent.

Similarly comparing:

func Mremap(oldData []byte, newLength int, flags int) (data []byte, err error)
func MremapPtr(oldaddr unsafe.Pointer, oldsize uintptr, newaddr unsafe.Pointer, newsize uintptr, flags int) (ret unsafe.Pointer, err error)

This looks right from a parallelism point of view.

@ncruces
Copy link
Contributor

ncruces commented Jun 20, 2024

Either decision is fine by me.

@ianlancetaylor
Copy link
Contributor

@ncruces Go with @rsc 's comment. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Likely Accept
Development

Successfully merging a pull request may close this issue.

7 participants