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

Added memSafe & memUnsafe #16371

Closed
wants to merge 12 commits into from
Closed

Added memSafe & memUnsafe #16371

wants to merge 12 commits into from

Conversation

Clonkk
Copy link
Contributor

@Clonkk Clonkk commented Dec 16, 2020

This is a draft implementation very loosely related to nim-lang/RFCs#302 and issues talked about in https://forum.nim-lang.org/t/7179 (as in it's about memory safety flag).

This only reflects my vision of what a good "unsafe" solution could be in Nim.

I'm not familiar with Nim's compiler, so maybe the way I did it is wrong. Maybe it will have unintended consequences.

I have some elementary tests in LOCAL_TEST_TO_REMOVE (not all are ok), haven't taken the time to refactor it properly into Nim's tests suite, for now it's just something to run locally for me.

MemUnsafe ideally should track :

  • addr
  • unsafeAddr
  • cast
  • ffi through importc

Tracking pointer dereference could be nice as well.

It is by no means an exhaustive list of "unsafe" memory operation; just the most common one.

I haven't found where derefenrecing happens in pointer. I noticed nkDeref, nkHiddenDeref, but I need to undertand it more
Error message needs to be improved.

I'm not sure if it will go anywhere but it is a fun experiment.

@Clonkk Clonkk marked this pull request as draft December 16, 2020 13:33
@juancarlospaco
Copy link
Collaborator

importjs and importcpp should be on the tests too.

@Clonkk
Copy link
Contributor Author

Clonkk commented Dec 17, 2020

Good point !

@juancarlospaco
Copy link
Collaborator

Rename MemUnsafe ➡️ unsafeMem, theres a lot of stuff using unsafe* on stdlib is kinda a convention-ish,
and makes all the unsafe stuff appear together on search, docs, autocompletions, etc.

@Clonkk
Copy link
Contributor Author

Clonkk commented Dec 18, 2020

I used MemUnsafe / MemSafe to be coherent with gcSafe since it was a flag, but it's true that in the lib folder everything is unsafe*.

Not sure which is best, I might switch later (i'll try to have the expected results first)

@snej
Copy link

snej commented Jan 4, 2021

Cool! Sorry I'm just noticing this. @Clonkk, could you describe briefly what this PR does? It sounds like it adds a memUnsafe effect?

@Clonkk
Copy link
Contributor Author

Clonkk commented Jan 4, 2021

I basically copied how gcSave worked by marking "dangerous" operation as memUnsafe.
This flag does absolutly nothing unless you mark a proc memSafe, in which case if both memSafe and memUnsafe are present it will prevent compilation. In the very same way you can use ``cast(gcSafe)

This is done at in the proc scope and so far I managed to track as unsafe :

  • cast
  • explicit dereferencing []
  • FFI (importc, importcpp, importjs but i've only tested importc so far)
  • addr, unsafeAddr

Example of thing that do not compile :

proc xx(x: int) : float =
  result = cast[float](x)

# Does not compile : xx is unsafe (cast) and yy is marked as safe
proc yy() {.memSafe.} =
  var x = 123456789
  echo xx(x)

But if you know what you're doing then you can do this :

proc xx(x: var string) =
  echo repr(addr(x))

# OK
proc yy() {.memSafe.}=
  var y = "yy"
  echo y
  # Force xx to be considered memory safe 
  {.cast(memSafe).}:
    xx(y)

Since it's an effect it also propagate through API and proc call (like a tag would):

proc xx() {.memUnsafe.} =
  echo "xx"

# yy will be marked as unsafe because it call xx
proc yy() =
  echo "yy"
  xx()

# Will not compile 
# zz is unsafe because it calls yy which is unsafe
proc zz() {.memSafe.} =
  echo "zz"
  yy()

The benefit of a side effect over a tag is that you can add it to keyword (cast) and pragma (emit, importc) that are dangerous and I'm not sure a tag would do that (and if not, it would rely on the developer marking its proc unsafe himself which is unreliable).

I wasn't able to find how to track hidden deref of pointers through dot operator for now though.

@Araq
Copy link
Member

Araq commented Jan 7, 2021

The implementation is definitely on the right track, the design is not backed up by an accepted RFC, but you know this.

@Clonkk
Copy link
Contributor Author

Clonkk commented Jan 12, 2021

I created the corresponding RFC nim-lang/RFCs#314

@Clonkk
Copy link
Contributor Author

Clonkk commented Mar 23, 2021

Closing afer RFC discussion which points to other solutions being better

@Clonkk Clonkk closed this Mar 23, 2021
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.

4 participants