Bug 119446 – [GTK] Parse Valgrind xml leak files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brian Holt]

Implement the parsing of the xml files that are generated by Valgrind.

[Brian Holt]

Created attachment 208024 [details]
Parse Valgrind XML files

[Brian Holt]

Created attachment 208025 [details]
Diff of common.py with Chromium trunk

This is a diff (without whitespace) of common.py which has been taken from Chromium.  The code has been reformatted to PEP8/pylint and the LICENSE file has been inserted at the top of the file.

[Brian Holt]

Created attachment 208026 [details]
Diff of gdb_helper.py with Chromium trunk

As above, code has been reformatted to PEP8/pylint and the LICENSE file has been inserted at the top of the file.

[Brian Holt]

Created attachment 208027 [details]
Diff of memcheck_analyze.py with Chromium trunk

As above the code has been reformatted to PEP8/pylint.  There are 2 non-cosmetic changes to this file:

1) a hunk to make suppressions platform dependent has been removed because it was found to break the parsing of suppressions.

2) Leaks_StillReachable, InvalidWrite and InvalidRead are explicitly excluded from the report (these can be added in at a later stage if deemed interesting).

[Mario Sanchez Prada]

Comment on attachment 208024 [details]
Parse Valgrind XML files

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

Quick informal review. Mostly coding style issues and some small issues

> Tools/Scripts/webkitpy/port/common.py:37
> +import logging
> +import platform
> +import os
> +import signal
> +import subprocess
> +import sys
> +import time

Please use alphabetical order

> Tools/Scripts/webkitpy/port/common.py:39
> +

Extra line

> Tools/Scripts/webkitpy/port/common.py:43
> +
> +

Two empty lines here, while one would be enough between definitions. Please fix all across the file (it happens all the time in this patch)

> Tools/Scripts/webkitpy/port/common.py:55
> +    """ Runs a subprocess, until it finishes or |timeout| is exceeded and the

Extra space before "Runs"

> Tools/Scripts/webkitpy/port/common.py:266
> +    """ Prints out the list of used suppressions in a format common to all the

Extra space after the """

> Tools/Scripts/webkitpy/port/gdb_helper.py:40
> +

Extra line here too

> Tools/Scripts/webkitpy/port/gdb_helper.py:42
> +    ''' Parse the gdb output line, return a pair (file, line num) '''

You use ''' instead of """ in this file for documenting functions. Please use """ always (to be consistent), avoid the extra space at the beginning and the end, and finish the sentence with a period

> Tools/Scripts/webkitpy/port/gdb_helper.py:49
> +
> +

As in the other file, please use one empty line only to mark separation between defined functions

> Tools/Scripts/webkitpy/port/gdb_helper.py:51
> +    ''' For each address, return a pair (file, line num) '''

Use """ and period at the end, and avoid the extra spaces for consistency. Please fix further functions in this file too

> Tools/Scripts/webkitpy/port/gtk.py:133
> +        # FIXME: This will include too many leaks in subsequent runs until the results directory is cleared!

Why we don't just clear the directory automatically each time this logic is run, right at the start?

That way, you would have the results available after one execution as long as you haven't triggered a new one, giving you time enough to copy them to a safe location in case you want to keep them

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:35
> +

Extra line

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:47
> +        self._memcheck_leak_analyzer = MemcheckAnalyzer(port.webkit_base(),
> +                                                        use_gdb=False)

You can make this a single line

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:54
> +        return self._filesystem.glob(self._filesystem.join(directory,
> +                                                           "*-leaks.xml"))

Likewise

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:63
> +

Extra line

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:75
> +

Extra line. Please fix the rest like this one in this file

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:94
> +    prefixes_to_cut = ["build/src/", "valgrind/coregrind/",
> +                       "out/Release/../../"]

This can be made just one line, as in the original version of the file

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:140
> +            # Try using gdb

Missing period at the end

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:245
> +                self._backtraces.append([description,
> +                                         gatherFrames(node, source_dir)])

Make this one single line, as in the original version of the code

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:282
> +                buf += (frame[FUNCTION_NAME] or \
> +                        frame[INSTRUCTION_POINTER]) + "\n"

Make this one single line, as in the original version of the code

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:296
> +                    foo = TheAddressTable.GetFileLine(
> +                        frame[OBJECT_FILE],
> +                        frame[INSTRUCTION_POINTER])

Make this one line

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:302
> +                    output += (" (" + frame[SRC_FILE_DIR] +
> +                               "/" + frame[SRC_FILE_NAME] +
> +                               ":" + frame[SRC_LINE] + ")")

These three lines can be only one too

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:323
> +        # Widen suppression slightly to make portable between mac and linux
> +        # TODO(timurrrr): Oops, these transformations should happen
> +        # BEFORE calculating the hash!

Missing periods at the end of sentences

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:330
> +        # Split into lines so we can enforce length limits

Missing period

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:406
> +            if prev_line.strip() in ["</error>",
> +                                     "</errorcounts>",
> +                                     "</status>"]:

This can be one line too

[Dirk Pranke]

Comment on attachment 208024 [details]
Parse Valgrind XML files

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

I didn't review everything in this patch yet (in particular, I haven't looked at memcheck_analyze.py much. However, this code is clearly not written to generally conform to the webkitpy conventions and (IMO) should be cleaned up to do so, first.

You also should write at least some unittests for this.

> Tools/Scripts/webkitpy/port/common.py:35
> +import subprocess

This code  duplicates a lot of the work in webkitpy.common.system and webkitpy.port.server_process . Please use that code instead and/or get rid of the duplicate logic here.

>> Tools/Scripts/webkitpy/port/common.py:43
>> +
> 
> Two empty lines here, while one would be enough between definitions. Please fix all across the file (it happens all the time in this patch)

Actually PEP-8 requires two blanks between top-level definitions.

> Tools/Scripts/webkitpy/port/common.py:219
> +         "_ZN4base8internal15RunnableAdapter*Run*"),

Most of this file is pretty generic, but this routine is clearly pretty specific to one particular caller or purpose. You should rename this to something more indicative. Also, somehow it seems unlikely that the base:: functions will be needed somewhere :).

> Tools/Scripts/webkitpy/port/gdb_helper.py:31
> +''' A bunch of helper functions for querying gdb.'''

Again, you should probably be using the routines in webkitpy.system instead.

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:27
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

I would probably just fold these classes into gtk.py rather than create a separate file.

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:666
> +def _main():

Normally people use main() for this rather than _main() . Also, in webkitpy land, we wouldn't have this at all and would either split this into a separate helper script in Tools/Scripts or a _unittest.py file.

[Brian Holt]

Comment on attachment 208024 [details]
Parse Valgrind XML files

Thanks for the review and comments.  Obviously my strategy was to import (reviewed) Chromium code but I didn't realise that quite a bit of the code is already in WebKit.  I will webkitpy'ify this code and submit another patch (with unit tests).

[Brian Holt]

Created attachment 215490 [details]
WIP Patch 2

[Brian Holt]

(In reply to comment #8)
> Created an attachment (id=215490) [details]
> WIP Patch 2

As the title suggests, this is a WIP patch but the unit test for the leakdetector passes and if anyone wants to use this its good to go.

[Brian Holt]

Created attachment 218681 [details]
Patch

[Alejandro G. Castro]

Comment on attachment 218681 [details]
Patch

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

Patch is looking good, but I would like a pythong reviewer to check it. I like the result it is very useful. I'm also uploading a blocking patch to this one because I found some issue in the gtk.py leak detector. Also I would like to remove the two parameters --leaks and --wrapper=valgrind, at least for the default situation.

> Tools/ChangeLog:7
> +
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).

You just can remove this.

> Tools/Scripts/webkitpy/port/gtk.py:-163
> -    # FIXME: We should find a way to share this implmentation with Gtk,
> -    # or teach run-launcher how to call run-safari and move this down to Port.

Why are you removing this comment from the show_results_html_file function?

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:716
> +        files['/Users/mock/Library/Logs/layout-test-results/drt-28529-db92e4843be411e3bae1d43d7e01ba08-leaks.xml'] = mock_valgrind_output1
> +        files['/Users/mock/Library/Logs/layout-test-results/drt-28530-dd7213423be411e3aa7fd43d7e01ba08-leaks.xml'] = mock_valgrind_output2
> +        #files['/Users/mock/Library/Logs/layout-test-results/drt-28531-e8c7d7b83be411e390c9d43d7e01ba08-leaks.xml'] = mock_incomplete_valgrind_output
> +        #files['/Users/mock/Library/Logs/layout-test-results/drt-28532-ebc9a6c63be411e399d4d43d7e01ba08-leaks.xml'] = None
> +        #files['/Users/mock/Library/Logs/layout-test-results/drt-28532-fa6d0cd63be411e39c72d43d7e01ba08-leaks.xml'] = misformatted_mock_valgrind_output
> +        filesystem = MockFileSystem(files)

I guess this commented lines mean this is still unfinished? I would add at least a some empty output, broken output and proper output in different  unit tests.

[Zan Dobersek]

Comment on attachment 218681 [details]
Patch

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

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:59
> +    return text
> +
> +# Constants that give real names to the abbreviations in valgrind XML output.

Nit: missing a newline here.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:80
> +def gather_frames(node, source_dir):
> +    frames = []
> +    for frame in node.getElementsByTagName("frame"):
> +        frame_dict = {
> +            INSTRUCTION_POINTER: get_text_of(frame, INSTRUCTION_POINTER),
> +            OBJECT_FILE: get_text_of(frame, OBJECT_FILE),
> +            FUNCTION_NAME: get_text_of(frame, FUNCTION_NAME),
> +            SRC_FILE_DIR: get_text_of(frame, SRC_FILE_DIR),
> +            SRC_FILE_NAME: get_text_of(frame, SRC_FILE_NAME),
> +            SRC_LINE: get_text_of(frame, SRC_LINE)}
> +
> +        frames += [frame_dict]
> +    return frames

You can put the frame_dict construction inside a lambda function and then simply do 'return [frame_dict(frame) for frame in node.getElementsByTagName("frame")]'.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:122
> +        output = ""
> +
> +        output += self._kind + "\n"

You can initialize output straight to self._kind + "\n".

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:186
> +        output += "\n".join(supplines) + "\n"
> +
> +        return output

You can go straight to returning the concatenated output and the joined suppression lines.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:280
> +            os.remove(f)

You should use self._filesystem.remove(f) here.
The os module import can be removed after that.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:717
> +        leakdetector_valgrind = LeakDetectorValgrind(filesystem, '/Users/mock/Library/Logs/layout-test-results/')

Nit: You could use a simpler path for the mock results.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:724
> +        oc = OutputCapture()
> +        #output.set_log_level(logging.INFO)
> +        oc.capture_output()
> +        leakdetector_valgrind.parse_and_print_leaks_detail(files)
> +        _, _, logfile = oc.restore_output()
> +        self.assertEqual(logfile, make_mock_valgrind_results())

I think you can use OutputCapture().assert_outputs(...) here.

[Brian Holt]

Comment on attachment 218681 [details]
Patch

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

So overall it seems like a few small changes and the extra tests and this could get in.  Does the simplicification of --leaks and --wrapper="valgrind" really block this?  Can I do that in a follow-up patch?

>> Tools/Scripts/webkitpy/port/gtk.py:-163
>> -    # or teach run-launcher how to call run-safari and move this down to Port.
> 
> Why are you removing this comment from the show_results_html_file function?

Because it was copied from mac.py and doesn't make sense anymore:  why would we want to share this with Gtk when we are in gtk.py?  
Still it is unrelated to the memory leak detection so I can leave that to another patch if you prefer.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:716
>> +        filesystem = MockFileSystem(files)
> 
> I guess this commented lines mean this is still unfinished? I would add at least a some empty output, broken output and proper output in different  unit tests.

Sure.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:717
>> +        leakdetector_valgrind = LeakDetectorValgrind(filesystem, '/Users/mock/Library/Logs/layout-test-results/')
> 
> Nit: You could use a simpler path for the mock results.

Good idea.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:724
>> +        self.assertEqual(logfile, make_mock_valgrind_results())
> 
> I think you can use OutputCapture().assert_outputs(...) here.

Ok, I'll look into it.

[Brian Holt]

Created attachment 218767 [details]
Patch

[Zan Dobersek]

Comment on attachment 218767 [details]
Patch

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

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:64
> +    return text
> +
> +# Constants that give real names to the abbreviations in valgrind XML output.
> +
> +INSTRUCTION_POINTER = "ip"

I was thinking of the missing newline between the get_CDATA_of return statement and the comment.
No newline required between the comment and the INSTRUCTION_POINTER definition.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:74
> +def gather_frames(node, source_dir):
> +        frame_dict = lambda frame: {
> +            INSTRUCTION_POINTER: get_text_of(frame, INSTRUCTION_POINTER),

The indentation here is off.

[Brian Holt]

(In reply to comment #15)
> (From update of attachment 218767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218767&action=review
> 
> > Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:64
> > +    return text
> > +
> > +# Constants that give real names to the abbreviations in valgrind XML output.
> > +
> > +INSTRUCTION_POINTER = "ip"
> 
> I was thinking of the missing newline between the get_CDATA_of return statement and the comment.
> No newline required between the comment and the INSTRUCTION_POINTER definition.
> 
> > Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:74
> > +def gather_frames(node, source_dir):
> > +        frame_dict = lambda frame: {
> > +            INSTRUCTION_POINTER: get_text_of(frame, INSTRUCTION_POINTER),
> 
> The indentation here is off.

Good catch on both of them: the pep8 style checker didn't get either.

[Brian Holt]

dpranke: would you mind taking a look over this patch?

[Alejandro G. Castro]

(In reply to comment #13)
> (From update of attachment 218681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218681&action=review
> 
> So overall it seems like a few small changes and the extra tests and this could get in.  Does the simplicification of --leaks and --wrapper="valgrind" really block this?  Can I do that in a follow-up patch?
> 

Yeah, it is not added with this patch, that behaviour was added in a previous patch.

[Brian Holt]

https://bugs.webkit.org/show_bug.cgi?id=125540 created to invoke valgrind with the run-leaks script.

[Alejandro G. Castro]

Comment on attachment 218767 [details]
Patch

Brian can you add Zan improvements and upload a new patch?

[Brian Holt]

Created attachment 220221 [details]
Patch

[Brian Holt]

(In reply to comment #20)
> (From update of attachment 218767 [details])
> Brian can you add Zan improvements and upload a new patch?

Alex, did you see the new patch I uploaded with Zan's changes?  Is there anymore that you think we need or can this be committed?

[Alejandro G. Castro]

Comment on attachment 220221 [details]
Patch

LGTM, let's add it. Thanks for the patch.

[WebKit Commit Bot]

Comment on attachment 220221 [details]
Patch

Clearing flags on attachment: 220221

Committed r161512: <http://trac.webkit.org/changeset/161512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This patch broke webkitpy regression tests on Mac.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126655

[Zan Dobersek]

I wasn't able to fix all of the introduced problems, so I'll be rolling out the patch shortly.

Here's an example buildbot output that has logged both of the failures:
http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/1429/steps/webkitpy-test/logs/stdio

I was able to fix one of the failures by enforcing LeakDetectorValgrind use only in the case of leak detection option being enabled:
http://trac.webkit.org/changeset/161517
(This will be rolled out as well.)

The other failure is stranger, the output isn't really helpful, and I have no idea why it would fail.

[Alexey Proskuryakov]

Looks like one of the tests still fails after the rollout :(

[Alexey Proskuryakov]

Sorry, I was confused - the rollout helped, it's an unrelated *API* failure that we have now.

[Alejandro G. Castro]

(In reply to comment #30)
> Sorry, I was confused - the rollout helped, it's an unrelated *API* failure that we have now.

Thanks cleaning the mess ap, I think zan has some proposals of how to fix it.

[Brian Holt]

Reopening to attach new patch.

[Brian Holt]

Created attachment 220722 [details]
Patch

[Brian Holt]

(In reply to comment #33)
> Created an attachment (id=220722) [details]
> Patch

As Zan recommended on IRC, I've changed from using subprocess.Popen to Executive().run_command.  We'll confirm that this work on mac before landing.

[Brian Holt]

Reopening to attach new patch.

[Brian Holt]

Created attachment 220837 [details]
Updated to use Executive and MockExecutive in the unittest

[Zan Dobersek]

Comment on attachment 220837 [details]
Updated to use Executive and MockExecutive in the unittest

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

Looks good, but left one more advice and another request to make the unit test picklable. More details below.

With the pickle thing fixed, the unit tests pass on Mac as well.

> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:143
> +            buf = ""
> +            for frame in backtrace[1]:
> +                buf += (frame[FUNCTION_NAME] or frame[INSTRUCTION_POINTER]) + "\n"
> +
> +            input = buf.encode('latin-1').split("\n")
> +            demangled_names = [self._executive.run_command(['c++filt', '-n', name]) for name in input if name]
> +
> +            i = 0
> +            for frame in backtrace[1]:
> +                output += ("  " + demangled_names[i])
> +                i = i + 1
> +
> +                if frame[SRC_FILE_DIR] != "":
> +                    output += (" (" + frame[SRC_FILE_DIR] + "/" + frame[SRC_FILE_NAME] +
> +                               ":" + frame[SRC_LINE] + ")")
> +                else:
> +                    output += " (" + frame[OBJECT_FILE] + ")"
> +                output += "\n"

Can this be turned into one iteration over backtrace[1]?

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:820
> +        def mock_run_cppfilt_command(args):
> +            if args[0] == 'c++filt':
> +                return valgrind_output_cppfilt_map[args[2]]
> +            return ""

This should be moved into the top level so it is picklable (i.e. the test case can be serialized and transferred to a different process).
This otherwise throws a PicklingError exception when running the webkitpy tests in parallel.

https://stackoverflow.com/questions/8804830/python-multiprocessing-pickling-error

[Brian Holt]

Comment on attachment 220837 [details]
Updated to use Executive and MockExecutive in the unittest

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

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind.py:143
>> +                output += "\n"
> 
> Can this be turned into one iteration over backtrace[1]?

Not sure, but I can try.

>> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:820
>> +            return ""
> 
> This should be moved into the top level so it is picklable (i.e. the test case can be serialized and transferred to a different process).
> This otherwise throws a PicklingError exception when running the webkitpy tests in parallel.
> 
> https://stackoverflow.com/questions/8804830/python-multiprocessing-pickling-error

Thanks for pointing that out.  Strangely I noticed that this test runs fine by itself but hangs when run as part of the suite.  Maybe this is the cause...

[Brian Holt]

Reopening to attach new patch.

[Brian Holt]

Created attachment 221033 [details]
Final version?

[Brian Holt]

(In reply to comment #40)
> Created an attachment (id=221033) [details]
> Final version?

Zan, would you mind running the unit tests on Mac?  They should work now that the c++filt output is mocked.

[Zan Dobersek]

(In reply to comment #41)
> (In reply to comment #40)
> > Created an attachment (id=221033) [details] [details]
> > Final version?
> 
> Zan, would you mind running the unit tests on Mac?  They should work now that the c++filt output is mocked.

All webkitpy tests on Mac pass.

[Brian Holt]

Created attachment 221057 [details]
Diff from previous r+'ed patch

[Alejandro G. Castro]

Comment on attachment 221033 [details]
Final version?

Thanks Zan for the help with the patch, let's try it again, recall to land it manually and wait for the bots to show you a happy face ;).

[Brian Holt]

Committed r161956: <http://trac.webkit.org/changeset/161956>

[Brian Holt]

All tests passed on Mac.  There are some new layout failures that are unrelated to this change.