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

Nested lambdas don't clone properly. #725

Open
ZekeMedley opened this issue Jan 6, 2020 · 1 comment · May be fixed by #735
Open

Nested lambdas don't clone properly. #725

ZekeMedley opened this issue Jan 6, 2020 · 1 comment · May be fixed by #735

Comments

@ZekeMedley
Copy link
Member

@ZekeMedley ZekeMedley commented Jan 6, 2020

Running the code below triggers an error internal error: Attempted to clone an id ('f') with no associated value.

event zeek_init()
        {
        local outer = 100;

        local lambda = function()
        {
                # `f` is detected as an outer id incorrectly.
                local inner = function(a: count, b: count, c: count, d: count, e: count, f: count)
                {
                        print outer + f;
                };
                inner(1, 2, 3, 4, 5, 6);
        };
        lambda();


        local copyLambda = copy(lambda);
        copyLambda();
        }

The trouble happens because when lambda is created it incorrectly detects f as an outer ID and therefore a variable in its closure.

In the above code the variable f has an offset of 5 in inner's frame (its the 6th variable declared - 5th index). So, when lambda is cloned, because it has incorrectly decided that f is in its closure, it attempts to clone the 6th value in its closure which triggers the error.

Using the variable a instead (print outer + a) does not cause the error, but likely causes a memory leak as when lambda is cloned it will clone the value with offset 0 in its closure twice. Once for a and once for outer.

The solution is likely to modify the logic in OuterIDBindingFinder so that it ignores variables which are local to nested lambdas.

zeek/src/Var.cc

Lines 413 to 422 in 7b9a27c

class OuterIDBindingFinder : public TraversalCallback {
public:
OuterIDBindingFinder(Scope* s)
: scope(s) { }
virtual TraversalCode PreExpr(const Expr*);
Scope* scope;
vector<const NameExpr*> outer_id_references;
};

@jsiwek

This comment has been minimized.

Copy link
Member

@jsiwek jsiwek commented Jan 6, 2020

Thanks for taking a look and confirming/creating this issue. Note that with the merge of #722 I had some prototype code to fix the OuterIDBindingFinder:

zeek/src/Var.cc

Lines 413 to 441 in 0cde6d2

class OuterIDBindingFinder : public TraversalCallback {
public:
OuterIDBindingFinder(Scope* s)
: scope(s) { }
virtual TraversalCode PreExpr(const Expr*);
virtual TraversalCode PostExpr(const Expr*);
Scope* scope;
vector<const NameExpr*> outer_id_references;
int lambda_depth = 0;
// Note: think we really ought to toggle this to false to prevent
// considering locals within inner-lambdas as "outer", but other logic
// for "selective cloning" and locating IDs in the closure chain may
// depend on current behavior and also needs to be changed.
bool search_inner_lambdas = true;
};
TraversalCode OuterIDBindingFinder::PreExpr(const Expr* expr)
{
if ( expr->Tag() == EXPR_LAMBDA )
++lambda_depth;
if ( lambda_depth > 0 && ! search_inner_lambdas )
// Don't inspect the bodies of inner lambdas as they will have their
// own traversal to find outer IDs and we don't want to detect
// references to local IDs inside and accidentally treat them as
// "outer" since they can't be found in current scope.
return TC_CONTINUE;

But toggling that search_inner_lambdas off led to some further cascade of memory/ref-counting/test-failure issues and wasn't sure how deep the rabbit hole would go -- nice to have your help looking at this.

@jsiwek jsiwek mentioned this issue Jan 6, 2020
0 of 25 tasks complete
@rsmmr rsmmr added this to the 3.1.0 milestone Jan 9, 2020
@jsiwek jsiwek linked a pull request that will close this issue Jan 10, 2020
@rsmmr rsmmr added this to Unassigned / Todo in Release 3.1.0 via automation Jan 14, 2020
@rsmmr rsmmr moved this from Unassigned / Todo to Assigned / In Progress in Release 3.1.0 Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release 3.1.0
  
Assigned / In Progress
3 participants
You can’t perform that action at this time.