Cppcheck: Reduction of False Positives with a MSVC Project File

[Luke Benes]

By using our MSVC Project file to teach cppcheck about includes and preprocessor configurations, I was able to reduce the number of warnings from 9,141 to 25. You can see the Report here:

https://drive.google.com/file/d/1NCV4Zl8vcWl30f6cDfTbGwxvW7lhMxw-/view?usp=sharing

While the results are thin, are there any valid issues here?

Keep in mind, that this is only testing the Windows code path. Also, the 'LibreOffice.sln' project is incomplete and cannot yet generate full builds.

This was an interesting experiment. But until we have a fully functional project file, we'll have to manually generate the configuration for cppcheck.

I've left my notes for anyone else interested on working on this over here:
https://wiki.documentfoundation.org/Development/Cppcheck

[Stephan Bergmann]

On 07/09/18 06:31, Luke Benes wrote:
> By using our MSVC Project file to teach cppcheck about includes and
> preprocessor configurations, I was able to reduce the number of warnings
> from 9,141 to 25.  You can see the Report here:
>
> https://drive.google.com/file/d/1NCV4Zl8vcWl30f6cDfTbGwxvW7lhMxw-/view?usp=sharing
>
> While the results are thin, are there any valid issues here?

Commenting on the referenced cppcheck_reports2/index.html:

> C:\Program Files (x86)\Windows
Kits\10\Include\10.0.17134.0\shared\WTypesbase.h
> 357 syntaxError error syntax error
> C:\Program Files (x86)\Windows Kits\10\Include\10.0.17134.0\ucrt\malloc.h
> 150 variableScope 398 style The scope of the variable '_Marker' can
be reduced.

The above two are in system code; nothing we can do about.

>
D:\cygwin\home\Hearthstone\lode\dev\core\dbaccess\win32\source\odbcconfig\odbcconfig.cxx
> 119 resourceLeak 775 error Resource leak: hModule
> 123 resourceLeak 775 error Resource leak: hModule

The above two should indeed be fixed

>
D:\cygwin\home\Hearthstone\lode\dev\core\framework\inc\helper\shareablemutex.hxx
> 73 noExplicitConstructor 398 style Class 'ShareGuard' has a
constructor with 1 argument that is not explicit.

For all the nine noExplicitConstructor occurrences: For one, what
puzzles me is Cppcheck's fixation on "1 argument" ctors, as "explicit"
has long surpassed being only relevant for ctors that can be called with
one argument; if it warns about these, why doesn't it warn about ctors
taking more arguments, too? For another, for each individual ctor, they
may be arguments for and against it being "explicit". Would need deeper
inspection, but generally feels on the level of rather unhelpful noise.

>
D:\cygwin\home\Hearthstone\lode\dev\core\framework\source\fwi\helper\shareablemutex.cxx
> 33 copyCtorPointerCopying 398 style Value of pointer 'm_pMutexRef',
which points to allocated memory, is copied in copy constructor instead
of allocating new memory.

The above is a false positive.

> D:\cygwin\home\Hearthstone\lode\dev\core\include\osl\endian.h
> 77 preprocessorErrorDirective error #error undetermined endianness

The two "preprocessorErrorDirective" are apparently fallout from
Cppcheck (not being called in a correct way and thus?) not being able to
determine which #if branch to take.

> D:\cygwin\home\Hearthstone\lode\dev\core\include\osl\mutex.hxx
> 120 noExplicitConstructor 398 style Class 'Guard' has a constructor
with 1 argument that is not explicit.
> 127 noExplicitConstructor 398 style Class 'Guard' has a constructor
with 1 argument that is not explicit.
> 154 noExplicitConstructor 398 style Class 'ClearableGuard' has a
constructor with 1 argument that is not explicit.
> 161 noExplicitConstructor 398 style Class 'ClearableGuard' has a
constructor with 1 argument that is not explicit.
> 200 noExplicitConstructor 398 style Class 'ResettableGuard' has a
constructor with 1 argument that is not explicit.
> 207 noExplicitConstructor 398 style Class 'ResettableGuard' has a
constructor with 1 argument that is not explicit.
> D:\cygwin\home\Hearthstone\lode\dev\core\include\rtl\byteseq.h
> 199 noExplicitConstructor 398 style Class 'ByteSequence' has a
constructor with 1 argument that is not explicit.
> 210 noExplicitConstructor 398 style Class 'ByteSequence' has a
constructor with 1 argument that is not explicit.

(see above)

> D:\cygwin\home\Hearthstone\lode\dev\core\include\sal\alloca.h
> 60 preprocessorErrorDirective error #error "unknown platform: please
check for alloca"

(see above)

> D:\cygwin\home\Hearthstone\lode\dev\core\sal\osl\w32\dllentry.cxx
> 224 ignoredReturnValue 252 warning Return value of function
CreateThread() is not used.
> 224 leakReturnValNotUsed 771 error Return value of allocation
function 'CreateThread' is not stored.

From the comments around the above two, they appear to be intentional.

> D:\cygwin\home\Hearthstone\lode\dev\core\sal\rtl\byteseq.cxx
> 43 variableScope 398 style The scope of the variable 'nElements' can
be reduced.

The above could indeed be fixed.

> 82 incorrectStringBooleanError 571 warning Conversion of string
literal "### null ptr!" to bool always evaluates to true.
> D:\cygwin\home\Hearthstone\lode\dev\core\sot\source\sdstor\stgavl.cxx
> 137 incorrectStringBooleanError 571 warning Conversion of string
literal "The pointer is not allowed to be NULL!" to bool always
evaluates to true.
> 149 incorrectStringBooleanError 571 warning Conversion of string
literal "The pointer is not allowed to be NULL!" to bool always
evaluates to true.
> 180 incorrectStringBooleanError 571 warning Conversion of string
literal "The pointer is not allowed to be NULL!" to bool always
evaluates to true.
> 191 incorrectStringBooleanError 571 warning Conversion of string
literal "The pointer is not allowed to be NULL!" to bool always
evaluates to true.
> 302 incorrectStringBooleanError 571 warning Conversion of string
literal "The pointers may not be NULL!" to bool always evaluates to true.

The above six are all occurrencs of the assert(X && "explanation")
idiom, and as such false positives. (Odd that there are only warnings
for these six occurrences; I'm pretty sure there's more across the code
base. Maybe that's because "the 'LibreOffice.sln' project is incomplete
and cannot yet generate full builds.")

[Kaganski Mike]

Hi!

https://gerrit.libreoffice.org/60119
https://gerrit.libreoffice.org/60122
https://gerrit.libreoffice.org/60120
https://gerrit.libreoffice.org/60121
https://gerrit.libreoffice.org/60123

are meant to take care of those that aren't false positives...

--
Best regards,
Mike Kaganski

[Stephan Bergmann]

On 07/09/18 11:26, Stephan Bergmann wrote:
> >
> D:\cygwin\home\Hearthstone\lode\dev\core\framework\inc\helper\shareablemutex.hxx
>
> > 73    noExplicitConstructor    398    style    Class 'ShareGuard' has
> a constructor with 1 argument that is not explicit.
>
> For all the nine noExplicitConstructor occurrences:  For one, what
> puzzles me is Cppcheck's fixation on "1 argument" ctors, as "explicit"
> has long surpassed being only relevant for ctors that can be called with
> one argument; if it warns about these, why doesn't it warn about ctors
> taking more arguments, too?  For another, for each individual ctor, they
> may be arguments for and against it being "explicit".  Would need deeper
> inspection, but generally feels on the level of rather unhelpful noise.

Especially, ctors that are part of the stable URE interface should
likely not be changed to be explicit, to avoid incompatible changes.
See the discussion at <https://gerrit.libreoffice.org/#/c/60123/>
"Cppcheck: make 1-argument ctors explicit".

[Luke Benes]

By manually specifying includes and preprocessor configurations, I was able to reduce the number of warning from ~9000 to 30.

https://drive.google.com/file/d/1Ai_Zcj18cdQxVIQESb4lJr5K9LnzjnpW/view?usp=sharing

You can view it by unzipping and opening 'index.html'.Did this uncover any valid issues?

The command line used to scan our code base was:

$ cppcheck -j4 -DLINUX -D__GNUC__ -DUNX -DNDEBUG -DCORE_LITTLE_ENDIAN -D__LITTLE_ENDIAN__ -D__x86_64__ -UMACOSX -UFREEBSD -U_WIN32 -i external/ -i workdir/ --includes-file=inc.txt --xml --suppressions-list=cppcheck_supp.txt --enable=all --max-configs=100 ./ 2&gt; ./err.xml

inc.txt was generated with:
$ find . -type d \( -name "inc" -o -name "include" \) |sort &gt; inc.txt

If this is useful, I could modify the weekly Cppcheck Report script to include these improvements. Who could set me up with permissions?

Ideally, the next step would be to extract the "DEFS": and "INCLUDE": from gbuild-to-ide and pass that to cppcheck. But that's for another time.

-Luke

[Mike Kaganski]

Hi!

On 9/13/2018 8:59 AM, Luke Benes wrote:
> By manually specifying includes and preprocessor configurations, I was able to reduce the number of warning from ~9000 to 30.

Great!

> Ideally, the next step would be to extract the "DEFS": and "INCLUDE": from gbuild-to-ide and pass that to cppcheck. But that's for another time.

> dbaccess/source/shared/registrationhelper.cxx
> 23    preprocessorErrorDirective        error    #error "don't build
this file directly! use dbu_reghelper.cxx instead!"

This one is because a .cxx file was processed directly. The
workdir/GbuildToJson/Library/dbulo.dll does contain
dbaccess/source/shared/dbu_reghelper in its CXXOBJECTS, but no entry for
dbaccess/source/shared/registrationhelper. So extracting this
information seems helpful, too, to reduce false positives (OTOH I wonder
what this inclusion technique is useful here for.)

--
Best regards,
Mike Kaganski

[Luke Benes]

As I mentioned before, by manually specifying  includes and preprocessor configurations, I was able to reduce the number of warning from ~9000 to 30.

https://drive.google.com/file/d/1Ai_Zcj18cdQxVIQESb4lJr5K9LnzjnpW/view?usp=sharing

You can view it by unzipping and opening 'index.html'.

Caolán or Stephan,
Could either one of you take a quick pass at this to see if my improvements are useful?

If this is useful, I could modify the weekly Cppcheck Report script to include these improvements. Who could set me up with permissions?

-Luke

[Maarten Hoes]

Hi,


If, by specifying (additional ?) include files/directories and adding
defines, you managed to bring down the massive zillions of warnings back to
just 30, it seems to me that you managed to do what the script should have
been doing all along to begin with.
;-)

Just a small remark (and you probably intend to do so already anyway):
besides changing the way cppcheck itself is called, you also make use of a
'inc.txt' file (which I assume contains the names of directories that
contain include files). I would personally re-create that file dynamically
from within the script (instead of providing a static version) on each run,
so that if in the future additional similar directories are added (or the
directory names change) these will get picked up automatically by the
script, instead of someone having to manually re-recreate the 'inc.txt' file
by hand (which people might/will forget to do). Yes, this will mean that the
script will take slightly longer to complete it's run (my personal
measurements based on the example included below are an added 15 seconds or
so currently), but since it is run automatically without any live human
being waiting for the output as it is running (and it is only run once a
week anyway as I recall) this does not really matter.

How about something like the thing below ? It would find all header files in
the libreoffice source tree, regardless of what directory they are located
in, and then use 'dirname' to get just the directory names they are located
in, and 'sort -u' to mention each directory just once. Would that be
sufficient for your suggested modifications ?


- Maarten Hoes



#!/bin/sh

TMPFILE=/tmp/tmpfile.txt
INCLUDEFILE=/tmp/incfile.txt


rm -f "$TMPFILE"


find . -type f \( -name \*.hxx -o -name \*.h \) | \
while read FILE
do
DIRNAME=$(dirname "$FILE")
echo "$DIRNAME" >> "$TMPFILE"
done

sort -u "$TMPFILE" > "$INCLUDEFILE"

cppcheck --includes-file="$INCLUDEFILE"




--
Sent from: http://document-foundation-mail-archive.969070.n3.nabble.com/Dev-f1639786.html

[Luke Benes]

In my first attempt to improve the quality of the cppcheck reports, Tamás Zolnai pointed out that including every possible header resulted in some valid warnings not being reported.

Instead, how about just including only our primary include folder of ./include with the '-Iinclude' parameter? This reduced the warnings from 4005 to 523.

You can see and compare the results by opening the 'index.html' file at
https://drive.google.com/open?id=1HaCVA_udd044uwRNn3RiyGJ01pQdYjwn

It seems many valid variableScope warnings are still being omitted. I'm still looking into that. Are there any other categories of valid errors that are missing? Specific examples would be helpful.

Overall, does this report have a higher signal-to-noise ratio than our current weekly report?


The full command to generate the report was:
../cppcheck/cppcheck -D__GNUC__ -DBOOST_ERROR_CODE_HEADER_ONLY -DBOOST_SYSTEM_NO_DEPRECATED -DCPPU_ENV=gcc3 -DLINUX -DNDEBUG -DOSL_DEBUG_LEVEL=0 -DUNIX -DUNX -DX86_64 -D_PTHREADS -D_REENTRANT -j4 -i external/ -i workdir/ -Iinclude --xml --suppressions-list=cppcheck_supp.txt --enable=all --max-configs=100 ./ 2> ./err.xml

[Jan-Marek Glogowski]

Hi Luke,

Am October 25, 2018 12:40:33 AM UTC schrieb Luke Benes <***@hotmail.com>:
>In my first attempt to improve the quality of the cppcheck reports,
>Tamás Zolnai pointed out that including every possible header resulted
>in some valid warnings not being reported.

[snip]

>It seems many valid variableScope warnings are still being omitted. I'm
>still looking into that. Are there any other categories of valid errors
>that are missing? Specific examples would be helpful.
>
>Overall, does this report have a higher signal-to-noise ratio than our
>current weekly report?

No idea, you're the expert here - probably that's easier to evaluate in comparison to your original, even shorter report.

Why not have two reports? If your final report has much less false positives and probably even generally with a higher error severity (variableScope normally doesn't indicate an error, can it?), then people can concentrate on these first.

Now I don't know how long it takes to generate them, but one can still toggle between them.

Jan-Marek

[Tamás Zolnai]

Hi Luke, all

It does not matter that variableScope is a low priority issue or a can be
dangerous . If someone want to disable a check it can be disabled
explictiely (I guess).
The main point that this change seems to simply reduce the scope of
cppcheck. If this is the purpose then we can just run cppcheck on an empty
file and so we won't see any issue (all false positives will disappear).

>Overall, does this report have a higher signal-to-noise ratio than our
>current weekly report?

@Luke
You do ask it? Was it not you how came up with the idea to reduce the false
positives with specifying the includes? Now you are asking others what the
result of your change. I'm sure it used to happen in the opposite direction.

It does not make any sense to have two reports if the only difference is
that first report contains less bugs than the other, if the filtering has
no logical meaning.

Best Regards,
Tamás

[Luke Benes]

> Was it not you how came up with the idea to reduce the false positives with specifying the includes?



No, it was not my idea. On #cppcheck, I was told by danmar, the primary developer of cppcheck, that our script is using cppcheck incorrectly. Without being passed the same include locations as we pass the compiler, we should expect a large amount of garbage.



In fact, according to the developer, we should not get any False Postives if we call cppcheck correctly. He encouraged me to file bug reports for any FP that remain, once cppcheck is being run properly.



> The main point that this change seems to simply reduce the scope of cppcheck. If this is the purpose then we can just run cppcheck on an empty file and so we won't see any issue (all false positives will disappear).



Again, No my goal is to improve the Signal-to-noise. FPs can be dangerous as in tdf#96089 and make it much harder to spot real issues.



Currently, I am in the process of comparing old cppcheck fixes with and without the '-Iinclude' option. So far, the three that I have checked would not be filtered out. In other words, had we been calling cppcheck the way I propose, these issues would have been much easier for developers to spot(4000 vs 500).


-Luke

[Kaganski Mike]

Hi Luke,

I believe that continuing the "manual approach" is moot. It was
mentioned that the correct way is to generate the proper project
structure for cppcheck using our existing tools that generate IDE
integrations (and they have correct include correct includes, deps and all).

--
Best regards,
Mike Kaganski

[Luke Benes]

> It was mentioned that the correct way is to generate the proper project
> structure for cppcheck using our existing tools that generate IDE
> integrations (and they have correct include correct includes, deps and all).

Mike,
Yes, agreed. But AFAIK, no one other than me is looking into this. Quite a few issues with the GbuildToIDE approach need to be solved.

In the meantime, our current cppcheck script generates tens of thousands of config errors (if run with check-config or verbose),reports hundred or possibly thousands of false positives, and is being called with depreciated parameters. These issues can be fixed now.

I am in the process of going through past cppcheck related commits to verify how many, if any at all, would be missed had we been using the '-I include/' parameter. So far, I have not identified any. If the consensus is that you do want to improve the script with the manual approach, I will not continue testing. Please advise.

However, keep in mind that from the GbuildToJson output, any framework that is built in the future will also call cppcheck with the '-I include/' parameter. Therefore, if there is a bug with cppcheck not reporting valid errors when called this way, then that approach too will suffer too.

[Maarten Hoes]

Hi,


slacka wrote
> Ideally, the next step would be to extract the "DEFS": and "INCLUDE": from
> gbuild-to-ide and pass that to cppcheck. But that's for another time.


Well, the very name 'gbuild-to-ide' sounds intriguing, but I can't figure
out what it is supposed to do (and how could it help in the cppcheck-report
script context) or how to make it do it's thing (I *really* can't read
python code).

So, my futile attempt was :

./bin/gbuild-to-ide --ide vim --make make


Which resulted in this:

Traceback (most recent call last):
File "./bin/gbuild-to-ide", line 1664, in <module>
gbuildparser = GbuildParser(args.makecmd).parse()
File "./bin/gbuild-to-ide", line 83, in __init__
self.binpath = os.path.dirname(os.environ['GPERF']) # woha, this is
quite a hack
File "/usr/lib64/python3.6/os.py", line 669, in __getitem__
raise KeyError(key) from None
KeyError: 'GPERF'


Appearantly, 'gbuild-to-ide' assumes some environment variables are set
(GPERF, SRCDIR, BUILDDIR, INSTDIR, and WORKDIR), but I cannot determine what
would be sane/expected values for these variables. Okay,
SRCDIR/BUILDDIR/INSTDIR seem self-explanatory (but you know what happens
when we assUme), but what are WORKDIR and GPERF ?



- Maarten




--
Sent from: http://document-foundation-mail-archive.969070.n3.nabble.com/Dev-f1639786.html

[Stephan Bergmann]

On 30/09/2018 15:04, Maarten Hoes wrote:
> So, my futile attempt was :
>
> ./bin/gbuild-to-ide --ide vim --make make
>
>
> Which resulted in this:
>
> Traceback (most recent call last):
> File "./bin/gbuild-to-ide", line 1664, in <module>
> gbuildparser = GbuildParser(args.makecmd).parse()
> File "./bin/gbuild-to-ide", line 83, in __init__
> self.binpath = os.path.dirname(os.environ['GPERF']) # woha, this is
> quite a hack
> File "/usr/lib64/python3.6/os.py", line 669, in __getitem__
> raise KeyError(key) from None
> KeyError: 'GPERF'
>
>
> Appearantly, 'gbuild-to-ide' assumes some environment variables are set
> (GPERF, SRCDIR, BUILDDIR, INSTDIR, and WORKDIR), but I cannot determine what
> would be sane/expected values for these variables. Okay,
> SRCDIR/BUILDDIR/INSTDIR seem self-explanatory (but you know what happens
> when we assUme), but what are WORKDIR and GPERF ?

That smells like gbuild-to-ide needs to be run from within gbuild's
"config_host.mk polluted" environment, which can be done via the "cmd"
make target, i.e., something like

make cmd cmd='bin/gbuild-to-ide --ide vim --make make'

[Miklos Vajna]

Hi,

On Mon, Oct 01, 2018 at 08:52:19AM +0200, Stephan Bergmann <***@redhat.com> wrote:
> That smells like gbuild-to-ide needs to be run from within gbuild's
> "config_host.mk polluted" environment, which can be done via the "cmd" make
> target, i.e., something like
>
> make cmd cmd='bin/gbuild-to-ide --ide vim --make make'

This is exactly what

make vim-ide-integration

does. :-) (Though this later first runs gbuildtojson.)

Regards,

Miklos

[Luke Benes]

Maarten,
Thanks for your suggestion here and your earlier contributions to the Cppcheck Report. I agree that we should create the include file dynamically. However the approach used by your script seems like overkill. Cppcheck already finds that quoted includes like
#include "GraphicExportFilter.hxx"
.
Also when there seems to have been a coding style that all <> includes outside of /inc folders should be defined by their relative path. Cppcheck only complains about 4 missing includes that do not follow this pattern.(see my earlier email on oddball includes).

Unless, I'm missing something, I still prefer this approach:
$ find . -type d \( -name "inc" -o -name "include" \) |sort > inc.txt

inc.txt only has ~200 entries, where as /tmp/tmpfile.txt has ~1,800 after sorting it.

-Luke

[Tamás Zolnai]

Hello Luke,

I'm not sure what your modification is doing. I just checked the report you
attached and I compared it with the full report and I see your change
filters out not only false positives. I used to use the cppcheck report to
give my students a small task for their first patch. So it would be helpful
to keep the full report (it can be kept next to the short one I guess),
otherwise I would need to run cppcheck myself.

Thanks,
Tamás

Luke Benes <***@hotmail.com> ezt írta (időpont: 2018. szept. 30., V,
5:20):

> Maarten,
> Thanks for your suggestion here and your earlier contributions to the
> Cppcheck Report. I agree that we should create the include file
> dynamically. However the approach used by your script seems like overkill.
> Cppcheck already finds that quoted includes like
> #include "GraphicExportFilter.hxx"
> .
> Also when there seems to have been a coding style that all <> includes
> outside of /inc folders should be defined by their relative path. Cppcheck
> only complains about 4 missing includes that do not follow this
> pattern.(see my earlier email on oddball includes).
>
> Unless, I'm missing something, I still prefer this approach:
> $ find . -type d \( -name "inc" -o -name "include" \) |sort > inc.txt
>
> inc.txt only has ~200 entries, where as /tmp/tmpfile.txt has ~1,800 after
> sorting it.
>
> -Luke
>
> _______________________________________________
> LibreOffice mailing list
> ***@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libreoffice
>

[Maarten Hoes]

Hi,

On Sun, Sep 30, 2018 at 11:29 AM Tamás Zolnai <***@gmail.com>
wrote:

>
> I'm not sure what your modification is doing. I just checked the report
> you attached and I compared it with the full report and I see your change
> filters out not only false positives.
>
>
Am I interpreting correctly here that the proposed approach of doing this :

$ find . -type d \( -name "inc" -o -name "include" \) |sort > inc.txt
$ cppcheck -DLINUX -D__GNUC__ -DUNX -DNDEBUG -DCORE_LITTLE_ENDIAN
-D__LITTLE_ENDIAN__ -D__x86_64__ -UMACOSX -UFREEBSD -U_WIN32 -i external/
-i workdir/ --includes-file=inc.txt --xml
--suppressions-list=cppcheck_supp.txt --enable=all --max-configs=100 ./
2> ./err.xml


Versus what the cppcheck-report script currently is doing, results in
actual real issues not being reported anymore ? If so, that's bad(TM), and
needs further investigation at the very least before that suggested change
gets implemented.


- Maarten.

[slacka]

The goal of my manual approach was to configure Cppcheck to minimize false
positives. In doing so, I was forced to only scan the Linux code base, as
only Linux has Linux system headers and does not have Window's or BSD's...
So I am not surprised that some valid issues were not reported.

There are many knobs I could tweak. For example, since my last post, I
discovered I could remove the "-DNDEBUG" to scan the debug code path. I
could also remove the "-j 4" option to allow Cppcheck to scan for unused
functions. I don't know what is most useful, and what valid issues were not
being reported. This is why I have asked the ML for feedback.

So if a dev wants give me some guidance, I could continue tweaking, or as
you suggested, we could run 2 reports.
1) a limited Linux only scan with few false positives (ala my manual
approach), and
2) a general scan with many false positives (the current Cppcheck Report).

If you try to limit the false positives with include locations without also
limiting configuration, Cppcheck gets overloaded and generates tens of
thousands of "too many configuration" errors.



--
Sent from: http://document-foundation-mail-archive.969070.n3.nabble.com/Dev-f1639786.html

[Stephan Bergmann]

On 02/10/2018 04:05, slacka wrote:
> There are many knobs I could tweak. For example, since my last post, I
> discovered I could remove the "-DNDEBUG" to scan the debug code path. I
> could also remove the "-j 4" option to allow Cppcheck to scan for unused
> functions. I don't know what is most useful, and what valid issues were not
> being reported. This is why I have asked the ML for feedback.

The knobs you can tweak are the configure options passed to LO's
autogen.sh. For example, -DNDEBUG is controlled by
--en-/disable-assert-always-abort. There is not per-se an "ideal" set
of configure options for static analysis purposes. One reasonable
approach is of course to maximize code coverage, both at the large scale
(enabling as many optional modules as possible, from e.g. --enable-kde5
to --enable-ext-nlpsolver) and at small (e.g., enabling asserts with
--enable-assert-always-abort or debug-only code blocks with
--enable-debug or even --enable-dbgutil, both of which imply
--enable-assert-always-abort).

[Tamás Zolnai]

Hi Luke,

slacka <***@hotmail.com> ezt írta (időpont: 2018. okt. 2., K, 4:05):

> The goal of my manual approach was to configure Cppcheck to minimize false
> positives. In doing so, I was forced to only scan the Linux code base, as
> only Linux has Linux system headers and does not have Window's or BSD's...
> So I am not surprised that some valid issues were not reported.
>
> There are many knobs I could tweak. For example, since my last post, I
> discovered I could remove the "-DNDEBUG" to scan the debug code path. I
> could also remove the "-j 4" option to allow Cppcheck to scan for unused
> functions. I don't know what is most useful, and what valid issues were
> not
> being reported. This is why I have asked the ML for feedback.
>

I used to find valid issues amongs the variableScope warnings for example.
Check a frequently modified module (e.g. sw, sc, sd).
One example:
https://dev-builds.libreoffice.org/cppcheck_reports/master/1501.html#line-1729
I just checked a few of these warnings now and they should be there in a
Linux specific analysis too. The linked one seems not a platform specific
or debug code. So it would be good to find out why your report does not
contains this one.

In general I doubt that a static analyzer does not find any issue (at least
some false positives) in sw module for example (if it was not cleaned up
with this analyzer earlier). You report contains no issue in sw. It seems
to me the scope of the analysis is greatly reduced by your change, that's
why it does not find a lot of issues. I'm not sure how the false positives
can be reduced by specifing the includes. Which false positives are coming
from wrong includes?

Best Regards,
Tamás

[Maarten Hoes]

Hi,

On Sun, Sep 30, 2018 at 5:20 AM Luke Benes <***@hotmail.com> wrote:

> Maarten,
> Thanks for your suggestion here and your earlier contributions to the
> Cppcheck Report. I agree that we should create the include file
> dynamically. However the approach used by your script seems like overkill.
> Cppcheck already finds that quoted includes like
> #include "GraphicExportFilter.hxx"
> .
> Also when there seems to have been a coding style that all <> includes
> outside of /inc folders should be defined by their relative path. Cppcheck
> only complains about 4 missing includes that do not follow this
> pattern.(see my earlier email on oddball includes).
>
> Unless, I'm missing something, I still prefer this approach:
> $ find . -type d \( -name "inc" -o -name "include" \) |sort > inc.txt
>
> inc.txt only has ~200 entries, where as /tmp/tmpfile.txt has ~1,800 after
> sorting it.
>
> -Luke
>
>

I was about to speak of the supposedly preferred approach that I took, for
the reasons that my approach :


1.) Does not depend on the existence of and/or adherence to any
customs/practices/coding styles, which can and do change over time, or are
simply overlooked, causing errors that people will have to correct manually
or even cause silent failure that no-one notices.
2.) Is fully dynamic, and does not have any paths 'hard-coded' into the
script, which one would have to adjust if in the future the paths/names
change. I already am worried about the fact that the approach uses
hard-coded defines (-DFOO/-UBAR) on the commandline instead of determining
them dynamically, but I currently see no way to do this differently, so I
guess that has to stay for now. Also, I guess the chance that these defines
change over time is far less likely than the chance that directory names
and locations change.


But then I decided to put my money where my mouth is, and made the
necessary changes to the cppcheck-report script, and guess what ? Apart
from approach differences, your version completed in an acceptable
execution time, whereas my approach took *ages* (after 40 minutes cppcheck
was still only about 1% completed, after which I aborted the attempt).


As of now, I cannot tell what causes this massive difference in execution
time, but the only visible difference in cppcheck output was this :


my version: Checking basctl/source/basicide/basides1.cxx:
LINUX=1;__GNUC__=1;UNX=1;NDEBUG=1;CORE_LITTLE_ENDIAN=1;__LITTLE_ENDIAN__=1;__x86_64__=1;LINUX=1;__GNUC__=1;UNX=1;NDEBUG=1;CORE_LITTLE_ENDIAN=1;__LITTLE_ENDIAN__=1;__x86_64__=1...
your version: Checking basctl/source/basicide/basides1.cxx ...

Obvious things: 'my version' listed the -D in the output, your's did not.
'My version' lists the defines *twice* (which can't be good).

I don't know what's going on here.

- Maarten

[Stephan Bergmann]

On 30/09/2018 05:20, Luke Benes wrote:
> Also when there seems to have been a coding style that all <> includes outside of /inc folders should be defined by their relative path. Cppcheck only complains about 4 missing includes that do not follow this pattern.(see my earlier email on oddball includes).

See the root README.md for "Rules for #include directives (C/C++)".