Bug 36500

Summary: Generated JIT stubs does not compile because of compiler directives
Product: WebKit Reporter: David Leong <david.leong>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: barraclough, eric, laszlo.gombos, loki, oliver, paroga, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Attachments:
Description Flags
Patch to handle compiler directives
none
Updated patch, as per review comments
none
Updated as per Patrick's comments eric: review-

Description David Leong 2010-03-23 10:27:46 PDT
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.
Comment 1 Eric Seidel (no email) 2010-03-25 01:55:28 PDT
CCing the JIT ninjas.
Comment 2 Gabor Loki 2010-04-06 01:35:21 PDT
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.
Comment 3 Patrick R. Gansterer 2010-04-06 09:25:46 PDT
For the WinCE port i generate a asm file, where the #if won't be valid.
Comment 4 David Leong 2010-04-06 09:38:53 PDT
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.
Comment 5 Laszlo Gombos 2010-04-06 09:50:17 PDT
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 ?
Comment 6 Patrick R. Gansterer 2010-04-06 09:56:49 PDT
(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).
Comment 7 David Leong 2010-04-14 14:55:37 PDT
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.
Comment 8 Patrick R. Gansterer 2010-04-15 00:21:46 PDT
>  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.
Comment 9 David Leong 2010-04-15 09:32:20 PDT
(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.
Comment 10 David Leong 2010-04-15 11:09:03 PDT
Created attachment 53452 [details]
Updated as per Patrick's comments
Comment 11 Patrick R. Gansterer 2010-04-15 11:31:09 PDT
(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!
Comment 12 Nikolas Zimmermann 2010-07-30 22:30:25 PDT
Could any of the JSC hackers have a look, this is pending so long?
Comment 13 Gavin Barraclough 2010-08-01 18:59:26 PDT
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 14 Eric Seidel (no email) 2010-08-02 07:33:46 PDT
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!?
Comment 15 Laszlo Gombos 2012-08-05 14:34:22 PDT
I do not think this is relevant anymore. I also checked with David.