Re: [ZEND-ENGINE-CVS] cvs: ZendEngine2(PHP_5_3) / zend.h zend_ptr_stack.h zend_types.h zend_vm_def.h zend_vm_execute.h

[email protected] (Arnaud Le Blanc) Fri, 05 Jun 2009 13:24:31 +0200
Newsgroups php.zend-engine.cvs
Message-ID <1244201071.4853.68.camel@localhost>
On Wed, 2009-06-03 at 22:11 +0100, Nuno Lopes wrote:
> >> So I'm not particularly happy this solution. First it relies on gcc: the
> >> code is still broken for other compilers following C99 strict aliasing
> >> rules.
> >> Second you're using may_alias and restrict at the same time, which have
> >> opposite meanings. They refer to different pointers, but it's still
> >> confusing.
> >
> > may_alias means a particular type can alias any other type (like char
> > does)
> > restrict means a particular variable cannot be accessed through other
> > variables in the current scope/block.
> >
> > So I used restrict to cancel the performance effect of may_alias on the
> > function's code.
> 
> exactly my point: it's very confusing :)
> The interplay of two variables with these two attributes is also tricky.
> 
> 
> >> As these functions are only called from 2 sites, I think fixing the 
> >> function
> >> signature to the right types is the right solution.
> >
> > I agree, however there is some other potential problems that may need to
> > be fixed, and which will not be fixable in this way.
> 
> like what? I haven't checked the code, but what are the other potential 
> problems?
> 
> 
> > So this fix is an attempt to do something generic and useable for other
> > cases. I found this solution not too bad as this is still a pointer type
> > which can be assigned to (as opposed to using a char type), and it is
> > compatible with the original type.
> >
> > As you pointed, may_alias is gcc-specific, but is there other compiler
> > with this level of strictness without such attribute ?
> 
> I don't know. I know that at least ICC and suncc have strict aliasing 
> analysis, but I dunno what's their support for these attributes (or if they 
> will be detected as gcc by the configure script).
> 

You are right, thanks for having reviewed this. I have reverted this to
something simpler/better.

Regards,

Arnaud