Attachment 343251 Details for Bug 186861 – Patch

[patch] Patch

bug-186861-20180621111212.patch (text/plain), 12.34 KB, created by Keith Rollin on 2018-06-21 11:12:13 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Rollin

Created: 2018-06-21 11:12:13 PDT

Size: 12.34 KB

patch

obsolete

>Subversion Revision: 232987
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index a313fe797a205615c62cf1b4353d741023468a00..8e0bb5dbe3a6c31cded8d66980b30f90d7c00fa6 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -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-19  Keith Rollin  <krollin@apple.com>
> 
>         Crash running check-webkit-style on webrtc/.../exceptions.py
>diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp.py b/Tools/Scripts/webkitpy/style/checkers/cpp.py
>index 5836c4f4008d290e287185205e23302b12633814..4ecaba7988b6ff6d3468ab1b46eef918b8a0edb3 100644
>--- a/Tools/Scripts/webkitpy/style/checkers/cpp.py
>+++ b/Tools/Scripts/webkitpy/style/checkers/cpp.py
>@@ -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',
>diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
>index c501a42b49690f496f2f1c355dce1fc5f2a41690..ac10cefba0680e798d5fae1d301e6d911326aa3f 100644
>--- a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
>+++ b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
>@@ -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)
>

Actions: View | Formatted Diff | Diff

Attachments on bug 186861: 343179 | 343180 | 343233 | 343251