Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[report-converter] Smatch Parser #2968

Merged
merged 5 commits into from
Oct 19, 2020
Merged

[report-converter] Smatch Parser #2968

merged 5 commits into from
Oct 19, 2020

Conversation

jay24rajput
Copy link
Contributor

Smatch report converter tool for parsing Smatch output of kernel sources

@csordasmarton
Copy link
Contributor

There is already a different pull requests with this support request: #2967. Do you know anything about that @jay24rajput or @bulwahn? Which one should be reviewed?

@csordasmarton csordasmarton added documentation 📖 Changes to documentation. test ☑️ Adding or refactoring tests tools 🛠️ Meta-tag for all the additional tools supplied with CodeChecker: plist2html, tu_collector, etc. labels Oct 12, 2020
@bulwahn
Copy link

bulwahn commented Oct 12, 2020

Sorry, we are a larger group of developers playing around and implementing some proof of concepts and basic experiments on CodeChecker, and we implemented something here independently twice.
@jay24rajput compared the two pull requests and they are basically identical; so, I think we can drop Sudip's pull request and @jay24rajput will continue work on this pull request.

@bulwahn bulwahn mentioned this pull request Oct 12, 2020
Closed
@jay24rajput
[report-converter][cmd] Parse Smatch Output
1fd6761 
- Smatch Output is parsed and stored into the format:
   file:line message
- Parser is called from report conveter CLI with type 'smatch'
@jay24rajput jay24rajput requested a review from gyorb October 13, 2020 12:02
@jay24rajput
Copy link
Contributor Author

@gyorb @bulwahn I have made the changes suggested by you. Kindly have a look 😄

csordasmarton
csordasmarton suggested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@csordasmarton csordasmarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small tiny comments, otherwise LGTM!

tools/report-converter/README.md Show resolved Hide resolved
tools/report-converter/tests/unit/smatch_output_test_files/files/sample.c Outdated Show resolved Hide resolved
@jay24rajput
[test] Test Smatch Output with sample output file
5a04a8e 
- Smatch Parser is tested with sample.c and sample.out files
@jay24rajput
[docs] Explain how to use Smatch and it's converter on the kernel
7d83246 
- Smatch documentation in report converter explains the usage
  of smatch tool on kernel sources
- Smatch is accessible from the main readme file and supported
  code analyzer file
- Smatch and Coccinelle updated in usage
@jay24rajput
Copy link
Contributor Author

@csordasmarton there you go

csordasmarton
csordasmarton approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@csordasmarton csordasmarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jay24rajput
[report-converter] Parse Smatch ouput with checker name
6daa0e6 
- Checker name is included in the parsed smatch output
- The message no more displays the function name
@jay24rajput
Copy link
Contributor Author

@csordasmarton @gyorb @bulwahn I have added support for checker name in smatch as well. Kindly have a look

@bulwahn
Copy link

bulwahn commented Oct 15, 2020

@jay24rajput You should also add documentation how smatch will provide the rule name and which commit in smatch is required for that feature.

bulwahn
bulwahn suggested changes Oct 15, 2020
View reviewed changes
Copy link

@bulwahn bulwahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/ouput/output

@bulwahn
Copy link

bulwahn commented Oct 15, 2020

Other than that, looks good to me.

@jay24rajput
Copy link
Contributor Author

@jay24rajput You should also add documentation how smatch will provide the rule name and which commit in smatch is required for that feature.

The patch for checker name hasnt been released yet I guess. How do we make that patch available to the developers? Probably a new smatch github repo with that patch applied?

@bulwahn
Copy link

bulwahn commented Oct 15, 2020

@jay24rajput You should also add documentation how smatch will provide the rule name and which commit in smatch is required for that feature.

The patch for checker name hasnt been released yet I guess. How do we make that patch available to the developers? Probably a new smatch github repo with that patch applied?

Not a good idea; just be patient a few days and Dan will publish it. We just remember to extend documentation as soon as the patch is available in Dan's repository.

@jay24rajput
[test] Test for smatch with checker name
5538d6d 
- Smatch Parser tested with sample output containing checker name
@gyorb
Copy link
Contributor

gyorb commented Oct 15, 2020

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- tools/report-converter/codechecker_report_converter/smatch/output_parser.py  3
- tools/report-converter/codechecker_report_converter/smatch/analyzer_result.py  2
- tools/report-converter/tests/unit/test_smatch_parser.py  3
         

Clones added
============
- tools/report-converter/codechecker_report_converter/smatch/output_parser.py  1
- tools/report-converter/tests/unit/test_smatch_parser.py  3

See the complete overview on Codacy

@jay24rajput
Copy link
Contributor Author

jay24rajput commented Oct 15, 2020 •
edited
Loading

@bulwahn I will keep an eye on smatch repository and update the documentation as soon as the patch is released. I have also fixed the error in sample.out file.

@gyorb
Copy link
Contributor

gyorb commented Oct 16, 2020

Should we prepare the smatch parser for a report from an older version where the checker name is not yet available in the output or we assume the user uses the latest version with this feature? In that case maybe it would be worth to mention in the documentation which smatch version is supported.

@jay24rajput
Copy link
Contributor Author

Should we prepare the smatch parser for a report from an older version where the checker name is not yet available in the output or we assume the user uses the latest version with this feature? In that case maybe it would be worth to mention in the documentation which smatch version is supported.

Yes we assume that the user is on the latest version with the checker name available. The issue is that we are waiting for Dan( the developer of smatch) to actually push the checker name patch to his repository( which he will soon, I assume). Once that is in place, I will update the documentation of smatch accordingly

@bulwahn
Copy link

bulwahn commented Oct 16, 2020

@gyorb At this stage, we know all users (@sudipm-mukherjee) and we will only use the latest version as @jay24rajput wrote. The next users are probably still a few months in the future, so we really do not need to support an older version of smatch. The number of smatch users is probably just a few dozen.

@bulwahn
Copy link

bulwahn commented Oct 19, 2020

This is ready to be merged from our side. We will already start testing it.

csordasmarton
csordasmarton approved these changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@csordasmarton csordasmarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@csordasmarton csordasmarton merged commit 69b9f14 into Ericsson:master Oct 19, 2020
@sudipm-mukherjee
Copy link

@bulwahn @gyorb @jay24rajput: I am seeing a problem with this. The checker name from smatch is "smatch.*" like "smatch.check_uninitialized" but the generated report only lists it as "check_uninitialized". Whereas the checker name for clang contains the full checker name. You can see the reports together at https://sudipm-mukherjee.github.io/next-20201021
It will be great to have the full name so that its easy to differentiate.

@jay24rajput
Copy link
Contributor Author

@sudipm-mukherjee That should not be difficult. I just kept it this way so that smatch. does not show up every time in the report. But if this is what the community needs, I will issue a fix that will output the entire checker name

@bulwahn
Copy link

bulwahn commented Oct 21, 2020

I agree with @sudipm-mukherjee. It seems to make sense to prefix with the tool name; I think for coccinelle, we should also prefix with coccinelle as well. @jay24rajput, will you provide a pull request?

@jay24rajput
Copy link
Contributor Author

@bulwahn Alright! I will make the changes and issue a pull request

This was referenced Oct 22, 2020
Merged
Merged
@gyorb gyorb added this to the release 6.15.0 milestone Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bulwahn bulwahn bulwahn requested changes

@csordasmarton csordasmarton csordasmarton approved these changes

@bruntib bruntib Awaiting requested review from bruntib bruntib is a code owner

@dkrupp dkrupp Awaiting requested review from dkrupp dkrupp is a code owner

@gyorb gyorb Awaiting requested review from gyorb

Assignees
No one assigned
Labels
documentation 📖 Changes to documentation. test ☑️ Adding or refactoring tests tools 🛠️ Meta-tag for all the additional tools supplied with CodeChecker: plist2html, tu_collector, etc.
Projects
None yet
Milestone
release 6.15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jay24rajput @csordasmarton @bulwahn @gyorb @sudipm-mukherjee