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

Calling object methods from static context yields segmentation fault when internal-call-transformation: true #2000

Closed
andrewdalpino opened this issue Nov 2, 2019 · 14 comments
Labels

Comments

@andrewdalpino
Copy link

andrewdalpino commented Nov 2, 2019

Hi fellas,

As recently as 0.12.11, the following block of code results in a segmentation fault ...

    /**
     * Return the elementwise maximum of two vectors.
     *
     * @param \Tensor\Vector a
     * @param \Tensor\Vector b
     * @throws \InvalidArgumentException
     * @return self
     */
    public static function maximum(const <Vector> a, const <Vector> b) -> <Vector>
    {
        if a->n() !== b->n() {
            throw new InvalidArgumentException("Vector A requires "
                . (string) a->n() . " elements but Vector B has "
                . (string) b->n() . ".");
        }

        return static::quick(array_map("max", a->asArray(), b->asArray()));
    }

Removing the calls to a->n(), b->n(), a->asArray(), and b->asArray() fixes the problem. There is no issue when compiled with internal-call-transformation: false. I am inferring that there is something wrong with how Zephir handles instance methods in the static context, but I could be wrong.

Here is a link to the source file https://github.com/RubixML/Tensor/blob/master/tensor/vector.zep

And the test that fails/segfaults https://github.com/RubixML/Tensor/blob/master/tests/VectorTest.php

It may be important to note that a->n(), b->n(), a->asArray(), and b->asArray() access properties on the object using this which was the subject of a previous related bug #1956

@sergeyklay
Copy link
Contributor

@andrewdalpino Is problem actual with previous Zephir release, e.g. 0.12.10?

@andrewdalpino
Copy link
Author

andrewdalpino commented Nov 2, 2019

Hi @sergeyklay the problem occurs with the version that was released earlier today - version 0.12.11

Note that before 0.12.11 the extension would not even compile with internal-call-transformation: true due to #1956

There are a few more issues related to the internal-call-transformation optimization however, they might be related to this one so I figured we'll start here

@sergeyklay sergeyklay added the bug label Nov 3, 2019
@sergeyklay
Copy link
Contributor

@dreamsxin Could you please take a look?

@dreamsxin
Copy link
Contributor

@sergeyklay Can change

ZEPHIR_BACKUP_SCOPE(); \
ZEPHIR_BACKUP_THIS_PTR(); \
ZEPHIR_SET_THIS(object); \
ZEPHIR_SET_SCOPE((Z_OBJ_P(object) ? Z_OBJCE_P(object) : NULL), (Z_OBJ_P(object) ? Z_OBJCE_P(object) : NULL)); \
ZEPHIR_INIT_NVAR((return_value_ptr)); \
		method(0, execute_data, return_value_ptr, object, 1); \
		ZEPHIR_LAST_CALL_STATUS = EG(exception) ? FAILURE : SUCCESS; \
ZEPHIR_RESTORE_THIS_PTR(); \
ZEPHIR_RESTORE_SCOPE(); \

To

method(0, execute_data, return_value, object, 1); 

Test it, May be switch context error.

@dreamsxin
Copy link
Contributor

I'll test it later.

@dreamsxin
Copy link
Contributor

dreamsxin commented Nov 4, 2019

I test this code, no error.

use Tensor\Tensor;
use Tensor\Vector;
use Tensor\Matrix;
use Tensor\ColumnVector;

class VectorTest
{
    protected $a;
    protected $b;
    protected $c;
    protected $d;
    public function setUp()
    {
        $this->a = Vector::build([-15, 25, 35, -36, -72, 89, 106, 45]);
        $this->b = Vector::quick([0.25, 0.1, 2., -0.5, -1., -3.0, 3.3, 2.0]);
        $this->c = Vector::quick([4., 6.5, 2.9, 20., 2.6, 11.9]);
        $this->d = Matrix::quick([
            [6.23, -1., 0.03, -0.01, -0.5, 2.],
            [0.01, 2.01, 1.0, 0.02, 0.05, -1.],
            [1.1, 5., -5., 30, -0.005, -0.001],
        ]);
    }
    public function test_maximum()
    {
        $this->setUp();

        $z = Vector::maximum($this->a, $this->b);
        $expected = [0.25, 25, 35, -0.5, -1.0, 89, 106, 45];
        var_dump($z);
    }
}
$test = new VectorTest;
$test->test_maximum();

Forget to configure "internal call transformation": true

dreamsxin added a commit to dreamsxin/zephir that referenced this issue Nov 4, 2019
@sergeyklay
Copy link
Contributor

@andrewdalpino Could you helps us with test case? Feel free to create a PR with a test that will fall to show the problem. For example you can modify existing test: fc2bae3 or create a new one.

@andrewdalpino
Copy link
Author

@sergeyklay I'd be happy to help however I've never written tests like that before

@dreamsxin were you able to reproduce the error by compiling the Tensor extension with "internal call transformation": true and then running the unit test?

@dreamsxin
Copy link
Contributor

@sergeyklay static method will happen,The problem is here.

Z_OBJ(EG(current_execute_data)->This) ? Z_OBJ(EG(current_execute_data)->This) : NULL;

Change to

Z_TYPE(EG(current_execute_data)->This) ? Z_OBJ(EG(current_execute_data)->This) : NULL;

@dreamsxin dreamsxin mentioned this issue Nov 5, 2019
3 tasks
@sergeyklay
Copy link
Contributor

@andrewdalpino I just merged #2001 with a fix for this issue. Any feedback before release are more than welcome. Feel free to ping us if you'll have any issue with testing using Zephir development branch. Thank you for contributing!

@andrewdalpino
Copy link
Author

andrewdalpino commented Nov 5, 2019

Thank you @dreamsxin and @sergeyklay

The segfault is gone but some of the other issue still remain

At first glance, it appears we can't access public instance methods from a class' static context when compiled with internal-call-transformation: true

9) Tensor\Tests\VectorTest::test_maximum
Invalid callback static::quick, cannot access static:: when no class scope is active

/mnt/c/Users/User/Workspace/Rubix/Tensor/tests/VectorTest.php:144

10) Tensor\Tests\VectorTest::test_minimum
Invalid callback static::quick, cannot access static:: when no class scope is active

/mnt/c/Users/User/Workspace/Rubix/Tensor/tests/VectorTest.php:154

I'll do some more debugging and write up another issue if need be

Thanks again, looking forward to training neural networks in PHP in under an hour ;)

@dreamsxin
Copy link
Contributor

@andrewdalpino Thank you
Please test again, use this change #2002

@andrewdalpino
Copy link
Author

Nice, @dreamsxin that worked

I still have more errors in my tests to go through, I'll make them in a separate issue

Thanks again guys, great work as always

@sergeyklay
Copy link
Contributor

You're welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants