Add Clang static analyzer (scan-build) to CI

Hello!

From fast few days I have been using scan-build to uncover bugs in xfce projects and it's quite effective. Found multiple memory leaks, use-after-free etc. issues.

It would be nice if we could add scan-build to CI itself thus any new code introducing the bug can be caught before merging.

scan-build usage: https://clang-analyzer.llvm.org/scan-build.html
Here's how we do it in wget2: https://gitlab.com/gnuwget/wget2/-/blob/master/.gitlab-ci.yml#L259

I suggest we do:

$ scan-build --force-analyze-debug-code --show-description --status-bugs -analyzer-config stable-report-filename=true -enable-checker valist,nullability,optin -o scan-build ./autogen.sh $AUTOGEN_OPTIONS
$ scan-build --force-analyze-debug-code --show-description --status-bugs -analyzer-config stable-report-filename=true -enable-checker valist,nullability,optin -o scan-build make -j4

On failure, artifacts will be in scan-build directory (See the above linked CI file)

Thanks!

mentioned in issue apps/ristretto#48 (closed)

changed the description

We can continue using gcc. Just need to install scan-build (clang-tools package) in docker container. It'll install clang as dependency but will use gcc for compilation and clang for static analysis.

Or we can add a separate job: Compilation with clang and then use scan-build in that job.

I was wondering if it wouldn't be good, on this occasion, to use both compilers, which sometimes find different warnings (maybe even errors?). I don't know if this is possible at the CI level as it is locally: the second compilation pass with Clang would simply be done during the scan-build (the first one with GCC remains unchanged).

By the way, I was also wondering if it wouldn't be possible to make the pipeline status come out as a warning (orange, not blocking, like you often see e.g. on GNOME's Gitlab) when there are compilation warnings (other than deprecation warnings).

to use both compilers, which sometimes find different warnings

+1. We can have 2 separate jobs using gcc and clang + scan-build.

make the pipeline status come out as a warning (orange, not blocking, like you often see e.g. on GNOME's Gitlab) when there are compilation warnings

+1. This can be done using allow_failure .gitlab-ci.yml variable. See https://stackoverflow.com/questions/59680450/how-to-detect-compiler-warnings-in-gitlab-ci

In both the jobs gcc and clang + scan-build we save compiler warnings to 2 separate text files.

We add third job which checks the existence of the warnings in those 2 files and existence of scan-build dir. We set allow_failure: true for this job. And before exiting the script we print which files we detected i.e. which compiler produced warnings, was it scan-build etc.

This way, compiler warnings and scan-build errors will be non-blocking but the failure will show up in pipeline with orange mark.

PS - this means dropping --status-bugs from scan-build commands suggested above.

other than deprecation warnings

Why not also include deprecation warnings? Anyways the warnings are non-blocking. And besides it serves as a nice reminder to upgrade the deprecated code.

I think we should have -Wall in CFLAGS.

Why not also include deprecation warnings?

Yes, actually I agree. I said that a bit "reflexively", because there have already been discussions showing that the subject of deprecation warnings is controversial. But maintainers who want to can very well disable these warnings in their project by various means.

I think we should have -Wall in CFLAGS.

Wouldn't it be best to follow the warnings enabled in xdt-features.m4?

Wouldn't it be best to follow the warnings enabled in xdt-features.m4?

I agree. +1

We can have these 2 jobs gcc and clang+scan-build in stage build so that these two will be run in parallel.

And third job (say warnings) in stage test so that this warnings job will be run only when both the gcc and clang+scan-build from build have completed successfully.

We add third job which checks the existence of the warnings in those 2 files and existence of scan-build dir.

On a second thought, may be we should have 3 separate jobs in test stage: gcc-warnings, clang-warnings and scan-build. So that devs don't have to actually click and go through logs to figure out which compiler produced the warnings or was it scan-build etc. Orange mark on gcc-warnings job means gcc produced warnings. Orange mark on scan-build job means there are scan-build errors etc.

So the final jobs layout could be:

gcc:
  stage: build
  script:
    - <existing build commands with `-fanalyzer` in `CFLAGS` saving compiler warnings to gcc-warnings.txt (`2> gcc-warnings.txt`)>
  artifacts:
    paths:
      - gcc-warnings.txt

clang:
  stage: build
  script:
    - <scan-build commands suggested above with saving compiler warnings to clang-warnings.txt (`2> clang-warnings.txt`) and output to clang-output.txt (`1> clang-output.txt`)>
  artifacts:
    paths:
      - clang-warnings.txt  
      - scan-build/

gcc-warnings:
  stage: test
  script:
    - <check if gcc-warnings.txt exists and has warnings which are not analyzer warnings (`grep 'warning:\|Warning:' gcc-warnings.txt | wc -l` > `grep Wanalyzer gcc-warnings.txt | wc -l`)>
    - <if true then we can cat gcc-warnings.txt if false then job passed! No gcc warnings!>
  allow_failure: true

clang-warnings:
  stage: test
  script:
    - <check if clang-warnings.txt doesn't exist>
    - <if exists then get total no. of warnings from clang-warnings.txt> (`grep warning:\|Warning: clang-warnings.txt | wc -l`)
    - <get total no. of scan-build warnings generated from clang-output.txt which has line "scan-build: #n bugs found.">
    - <if `total warnings > scan-build warnings` then job failed `cat clang-warnings.txt`>
  allow_failure: true

clang-analyzer (scan-build):
  stage: test
  script:
    - <check if scan-build dir doesn't exist>
    - <if exists then we can ask to checkout `clang` job's artifacts>
  allow_failure: true

gcc-analyzer:
  stage: test
  script:
    - <check if gcc-warnings.txt exists and has analyzer warning (`grep Wanalyzer gcc-warnings.txt`)>
    - <if yes then we can cat gcc-warnings.txt>
  allow_failure: true

That would be nice :)

One could also enable the GCC static analyzer by adding -fanalyzer to CFLAGS.

Cool! I didn't know about GCC static analyzer. Looks like they added it recently.

We should also keep an eye on related flag -fanalyzer-checker=name to enable additional checkers.

"Some checkers are disabled by default (even with -fanalyzer), such as the taint checker that implements -Wanalyzer-tainted-array-index, and this option is required to enable them." https://gcc.gnu.org/onlinedocs/gcc/Static-Analyzer-Options.html

This also means that we should add one additional job to CI, say, gcc-analyzer with allow_failure: true.

Nice find! It's available since gcc 10: https://developers.redhat.com/blog/2020/03/26/static-analysis-in-gcc-10

The current build container uses ubuntu 20.04 LTS and gcc 9. But I guess we will soon update it once the next LTS will be out…

I tried it locally on libxfce4ui, it's a bit… verbose

  CC       libxfce4kbd_private_3_la-xfce-shortcuts-provider.lo
  CC       libxfce4kbd_private_3_la-xfce-shortcuts-grabber.lo
xfce-shortcuts-grabber.c: In function ‘xfce_shortcuts_grabber_grab’:
xfce-shortcuts-grabber.c:640:26: warning: dereference of NULL ‘keys’ [CWE-476] [-Wanalyzer-null-dereference]
  640 |       g.keycode = keys[i].keycode;
      |                   ~~~~~~~^~~~~~~~
  ‘xfce_shortcuts_grabber_grab’: events 1-4
    |
    |  589 | xfce_shortcuts_grabber_grab (XfceShortcutsGrabber *grabber, XfceKey *key)
    |      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      | |
    |      | (1) entry to ‘xfce_shortcuts_grabber_grab’
    |......
    |  610 |   if (!map_virtual_modifiers (keymap, key->modifiers, &non_virtual_modifiers))
    |      |      ~
    |      |      |
    |      |      (2) following ‘false’ branch...
    |  611 |     return;
    |  612 |   if (!get_entries_for_keyval (keymap, group, key->keyval, &keys, &n_keys))
    |      |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |        |
    |      |        (3) ...to here
    |      |        (4) calling ‘get_entries_for_keyval’ from ‘xfce_shortcuts_grabber_grab’
    |
    +--> ‘get_entries_for_keyval’: event 5
           |
           |  338 | get_entries_for_keyval (GdkKeymap     *keymap,
           |      | ^~~~~~~~~~~~~~~~~~~~~~
           |      | |
           |      | (5) entry to ‘get_entries_for_keyval’
           |
         ‘get_entries_for_keyval’: events 6-7
           |
           |  347 |   *keys = NULL;
           |      |         ^
           |      |         |
           |      |         (6) ‘keys’ is NULL
           |......
           |  351 |   if (!gdk_keymap_get_entries_for_keyval (keymap, keyval, &keys1, &n_keys1))
           |      |      ~   
           |      |      |
           |      |      (7) following ‘false’ branch...
           |
         ‘get_entries_for_keyval’: event 8
           |
           |  357 |   if (G_UNLIKELY (n_keys1 <= 0))
/usr/include/glib-2.0/glib/gmacros.h:1091:27: note: in definition of macro ‘G_UNLIKELY’
           | 1091 | #define G_UNLIKELY(expr) (expr)
           |      |                           ^~~~
           |
         ‘get_entries_for_keyval’: event 9
           |
           |xfce-shortcuts-grabber.c:357:6:
           |  357 |   if (G_UNLIKELY (n_keys1 <= 0))
           |      |      ^
           |      |      |
           |      |      (9) following ‘false’ branch...
           |
         ‘get_entries_for_keyval’: events 10-16
           |
           |  372 |     group0_only = TRUE;
           |  373 |     n_matches = 0;
           |  374 |     for (i = 0; i < n_keys1; i++)
           |      |                 ~~~~~~~~~~~
           |      |                   |
           |      |                   (11) following ‘true’ branch...
           |  375 |       {
           |  376 |         group0_only &= (keys1[i].group == 0) ? TRUE : FALSE;
           |      |                              ~
           |      |                              |
           |      |                              (12) ...to here
           |......
           |  384 |         for (i = 0; i < n_keys1;)
           |      |                     ~~~~~~~~~~~
           |      |                       |
           |      |                       (13) following ‘true’ branch...
           |  385 |           if (keys1[i].group == group)
           |      |                    ~
           |      |                    |
           |      |                    (14) ...to here
           |......
           |  392 |   if (G_UNLIKELY (n_keys1 == 0))
           |      |      ~           
           |      |      |
           |      |      (15) following ‘true’ branch...
           |  393 |     {
           |  394 |       g_free (keys1);
           |      |       ~~~~~~~~~~~~~~
           |      |       |
           |      |       (16) ...to here
           |
         ‘get_entries_for_keyval’: events 17-19
           |
           |  395 |       keys1 = NULL;
           |      |             ^
           |      |             |
           |      |             (17) ‘keys1’ is NULL
           |......
           |  398 |   *keys = keys1;
           |      |   ~~~~~~~~~~~~~
           |      |         |
           |      |         (18) ‘keys1’ is NULL
           |      |         (19) ‘keys1’ is NULL
           |
    <------+
    |
  ‘xfce_shortcuts_grabber_grab’: events 20-21
    |
    |  612 |   if (!get_entries_for_keyval (keymap, group, key->keyval, &keys, &n_keys))
    |      |      ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |      | |
    |      |      | (20) returning to ‘xfce_shortcuts_grabber_grab’ from ‘get_entries_for_keyval’
    |      |      (21) following ‘false’ branch...
    |
  ‘xfce_shortcuts_grabber_grab’: event 22
    |
    |  628 |   if (G_UNLIKELY (key->n_keys != 0))
    |      |                   ~~~^~~~~~~~
    |      |                      |
    |      |                      (22) ...to here
/usr/include/glib-2.0/glib/gmacros.h:1091:27: note: in definition of macro ‘G_UNLIKELY’
    | 1091 | #define G_UNLIKELY(expr) (expr)
    |      |                           ^~~~
    |
  ‘xfce_shortcuts_grabber_grab’: events 23-27
    |
    |xfce-shortcuts-grabber.c:635:17:
    |  635 |   for (i = 0; i < n_keys;)
    |      |               ~~^~~~~~~~
    |      |                 |
    |      |                 (23) following ‘true’ branch...
    |......
    |  640 |       g.keycode = keys[i].keycode;
    |      |                   ~~~~~~~~~~~~~~~
    |      |                       |  |
    |      |                       |  (27) dereference of NULL ‘keys + (long unsigned int)i * 12’
    |      |                       (24) ...to here
    |      |                       (25) ‘keys’ is NULL
    |      |                       (26) ‘keys’ is NULL
    |
  CC       libxfce4kbd_private_3_la-xfce-shortcut-dialog.lo
  CC       libxfce4kbd_private_3_la-xfce-shortcuts.lo
  CC       libxfce4kbd_private_3_la-xfce-shortcuts-xfwm4.lo
  CCLD     libxfce4kbd-private-3.la

it's a bit… verbose

Well, there is -fanalyzer-verbosity=level but I think default level of 2 is just fine (and quite helpful actually). :)

BTW, there is one small issue with gcc-analyzer and its integration in CI:

gcc-analyzer emits warnings along with other gcc warnings unlike clang which places clang-analyzer (scan-build) warnings separate from other clang warnings.

So, in above jobs scheme where we want to allow gcc-warnings, clang-warnings, gcc-analyzer & clang-analyzer jobs to fail separately ("orange marks") it makes things a bit... not simple for gcc.

What we can do is:

1. In gcc job, build with -fsanitizer in CFLAGS saving all the warnings in gcc-warnings.txt

2. In gcc-warnings job, check a. if gcc-warnings.txt exist and b. has warnings which are not analyzer warnings. i.e. grep 'warning\|Warning' gcc-warnings.txt | wc -l > grep Wanalyzer gcc-warnings.txt | wc -l

3. In gcc-analyzer job, check a. if gcc-warnings.txt exists and b. has analyzer warning i.e. grep Wanalyzer gcc-warnings.txt

I'm not sure if there is another clean way to have these separate jobs fail... Anyways, for now I've updated jobs' layout in previous comment. :)

Won't scan-build also write to stderr, so that clang-warnings.txt could be non-empty even when there is no compiler warning? Or does one of the options in your scan-build commands above avoid this? I can't find all these options in the scan-build 13.0.1 manual, by the way.

does one of the options in your scan-build commands above avoid this?

By default, scan-build creates a directory containing HTML report files in /tmp. One can use -o (already done in original post) to select the custom directory.

BTW, this is why script in scan-build job above says "<check if scan-build dir doesn't exist>" :)

I can't find all these options in the scan-build 13.0.1 manual

Run scan-build without any arguments to see all the options. Regarding options missing in docs, I guess MR against scan-build docs is in order. :)

Sorry for being too hasty earlier...

Yes, the same problem exists with clang-warnings job where clang warnings and scan-build warnings both are saved in clang-warnings.txt.

Not sure how to get around it since apparently, there is no scan-build option to turn off printing warnings on stderr.

Can't help but think about using grep...

So I was thinking about this.

One way to do would be: (The issue is with clang-warnings job only)

1. In clang build job run scan-build command saving warnings to clang-warnings.txt (2> clang-warnings.txt) and output to clang-output.txt (1> clang-output.txt)

2. In clang-warnings job get total no. of scan-build warnings from clang-output.txt which has line "scan-build: <n> bugs found." (can use awk, grep, cut etc.)

3. Then grep warning:\|Warning: clang-warnings.txt | wc -l to get total no. of all the warnings.

4. If total warnings > scan-build warnings then job failed! cat clang-warnings.txt

PS - I'm deliberately not doing grep "[-W*]" clang-warnings.txt to directly get the clang/gcc warnings as it misses some other build warnings like the ones emitted during doc generation etc.

We should also keep an eye on related flag -fanalyzer-checker=name to enable additional checkers.

I recently found out that -fanalyzer -fanalyzer-checker=taint enables additional taint warnings but disables some default warnings from -fanalyzer.

$ man gcc doesn't document this. I've submitted the patch upstream fixing the docs: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592144.html

This method should work I think.

I recently found out that -fanalyzer -fanalyzer-checker=taint enables additional taint warnings but disables some default warnings from -fanalyzer.

Thanks a lot for picking this up, I had been fooled too. So I'll have to redo a tour of the projects I maintain :) (that also explains why I couldn't find the warning Romain mentioned above about Libxfce4ui).

mentioned in merge request !65 (merged)

mentioned in issue #47 (closed)

changed the description

There should also be a way to exclude false positives, so that they do not pollute the pipeline status until they have been fixed upstream. Probably a pattern file to be added to the repository in a conventional location, on which to then do a grep -vf.

+1

There is --exclude <path-of-dir> option in scan-build but it doesn't provide fine grain control. (Can't exclude specific warning from a file but is useful to exclude third party libs)

I think, your suggestion is very good. we can have exclude-gcc-warnings.txt and exclude-clang-warnings.txt in repo. These files shall simply contain the warning: lines verbatim (of the compiler/analyzer warnings we want to exclude).

Then we can have one additional job (e.g. sanitize-warnings) after build stage and before test stage which will edit the files clang-warnings.txt and gcc-warnings.txt (if they exist) removing all the warnings matching with the warnings specified in respective exclude-*.txt files.

While removing the warnings we should also remove the accompanying lines which follow some warnings. (Just remove all the lines starting from warning: line matched with the one from exclude-*.txt until next warning: line in the same warnings file)

Then in test stage job we can test as suggested above and if the job fails then cat the respective warning file.

BTW this one additional job (say sanitize-warnings) could also separate gcc compiler warnings from gcc analyzer warnings in, say, gcc-compiler-warnings.txt and gcc-analyzer-warnings.txt from one single file gcc-warnings.txt so that when we cat warnings in gcc-warnings job and gcc-analyzer job we only get the respective warnings.

And could also do the same for clang-warnings.txt. So when the clang-warnings job fails we only get to see clang warnings and not analyzer warnings. (cat clang-compiler-warnings.txt)

Though I'm not sure how much of this we need...

But I don't see it being a burden so may be this could indeed be used.

mentioned in merge request !112 (merged)

See !112 (merged), which should address just about everything here.

assigned to @Tamaranch

changed milestone to %Xfce 4.20

closed with commit 8bf2175d