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

Encapsulate reference-counting primitives. #2880

Merged
merged 2 commits into from Oct 30, 2017

Conversation

5 participants
@dstogov
Contributor

dstogov commented Oct 26, 2017

Prohibit direct update of GC_REFCOUNT().
GC_SET_REFCOUNT(), GC_ADDREF() and GC_DELREF() should be used instead.
Added macros to validate reference-counting (disabled for now).
These macros are going to be used to eliminate race-conditions during reference-counting on data shared between threads.

Encapsulate reference-counting primitives.
Prohibit direct update of GC_REFCOUNT(), GC_SET_REFCOUNT(), GC_ADDREF() and GC_DELREF() shoukf be instead.
Added mactros to validate reference-counting (disabled for now).
These macros are going to be used to eliminate race-condintions during reference-counting on data shared between threads.
@dstogov

This comment has been minimized.

Contributor

dstogov commented Oct 26, 2017

@nikic this is just a source instrumentation to find possible reference-counting race-conditions.
Using this, I'm going to verify problematic places.
I'm not sure, if I like to merge this into master right now.
However, this kind instrumentation may be useful for other things as well.

Merge branch 'master' into rc_debug
* master:
  Use per-request heap instead of system one
  Extend zend_register_class_alias_ex() with additional argument to allow creating persistent or per-request aliases
  Makrk persistent resources and references with GC_PERSISTENT flag
}
static zend_always_inline uint32_t zend_gc_set_refcount(zend_refcounted_h *p, uint32_t rc) {
p->refcount = rc;

This comment has been minimized.

@staabm

staabm Oct 27, 2017

Contributor

indent

@nikic

This comment has been minimized.

Member

nikic commented Oct 27, 2017

@dstogov I think we should definitely merge the encapsulation macros. I've used similar code in the past as well (iirc for using mprotect to enforce cow).

Regarding the specific checks, they seem a bit strict (as it's not generally wrong for permanent but thread-local data), but I guess this is close enough to be useful.

@php-pulls php-pulls merged commit 8203a06 into php:master Oct 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dstogov

This comment has been minimized.

Contributor

dstogov commented Oct 30, 2017

@nikic I agree about too strict assertions, but even thread-local permanent data should be updated carefully, because of possible unclean shutdown and unexpected effect on next request.
For now, I use this strict conditions to identify problematic places, later they should be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment