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

Revert updated zephir_get_global #1965

Merged
merged 1 commit into from
Oct 12, 2019
Merged

Conversation

dreamsxin
Copy link
Contributor

Hello!

In raising this pull request, I confirm the following:

  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I updated the CHANGELOG

Small description of change:

Thanks

@codecov
Copy link

codecov bot commented Oct 12, 2019

Codecov Report

Merging #1965 into development will decrease coverage by 2.92%.
The diff coverage is n/a.

@@                Coverage Diff                @@
##             development    #1965      +/-   ##
=================================================
- Coverage          33.94%   31.02%   -2.93%     
  Complexity          8256     8256              
=================================================
  Files                561      561              
  Lines              44937    46859    +1922     
=================================================
- Hits               15254    14538     -716     
- Misses             29683    32321    +2638

@sergeyklay sergeyklay merged commit 3646a14 into zephir-lang:development Oct 12, 2019
@sergeyklay
Copy link
Contributor

Thank you

@@ -100,8 +100,7 @@ int zephir_get_global(zval *arr, const char *global, unsigned int global_length)
if ((gv = zend_hash_find_ind(&EG(symbol_table), str)) != NULL) {
ZVAL_DEREF(gv);
if (Z_TYPE_P(gv) == IS_ARRAY) {
ZVAL_DUP(arr, gv);
zend_hash_update(&EG(symbol_table), str, arr);
Copy link
Contributor

@sergeyklay sergeyklay Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dreamsxin This change fails assertion:

zend_hash_copy: Assertion `((target)->gc.refcount == 1) || ((target)->u.flags & (1<<6))' failed.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff4bbe535 in __GI_abort () at abort.c:79
#2  0x00007ffff4bbe40f in __assert_fail_base (fmt=0x7ffff4d20ee0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x55555674f128 "((target)->gc.refcount == 1) || ((target)->u.flags & (1<<6))", file=0x55555674ef80 "/home/klay/src/php/source/7.2.20/Zend/zend_hash.c", line=1622, function=<optimized out>) at assert.c:92
#3  0x00007ffff4bcc102 in __GI___assert_fail (assertion=0x55555674f128 "((target)->gc.refcount == 1) || ((target)->u.flags & (1<<6))", file=0x55555674ef80 "/home/klay/src/php/source/7.2.20/Zend/zend_hash.c", line=1622, function=0x55555674f5d8 <__PRETTY_FUNCTION__.12570> "zend_hash_copy") at assert.c:101
#4  0x0000555555ec3064 in zend_hash_copy (target=0x7ffff3c65000, source=0x7ffff3c65a20, pCopyConstructor=0x555555ea596e <zval_add_ref>) at /home/klay/src/php/source/7.2.20/Zend/zend_hash.c:1622
#5  0x00007ffff07e25e9 in zim_Test_Assign_testAssignSuperGlobalsGET (execute_data=0x7ffff3c21110, return_value=0x7fffffffb1a0) at /mnt/work/zephir/php-zephir/ext/test/assign.zep.c:2403
#6  0x0000555555f1691e in ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER () at /home/klay/src/php/source/7.2.20/Zend/zend_vm_execute.h:907
#7  0x0000555555f9e171 in execute_ex (ex=0x7ffff3c21030) at /home/klay/src/php/source/7.2.20/Zend/zend_vm_execute.h:59765
#8  0x0000555555fa361f in zend_execute (op_array=0x7ffff3c7c400, return_value=0x0) at /home/klay/src/php/source/7.2.20/Zend/zend_vm_execute.h:63776
#9  0x0000555555eab03a in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /home/klay/src/php/source/7.2.20/Zend/zend.c:1498
#10 0x0000555555deaca8 in php_execute_script (primary_file=0x7fffffffd840) at /home/klay/src/php/source/7.2.20/main/main.c:2594
#11 0x0000555555fa6236 in do_cli (argc=4, argv=0x5555569aff20) at /home/klay/src/php/source/7.2.20/sapi/cli/php_cli.c:1011
#12 0x0000555555fa74d6 in main (argc=4, argv=0x5555569aff20) at /home/klay/src/php/source/7.2.20/sapi/cli/php_cli.c:1403

@dreamsxin
Copy link
Contributor Author

public function testAssignSuperGlobalsGET()
	{
	    let _GET = array_merge(_GET, ["g1": "aaa", "g2": "bbb"]);
	}

Can't use ZEPHIR_HASH_COPY

#define ZEPHIR_HASH_COPY(z, v) \
	if (Z_TYPE_P(z) == IS_ARRAY && Z_TYPE_P(v) == IS_ARRAY) { \
		zend_hash_copy(Z_ARRVAL_P(z), Z_ARRVAL_P(v), (copy_ctor_func_t) zval_add_ref); \
	}


ZEPHIR_MM_GROW();
	zephir_get_global(&_GET, SL("_GET"));

	ZEPHIR_INIT_VAR(&_0);
	ZEPHIR_INIT_VAR(&_1);
	zephir_create_array(&_1, 2, 0 TSRMLS_CC);
	add_assoc_stringl_ex(&_1, SL("g1"), SL("aaa"));
	add_assoc_stringl_ex(&_1, SL("g2"), SL("bbb"));
	zephir_fast_array_merge(&_0, &_GET, &_1 TSRMLS_CC);
	ZEPHIR_HASH_COPY(&_GET, &_0);
	ZEPHIR_MM_RESTORE();

zend_hash_copy has :HT_ASSERT_RC1`.

Test

if (Z_TYPE_P(gv) == IS_ARRAY) {
ZVAL_COPY_VALUE(arr, gv);
ZVAL_ARR(arr, zend_array_dup(Z_ARR_P(arr)));
				zend_hash_update(&EG(symbol_table), str, arr);
Return;
}

@dreamsxin
Copy link
Contributor Author

Or add zephir_set_global

@sergeyklay
Copy link
Contributor

https://travis-ci.org/phalcon/zephir/jobs/596879980#L1637-L1652

There was 1 error:

1) Extension\AssignTest::testGlobalVarAssign

Undefined index: test_index

/home/travis/build/phalcon/zephir/unit-tests/Extension/AssignTest.php:152

--

There was 1 failure:

1) Extension\Globals\SessionTest::destroy

Failed asserting that an array has the key 'test'.

/home/travis/build/phalcon/zephir/unit-tests/Extension/Globals/SessionTest.php:39

ERRORS!

Tests: 541, Assertions: 1639, Errors: 1, Failures: 1.

@sergeyklay
Copy link
Contributor

@dreamsxin After changing

			if (Z_TYPE_P(gv) == IS_ARRAY) {
				ZVAL_COPY_VALUE(arr, gv);
+				ZVAL_ARR(arr, zend_array_dup(Z_ARR_P(arr)));
+				zend_hash_update(&EG(symbol_table), str, arr);

				zend_string_release(str);
				return SUCCESS;
			}

This issue #1961 remains:

/home/klay/src/php/source/7.2.20/Zend/zend_hash.c(1968) : ht=0x7fa018465a80 is already destroyed
php: /home/klay/src/php/source/7.2.20/Zend/zend_hash.c:64: _zend_is_inconsistent: Assertion `0' failed.
[1]    19278 abort      php -d extension=ext/modules/test.so manual.php

@sergeyklay
Copy link
Contributor

sergeyklay commented Oct 13, 2019

Changing assign.zep.c:

        add_assoc_stringl_ex(&_1, SL("g1"), SL("aaa"));
        add_assoc_stringl_ex(&_1, SL("g2"), SL("bbb"));
        zephir_fast_array_merge(&_0, &_GET, &_1 TSRMLS_CC);
+       if (Z_REFCOUNTED_P(&_GET) && Z_REFCOUNT_P(&_GET) > 1) {
+               Z_DELREF_P(&_GET);
+       }
+
        ZEPHIR_HASH_COPY(&_GET, &_0);
        ZEPHIR_MM_RESTORE();

does the trick but test failed like at Travis https://travis-ci.org/phalcon/zephir/jobs/596879980#L1637-L1652

@sergeyklay sergeyklay mentioned this pull request Oct 13, 2019
3 tasks
sergeyklay added a commit that referenced this pull request Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants