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

Returning by-ref values from overridden methods and not capturing them at call site #6542

Closed
e-kayrakli opened this issue Jun 26, 2017 · 4 comments

Comments

@e-kayrakli
Copy link
Contributor

e-kayrakli commented Jun 26, 2017

Summary of Problem

Returning some values from overridden methods seems to cause compiler crashes, if the value returned is not captured at the call site.

(I am not entirely sure if the title of the issue captures the problem in a good way)

A little bit of GDB'ing shows that calledFn in https://github.com/chapel-lang/chapel/blob/master/compiler/resolution/callDestructors.cpp#L515 seems to be null if the call is a PRIM_VIRTUAL_METHOD_CALL causing a null dereference in the next line.

Steps to Reproduce

Source Code:

class A {
  proc foo() { return (3, 3); }
}

class B:A {
  proc foo() { return (1,1); }
}

var b:A = new B();
b.foo();

I have tried and confirmed that this crashes if foo returns: homogeneous/heterogeneous tuples and strings. Returning a class instance field of A seems to compile fine, any primitives are also fine. Moreover returning a primitive class field as such also seems fine:

proc foo() ref { return someIntField; }

Compile command:
chpl foo.chpl

Execution command:
Doesn't compile.

Associated Future Test(s):
test/classes/elliot/non-captured-virtual-return.chpl #7318

Configuration Information

  • Output of chpl --version:
chpl Version 1.16.0 pre-release (405f2a2a7a)
Copyright (c) 2004-2017, Cray Inc.  (See LICENSE file for more details)
  • Output of $CHPL_HOME/util/printchplenv --anonymize:
CHPL_TARGET_PLATFORM: linux64
CHPL_TARGET_COMPILER: gnu
CHPL_TARGET_ARCH: native
CHPL_LOCALE_MODEL: flat
CHPL_COMM: none
CHPL_TASKS: qthreads
CHPL_LAUNCHER: none
CHPL_TIMERS: generic
CHPL_UNWIND: none
CHPL_MEM: jemalloc
CHPL_MAKE: make
CHPL_ATOMICS: intrinsics
CHPL_GMP: gmp
CHPL_HWLOC: hwloc
CHPL_REGEXP: re2
CHPL_WIDE_POINTERS: struct
CHPL_AUX_FILESYS: none
@e-kayrakli
Copy link
Contributor Author

P.S this was also something discovered by @LouisJenkinsCS

@mppf mppf self-assigned this Sep 12, 2017
ronawho added a commit to ronawho/chapel that referenced this issue Sep 14, 2017
ronawho added a commit that referenced this issue Sep 14, 2017
Add futures for some bugs exposed by GSoC collections work

Add futures for some bugs exposed by GSoC collections work
 - Add a future for #6542 (not capturing return of virtual method segfaults) 
 - Add a future for #6998 (virtual parallel iterators cause compiler segfault)
mppf added a commit to mppf/chapel that referenced this issue Sep 15, 2017
mppf added a commit that referenced this issue Sep 18, 2017
Add resolvedOrVirtualFunction, fix issue #6542 

This PR takes two steps to fix issue #6542:

  1. It introduces a function, `CallExpr::resolvedOrVirtualFunction`, factoring
out a code pattern used already in callDestructors and errorHandling. The
pattern is to find the resolved function, or, for a PRIM_VIRTUAL_METHOD_CALL,
find one of the possible virtual methods that could be called at runtime. This
function is appropriate to call for code that identifies calls to transform
based upon the signature of the called function.

  2. It calls this function in two places in order to resolve issue #6542.

What was going wrong with issue #6542? The compiler includes a transformation
that happens during resolution, `insertReturnTemps`, which finds all calls to
functions that return a value that don't use the result of that return. In that
case it adds a temporary variable to capture the returned value.

Code in callDestructors / ReturnByRef relied on calls to transformable
functions having a variable to capture the returned value.

In the case of virtual method calls, `insertReturnTemps` was not transforming
the call, and so the code pattern in issue #6542 led to failed assertions in
callDestructors.

The important part of this fix is to adjust `insertReturnTemps` to handle
virtual calls (PRIM_VIRTUAL_METHOD_CALL).

Passed full local testing.
Reviewed by @noakesmichael - thanks!
@mppf
Copy link
Member

mppf commented Sep 18, 2017

@e-kayrakli , @LouisJenkinsCS - this bug should be fixed in master now. Could you check over the Collections modules & update any documentation / fix any workarounds? Once you do that we should close this issue. Thanks.

@LouisJenkinsCS
Copy link
Member

I'll have it done soon.

@LouisJenkinsCS
Copy link
Member

@e-kayrakli

You can close the issue now.

@ronawho ronawho closed this as completed Sep 20, 2017
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

5 participants