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

Double closures = wrong byref closure capture #882

Closed
fasterthanlime opened this issue Jul 7, 2015 · 3 comments
Closed

Double closures = wrong byref closure capture #882

fasterthanlime opened this issue Jul 7, 2015 · 3 comments
Milestone

Comments

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jul 7, 2015

Sample:

Gift: cover {
  msg: String
  pad: Int
}

please: func (f: Func) { f() }

main: func {
  g := ("One", 0)

  please(||
    g = ("Two", 0)
    please(||
      g = ("Three", 0)
    )
  )

  g msg println()
}

Expected behaviour

Print "Three"

Current behavior

Print "Two" and writes to a random memory location.. which is really really bad. C compiler only emits a warning (Gift* vs Gift**)

Explanation

Each closure is a struct of thunk (address of the actual C function) and context (optional pointer to context struct).

When calling closures, context needs to be created (on the stack). If a variable is assigned to in a closure, it's captured "by reference", which means we pass the address to the closure instead of the value.

Currently, when creating context, variables that are captured byref are always passed with &someVariable, even if they're in a closure that has itself captured them byref. That means the inner closure gets the address of the pointer to the address of the actual variable. Hence the random memory write.

@davidhesselbom
Copy link
Contributor

@davidhesselbom davidhesselbom commented Jul 7, 2015

Nice, need to see if this fixes #864.

Loading

@fasterthanlime
Copy link
Collaborator Author

@fasterthanlime fasterthanlime commented Jul 7, 2015

Hey @davidhesselbom! Hmm no, looks like a different issue but it shouldn't be a hard fix.

Sorry about being the worst maintainer ever in the history of open source projects btw, just doing some selfish bugfixes because our engine for Jaakan is written in ooc.

Loading

@fasterthanlime
Copy link
Collaborator Author

@fasterthanlime fasterthanlime commented Jul 7, 2015

@davidhesselbom well that wasn't hard at all :) props to @zhaihj for finding the erroneous comparison.

Loading

fasterthanlime added a commit that referenced this issue Jul 8, 2015
@fasterthanlime fasterthanlime added this to the 0.9.10 milestone Jul 10, 2015
@fasterthanlime fasterthanlime added this to the 0.9.10 milestone Jul 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants