Last night, was an all nighter. I stayed up to hack out some javascript code for yahoo!. In the middle of all that, something new came up - bug #7070. You can read the bug report or you could see what happened on IRC. All this is leading up to something very important, at least to me, so read on.
<edink> Rasmus: commenting out my_fetch_global_vars() and having auto_globals_jit = off
makes apc work on windows
<Rasmus> edink: could you add that to the test case?
<Rasmus> I'm busy breaking apc further
<edink> I'll add comment to #7070
<Rasmus> thanks
<Rasmus> g0pz: edink updated bug 7070
<edink> g0pz: seems that calling zend_is_auto_global() with any value from apc_copy_function_for_execution()
crashes the thing on windows
Ok, so I had a good long long look at the code and started guessing what went wrong. There's one thing I still don't understand about Zend engine - how does the TSRM stuff works. So following the path of ancestors, who relied on the dark and mysterious and of course, mostly unknown powers of evil to explain bad things happening to good people, I too blamed the unknown.
<g0pz> edink: reall weird
<edink> g0pz: yeah
<g0pz> has something to do with tsrm ?
<edink> g0pz: i cannot tell if its tsrm related
<g0pz> because that looks like a bad address there in the tsrm ptr ?
<g0pz> 0x00d5b3f6 seems to be a little on the low side
<g0pz> sort of makes sense
<g0pz> as the apc_copy_function_for_execution_ex is passed as ht_copy_fun_t to copy_hashtable
<g0pz> which just calls the apc_copy_function_for_execution_ex with 4 args
<g0pz> apc_compile.c:926 needs to be fixed to pass the thread safety macros ?
<g0pz> *but* I cannot test anything I fix and neither do I have any idea what any TSRM macro means
<g0pz> so help !!! :)
<Rasmus> TSRM just wraps all the globals in a struct
<edink> g0pz: its just passing void ***tsrm_ls around
<g0pz> so just a TSRMLS_FETCH() in scope is enough ?
<SaraMG> g0pz: You don't need to understand TSRM.... TSRM understands you
<g0pz> SaraMG: in soviet russia ...
<SaraMG> >exactly<
<SaraMG> Now you're getting it
<g0pz> edink: as much as I'd like to help you, this thing needs a professional :)
<g0pz> ok, here's how you fix it :)
<Rasmus> ctrl-alt-del <insert Ubuntu cd>
<g0pz> remove the TSRMLS_DC in apc_copy_function_for_execution_ex
<edink> so your func arglist should have TSRMLS_D (no other args) or TSRMLS_DC (other args) in function definition and
TSRMLS_C or TSRMLS_CC when calling it
<g0pz> and add a TSRMLS_FETCH(); as the first statment in that function
<g0pz> now rebuild and hope it works
<SaraMG> *ick*
<edink> g0pz: TSRMLS_FETCH(); cannot be used if you have TSRMLS_D(C) in function declaration
<g0pz> according to a significant proportion of my brain cells, that is how that could be fixed :)
<g0pz> remove the declaration
<g0pz> you're anyway passing stack junk there
<g0pz> the pointer you got was the apc_php_malloc in place of tsrm_ls
<SaraMG> Ah, yes
<SaraMG> Didn't realize that proto had to conform to a callback definition
<g0pz> SaraMG: the shocking part is that it doesn't
<SaraMG> The callback typedef being (Bucket*,va_list)
<g0pz> that's the check, if I'm not wrong ?
<SaraMG> (void*, void*, apc_malloc_t, apc_free_t)
<Rasmus> hey hey, no peeking under the skirts unless you are going to dig in and fix stuff
<SaraMG> apc_copy_function_for_execution_ex looks NOTHING like the callback's typdef
<SaraMG> Like, not even close
<Rasmus> details ;)
<edink> :)
So, finally I still need to get the other guy to build and test it. Of course, the correctness of the patch has been verified in theory - it was still upto someone to figure out whether that was the only problem in the mix.
<g0pz> edink: don't just stand there, make the changes and rebuild :)
<edink> g0pz: made too many changes to my sources :)
<g0pz> this is just one more :)
<SaraMG> g0pz: So yeah, nix the _DC, use _FETCH, but also add some dummies to that declaration
so it fits the calling semantics
<g0pz> SaraMG: I haven't got commits
<SaraMG> oh.... who are you again?
<edink> Rasmus: just make him an accout :)
<Rasmus> gah, just fill in your username and password
<Rasmus> and garbage in the description field
<Rasmus> those warnings don't apply to people who understand the guts of the engine
<g0pz> tinker with != understand
<Rasmus> close enough
As usualy, we get into optimisations and all that... before it is actually tested.
<edink> g0pz: are you not checking every var if it's an autoglobal? <g0pz> edink: if there's a better way, I'd love to know about it :) <g0pz> because that's what the fetch_simple_variable_ex in zend_compile.c does <g0pz> of course, I could optimize easily <g0pz> with an if(name[0] == '_') :) <edink> g0pz: is the sole purpose of it to load superglobals when jit is on? <g0pz> yes
Finally, the bug is closed - in less than a couple of hours after I saw the bug report.
<g0pz> edink: with that fix, does APC work ? <edink> g0pz: yeah, like a charm <edink> let's see what Rasmus broke :) <edink> well, it compiles :) <Rasmus> everything most likely
End result was that I got commit access to PHP CVS. I am now gopalv of the php - Resistance is Futile. I am yet to be marked as the maintainer of anything, so I'm still in the zone where everything's convenient but nothing really bugs you. Haven't checked in anything yet. That's for a day when I am actually sane and not hopped up on coffee.
Look ma, I'm a php dev :)
--It is not doing the thing we like to do, but liking the thing we have to do, that makes life blessed.
-- Goethe