VS Code "Run Code Analysis on Active File" support

[Burt]

Hello,

It is not clear to me whether you intend to support this key feature built into the C/C++ extension. Just to make sure we are on the same page; this feature is activated by right clicking in a file editor window and selecting the option I show in quotes in the Subject line above. [Or it can be configured to run automatically without a right click when a file is opened, but that is not the default.] If you attempt to run this on files in the nRF Connect SDK, for example, I like to use location_module.c in the asset_tracker_v2 application, you will find that it does what it is supposed to do...it finds errors. It finds errors that should have been corrected 2 or 3 years ago. Not only are those errors misleading, but they will likely prevent code analysis from finding more interesting errors/warnings that I may have made while developing code.

My first fix is not perfect, but it gets rid of a problem that has plagued the problematic C/C++ extension for years despite people looking at the issue and thinking they have fixed it. Now, this only happens in Windows; not on Linux: I forget whether it happens on Mac. Without further ado, the fix is:

diff -Naur toolchains0/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/lib/gcc/arm-zephyr-eabi/12.2.0/include/stddef.h toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/lib/gcc/arm-zephyr-eabi/12.2.0/include/stddef.h
--- toolchains0/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/lib/gcc/arm-zephyr-eabi/12.2.0/include/stddef.h 2023-05-11 13:33:12.000000000 -0400
+++ toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/lib/gcc/arm-zephyr-eabi/12.2.0/include/stddef.h 2023-11-25 00:20:16.950551600 -0500
@@ -211,7 +211,12 @@
#define __SIZE_TYPE__ long unsigned int
#endif
#if !(defined (__GNUG__) && defined (size_t))
+#if __CDT_PARSER__ != 1
typedef __SIZE_TYPE__ size_t;
+#else
+#define __GNUC__ 12
+typedef unsigned long long size_t;
+#endif
#ifdef __BEOS__
typedef long ssize_t;
#endif /* __BEOS__ */

Note that I have used your __CDT_PARSER__ trick to change the type of size_t to unsigned long long for only the IntelliSense/clang-tidy case and I leave the existing code in place for regular builds. Please understand that the __GNUC__ definition is here to solve another IntelliSense issue that I will show you after you have had a chance to digest this change.

To see the usefulness of this, on Windows, please run Code Analysis on a source file that is part of asset_tracker_v2, and then look closely how the problem list has shortened from when you ran Code Analysis without the fix.

The downside is that size_t should not really be unsigned long long, but as it is a bug to re-type a variable except in clang with -Wno-typedef-redefinition, we are forced to use this. Actually, you can take my fix above but leave out the "typedef unsigned long long size_t;" line, since it is merely a re-typing without any change (not a bug to re-type to the same type).

Please give me a schedule for either using my fix or your own fix to solve the issue on Windows I have shown. Thank you.

[Burt]

OOOOOOOOOOOOOOOHHHHHHHHHHHHHHHHHHHHHHHHHHHHH now I see what's going on (sort of). Boss, your fix has a problem. I hadn't tested it enough when you gave it to me (famous last words?) I switched back to the ugly fix that I had started with. Then I could remove not only the target extra-arg, but also --system-headers. Then when I initiated code analysis, the warning for strcat showed up in the OUTPUT without all the goop related to the system headers, and at the same time, the warnings showed up in the editor window.

What's the next step?

Burt

[Trond Einar Snekvik]

I'm seeing the same thing as you, where --system-headers lets us see extra failures in app_module_event.c in the output along with a bunch of errors in the system headers, but not in the editor. This appears to fix it for me though:

 "C_Cpp.codeAnalysis.clangTidy.args": [
    "--extra-arg=--target=arm-zephyr-eabi-gcc",
 ],
 "C_Cpp.codeAnalysis.clangTidy.checks.enabled": ["*"],

I'm not sure what the extension does to the checks here, but it looks like passing checks this way changes the post processing somehow.
Now, I get 18 failures in app_module_event.c showing up in the editor, and no errors in system headers.
It's not providing particularly interesting warnings about strcat, so I'm not sure if this is what you were looking for.

[Burt]

Thanks very much, Trond. strcat warnings are kind of boring, especially in app_module_event.c where the authors put enough code around the strcat() to make it work like strncat(). But maybe my teammate (he writes most of the code) would use strcat() less carefully. Anyway, 10 years ago we were doing static analysis at IBM and there was a lot of grunt work involving strcat() cleanup and etc. 

If I add --system-headers to the args, and perhaps disable some checks to keep the OUTPUT from being huge...well, the strange thing about not marking strcat in the editor is that I can "Follow link(ctrl + click)" in the OUTPUT strcat warning and it will take me up to the strcat in the editor window. So I wonder if it is by design that the strcat warning does not show in the editor window, or is that more or less a bug. That's my only question.

Thanks again, Trond--the help has been greatly appreciated.

Burt

[Burt]

Trond, I opened a ticket #11757 in vscode-cpptools, and after a hint from Sean McManus and some cogitating on my part, I learned about macro _FORTIFY_SOURCE. I found that if I add
--extra-arg=-D_FORTIFY_SOURCE=0
to the clang-tidy args in settings.json, then the strcat [and I guess some other possible ones] warnings will get put into PROBLEMS/squiggles. So I guess I am overwriting the -D_FORTIFY_SOURCE=1 that comes out of the nrf-connect configuration provider. [I am slightly confused about options to clang vs options to the target compiler but I know that --extra-arg works and --extra-arg-before does not work.]

Is it best to have -D_FORTIFY_SOURCE=1 in the nrf-connect options and force the user to know about the extra-arg method--or otherwise?

I want to go back, reenable more checks and then see if there are other macros besides _FORTIFY_SOURCE that lead to warnings not gettng to PROBLEMS/squiggles.

Burt

[Burt]

The ATOMIC_BITMAP_SIZE fix has been approved by one Zephyr Project reviewer after he had me modify it to

  #define ATOMIC_BITMAP_SIZE ROUND_UP(num_bits, ATOMIC_BITS)/ATOMIC_BITS

Funny, none of the 3 definitions, yours, mine, and Andy Ross's are mathematically identical but they all keep away crazy large positive numbers.

I'll close this ticket as I have everything I need at this point.