Bug 88552 – Explictly mark stubs called by JIT as being internal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Wingo]

Explictly mark stubs called by JIT as being internal

[Andy Wingo]

Created attachment 146321 [details]
Patch

[Andy Wingo]

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.

[WebKit Review Bot]

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.

[Andy Wingo]

Adding Darin as another potential reviewer :)

[Filip Pizlo]

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?

[Andy Wingo]

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.

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Andy Wingo]

Comment on attachment 146321 [details]
Patch

Deal :-)  The commit queue looks pretty free, so let's see what happens...

[WebKit Review Bot]

Comment on attachment 146321 [details]
Patch

Clearing flags on attachment: 146321

Committed r119857: <http://trac.webkit.org/changeset/119857>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.