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
bug

Comments

@andrewdalpino
Copy link

@andrewdalpino 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

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Nov 2, 2019

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

@andrewdalpino

This comment has been minimized.

Copy link
Author

@andrewdalpino 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

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Nov 3, 2019

@dreamsxin Could you please take a look?

@dreamsxin

This comment has been minimized.

Copy link
Member

@dreamsxin dreamsxin commented Nov 3, 2019

@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

This comment has been minimized.

Copy link
Member

@dreamsxin dreamsxin commented Nov 3, 2019

I'll test it later.

@dreamsxin

This comment has been minimized.

Copy link
Member

@dreamsxin 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

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Nov 4, 2019

@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

This comment has been minimized.

Copy link
Author

@andrewdalpino andrewdalpino commented Nov 4, 2019

@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

This comment has been minimized.

Copy link
Member

@dreamsxin dreamsxin commented Nov 4, 2019

@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;
@sergeyklay sergeyklay closed this in 6a6984e Nov 5, 2019
@dreamsxin dreamsxin mentioned this issue Nov 5, 2019
1 of 3 tasks complete
@sergeyklay

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Nov 5, 2019

@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

This comment has been minimized.

Copy link
Author

@andrewdalpino 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

This comment has been minimized.

Copy link
Member

@dreamsxin dreamsxin commented Nov 6, 2019

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

@dreamsxin dreamsxin mentioned this issue Nov 6, 2019
0 of 3 tasks complete
@andrewdalpino

This comment has been minimized.

Copy link
Author

@andrewdalpino andrewdalpino commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Nov 7, 2019

You're welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.