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.

Sign up for GitHub

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

Jump to bottom

Missed forwarding-cycle detection #13631

Closed
vasslitvinov opened this issue Aug 2, 2019 · 0 comments · Fixed by #14026
Closed

Missed forwarding-cycle detection #13631

vasslitvinov opened this issue Aug 2, 2019 · 0 comments · Fixed by #14026

Comments

@vasslitvinov
Copy link
Member

When switching the field type to nilable for data fields in A and B, the compiler no longer finds a forwarding cycle here:

// test/classes/forwarding/forwarding-cycle.chpl

class A {
  forwarding var data: unmanaged B;
}
class B {
  forwarding var data: unmanaged A;
}
@vasslitvinov vasslitvinov mentioned this issue Aug 2, 2019
Closed
37 tasks
vasslitvinov added a commit to vasslitvinov/chapel that referenced this issue Sep 6, 2019
@vasslitvinov
Do not forward init= and deinit.
35734bc 
Resolves chapel-lang#13631.

The compiler in resolveForwardedCall() already aborted forwarding attempts
when the callee name was `init`. Now do the same also for `init=` and `deinit`.

Surprisingly, the compiler already was not forwarding `deinit`.
With the only exception of `test/classes/forwarding/forwarding-cycle.chpl`
and perhaps `test/classes/forwarding/forwarding-self.chpl`.
This is unaffected by `--legacy-classes` afaik.
So this change is less impactful than it appears.

I looked and did not see why the compiler was not forwarding `deinit`.
Below are my observations about the compiler prior to this change.

In the earlier version of `forwarding-cycle.chpl` it was forwarding `deinit`
that was invoked from `chpl__delete(arg: unmanaged A)`:
```chpl
class A {
  forwarding var data: unmanaged B;
}
class B {
  forwarding var data: unmanaged A;
}
proc main() {
  var a = new unmanaged A();
  delete a;
}
```

It is during forwarding of `deinit` that the compiler detected the forwarding
cycle, which is what this test was testing.

If we switch the type of the two `data` fields to be nilable, ex.
```chpl
forwarding var data: unmanaged B?;
```

then the compiler attempted forwarding, however it did not succeed because
`tryResolveCall(getTgt)` in adjustAndResolveForwardedCall() returned false.
This is puzzling because it appears to work just fine when forwarding
a user method, ex. in the current version of `forwarding-cycle.chpl`:

```chpl
class A {
  forwarding var data: unmanaged B?;
  proc fun(i:int) {}
}
class B {
  forwarding var data: unmanaged A?;
}
proc main() {
  var a = new unmanaged A();
  a.fun(1.1);
  delete a;
}
```

`deinit()` was not forwarded either when the user defined one.
Ex. the following code invokes B.deinit then A.deinit upon `delete c`:

```chpl
class A {
  proc init()   { writeln("A.init");   }
  proc init(arg:int) { writeln("A.init with arg"); withArg=true; }
  proc deinit() { writeln("A.deinit",
                    if withArg then " with arg" else ""); }
  const withArg=false;
}

class B: A {
  proc init()   { writeln("B.init");   }
  proc deinit() { writeln("B.deinit"); }
}

class C: B {
  // If de/init() are forwarded, they will bypass B.
  forwarding var fld = new unmanaged A(0);  // the "with arg" version
}

proc main {
  var c = new unmanaged C();
  delete c.fld;
  delete c;
}
```

Discussed with @benharsh.
@vasslitvinov vasslitvinov mentioned this issue Sep 6, 2019
Merged
vasslitvinov added a commit that referenced this issue Sep 6, 2019
@vasslitvinov
Merge pull request #14026 from vasslitvinov/dont-forward-deinit
2e11281 
Do not forward init= and deinit

Resolves #13631.

The compiler in resolveForwardedCall() already aborted forwarding attempts
when the callee name was `init`. Now do the same also for `init=` and `deinit`.

Surprisingly, the compiler already was not forwarding `deinit`.
With the only exception of `test/classes/forwarding/forwarding-cycle.chpl`
and perhaps `test/classes/forwarding/forwarding-self.chpl`.
This is unaffected by `--legacy-classes` afaik.
So this change is less impactful than it appears.

I looked and did not see why the compiler was not forwarding `deinit`.
Below are my observations about the compiler prior to this change.

In the earlier version of `forwarding-cycle.chpl` it was forwarding `deinit`
that was invoked from `chpl__delete(arg: unmanaged A)`:

class A {
  forwarding var data: unmanaged B;
}
class B {
  forwarding var data: unmanaged A;
}
proc main() {
  var a = new unmanaged A();
  delete a;
}

It is during forwarding of `deinit` that the compiler detected the forwarding
cycle, which is what this test was testing.

If we switch the type of the two `data` fields to be nilable, ex.

    forwarding var data: unmanaged B?;

then the compiler attempted forwarding, however it did not succeed because
`tryResolveCall(getTgt)` in adjustAndResolveForwardedCall() returned false.
This is puzzling because it appears to work just fine when forwarding
a user method, ex. in the current version of `forwarding-cycle.chpl`:

class A {
  forwarding var data: unmanaged B?;
  proc fun(i:int) {}
}
class B {
  forwarding var data: unmanaged A?;
}
proc main() {
  var a = new unmanaged A();
  a.fun(1.1);
  delete a;
}

`deinit()` was not forwarded either when the user defined one.
Ex. the following code invokes B.deinit then A.deinit upon `delete c`:

class A {
  proc init()   { writeln("A.init");   }
  proc init(arg:int) { writeln("A.init with arg"); withArg=true; }
  proc deinit() { writeln("A.deinit",
                    if withArg then " with arg" else ""); }
  const withArg=false;
}

class B: A {
  proc init()   { writeln("B.init");   }
  proc deinit() { writeln("B.deinit"); }
}

class C: B {
  // If de/init() are forwarded, they will bypass B.
  forwarding var fld = new unmanaged A(0);  // the "with arg" version
}

proc main {
  var c = new unmanaged C();
  delete c.fld;
  delete c;
}

Discussed with @benharsh.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not forward init= and deinit vasslitvinov/chapel
1 participant
@vasslitvinov