Tidy hash generation bugfixes #1114

bruntib · 2017-11-10T15:44:42Z

On server side the linecache Python module cached the files from which
it returned a given line. If the file changed and the file was queried
from the cache then the given line returned from the old file version.

In case of ClangTidy the bug hash is generated by CodeChecker. Because
of the previous bug a false bug hash was generated if the line number of
the bug event changed. So now the bug hash is computed on client side
and is written to the .plist file. Server will use this hash instead of
generating it again.

When a bug path changed because it is shifted a few lines downer then
the corresponding line number was only updated in BagPathEvent and
BugReportPoint tables, but not in Report table. This caused a GUI
glitch, i.e. it didn't jump to the correct position when a report was
opened.

Fixes #1106

gyorb · 2017-11-10T15:54:49Z

libcodechecker/util.py

+    Return the given line from the file. If line_no is larger than the number
+    of lines in the file then empty string returns.
+    """
+    with open(file_name) as f:


Handling the file error exceptions would be nice here.

gyorb · 2017-11-10T15:56:03Z

libcodechecker/analyze/tidy_output_converter.py

@@ -298,6 +300,18 @@ def _create_diag(message, fmap):
        diag['path'].append(PListConverter._create_event_from_note(message,
                                                                   fmap))

+        hash_content = [os.path.basename(message.path),


Please factor out the hash generation to one module and add comment why is it generated this way.

Also, why did we add code to generate the hash? Didn't we already have such code? If that is not used shouldn't we delete that?

csordasmarton · 2017-11-13T08:12:12Z

@bruntib Unit test cases are failed!

Xazax-hun

Shouldn't we have a test-case for this?

Xazax-hun · 2017-11-13T08:44:33Z

libcodechecker/analyze/tidy_output_converter.py

+                        message.message,
+                        get_line(message.path, message.line),
+                        str(message.column),
+                        str(message.column)]


Is it intentional to include the column twice?

Xazax-hun · 2017-11-13T08:48:26Z

libcodechecker/analyze/tidy_output_converter.py

@@ -298,6 +300,18 @@ def _create_diag(message, fmap):
        diag['path'].append(PListConverter._create_event_from_note(message,
                                                                   fmap))

+        hash_content = [os.path.basename(message.path),


Also, why did we add code to generate the hash? Didn't we already have such code? If that is not used shouldn't we delete that?

dkrupp

-Make sure that you create a ticket for the incorrect processing of report hashes in case the report hash is missing from the test file

dkrupp · 2017-11-13T17:54:30Z

libcodechecker/report.py

@@ -84,7 +84,7 @@ def compare_ctrl_sections(curr, prev):
        from_col = m_loc.get('col')
        until_col = m_loc.get('col')

-        line_content = linecache.getline(source_file, source_line)
+        line_content = get_line(source_file, source_line)

        if line_content == '' and not os.path.isfile(source_file):
            LOG.debug("Failed to generate report hash.")


This should be an ERROR level log

dkrupp · 2017-11-13T17:56:22Z

tests/unit/tidy_output_test_files/tidy1.plist

@@ -11,6 +11,8 @@
 			<string>clang-analyzer-core.DivideZero</string>
 			<key>description</key>
 			<string>Division by zero</string>
+			<key>issue_hash_content_of_line_in_context</key>
+			<string>b650887dabcbf08d794479d8d9c652b1</string>


Fix this hash. This hash does not contain the source code line.

dkrupp · 2017-11-13T18:00:59Z

libcodechecker/report.py

@@ -84,7 +84,7 @@ def compare_ctrl_sections(curr, prev):
        from_col = m_loc.get('col')
        until_col = m_loc.get('col')

-        line_content = linecache.getline(source_file, source_line)
+        line_content = get_line(source_file, source_line)


If the hash is generated on the server side, because the original PLIST does not contain it, then the source_file will point to the path that is only valid on the client's machine.

Either fix this, or put here a fixme and create an issue

Before the store do we overwrite the file paths in the plist files, to the directory where they are extracted, or the path changes are only done during the report processing and the actual plist files are not modified?

csordasmarton · 2017-11-14T08:31:57Z

libcodechecker/analyze/tidy_output_converter.py

@@ -9,12 +9,15 @@
 """

 import copy
+import hashlib


Unused import.

csordasmarton · 2017-11-14T08:32:58Z

libcodechecker/analyze/tidy_output_converter.py

 import json
 import os
 import plistlib
 import re

 from libcodechecker.logger import LoggerFactory
+from libcodechecker.report import generate_report_hash
+from libcodechecker.util import get_line


Unused import

csordasmarton · 2017-11-14T08:36:23Z

libcodechecker/util.py

+                if line_no == 0:
+                    return line
+            return ''
+    except:


No exception type(s) specified. For more information see Codacy.

Xazax-hun

LGTM!

On server side the linecache Python module cached the files from which it returned a given line. If the file changed and the file was queried from the cache then the given line returned from the old file version. In case of ClangTidy the bug hash is generated by CodeChecker. Because of the previous bug a false bug hash was generated if the line number of the bug event changed. So now the bug hash is computed on client side and is written to the .plist file. Server will use this hash instead of generating it again. When a bug path changed because it is shifted a few lines downer then the corresponding line number was only updated in BagPathEvent and BugReportPoint tables, but not in Report table. This caused a GUI glitch, i.e. it didn't jump to the correct position when a report was opened.

bruntib added the bugfix 🔨 label Nov 10, 2017

bruntib added this to the release 6.2 milestone Nov 10, 2017

bruntib requested review from whisperity, csordasmarton and gyorb November 10, 2017 15:44

whisperity added the analyzer 📈 Related to the analyze commands (analysis driver) label Nov 10, 2017

gyorb suggested changes Nov 10, 2017

View reviewed changes

Xazax-hun suggested changes Nov 13, 2017

View reviewed changes

bruntib force-pushed the tidy_hash_bugfix branch 2 times, most recently from 89b547b to ca8b7e0 Compare November 13, 2017 17:36

dkrupp requested changes Nov 13, 2017

View reviewed changes

bruntib force-pushed the tidy_hash_bugfix branch from ca8b7e0 to 16e8724 Compare November 13, 2017 18:06

csordasmarton reviewed Nov 14, 2017

View reviewed changes

bruntib force-pushed the tidy_hash_bugfix branch from 16e8724 to f650608 Compare November 14, 2017 09:08

Xazax-hun approved these changes Nov 14, 2017

View reviewed changes

bruntib force-pushed the tidy_hash_bugfix branch from f650608 to b7d908a Compare November 14, 2017 10:43

dkrupp merged commit 9050cb4 into Ericsson:master Nov 14, 2017

bruntib deleted the tidy_hash_bugfix branch November 14, 2017 15:18

csordasmarton mentioned this pull request Dec 8, 2017

Update plist file with report hash #1239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy hash generation bugfixes #1114

Tidy hash generation bugfixes #1114

bruntib commented Nov 10, 2017 •

edited

Loading

gyorb Nov 10, 2017

gyorb Nov 10, 2017

Xazax-hun Nov 13, 2017

csordasmarton commented Nov 13, 2017

Xazax-hun left a comment

Xazax-hun Nov 13, 2017

Xazax-hun Nov 13, 2017

dkrupp left a comment

dkrupp Nov 13, 2017

dkrupp Nov 13, 2017

dkrupp Nov 13, 2017

gyorb Nov 14, 2017

csordasmarton Nov 14, 2017

csordasmarton Nov 14, 2017

csordasmarton Nov 14, 2017

Xazax-hun left a comment

Tidy hash generation bugfixes #1114

Tidy hash generation bugfixes #1114

Conversation

bruntib commented Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csordasmarton commented Nov 13, 2017

Xazax-hun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazax-hun left a comment

Choose a reason for hiding this comment

bruntib commented Nov 10, 2017 •

edited

Loading