Bug 186861 – check-webkit-style should warn about exported inline functions

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Keith Rollin]

When checking binaries compiled with LTO enabled, WebKit's check-for-weak-vtables-and-externals script can complain about exported inline functions. For instance, in Source/WebCore/page/scrolling/ScrollingTree.h, the following:

    WEBCORE_EXPORT virtual void reportSynchronousScrollingReasonsChanged(MonotonicTime, SynchronousScrollingReasons) { }
    WEBCORE_EXPORT virtual void reportExposedUnfilledArea(MonotonicTime, unsigned /* unfilledArea */) { }

Can result in the following error messages:

    ERROR: WebCore has a weak external symbol in it (.../OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore)
    ERROR: A weak external symbol is generated when a symbol is defined in multiple compilation units and is also marked as being exported from the library.
    ERROR: A common cause of weak external symbols is when an inline function is listed in the linker export file.
    ERROR: symbol __ZN7WebCore13ScrollingTree25reportExposedUnfilledAreaEN3WTF13MonotonicTimeEj
    ERROR: symbol __ZN7WebCore13ScrollingTree40reportSynchronousScrollingReasonsChangedEN3WTF13MonotonicTimeEj

Unfortunately, these errors are not emitted when LTO is not enabled, meaning that a developer could check-in a file that will fail an LTO build if they don't build with that option locally. Therefore, try to head this off by updating check-webkit-style to identify and warn about these cases (which includes when an export macro is applied directly to an inline method as well as when an inline method is part of an exported class).

[Radar WebKit Bug Importer]

<rdar://problem/41303668>

[Keith Rollin]

Created attachment 343179 [details]
Patch

[Keith Rollin]

OK, for the record, I ran test-webkitpy locally and it passed. I don't know why it's failing here. Harrumph.

[Keith Rollin]

Created attachment 343180 [details]
Patch

[EWS Watchlist]

Comment on attachment 343180 [details]
Patch

Attachment 343180 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8275529

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html

[EWS Watchlist]

Created attachment 343233 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brent Fulgham]

Comment on attachment 343180 [details]
Patch

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

Looks good. Hopefully we can activate LTO soon! Removing cq- since this checker change can't affect windows tests.

> Tools/ChangeLog:25
> +        Unfortunately, these errors are not emitted when LTO is not enabled,

I had to read this sentence a couple of times (double negatives). I propose:

"Unfortunately, these errors are only emitted when LTO is enabled,"

[Keith Rollin]

(In reply to Brent Fulgham from comment #7)
> > Tools/ChangeLog:25
> > +        Unfortunately, these errors are not emitted when LTO is not enabled,
> 
> I had to read this sentence a couple of times (double negatives). I propose:
> 
> "Unfortunately, these errors are only emitted when LTO is enabled,"

You don't have a lack of problems with double-negative sentences? Fine, I'll not keep it the same.

[Keith Rollin]

Created attachment 343251 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 343251 [details]
Patch

Clearing flags on attachment: 343251

Committed r233054: <https://trac.webkit.org/changeset/233054>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.