Created attachment 51434 [details] Patch to handle compiler directives The 'create_jit_stubs' script under JavaScriptCore does not take into account of #if compiler directives. The trunk currently does not compile for RVCT with JIT enabled because new functions like 'op_eq_strings' are flagged by JSVALUE32_64. I modified the script to look for #if, #endif and transplant them into the generated stub file.
CCing the JIT ninjas.
Although I did not like this patch at first sight, now I am starting to think this solution is fine. I was playing with RVCT last weeks, and saw this problem as well. My quick fix was to emit each stub functions once by create_jit_stubs script, and add extra stub function bodies for all cases of #if directives in JITStubs.cpp. Laszlo's fix was similar (r56842) without the script modification. David's patch considers those macros which can enable/disable a JIT feature in JITStubs.cpp. So it is useful if someone wants to change the default behavior. In additional this patch will not increase the binary with dead codes. Btw, I have some suggestions about the patch. The $if, $endif and $hash should consider whitespaces as well. For example: my $if="[\t ]*#[\t ]*if" , or something similar. It would be nice if $hash is renamed to $else which contains "[\t ]*#[\t ]*el(se|if)" regexp. Finally, David, you should revert Laszlo's quick fix in your patch as well.
For the WinCE port i generate a asm file, where the #if won't be valid.
Thank you for your comments Gabor, Laszlo. I will work on the updated script change and include the revert for the quick fix later today.
Patrick, I'm not sure I understand your comment. Do you prefer the #if's inside our outside of the function body in JITStubs.cpp ?
(In reply to comment #5) > Patrick, I'm not sure I understand your comment. Do you prefer the #if's inside > our outside of the function body in JITStubs.cpp ? I prefer it inside of the function body. http://trac.webkit.org/changeset/56842 fixed my build problem for WinCE with JSVALUE32_64 too. When you print the #if's in the 'create_jit_stubs' script I won't be able to generate a asm file out of it (see bug 34953).
Created attachment 53371 [details] Updated patch, as per review comments Sorry for the late reply and update. I've been a bit busy with my other job :) Updated the patch as per Gabor's comments, Patrick's comments and Laszlo's comments. Addressed Gabor's comments by adding the white space handling Addressed Patrick's comments by adding a new '--asm' argument to not print out compiler directives. [A better fix in JITStubs.cpp is still needed I believe, instead of moving the #ifs to inside functions. It works right now because JIT_OPTIMIZE_PROPERTY_ACCESS and JIT_OPTIMIZE_CALL are defined. Addressed Laszlo's comments (talked to him offline) by not reverting the JITStub changes from 56842.
> while ( $_ = <IN> ) { > if ( /^$prefix\((.*)\)/ ) { > $stub_template .= $1 . "\n"; > + #don't generate compiler directives > + if( !$asm ) { > + $makingstubs=1; > + } > } Why do you have the if(!$asm) inside of the loop? > + #don't print out extra #if blocks inside of functions > + if( m/$openbrace/ && $makingstubs ) { > + if( m/$closebrace/ && $makingstubs ) { I think the && $makingstubs can be removed.
(In reply to comment #8) > > while ( $_ = <IN> ) { > > if ( /^$prefix\((.*)\)/ ) { > > $stub_template .= $1 . "\n"; > > + #don't generate compiler directives > > + if( !$asm ) { > > + $makingstubs=1; > > + } > > } > Why do you have the if(!$asm) inside of the loop? > > > + #don't print out extra #if blocks inside of functions > > + if( m/$openbrace/ && $makingstubs ) { > > + if( m/$closebrace/ && $makingstubs ) { > I think the && $makingstubs can be removed. Good catches, will correct this.
Created attachment 53452 [details] Updated as per Patrick's comments
(In reply to comment #10) > Created an attachment (id=53452) [details] > Updated as per Patrick's comments Maybe you rename the $hash to something better? Even if not LGTM. Maybe you can declare the old patches as obsolete or at least remove the review flag. THX!
Could any of the JSC hackers have a look, this is pending so long?
Nikolas: I'm afraid I don't speak perl, so I don't think I can really sensibly review this. May need a build system reviewer?
Comment on attachment 53452 [details] Updated as per Patrick's comments No one speaks perl. :) It's a dead language! :p Your use of no space around operators is very confusing and against webkit style. Please fix. + $functiondepth-=()=/[\\}]/g; especially!?
I do not think this is relevant anymore. I also checked with David.