updated Cppcheck to 2.12.1 / provide more includes to Cppcheck #2785

firewave · 2023-09-04T10:40:44Z

Cppcheck is by design lenient with incorrect and incomplete code. This allows code to be scanned more easily but without all the code provided it might not be possible to detect certain issues. An indicator for this is the missingInclude warning.

This provides the most common missing headers to the analysis and fixes the additionally detected issues. Unfortunately it causes the time of the analysis to balloon but that is expected.

firewave · 2023-09-04T10:42:48Z

There's several more includes to add but I wanted to keep it to the basics first.

I still have to look into the shiftTooManyBits warnings - it is possible these might be false positives.

And I will prepare an upstream PR for the warnings in ulalaca - if necessary.

matt335672 · 2023-09-04T11:00:51Z

Thanks @firewave

A couple of initial comments. This isn't a full review, but I like what you're doing here.

Increase in time is fine. We're using C, so a more detailed static analysis is always welcome.
Suggest you add an ignore for ulalaca for now. @unstabler - is this OK with you?

firewave · 2023-09-04T13:56:12Z

Increase in time is fine. We're using C, so a more detailed static analysis is always welcome.

It is a bit steep though. I will take a look into it. Maybe there is something to improve upstream.

Suggest you add an ignore for ulalaca for now.

Done. Upstream PR will follow soon.

firewave · 2023-09-04T18:19:24Z

The shiftTooManyBits appears to be a false positive. I filed an upstream ticket about it: https://trac.cppcheck.net/ticket/11916 and suppress it for now.

firewave · 2023-09-04T18:32:08Z

This is complete for now. Please have a close look at the nullPointerRedundantCheck fixes.

I will let it soak for a day before I will flag it as ready so I can have a fresh look at it tomorrow.

matt335672

A couple of comments. Your changes to the clipboard code look fine to me. There may be some other dragons in this code, but let's not worry about those now.

matt335672 · 2023-09-06T08:40:48Z

scripts/run_cppcheck.sh

@@ -45,6 +45,7 @@ if [ -z "$CPPCHECK_FLAGS" ]; then
    CPPCHECK_FLAGS="--quiet --force --std=c11 --std=c++11 --inline-suppr \
                    --enable=warning --error-exitcode=1 -i third_party \
                    --suppress=uninitMemberVar:ulalaca/ulalaca.cpp \
+                    --suppress=shiftTooManyBits:libxrdp/xrdp_mppc_enc.c \


Personally I'd prefer to add this inline to the code - see (e.g.) sesman/chansrv/chansrv_fuse.c:2779 for an example. Or is there a good reason why it's better here?

No real reason. Possibly because it is several warnings and I suppressed the other ones like this.

matt335672 · 2023-09-06T08:55:21Z

sesman/chansrv/clipboard.c

            if (cptr == NULL)
            {
+                g_free(g_clip_s2c.data);
                g_clip_s2c.data = 0;

                /* cannot add any more data */


Move this comment to start of block?

firewave · 2023-09-09T18:09:16Z

To mitigate the performance regression a bit I build Cppcheck with Boost enabled. To make sure it is being rebuild I added -boost to the cache key. That can be removed when you update to the next version. This reduces the runtime from >10 minutes to <7 minutes. It used to be ~1 minute.

There are possible further optimizations like LTO, -O3 or using Clang as compiler as well as PGO/BOLT but most of these have not yet been properly evaluated yet. I will add more information about these to the Cppcheck documentation in the future.

matt335672 · 2023-09-10T13:00:26Z

That's a worthwhile optimisation.

As it happens cppcheck 2.12.0 has been released today (I've got a watch on the repo for new releases).

I've just run 2.12.0 over the existing devel branch and using the existing scripts no new errors were found. Do you want to add 2.12.0 support in to this PR? We can then merge it in one go.

firewave · 2023-09-10T15:00:43Z

That's a worthwhile optimisation.

I also just realized you are not using the matchcompiler. Will add it as well.

As it happens cppcheck 2.12.0 has been released today (I've got a watch on the repo for new releases).

I've just run 2.12.0 over the existing devel branch and using the existing scripts no new errors were found. Do you want to add 2.12.0 support in to this PR? We can then merge it in one go.

Already done. Unfortunately the performance regressions in 2.12 (caused by more coverage in the analysis) are more than doubling the analysis time...

firewave · 2023-09-10T15:17:14Z

Already done. Unfortunately the performance regressions in 2.12 (caused by more coverage in the analysis) are more than doubling the analysis time...

With the matchcompiled code this drops from > 15 minutes to ~9 Minutes.

matt335672 · 2023-09-20T09:29:34Z

@firewave - this PR is still in draft. Are you proposing to move it out of draft?

PS: cppcheck 2.12.1 has just bee released, but I don't think it's relevant here.

firewave · 2023-09-20T09:33:29Z

@firewave - this PR is still in draft. Are you proposing to move it out of draft?

Sure. I was still waiting for feedback.

Also I saw that you are using merge commits. That would lead to all the individual commits to show up which might not be intended. So far I have mainly worked on projects with "squash merge" and only did the commits so the changes are easier to track. What the desired results of this merge?

PS: cppcheck 2.12.1 has just bee released, but I don't think it's relevant here.

I could update. That would also allow me to drop the -mc suffix on the cache key which I do not want to have in.

matt335672 · 2023-09-20T10:45:05Z

Well, I'm OK with this so I'd be happy to merge it. I can't do that until it's out of draft.

By all means update to 2.12.1 if that makes things easier.

In terms of commits, are you able to re-arrange to 2 - one for the 2.12.1 update and one for the other changes? If not let me know and we'll work something else out.

firewave · 2023-09-20T13:16:28Z

Well, I'm OK with this so I'd be happy to merge it. I can't do that until it's out of draft.

By all means update to 2.12.1 if that makes things easier.

Not easier but will get rid of a unnecessary nit. Will do.

In terms of commits, are you able to re-arrange to 2 - one for the 2.12.1 update and one for the other changes? If not let me know and we'll work something else out.

Will do as well.

firewave · 2023-10-16T19:08:37Z

Okay - something went wrong with the rebase. Because of the multi-repo stuff my IDE cannot do the Gi>Hub sync/rebase and I have to figure out how to clean this up via the command-line.

matt335672 · 2023-10-17T08:12:38Z

Let me know if you need a hand to sort it out. I'm a bit busy today in the real world, but I should have some time tomorrow.

- added (temporary) suppression of Cppcheck `shiftTooManyBits` false positives in `libxrdp/xrdp_mppc_enc.c` - added (temporary) suppression of Cppcheck `uninitMemberVar` true positives in `ulalaca/ulalaca.cpp` until fixes land downstream - fix Cppcheck `nullPointerRedundantCheck` in `sesman/chansrv/clipboard.c` - fix Cppcheck `syntaxError` in `fontutils/mkfv1.c` because it doesn't see the `freetype/fterrors.h` header / removed astyle workaround

- build Cppcheck with matchcompiler for improved performance - build Cppcheck with Boost for improved ValueFlow performance

firewave · 2023-10-17T13:22:18Z

Let me know if you need a hand to sort it out. I'm a bit busy today in the real world, but I should have some time tomorrow.

Thanks. Turns out I just had to rebase against upstream.

matt335672 · 2023-10-18T08:09:59Z

@firewave - thanks for your contribution!

matt335672 · 2023-10-18T08:13:03Z

@metalefty - do you want this backported to v0.9?

metalefty · 2023-10-18T09:02:23Z

Yes, let's ship it.

matt335672 · 2023-10-18T09:43:50Z

Done.

firewave force-pushed the cppcheck-inc branch from 7741c7e to 6d318e8 Compare September 4, 2023 13:54

firewave force-pushed the cppcheck-inc branch from 6d318e8 to 11d2ceb Compare September 4, 2023 18:16

firewave force-pushed the cppcheck-inc branch from 11d2ceb to 9320300 Compare September 4, 2023 18:30

matt335672 reviewed Sep 6, 2023

View reviewed changes

firewave force-pushed the cppcheck-inc branch 2 times, most recently from d970bd2 to 13a6be0 Compare September 9, 2023 17:54

firewave force-pushed the cppcheck-inc branch from 455a5e2 to dce3312 Compare September 10, 2023 15:02

firewave changed the title ~~provide more includes to Cppcheck~~ updated Cppcheck to 2.12.0 / provide more includes to Cppcheck Sep 10, 2023

firewave force-pushed the cppcheck-inc branch from 3fcda50 to d6179a8 Compare October 16, 2023 19:06

firewave added 2 commits October 17, 2023 15:14

bumped Cppcheck to 2.12.1

8c21428

- build Cppcheck with matchcompiler for improved performance - build Cppcheck with Boost for improved ValueFlow performance

firewave force-pushed the cppcheck-inc branch from d6179a8 to 8c21428 Compare October 17, 2023 13:15

firewave changed the title ~~updated Cppcheck to 2.12.0 / provide more includes to Cppcheck~~ updated Cppcheck to 2.12.1 / provide more includes to Cppcheck Oct 17, 2023

firewave marked this pull request as ready for review October 17, 2023 13:37

matt335672 merged commit da8fa05 into neutrinolabs:devel Oct 18, 2023
13 checks passed

firewave deleted the cppcheck-inc branch October 18, 2023 08:37

matt335672 mentioned this pull request Oct 18, 2023

[V0.9] updated Cppcheck to 2.12.1 / provide more includes to Cppcheck #2830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated Cppcheck to 2.12.1 / provide more includes to Cppcheck #2785

updated Cppcheck to 2.12.1 / provide more includes to Cppcheck #2785

firewave commented Sep 4, 2023

firewave commented Sep 4, 2023 •

edited

Loading

matt335672 commented Sep 4, 2023

firewave commented Sep 4, 2023

firewave commented Sep 4, 2023

firewave commented Sep 4, 2023

matt335672 left a comment

matt335672 Sep 6, 2023

firewave Sep 6, 2023

matt335672 Sep 6, 2023

firewave Sep 6, 2023

firewave Sep 9, 2023

firewave commented Sep 9, 2023

matt335672 commented Sep 10, 2023

firewave commented Sep 10, 2023

firewave commented Sep 10, 2023

matt335672 commented Sep 20, 2023

firewave commented Sep 20, 2023

matt335672 commented Sep 20, 2023

firewave commented Sep 20, 2023

firewave commented Oct 16, 2023

matt335672 commented Oct 17, 2023

firewave commented Oct 17, 2023

matt335672 commented Oct 18, 2023

matt335672 commented Oct 18, 2023

metalefty commented Oct 18, 2023

matt335672 commented Oct 18, 2023

updated Cppcheck to 2.12.1 / provide more includes to Cppcheck #2785

updated Cppcheck to 2.12.1 / provide more includes to Cppcheck #2785

Conversation

firewave commented Sep 4, 2023

firewave commented Sep 4, 2023 • edited Loading

matt335672 commented Sep 4, 2023

firewave commented Sep 4, 2023

firewave commented Sep 4, 2023

firewave commented Sep 4, 2023

matt335672 left a comment

Choose a reason for hiding this comment

matt335672 Sep 6, 2023

Choose a reason for hiding this comment

firewave Sep 6, 2023

Choose a reason for hiding this comment

matt335672 Sep 6, 2023

Choose a reason for hiding this comment

firewave Sep 6, 2023

Choose a reason for hiding this comment

firewave Sep 9, 2023

Choose a reason for hiding this comment

firewave commented Sep 9, 2023

matt335672 commented Sep 10, 2023

firewave commented Sep 10, 2023

firewave commented Sep 10, 2023

matt335672 commented Sep 20, 2023

firewave commented Sep 20, 2023

matt335672 commented Sep 20, 2023

firewave commented Sep 20, 2023

firewave commented Oct 16, 2023

matt335672 commented Oct 17, 2023

firewave commented Oct 17, 2023

matt335672 commented Oct 18, 2023

matt335672 commented Oct 18, 2023

metalefty commented Oct 18, 2023

matt335672 commented Oct 18, 2023

firewave commented Sep 4, 2023 •

edited

Loading