Page MenuHomePhabricator

Fix IssueHash generation
ClosedPublic

Authored by o.gyorgy on Nov 23 2015, 5:50 AM.

Details

Summary

Fixing IssueHash generation.
There were some problems with the current version.

If the Decl *D pointer is nullptr the NormalizeLine would crash while getting the LangOptions. The required LangOptions can be provided by the HTML or Plist Diagnostics.

It is possible that the column number provided by the find_first_not_of is 0 in that case the Lexer reads up a wrong source line (translateLineCol requires that column numbering should start at 1).

I think there should be an assert checking the line and col values in the sourcemanagers translateLineCol. The same way as in translateFileLineCol which does this check.

Diff Detail

Event Timeline

o.gyorgy updated this revision to Diff 40916.Nov 23 2015, 5:50 AM
o.gyorgy retitled this revision from to Fix IssueHash generation.
o.gyorgy updated this object.
o.gyorgy added a subscriber: cfe-commits.
o.gyorgy updated this revision to Diff 40920.Nov 23 2015, 6:15 AM

Regenerate patch with context and clang format HTMLDiagnostics.cpp.

xazax.hun added inline comments.Nov 24 2015, 3:32 AM
include/clang/StaticAnalyzer/Core/IssueHash.h
42

Please put & next to he variable name, here and some other places.

o.gyorgy updated this revision to Diff 41024.Nov 24 2015, 4:51 AM

Some small format changes. Based on the review.

If the Decl *D pointer is nullptr the NormalizeLine would crash while getting the LangOptions.

Do you have a reproducible test case for this?

o.gyorgy marked an inline comment as done.Nov 25 2015, 7:28 AM
o.gyorgy added a subscriber: o.gyorgy.

I did not create a test checker for the NormalizeLine error in the patch.
Should I add a test checker for this? Do you have any suggestions
where to put it if required?

GetEnclosingDeclContextSignature already checks for nullptr, this
should have been done in the NormalizeLine function also, but in that
case it is not enough because the Lexer needs the LangOpts.

I did not create a test checker for the NormalizeLine error in the patch.
Should I add a test checker for this?

I was wondering what sort of source code causes the crash, but I do think it would be good to have a regression test.

Do you have any suggestions where to put it if required?

Maybe in bug_hash_test.cpp, although I'm not sure its necessary to check the plist output when testing for a crash.

xazax.hun edited edge metadata.Nov 26 2015, 1:55 AM

I was wondering what sort of source code causes the crash, but I do think it would be good to have a regression test.

It is possible to generate bug reports without associated declaration, altough it is not advies. I think the reason why it is possible, to give flexibility. E.g.: one could report issues regarding documentation.

zaks.anna accepted this revision.Nov 30 2015, 6:03 PM
zaks.anna edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 30 2015, 6:03 PM
xazax.hun closed this revision.Dec 1 2015, 1:07 AM

Committed in r254394.