Skip to content

escape analysis broken with lent #14557

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

Closed
timotheecour opened this issue Jun 4, 2020 · 14 comments
Closed

escape analysis broken with lent #14557

timotheecour opened this issue Jun 4, 2020 · 14 comments

Comments

@timotheecour
Copy link
Member

escape analysis broken with lent

the lent analog of https://nim-lang.github.io/Nim/manual.html#procedures-var-return-type is unsafe:

Example

when true: # gitissue D20200603T234937:here
  type A = array[10,float]
  proc byLent*[T](a: T): lent T = a
  proc fn(): lent A =
    var a: A
    result = byLent(a) # bug: gives random stack data
    # result = a # ok: Error: 'a' escapes its stack frame

  proc main() =
    let s = fn()
    echo s
  main()

Current Output

random stack garbage

[nan, 3.952525166729972e-322, 6.953126947802652e-310, 6.953126947811348e-310, 6.953126947807395e-310, 2.153931747677107e-314, 6.953126947807395e-310, 2.1539271370565e-314, 0.0, 3.952525166729972e-322]

Expected Output

result = byLent(a) should proabably give: Error: 'a' escapes its stack frame.
escape analysis should track that byLent implicit address comes from a which is stack variable, and prevent leaking to result

Additional Information

devel 4301a3d 1.3.5

@timotheecour
Copy link
Member Author

just curious, is showstopper higher priority than high priority ?

@Araq
Copy link
Member

Araq commented Jun 4, 2020

Yes.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 4, 2020

I think we need some kind of annotation here, bc the convention (1st param escapes for var result from https://nim-lang.github.io/Nim/manual.html#procedures-var-return-type) doesn't translate well here.

is it time for memory tracking using from or similar ? (https://nim-lang.github.io/Nim/manual.html#var-return-type-future-directions)

  • proc foo(other: Y; container: var X): var T from container
  • proc foo(other: Y; container: X): lent T from container

I'm thinking of something like this could also be used to implement memory safe openArray/slices (data: ptr T from foo, n: int) (parsed as ptr[T from foo])
This would be a type annotated with a symbol to track where memory came from; here container could be allocated on heap or stack, but it'd give var T from container the same memory location as container

it may even help with #8463 (comment) to track that a memory location came from RODATA, to prevent SIGBUS at CT... and then this in turn could allow static const data (nim-lang/RFCs#126 (comment))

example

type Slice[T] = object
  ptr T
  n: int

var myGlobal: Slice[char]

proc fn =
  var buf: array[10, char]
  # fill buf
  let path = buf.sliced(2..5)
  doAssert type(path) is Slice[char from buf]
  let (dir, name, ext) = path.splitFile() # these are all pointing inside buf
  doAssert $ext == ".nim"
  var myLocal: Slice[char from buf]
  myLocal = dir # ok
  myGlobal = dir # Error: 'a' escapes its stack frame

@Araq
Copy link
Member

Araq commented Jun 4, 2020

I don't understand why it cannot work with the first parameter as var does. Your example doesn't require from.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 4, 2020

when true:
  type A = array[10,float]

  proc getByIndex(index: int): lent A =
    var a {.global.}: A
    var b {.global.}: A
    if index == 0: result = a else: result = b

  proc fn(index: int): lent A =
    result = getByIndex(index)

  proc main() =
    let s = fn(0)
    echo s
    echo fn(1)

  main()

@Araq
Copy link
Member

Araq commented Jun 4, 2020

So what? You cannot lent from all locations, it must be even more restrictive than var T. (There is a different way to handle this too, but this is all independent from the first parameter rule.)

@timotheecour
Copy link
Member Author

timotheecour commented Jun 4, 2020

could you provide more details? the code i gave in #14557 (comment) works today, is sound and should keep working, but the first parameter rule would make it not work IIUC.

also, down the line I really want to express something like this (but i can move this out to a separate thread):

when true:
  type A = array[10,float]
  type Slice[T,sym] = object
    n: int
    data: ptr[T, sym]

  proc slice[N,T] (a: array[N, T]): Slice[T,a]
    result.n = a.len
    result.data = a[0].safeAddr # safeAddr is a new magic that returns a ptr[T,sym]

  proc main: Slice[char] =
    var buf: array[10,char]
    var s = buf.slice
    var s2 = s[0..3]
    doAssert s[0].addr == s2[0].addr
    let mytup = (s, s2, "foo") # tuple[Slice[char, buf], Slice[char, buf], string]
    # return s2 # error
    return s2.dup # ok
  main()

there's obviously some hand waving there but the idea should be clear (safe slices that can be embedded in arbitrary types); the tracking is done by attaching a symbol in the type, and it should compose, eg: tuple[Slice[char, buf1], Slice[char, buf2], string]

is there a simpler way to express it?

Do you have anything in mind that is along those lines?

@Araq
Copy link
Member

Araq commented Jun 4, 2020

There is an RFC for more borrowing.

Anyhow, IMO proc byLent*[T](a: T): lent T = a (or rather its instantiation) shouldn't compile.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 4, 2020

ok how about the following, which doesn't change anything in the type system (no ptr[T,sym], no T from sym), but only uses a pragma to denote what can escape:

proc byLent*[T](a: T): lent T {.escapes: [a].} =
  a
proc byLent*[T](a: T, b: T, c: T): lent T {.escapes: [a, c].} =
  if cond: a else: c

proc byLent*[T](a: T): lent T {.escapes: [].} =
  someGlobal

=> this keeps all the analysis local
=> works with lent or var (and no need for https://nim-lang.github.io/Nim/manual.html#procedures-var-return-type if escapes: is specified)
=> memory safe

and it should be pretty close to existing code that does escape analysis

come to think of it, this might also be enough to enable the Slice[T] I was shooting for; the escapes pragma could also be auto-inferred or (unless proc is forward declared)...

@Araq
Copy link
Member

Araq commented Jun 4, 2020

Why can't we simply fix the bug and introduce more borrowing later?

@timotheecour
Copy link
Member Author

timotheecour commented Jun 4, 2020

as a temporary measure until something like escapes or equivalent is implemented, the rule could be:
a proc returning lent is assumed to return an address derived from 1st parameter; then a similar escape analysis as for var return is performed, so that the example in top post would become illegal because of assignment to result leaks 1st param a which is local

this would stay legal:

proc byLent*[T](a: T): lent T  = a
proc fn(a: T, b: T): lent T = # ditto with proc fn(a: var T, b: T): lent T
  var c: T
  # result = byLent(c) # good: CT error
  result = byLent(a) # ok
  # result = byLent(b) # bad: not ok, can only return address derived from 1st param

the downside is it would also prevent many useful valid cases eg #14557 (comment)

it's fine as a temporary measure.

I quite like {.escapes: [a, c].} though, as next step.

@Clyybber
Copy link
Contributor

Clyybber commented Jun 4, 2020

@timotheecour As Araq already said proc byLent*[T](a: T): lent T = a must not compile.
The correct version is proc byLent*[T](a: lent T): lent T = a. Maybe lent-inference can be introduced later, but thats a different topic.

@Araq
Copy link
Member

Araq commented Jun 4, 2020

Nah, lent T is not even valid for parameters. Currently the code generator uses pass-by-ref for the first parameter if the return type is lent. That means proc byLent*[T](a: T): lent T = a is safe. The error is somewhere else.

@Araq
Copy link
Member

Araq commented Jun 4, 2020

as a temporary measure until something like escapes or equivalent is implemented, the rule could be:
a proc returning lent is assumed to return an address derived from 1st parameter; then a similar escape analysis as for var return is performed,

Well yeah, that's what the compiler should have done to begin with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants