carp-lang / Carp

A statically typed lisp, without a GC, for real-time applications.
Apache License 2.0
5.47k stars 173 forks source link

fix: don't allow lambdas to leak captures #1440

Open scolsen opened 1 year ago

scolsen commented 1 year ago

When lambdas close over some external environment variable, if that variable is a linear value, their environment becomes the owner of the captured value and the value will be freed with the environment. If the lambda moves this variable out of its own scope, however, e.g. by returning it in its body, both the caller and the lambda environment will wind up owning the value, resulting in double frees.

For now, we prevent this scenario by simply disallowing functions to leak variables captured from another scope. Doing so is now an error. Of course, one can still copy these values without issue.

We achieve this through set operations. If the memory state loses the fake deleters for the set of a lambdas captured bindings after its body is evaluated, we've encountered an ownership leak, and report an error.

(defn example []
  (let [capture @""
        lambda (fn [] capture)])) ;; this is now an error

(defn example []
  (let [capture @""
        lambda (fn [] @&capture)])) ;; this is still OK

fixes #1040

scolsen commented 1 year ago

Note: here's an example of what the error currently looks like:

The function (defn <s : String> _Lambda_foo_12_env [_env] s) gives away the captured variables: s. Functions must keep ownership of variables captured from another environment. at REPL:2:2.

Traceback:
  (defn foo [] (let [s (copy "") f (fn [] s)] (f))) at REPL:2:1.

fairly unhelpful function name b/c we check this after the lambda has been lifted. I'll tackle this as a later improvement. I gotta add an error test as well.

scolsen commented 1 year ago

op, looks like there are some regressions I'll have to take care of first

scolsen commented 1 year ago

Note: MacOS failures are due to new github runner images which have a new version of clang that throws a waring that our generated C has triggered. I'll open a separate PR to fix this.

scolsen commented 1 year ago

oh no... looks like the clang version diffs on different platforms are leading to problems. Linux and Nix CI images are using older clang versions which means the recent macOS CI fix breaks them.

I guess we need to fix this on the CI side instead of baking in warning flag options into default Carp projects.

eriksvedang commented 1 year ago

Dang... perhaps the cleanest thing would be to check for clang version in dynamic carp and add the warning flag there (while loading core) if it is needed. Or can you think of an easier way?

scolsen commented 1 year ago

Dang... perhaps the cleanest thing would be to check for clang version in dynamic carp and add the warning flag there (while loading core) if it is needed. Or can you think of an easier way?

It looks like github has other images of linux at least that have later versions of clang, so we might be able to fix our CI issues just by figuring out how to use those images instead.

As for the more general problem of default project settings--checking the compiler version dynamically seems like a good idea.

The ultimate fix would be to make our generated code -Wall compliant for at least clang since we use that as a default. Maybe we're better off just spending time on that and removing the -W-no warning disable flags?

At the moment, it seems like the only thing we'd have to implement is some check in the emitter that drops unused variables and remove some self-assigns. Not sure how much work that will be.

Another hacky fix would be to remove the -Werror flag from the project during CI runs, which we could probably do in the tests/ script.