Bug 88552 - Explictly mark stubs called by JIT as being internal
Summary: Explictly mark stubs called by JIT as being internal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Wingo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-07 10:40 PDT by Andy Wingo
Modified: 2012-06-08 12:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (59.94 KB, patch)
2012-06-07 10:45 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.