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

follow on #50 #81

Merged
merged 3 commits into from
Nov 1, 2022
Merged

Conversation

krakjoe
Copy link
Contributor

@krakjoe krakjoe commented Nov 1, 2022

Fix assertion error introduced in #76

I was not able to reproduce on (intel) mac running the normal suite, do I need to do something special to reproduce, m1 maybe (I have one) ?

This looks like the correct thing to do anyway.

@dunglas
Copy link
Owner

dunglas commented Nov 1, 2022

Still the same issue with this patch. Yes, I'm using an M1 but I've no idea if it's related or not.

@dunglas
Copy link
Owner

dunglas commented Nov 1, 2022

Here is a better trace:

$ go test -c
$ lldb frankenphp.test 
(lldb) target create "frankenphp.test"
Current executable set to '/Users/dunglas/workspace/frankenphp/frankenphp.test' (arm64).
(lldb) run
Process 51017 launched: '/Users/dunglas/workspace/frankenphp/frankenphp.test' (arm64)
Assertion failed: (zval_gc_type(Assertion failed: (zval_gc_type((ref)->gc.u.type_info) == 7 || z(ref)->gc.u.type_info) == 7 || zval_gc_type((ref)->gc.u.type_infval_gc_type((ref)->gc.u.type_info) == 8), function gc_possible_ro) == 8), function gc_possible_root, file zend_gc.c, line 647.
oot, file zend_gc.c, line 647.
Assertion failed: (zval_gc_type((ref)->gc.u.type_info) == 7 || zval_gc_type((ref)->gc.u.type_info) == 8), function gc_possible_root, file zend_gc.c, line 647.
Assertion failed: (zval_gc_type((ref)->gc.u.type_info) == 7 || zval_gc_type((ref)->gc.u.type_info) == 8), function gc_possible_root, file zend_gc.c, line 647.
Assertion failed: (zval_gc_type((ref)->gc.u.type_info) == 7 || zval_gc_type((ref)->gc.u.type_info) == 8), function gc_possible_root, file zend_gc.c, line 647.
Process 51017 stopped
* thread #15, name = 'thread-pool-3', stop reason = hit program assert
    frame #4: 0x00000001008ddbe4 frankenphp.test`gc_possible_root + 320
frankenphp.test`gc_possible_root:
->  0x1008ddbe4 <+320>: b      0x1008ddbe8               ; <+324>
    0x1008ddbe8 <+324>: ldur   x8, [x29, #-0x8]
    0x1008ddbec <+328>: ldr    w0, [x8, #0x4]
    0x1008ddbf0 <+332>: bl     0x1008de184               ; zval_gc_info
Target 0: (frankenphp.test) stopped.

(lldb) bt
* thread #15, name = 'thread-pool-3', stop reason = hit program assert
    frame #0: 0x0000000187906d98 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000018793bee0 libsystem_pthread.dylib`pthread_kill + 288
    frame #2: 0x0000000187876340 libsystem_c.dylib`abort + 168
    frame #3: 0x0000000187875754 libsystem_c.dylib`__assert_rtn + 272
  * frame #4: 0x00000001008ddbe4 frankenphp.test`gc_possible_root + 320
    frame #5: 0x00000001007cd1ec frankenphp.test`gc_check_possible_root + 124
    frame #6: 0x00000001007cc944 frankenphp.test`i_zval_ptr_dtor + 80
    frame #7: 0x00000001007cc8e8 frankenphp.test`zval_ptr_dtor + 24
    frame #8: 0x00000001007033d0 frankenphp.test`php_request_shutdown + 820
    frame #9: 0x00000001002d47b8 frankenphp.test`frankenphp_request_shutdown + 164
    frame #10: 0x00000001002d3928 frankenphp.test`_cgo_395c8cbf2c19_Cfunc_frankenphp_request_shutdown + 32
    frame #11: 0x000000010006a72c frankenphp.test`runtime.asmcgocall.abi0 + 124
    frame #12: 0x00000001001894dc frankenphp.test`crosscall2 + 60

@@ -302,7 +302,7 @@ uintptr_t frankenphp_request_shutdown()
int i;

for (i=0; i<NUM_TRACK_VARS; i++) {
zval_ptr_dtor(&PG(http_globals)[i]);
zval_ptr_dtor_nogc(&PG(http_globals)[i]);
}
} zend_end_try();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Re-adding a call to php_hash_environment() after this line prevents the crash (but is likely not a good fix).

@krakjoe krakjoe changed the title nogc variants follow on #50 Nov 1, 2022
@krakjoe
Copy link
Contributor Author

krakjoe commented Nov 1, 2022

So, with fresh eyes this morning, things look a bit clearer.

When request globals are reset, we need to leave them in an uninitialized (undef) state because request shutdown will unconditionally call dtors.

So I've cleaned up that logic into a routine, which is called for worker request shutdown, and in the "unclean shutdown" case, just in case, should the routine be entered twice for some reason, nothing bad will happen.

We should not apply anything to symbol table, it won't work as we want it too anyway; There may be symbols in the table which themselves hold references to super globals, and we don't want to recursively apply() to the symbol table. In any case we don't need too; if user code retains a ref to a super global (it must be at least rc=2), and we have destroyed the track vars ref, normal ref counting still works for the symbol in the user code, ie. we only have to take care of track vars.

@dunglas
Copy link
Owner

dunglas commented Nov 1, 2022

This works on Mac too now! Thank you very much for your time.

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