Bug 36500 – Generated JIT stubs does not compile because of compiler directives

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[David Leong]

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.

[Eric Seidel (no email)]

CCing the JIT ninjas.

[Gabor Loki]

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.

[Patrick R. Gansterer]

For the WinCE port i generate a asm file, where the #if won't be valid.

[David Leong]

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.

[Laszlo Gombos]

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 ?

[Patrick R. Gansterer]

(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).

[David Leong]

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.

[Patrick R. Gansterer]

>  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.

[David Leong]

(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.

[David Leong]

Created attachment 53452 [details]
Updated as per Patrick's comments

[Patrick R. Gansterer]

(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!

[Nikolas Zimmermann]

Could any of the JSC hackers have a look, this is pending so long?

[Gavin Barraclough]

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?

[Eric Seidel (no email)]

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!?

[Laszlo Gombos]

I do not think this is relevant anymore. I also checked with David.