Attachment 348647 Details for Bug 187716 – proposed patch

[patch] proposed patch

LeaksList.txt (text/plain), 9.06 KB, created by Alexey Proskuryakov on 2018-08-31 10:27:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alexey Proskuryakov

Created: 2018-08-31 10:27:22 PDT

Size: 9.06 KB

patch

obsolete

>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 235558)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,30 @@
>+2018-08-31  Alexey Proskuryakov  <ap@apple.com>
>+
>+        run-leaks should run leaks with --list (on Mojave)
>+        https://bugs.webkit.org/show_bug.cgi?id=187716
>+        <rdar://problem/42261676>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Also enabled dumping memgraphs. We'll need to see how much space these take on
>+        build.webkit.org though.
>+
>+        * Scripts/run-leaks:
>+        (main): Added an option to store memgraphs.
>+        (runLeaks): Ad there is no way to test whether the new format is supported in advance,
>+        we have to try with --list first, and retry if that fails. Also, made leaks operate
>+        on a memgraph file if we are saving it anyway.
>+
>+        * Scripts/webkitpy/port/leakdetector.py:
>+        (LeakDetector._leaks_args): Pass --memgraph-file to run-leaks.
>+        (LeakDetector.leaks_file_name): Removed an incorrect comment.
>+        (LeakDetector.memgraph_file_name): Added.
>+        (LeakDetector.check_for_leaks): Changed how arguments are passed to _leaks_args.
>+        It is a bit ugly that leaks path ends up being computed twice, but this is the least
>+        ugly approach that I could find.
>+
>+        * Scripts/webkitpy/port/leakdetector_unittest.py: Updated for _leaks_args changes.
>+
> 2018-08-31  Daniel Bates  <dabates@apple.com>
> 
>         lldb-webkit: KeyError thrown for OptionSet with invalid value
>Index: Tools/Scripts/run-leaks
>===================================================================
>--- Tools/Scripts/run-leaks	(revision 234902)
>+++ Tools/Scripts/run-leaks	(working copy)
>@@ -34,7 +34,7 @@ use warnings;
> use File::Basename;
> use Getopt::Long;
> 
>-sub runLeaks($);
>+sub runLeaks($$);
> sub parseLeaksOutput(\@);
> sub removeMatchingRecords(\@$\@);
> sub reportError($);
>@@ -47,17 +47,20 @@ sub main()
>         "  --exclude-callstack regexp   Exclude leaks whose call stacks match the regular expression 'regexp'.\n" .
>         "  --exclude-type regexp        Exclude leaks whose data types match the regular expression 'regexp'.\n" .
>         "  --output-file path           Store result in a file. If not specified, standard output is used.\n" .
>+        "  --memgraph-file path         Store memgraph in a file.\n" .
>         "  --help                       Show this help message.\n";
> 
>     my @callStacksToExclude = ();
>     my @typesToExclude = ();
>     my $outputPath;
>+    my $memgraphPath;
>     my $help = 0;
> 
>     my $getOptionsResult = GetOptions(
>         'exclude-callstack:s' => \@callStacksToExclude,
>         'exclude-type:s' => \@typesToExclude,
>         'output-file:s' => \$outputPath,
>+        'memgraph-file:s' => \$memgraphPath,
>         'help' => \$help
>     );
>     my $pidOrExecutableName = $ARGV[0];
>@@ -74,7 +77,7 @@ sub main()
>     }
> 
>     # Run leaks tool.
>-    my $leaksOutput = runLeaks($pidOrExecutableName);
>+    my $leaksOutput = runLeaks($pidOrExecutableName, $memgraphPath);
>     if (!$leaksOutput) {
>         return 1;
>     }
>@@ -112,16 +115,28 @@ sub main()
> exit(main());
> 
> # Returns the output of the leaks tool in list form.
>-sub runLeaks($)
>+sub runLeaks($$)
> {
>-    my ($pidOrExecutableName) = @_;
>-    
>-    my @leaksOutput = `leaks $pidOrExecutableName`;
>-    if (!@leaksOutput) {
>-        reportError("Error running leaks tool.");
>-        return;
>+    my ($target, $memgraphPath) = @_;
>+
>+    if (defined $memgraphPath) {
>+        `/usr/bin/leaks \"$target\" --outputGraph=\"$memgraphPath\"`;
>+        $target = $memgraphPath;
>+    }
>+
>+    # To get a result we can parse, we need to pass --list to all versions of the leaks tool
>+    # that recognize it, but there is no certain way to tell in advance (the version of the
>+    # tool is not always tied to OS version, and --help doesn't document the option in some
>+    # of the tool versions where it's supported and needed).
>+    print STDERR "Starting leaks\n";
>+    my @leaksOutput = `/usr/bin/leaks \"$target\" --list`;
>+
>+    # FIXME: Remove the fallback once macOS Mojave is the oldest supported version.
>+    my $leaksExitCode = $? >> 8;
>+    if ($leaksExitCode > 1) {
>+        @leaksOutput = `/usr/bin/leaks \"$target\"`;
>     }
>-    
>+
>     return \@leaksOutput;
> }
> 
>Index: Tools/Scripts/webkitpy/port/leakdetector.py
>===================================================================
>--- Tools/Scripts/webkitpy/port/leakdetector.py	(revision 234902)
>+++ Tools/Scripts/webkitpy/port/leakdetector.py	(working copy)
>@@ -63,14 +63,15 @@ class LeakDetector(object):
>         ]
>         return callstacks
> 
>-    def _leaks_args(self, pid, leaks_output_path):
>+    def _leaks_args(self, process_name, process_pid):
>         leaks_args = []
>         for callstack in self._callstacks_to_exclude_from_leaks():
>             leaks_args += ['--exclude-callstack=%s' % callstack]
>         for excluded_type in self._types_to_exclude_from_leaks():
>             leaks_args += ['--exclude-type=%s' % excluded_type]
>-        leaks_args += ['--output-file=%s' % leaks_output_path]
>-        leaks_args.append(pid)
>+        leaks_args += ['--output-file=%s' % self._filesystem.join(self._port.results_directory(), self.leaks_file_name(process_name, process_pid))]
>+        leaks_args += ['--memgraph-file=%s' % self._filesystem.join(self._port.results_directory(), self.memgraph_file_name(process_name, process_pid))]
>+        leaks_args.append(process_pid)
>         return leaks_args
> 
>     def _parse_leaks_output(self, leaks_output):
>@@ -83,9 +84,11 @@ class LeakDetector(object):
>         return self._filesystem.glob(self._filesystem.join(directory, "*-leaks.txt"))
> 
>     def leaks_file_name(self, process_name, process_pid):
>-        # We include the number of files this worker has already written in the name to prevent overwritting previous leak results..
>         return "%s-%s-leaks.txt" % (process_name, process_pid)
> 
>+    def memgraph_file_name(self, process_name, process_pid):
>+        return "%s-%s.memgraph" % (process_name, process_pid)
>+
>     def count_total_bytes_and_unique_leaks(self, leak_files):
>         merge_depth = 5  # ORWT had a --merge-leak-depth argument, but that seems out of scope for the run-webkit-tests tool.
>         args = [
>@@ -115,13 +118,13 @@ class LeakDetector(object):
> 
>     def check_for_leaks(self, process_name, process_pid):
>         _log.debug("Checking for leaks in %s" % process_name)
>-        leaks_filename = self.leaks_file_name(process_name, process_pid)
>-        leaks_output_path = self._filesystem.join(self._port.results_directory(), leaks_filename)
>         try:
>+            leaks_filename = self.leaks_file_name(process_name, process_pid)
>+            leaks_output_path = self._filesystem.join(self._port.results_directory(), leaks_filename)
>             # Oddly enough, run-leaks (or the underlying leaks tool) does not seem to always output utf-8,
>             # thus we pass decode_output=False.  Without this code we've seen errors like:
>             # "UnicodeDecodeError: 'utf8' codec can't decode byte 0x88 in position 779874: unexpected code byte"
>-            self._port._run_script("run-leaks", self._leaks_args(process_pid, leaks_output_path), include_configuration_arguments=False, decode_output=False)
>+            self._port._run_script("run-leaks", self._leaks_args(process_name, process_pid), include_configuration_arguments=False, decode_output=False)
>             leaks_output = self._filesystem.read_binary_file(leaks_output_path)
>         except ScriptError as e:
>             _log.warn("Failed to run leaks tool: %s" % e.message_with_output())
>Index: Tools/Scripts/webkitpy/port/leakdetector_unittest.py
>===================================================================
>--- Tools/Scripts/webkitpy/port/leakdetector_unittest.py	(revision 234902)
>+++ Tools/Scripts/webkitpy/port/leakdetector_unittest.py	(working copy)
>@@ -41,6 +41,9 @@ class LeakDetectorTest(unittest.TestCase
>                 self._filesystem = MockFileSystem()
>                 self._executive = MockExecutive()
> 
>+            def results_directory(self):
>+                return "/tmp/"
>+
>         return MockPort()
> 
>     def _make_detector(self):
>@@ -50,8 +53,8 @@ class LeakDetectorTest(unittest.TestCase
>         detector = self._make_detector()
>         detector._callstacks_to_exclude_from_leaks = lambda: ['foo bar', 'BAZ']
>         detector._types_to_exclude_from_leaks = lambda: ['abcdefg', 'hi jklmno']
>-        expected_args = ['--exclude-callstack=foo bar', '--exclude-callstack=BAZ', '--exclude-type=abcdefg', '--exclude-type=hi jklmno', '--output-file=leaks.txt', 1234]
>-        self.assertEqual(detector._leaks_args(1234, 'leaks.txt'), expected_args)
>+        expected_args = ['--exclude-callstack=foo bar', '--exclude-callstack=BAZ', '--exclude-type=abcdefg', '--exclude-type=hi jklmno', '--output-file=/tmp/DumpRenderTree-1234-leaks.txt', '--memgraph-file=/tmp/DumpRenderTree-1234.memgraph', 1234]
>+        self.assertEqual(detector._leaks_args("DumpRenderTree", 1234), expected_args)
> 
>     example_leaks_output = """Process 5122: 663744 nodes malloced for 78683 KB
> Process 5122: 337301 leaks for 6525216 total leaked bytes.

Flags:

lforschler: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 187716: 348647