check-webkit-style should warn about exported inline functions
authorkrollin@apple.com <krollin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Jun 2018 20:10:15 +0000 (20:10 +0000)
committerkrollin@apple.com <krollin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Jun 2018 20:10:15 +0000 (20:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186861
<rdar://problem/41303668>

Reviewed by Brent Fulgham.

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 only emitted when LTO is 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).

* Scripts/webkitpy/style/checkers/cpp.py:
(_FunctionState.begin):
(_FunctionState.export_macro):
(_ClassInfo.__init__):
(check_for_non_standard_constructs):
(check_function_definition):
(process_line):
(CppChecker):
* Scripts/webkitpy/style/checkers/cpp_unittest.py:
(FunctionDetectionTest.perform_function_detection):
(FunctionDetectionTest.test_webcore_export):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233054 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/style/checkers/cpp.py
Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py

index 417503d..d9d56cc 100644 (file)
@@ -1,3 +1,47 @@
+2018-06-21  Keith Rollin  <krollin@apple.com>
+
+        check-webkit-style should warn about exported inline functions
+        https://bugs.webkit.org/show_bug.cgi?id=186861
+        <rdar://problem/41303668>
+
+        Reviewed by Brent Fulgham.
+
+        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 only emitted when LTO is 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).
+
+        * Scripts/webkitpy/style/checkers/cpp.py:
+        (_FunctionState.begin):
+        (_FunctionState.export_macro):
+        (_ClassInfo.__init__):
+        (check_for_non_standard_constructs):
+        (check_function_definition):
+        (process_line):
+        (CppChecker):
+        * Scripts/webkitpy/style/checkers/cpp_unittest.py:
+        (FunctionDetectionTest.perform_function_detection):
+        (FunctionDetectionTest.test_webcore_export):
+
 2018-06-21  Aakash Jain  <aakash_jain@apple.com>
 
         [ews-build] unit-tests fail when run from another directory
index 5836c4f..4ecaba7 100644 (file)
@@ -529,9 +529,6 @@ class _FunctionState(object):
         self.is_declaration = clean_lines.elided[body_start_position.row][body_start_position.column] == ';'
         self.parameter_start_position = parameter_start_position
         self.parameter_end_position = parameter_end_position
-        self.is_final = False
-        self.is_override = False
-        self.is_pure = False
         characters_after_parameters = SingleLineView(clean_lines.elided, parameter_end_position, body_start_position).single_line
         self.is_final = bool(search(r'\bfinal\b', characters_after_parameters))
         self.is_override = bool(search(r'\boverride\b', characters_after_parameters))
@@ -550,6 +547,11 @@ class _FunctionState(object):
     def is_virtual(self):
         return bool(search(r'\bvirtual\b', self.modifiers_and_return_type()))
 
+    def export_macro(self):
+        export_match = match(
+            r'\b(WTF_EXPORT|WTF_EXPORT_PRIVATE|PAL_EXPORT|JS_EXPORT_PRIVATE|WEBCORE_EXPORT)\b', self.modifiers_and_return_type())
+        return export_match.group(1) if export_match else None
+
     def parameter_list(self):
         if not self._parameter_list:
             # Store the final result as a tuple since that is immutable.
@@ -1086,9 +1088,10 @@ def check_invalid_increment(clean_lines, line_number, error):
 class _ClassInfo(object):
     """Stores information about a class."""
 
-    def __init__(self, name, line_number):
+    def __init__(self, name, line_number, export_macro):
         self.name = name
         self.line_number = line_number
+        self.export_macro = export_macro
         self.seen_open_brace = False
         self.is_derived = False
         self.virtual_method_line_number = None
@@ -1357,9 +1360,9 @@ def check_for_non_standard_constructs(clean_lines, line_number,
     classinfo_stack = class_state.classinfo_stack
     # Look for a class declaration
     class_decl_match = match(
-        r'\s*(template\s*<[\w\s<>,:]*>\s*)?(class|struct)\s+(\w+(::\w+)*)', line)
+        r'\s*(template\s*<[\w\s<>,:]*>\s*)?(class|struct)(\s+(WTF_EXPORT|WTF_EXPORT_PRIVATE|PAL_EXPORT|JS_EXPORT_PRIVATE|WEBCORE_EXPORT))?\s+(\w+(::\w+)*)', line)
     if class_decl_match:
-        classinfo_stack.append(_ClassInfo(class_decl_match.group(3), line_number))
+        classinfo_stack.append(_ClassInfo(class_decl_match.group(5), line_number, class_decl_match.group(4)))
 
     # Everything else in this function uses the top of the stack if it's
     # not empty.
@@ -1659,7 +1662,7 @@ def _error_redundant_specifier(line_number, redundant_specifier, good_specifier,
           % (redundant_specifier, good_specifier))
 
 
-def check_function_definition(filename, file_extension, clean_lines, line_number, function_state, error):
+def check_function_definition(filename, file_extension, clean_lines, line_number, class_state, function_state, error):
     """Check that function definitions for style issues.
 
     Specifically, check that parameter names in declarations add information.
@@ -1701,6 +1704,22 @@ def check_function_definition(filename, file_extension, clean_lines, line_number
     if function_state.is_override and function_state.is_final:
         _error_redundant_specifier(line_number, 'override', 'final', error)
 
+    if not function_state.is_declaration:
+        if (function_state.export_macro()
+                and not function_state.export_macro() == 'JS_EXPORT_PRIVATE'):
+            error(line_number, 'build/webcore_export', 4,
+                  'Inline functions should not be annotated with %s. Remove the macro, or '
+                  'move the inline function definition out-of-line.' %
+                  function_state.export_macro())
+        elif (class_state.classinfo_stack
+                and class_state.classinfo_stack[-1].export_macro
+                and not class_state.classinfo_stack[-1].export_macro == 'JS_EXPORT_PRIVATE'):
+            error(line_number, 'build/webcore_export', 4,
+                  'Inline functions should not be in classes annotated with %s. Remove the '
+                  'macro from the class and apply it to each appropriate method, or move '
+                  'the inline function definition out-of-line.' %
+                  class_state.classinfo_stack[-1].export_macro)
+
 
 def check_for_leaky_patterns(clean_lines, line_number, function_state, error):
     """Check for constructs known to be leak prone.
@@ -3822,7 +3841,7 @@ def process_line(filename, file_extension,
     asm_state.process_line(raw_lines[line])
     if asm_state.is_in_asm():  # Ignore further checks because asm blocks formatted differently.
         return
-    check_function_definition(filename, file_extension, clean_lines, line, function_state, error)
+    check_function_definition(filename, file_extension, clean_lines, line, class_state, function_state, error)
     check_for_leaky_patterns(clean_lines, line, function_state, error)
     check_for_multiline_comments_and_strings(clean_lines, line, error)
     check_style(clean_lines, line, file_extension, class_state, file_state, enum_state, error)
@@ -3918,6 +3937,7 @@ class CppChecker(object):
         'build/using_std',
         'build/using_namespace',
         'build/cpp_comment',
+        'build/webcore_export',
         'build/wk_api_available',
         'legal/copyright',
         'readability/braces',
index c501a42..ac10cef 100644 (file)
@@ -386,6 +386,7 @@ class FunctionDetectionTest(CppStyleTestBase):
         self.assertEqual(function_state.is_pure, function_information['is_pure'])
         self.assertEqual(function_state.is_virtual(), function_information['is_virtual'])
         self.assertEqual(function_state.is_declaration, function_information['is_declaration'])
+        self.assertEqual(function_state.export_macro(), function_information['export_macro'] if 'export_macro' in function_information else None)
         self.assert_positions_equal(function_state.function_name_start_position, function_information['function_name_start_position'])
         self.assert_positions_equal(function_state.parameter_start_position, function_information['parameter_start_position'])
         self.assert_positions_equal(function_state.parameter_end_position, function_information['parameter_end_position'])
@@ -624,6 +625,75 @@ class FunctionDetectionTest(CppStyleTestBase):
              'is_pure': False,
              'is_declaration': True})
 
+    def test_webcore_export(self):
+        self.perform_function_detection(
+            ['void theTestFunctionName();'],
+            {'name': 'theTestFunctionName',
+             'modifiers_and_return_type': 'void',
+             'function_name_start_position': (0, 5),
+             'parameter_start_position': (0, 24),
+             'parameter_end_position': (0, 26),
+             'body_start_position': (0, 26),
+             'end_position': (0, 27),
+             'is_final': False,
+             'is_override': False,
+             'is_virtual': False,
+             'is_pure': False,
+             'is_declaration': True,
+             'export_macro': None})
+
+        self.perform_function_detection(
+            ['WEBCORE_EXPORT void theTestFunctionName();'],
+            {'name': 'theTestFunctionName',
+             'modifiers_and_return_type': 'WEBCORE_EXPORT void',
+             'function_name_start_position': (0, 20),
+             'parameter_start_position': (0, 39),
+             'parameter_end_position': (0, 41),
+             'body_start_position': (0, 41),
+             'end_position': (0, 42),
+             'is_final': False,
+             'is_override': False,
+             'is_virtual': False,
+             'is_pure': False,
+             'is_declaration': True,
+             'export_macro': 'WEBCORE_EXPORT'})
+
+        self.perform_function_detection(
+            ['void theTestFunctionName()',
+             '{',
+             '}'],
+            {'name': 'theTestFunctionName',
+             'modifiers_and_return_type': 'void',
+             'function_name_start_position': (0, 5),
+             'parameter_start_position': (0, 24),
+             'parameter_end_position': (0, 26),
+             'body_start_position': (1, 0),
+             'end_position': (2, 1),
+             'is_final': False,
+             'is_override': False,
+             'is_virtual': False,
+             'is_pure': False,
+             'is_declaration': False,
+             'export_macro': None})
+
+        self.perform_function_detection(
+            ['WEBCORE_EXPORT void theTestFunctionName()',
+             '{',
+             '}'],
+            {'name': 'theTestFunctionName',
+             'modifiers_and_return_type': 'WEBCORE_EXPORT void',
+             'function_name_start_position': (0, 20),
+             'parameter_start_position': (0, 39),
+             'parameter_end_position': (0, 41),
+             'body_start_position': (1, 0),
+             'end_position': (2, 1),
+             'is_final': False,
+             'is_override': False,
+             'is_virtual': False,
+             'is_pure': False,
+             'is_declaration': False,
+             'export_macro': 'WEBCORE_EXPORT'})
+
     def test_ignore_macros(self):
         self.perform_function_detection(['void aFunctionName(int); \\'], None)