Wine 1.7.18 & Cppcheck

[Chebanenko Igor]

I scanned the source code with Cppcheck 1.61

[oiaohm]

Chebanenko Igor I don't mean to sound down on what you have done but there are some things you need to be aware of.

Running some generic tools against wine unfortunately going to give thousands on thousands of false positives unless it supports enough c. Some of it is wine style in places is horible.

Code: Select all

(style) Variable 'ok' is assigned a value that is never used.

Every one of these is completely wrong. ok is a macro not a variable and is in fact never assigned a Variable.

From the wine/test.h what ok is.

Code: Select all

#define ok_(file, line)       (winetest_set_location(file, line), 0) ? (void)0 : winetest_ok
#define ok       ok_(__FILE__, __LINE__)

Now this is why it thinks its a variable.

Code: Select all

#define todo(platform) for (winetest_start_todo(platform); \
                            winetest_loop_todo(); \
                            winetest_end_todo(platform))
#define todo_wine      todo("wine")

So a line like this

Code: Select all

todo_wine ok(hr == S_OK, "Got 0x%08x\n", hr);

This is kinda C variable shape.
Is not a Variable once its been converted by the Macros. Following is what it looks like once converted by the Macros

Code: Select all

for (winetest_start_todo("wine"); \
                            winetest_loop_todo(); \  
                            winetest_end_todo("wine"))   (winetest_set_location(__FILE__,__LINE__), 0) ? (void)0 : winetest_ok (hr == S_OK, "Got 0x%08x\n", hr);

Its a set of function calls.

Cppcheck how ever you ran it was run in a incompatible mode to wine source. What ever checking tool is used must be able to handle macros to handle wine source correcting without generating a flood of false positives to trace down..

Maybe --includes-file= pointing to the wine code base include directory might have helped.

This is a case the tool is either broken or you have used it wrong.

Code: Select all

(style) Variable 'ok' is assigned a value that is never used.

If Cppcheck is spitting out this in its reports on wine you have a problem.

[Chebanenko Igor]

oiaohm, I rescanned Wine 1.7.18 with "--include=" option. I make it with --include=/* full path to wine include folder*/

[oiaohm]

Chebanenko Igor the include has 100 percent helped its now giving some more sane errors.

Even so wine source base is still breaking Cppcheck more than its finding errors in wine.

Code: Select all

(error) syntax error 

Every one of these appears to be somewhere cppcheck has itself failed. Just from my random sampling they all appear to be this format.
#ifdef SOMEDEFINEDVALUE
A line of code using SOMEDEFINEDVALUE with no syntax errors that cppcheck is showing error on.
#endif

These are all alterations at build time. So not every one of those SOMEDEFINEDVALUE will exist on every build config or platform. Of course I have not 100 percent checked every one of those syntax error calls.

Removing all those "(error) syntax error" leaves 56 items of interest. Thinking the size of wine I am quite nicely surprised how few that is.

Chebanenko Igor next would be open up a bug with the 56 items of interest. Even so I still suspect a lot of the 56 will be partly false. Like all the test in msvcrt/tests/file.c system some of those tests are meant to break the rules to make sure functions do fail when they should.

[Chebanenko Igor]

I will make some experiments with cppcheck options to see what they will change in scanning.

[Chebanenko Igor]

My second rescanning for Wine 1.7.18.

[oiaohm]

Chebanenko Igor I would say deal with the 56 first. Without fully digging into the wine source code heavily just I know some of those are falseish when you have turn on more options.

dlls/oledb32/tests/convert.c:336

Code: Select all

BYTE src[20];

dlls/oledb32/tests/convert.c:382

Code: Select all

*(FLOAT *)src = 10.75;

(warning) Casting between integer* and float* which have an incompatible binary data representation.

This is why I say falseish. Yes the message is right. But what is going on in convert.c is C abuse for performance. src is statically allocated then used for a huge stack of different tests. Yes this allows the tests to run faster and use less ram. This kind of error inside production code sections of wine you would be asking questions but even so it may not turn out to be invalid. In test code this a performance tweak.

Even so same error in some of the production code come from the fact Windows is implemented that way.
dlls/rpcrt4/ndr_marshall.c is example of that.

Chebanenko Igor as I said running a generic tool against wine should generate a lot of noise. Mostly because windows by design is not that clean and wine test suite is highly optimised in places(yes there is a argument its over optimised in places).

dlls/oleaut32/tests/vartype.c is classic it complain about results always being true. Of course some test sections have tests like that.

Wine project is run over by Converity http://www.coverity.com/ and wine caused those pros headaches for generating false positives mostly due to being forced to implement stuff as windows does and some of wine test suite optimisations. So most of what generic tools will find will be false positives. Chebanenko Igor the Cppcheck logs basically need to be pruned major-ally to be any use to core wine developers. There also needs to be a lot of notes created on don't scan X file for X fault because its full of it and its meant to be because that is how Windows is designed in that area or is a optimisation in a test suite.

At least now you have it running what appears correctly against wine instead of first run that was just completely broken. It is scary that Cppcheck does not yell when it cannot find header files and goes forwards blindly and guesses. In fact that is a bug to report to Cppcheck developers.

There is a lot of work to alter a checking tool to match wine and not miss valid errors. As a project we are very thankful that Converity provided free scanning services to open source projects.

If you want the hardest code base to scan for errors pick something like wine.