[Cppcheck 7.2.0.580 Working 100% File
Kody Coste
I've been having lots of problems trying to get the misra C warnings from the cppcheck gui. Actually this affects all the python addons. I found the source of the error (when running just misra checks) and am including it here in the hopes that someone can fix it. I'm a but rusty on how to create and submit requests but here is the fix for those interested.
Edit: The above worked when I just used the misra test in the project addons coupled with a specific path to c:\tools\python\python3.exe. When I removed python from the settings (defaulting to python in the path) and when I turned the other settings on (Cert and 2038y) they stopped working. Looks like the misra.py script is returning the opposite for success compared to the other addIns (and therefore it throws the internal error which is silently ignored)
After debugging things a bit further - here is the fix I ended up with. The comments should be self explanatory - basically misra.py returns 'false' for success while the other addons return 'true' for a successful return from the script. Ideally the misra.py should be fixed - but I an not familiar with how python scripts work. Here is the updated block of code in cppcheck.cpp while I await a fix to the misra script.
Daniel, for some unknown reason, the only way I can get misra and the other addins to work is to special case the misra return. BTW I can also confirm that having spaces in the misra text file path causes a crash. There need to be quotes placed around the parameter when it is taken out of the json settings and passed to cppcheck.exe process.
In our usage we run the addon directly via CMake so if if it finds any violation then it returns non-success and the CMake rule fails thus triggering a CI failure. If success is defined as no-exeptions then there will have to be some other method of determining that violations exist. That will require much more complex CMake rules.
My vote would be for 0 to mean no exceptions and no violations were found and then differientate the exceptions from violations via differnt return codes. This allows CMake to continue working normally.
For me it's in theory fine to return non-zero if violations are found by default. however I imagine that often addons have various violations that the user suppress/ignore. In misra there is some handling for that but the other addons just writes everything they find as far as I remember. If --addon is used then cppcheck itself will suppress the unwanted violations and it would be good if it returns with SUCCESS etc.
Hi, I tried to change misra.py main function return codes to try to fix this from your previous comments but with any return values I always get :
Bailing out from checking C:\Julien\myFile.c since there was an internal error: Failed to execute addon (command: 'python.exe "C:\Program Files\Cppcheck\addons\misra.py" --cli --rule-texts=C:/Julien/MISRA/misra_rules.txt C:\Julien\cppcheck-build-dir\myFile.a1.dump')
Using this same command from cmd line works well I can see MISRA check reports but the GUI is not reporting any MISRA errors.
Ok, the python script is working for me using command line but the issue is with :
command line using .cppcheck project -> cppcheck detects MISRA errors but does not find misra rules
using GUI -> no MISRA checks are done
Looking at the GUI code I am not sure it currently implements MISRA checks. It seems there are only two addons supported now : CLANG_ANALYZER and CLANG_TIDY.
I am not sure why it does not work for you . It works for me. Maybe there was a DATADIR problem also, I made another fix this evening to try to handle that better.
If you open the settings and look in the tab "Addons".. there you can configure the Python path and path to the misra rules file.. does it help if you configure those?
I am not sure how it should work in cmd. It seems unfortunate that you need to use --addon=misra.json. The quickest solution is to store python path and rule texts path (if configured) in the project file. I am not sure that they should be in the project file because those might be user dependent.
Note that a significant portion of the code is for handling unions, for which there is very little support in the CSA. In most cases, all fields of a union is regarded as unknown. I checked these cases by regarding unknown fields as uninitialized.
The test files also contain checks for heap allocated objects and unions, but heap regions are mostly conjured in the CSA and unions have little to no support, so the tests for them and unions should work theoretically, but its not 100%. so the tests are not yet functional (used no-warning at places where a warning is expected).
The idea looks reasonable. We're having specific warnings for, eg., assigning an uninitialized value, which fire much more often now that we have proper constructor support (eg. they'll fire in the copy constructor when the value was not initialized in the "actual" constructor). While some bugs can be found this way, i agree that it's not the best way to find them.
That said, your checker finds non-bugs, which is always risky. Sometimes the user should be allowed to initialize the field after construction but before the first use as a potentially important performance optimization. Did you find any such intentionally uninitialized fields with your checker? Were there many of those? If a user wants to have his codebase "analyzer-clean", would he be able to suppress the warning without changing the behavior of the program?
If union support is causing problems and you have a lot of workaround in the checker for these problems, i'll definitely suggest removing these workarounds from the initial commit and replacing them with FIXME comments (even if it assumes completely suppressing all warnings on classes that mention unions anywhere in the chain) because i'd much rather move towards better modeling in RegionStore than having checkers work around that. You might also be interested in D45241.
The literal 420 is repeated everywhere in this file. I think this (the same value appearing over and over again) will make debugging bad if something goes haywire and one has to look at memory dumps, control-flow-graphs, etc.
I know, and I tried to look for ways to split this checker into smaller logical chunks, but I didn't find a satisfying solution. I'll be sure to review the comments I have in the code so it's as easy as possible to understand what I did and why I did it. Also, I'd be delighted to help any way I can to guide you through it! :)
That said, your checker finds non-bugs, which is always risky. Sometimes the user should be allowed to initialize the field after construction but before the first use as a potentially important performance optimization. [...] If a user wants to have his codebase "analyzer-clean", would he be able to suppress the warning without changing the behavior of the program?
My checker is mainly a tool to enforce the rule that every field should be initialized in a C++ object. While I see that this approach could result reduced performance, I think it's fine to say that those users who find this important (by 'this' I mean not initializing every field) should not enable this checker.
I ran the checker on some projects, but it's little difficult to analyze larger ones as this checker emits very important information in notes, and those are not yet part of the plist output. But it's coming: ! I'll be sure to answer these questions as soon as I can.
As far as I understand, for every operation, the only relevant contribution of the non-last elements in the chain is the diagnostic message.
I wonder if you would build a string instead of a FieldRegion chain and only store the last FieldRegion would make this more explicit in the code.
Note that there was a comment made about the test files being too long. I still haven't split them, as I didn't find a good "splitting point". Is this okay, or shall I try to split these into numerous smaller ones?
This bug report also mentions assignment operator. But for that a warning may be not so useful. In that case the members of the assigned to object should have some initialized value already which the programmer may not want to overwrite in the assignment operator.
Added 3 new test cases to cover them. Interestingly, move constructors don't emit any warnings - the core can only assert that the fields after a move construction are valid (returns true for SVal::isValid()).
Came to think of it, I'm not 100% confident in the checkers name. It could be misleading, as this checker doesn't check constructors, but rather objects after construction. The end of a constructor call is only the point at which we know that analysis can be done.
Also, I managed to cause a crash with the class linked_ptr_internal from google's boringssl when I analyzed the grpc project. I'll look deeper into this, but I have a strong suspicion that the error lies within the CSA core.
That said, your checker finds non-bugs, which is always risky. Sometimes the user should be allowed to initialize the field after construction but before the first use as a potentially important performance optimization. Did you find any such intentionally uninitialized fields with your checker? Were there many of those?
Where I can vision the usefulness of this checker the most is code that is evolving. There are many communities and codebases where coding standards and practices aren't laid out in a well-formed manner like LLVM has. There are also projects which are traditionally C code that has evolved into C++-ish code. When moving into C++, people start to realise they can write things like constructors, so they write them, but then leave out some members, and we can only guess (and perhaps make use of other checkers related to dereference or read of undefineds) what needs to be initialised, what is used without initialisation.
795a8134c1