Bug 88552

Summary: Explictly mark stubs called by JIT as being internal
Product: WebKit Reporter: Andy Wingo <wingo>
Component: New BugsAssignee: Andy Wingo <wingo>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, fpizlo, ggaren, gustavo, mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andy Wingo 2012-06-07 10:40:11 PDT
Explictly mark stubs called by JIT as being internal
Comment 1 Andy Wingo 2012-06-07 10:45:32 PDT
Created attachment 146321 [details]
Patch
Comment 2 Andy Wingo 2012-06-07 10:47:23 PDT
This is a bit of a rough patch at this point, but I'd appreciate some early thoughts.  Bug 88421 tracks some broader symbol visibility issues in the GTK port, but for the JIT it should be possible to explicitly mark stubs as having local visibility, and it doesn't seem like too much of a burden.
Comment 3 WebKit Review Bot 2012-06-07 10:49:27 PDT
Attachment 146321 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/jit/JITStubs.h:463:  cti_register_file_check is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITStubs.h:464:  cti_vm_lazyLinkCall is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITStubs.h:465:  cti_vm_lazyLinkConstruct is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITStubs.h:466:  cti_vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Andy Wingo 2012-06-08 09:36:24 PDT
Adding Darin as another potential reviewer :)
Comment 5 Filip Pizlo 2012-06-08 10:48:33 PDT
Comment on attachment 146321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146321&action=review

LGTM other than the one comment.  Not R+'ing because I wanna know if that can be simplified.

> Source/JavaScriptCore/llint/LLIntSlowPaths.h:82
> +#define LLINT_SLOW_PATH_HIDDEN_DECL(name) \
> +    LLINT_SLOW_PATH_DECL(name) WTF_INTERNAL

Why rename?  Is LLINT_SLOW_PATH_DECL used anywhere after this rename?
Comment 6 Andy Wingo 2012-06-08 11:27:03 PDT
Thanks for the review.

(In reply to comment #5)
> > Source/JavaScriptCore/llint/LLIntSlowPaths.h:82
> > +#define LLINT_SLOW_PATH_HIDDEN_DECL(name) \
> > +    LLINT_SLOW_PATH_DECL(name) WTF_INTERNAL
> 
> Why rename?  Is LLINT_SLOW_PATH_DECL used anywhere after this rename?

The rename is because LLINT_SLOW_PATH_DECL is also used in the definitions, and __attribute__ annotations appear to be only allowed in declarations.
Comment 7 Filip Pizlo 2012-06-08 11:29:28 PDT
(In reply to comment #6)
> Thanks for the review.
> 
> (In reply to comment #5)
> > > Source/JavaScriptCore/llint/LLIntSlowPaths.h:82
> > > +#define LLINT_SLOW_PATH_HIDDEN_DECL(name) \
> > > +    LLINT_SLOW_PATH_DECL(name) WTF_INTERNAL
> > 
> > Why rename?  Is LLINT_SLOW_PATH_DECL used anywhere after this rename?
> 
> The rename is because LLINT_SLOW_PATH_DECL is also used in the definitions, and __attribute__ annotations appear to be only allowed in declarations.

Oooohhh yeah, I forgot about that ugliness!
Comment 8 Filip Pizlo 2012-06-08 11:30:34 PDT
R=me.  Land with care and make sure you watch the bots, as will I. ;-)  I don't expect problems but then again, spoon-feeding compilers tends to be an error-prone thingy.
Comment 9 Andy Wingo 2012-06-08 11:52:56 PDT
Comment on attachment 146321 [details]
Patch

Deal :-)  The commit queue looks pretty free, so let's see what happens...
Comment 10 WebKit Review Bot 2012-06-08 12:57:58 PDT
Comment on attachment 146321 [details]
Patch

Clearing flags on attachment: 146321

Committed r119857: <http://trac.webkit.org/changeset/119857>
Comment 11 WebKit Review Bot 2012-06-08 12:58:03 PDT
All reviewed patches have been landed.  Closing bug.