Bug 186861 - check-webkit-style should warn about exported inline functions
Summary: check-webkit-style should warn about exported inline functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-20 14:09 PDT by Keith Rollin
Modified: 2018-06-21 13:10 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.34 KB, patch)
2018-06-20 14:13 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (12.34 KB, patch)
2018-06-20 14:27 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.81 MB, application/zip)
2018-06-21 04:01 PDT, Build Bot
no flags Details
Patch (12.34 KB, patch)
2018-06-21 11:12 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2018-06-20 14:09:43 PDT
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).
Comment 1 Radar WebKit Bug Importer 2018-06-20 14:09:53 PDT
<rdar://problem/41303668>
Comment 2 Keith Rollin 2018-06-20 14:13:18 PDT
Created attachment 343179 [details]
Patch
Comment 3 Keith Rollin 2018-06-20 14:24:25 PDT
OK, for the record, I ran test-webkitpy locally and it passed. I don't know why it's failing here. Harrumph.
Comment 4 Keith Rollin 2018-06-20 14:27:21 PDT
Created attachment 343180 [details]
Patch
Comment 5 Build Bot 2018-06-21 04:01:01 PDT
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
Comment 6 Build Bot 2018-06-21 04:01:13 PDT
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
Comment 7 Brent Fulgham 2018-06-21 10:54:49 PDT
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,"
Comment 8 Keith Rollin 2018-06-21 11:09:12 PDT
(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.
Comment 9 Keith Rollin 2018-06-21 11:12:13 PDT
Created attachment 343251 [details]
Patch
Comment 10 WebKit Commit Bot 2018-06-21 13:10:22 PDT
Comment on attachment 343251 [details]
Patch

Clearing flags on attachment: 343251

Committed r233054: <https://trac.webkit.org/changeset/233054>
Comment 11 WebKit Commit Bot 2018-06-21 13:10:23 PDT
All reviewed patches have been landed.  Closing bug.