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.

[Trond Einar Snekvik]

Hi,

Thanks for proposing a solution to this. We have actually encountered this too, but our solution is a little different:

It's possible to set a --target for clang, to let it know what rules it should apply for the output: https://clang.llvm.org/docs/CrossCompilation.html 

This can actually be passed to the C/C++ extension through its configuration setting "C_Cpp.codeAnalysis.clangTidy.args" by giving it --extra-arg=--target=arm-zephyr-eabi-gcc, like this:

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

With this setting, the patch actually isn't necessary at all for me, as it lets clang-tidy runs in cross-compilation mode, where it's able to use the correct word size.

We think this is the intended way to solve this problem in clang, but our problem is that there's no way to pass this to the C/C++ extension programmatically without writing to the vscode settings. We could do that, but we probably shouldn't write it to the user settings, as this might interfere with other environments, and as we want users to be able to check their .vscode/settings.json file into source control, we don't want to start editing it without the user initiating the change.

I'd be interested in hearing your suggestions (and thoughts about this solution to the problem), as our problem is really just in the way we distribute this fix. We would struggle to get a patch into the suggested location, as it's part of the GCC compiler's headers.

Trond 

- VS Code team lead

[Burt]

Thank you very much, Trond. Your fix is great, and definitely an improvement over my hack. It also made two other code analysis problems disappear--more about that in a moment. I ended up adding the fix to the settings.json associated with my application. Are there situations in which people are developing an nRF Connect application but it would be inappropriate to have this setting? I mean, is there any way that when they create an application in nRF Connect for VS Code, or maybe coupled with the toolchain that they selected, that it would not make sense to add this setting to the application's settings.json, thus prioritizing it over the source control issue? If this is impossible, then perhaps the user documentation needs a "Hints and Kinks" section that explains what you have told me and shows an example settings.json (at first, I wrongly tried adding the json to c_cpp_properties.json). And somehow a not too intrusive pop up in VS Code at the right times to make people aware of the Hints and Kinks.

Here are the issues that disappeared in addition to above:

$ git diff
diff --git a/CMSIS/Core/Include/cmsis_gcc.h b/CMSIS/Core/Include/cmsis_gcc.h
index 67bda4e..5e265b9 100644
--- a/CMSIS/Core/Include/cmsis_gcc.h
+++ b/CMSIS/Core/Include/cmsis_gcc.h
@@ -1615,9 +1615,9 @@ __STATIC_FORCEINLINE void __set_FPSCR(uint32_t fpscr)
{
#if ((defined (__FPU_PRESENT) && (__FPU_PRESENT == 1U)) && \
(defined (__FPU_USED ) && (__FPU_USED == 1U)) )
-#if __has_builtin(__builtin_arm_set_fpscr)
-// Re-enable using built-in when GCC has been fixed
-// || (__GNUC__ > 7) || (__GNUC__ == 7 && __GNUC_MINOR__ >= 2)
+#if __has_builtin(__builtin_arm_set_fpscr) \
+/* Re-enable using built-in when GCC has been fixed */ \
+ || (__GNUC__ > 7) || (__GNUC__ == 7 && __GNUC_MINOR__ >= 2)
/* see gcc.gnu.org/.../msg00443.html */
__builtin_arm_set_fpscr(fpscr);
#else

I think it makes sense that I no longer needed this fix after I changed settings.json. But not needing the next patch bothers me just a bit:

diff --git a/include/zephyr/sys/atomic.h b/include/zephyr/sys/atomic.h
index 2521a6fff..98316f103 100644
--- a/include/zephyr/sys/atomic.h
+++ b/include/zephyr/sys/atomic.h
@@ -90,7 +90,7 @@ typedef atomic_ptr_t atomic_ptr_val_t;
*
* @param num_bits Number of bits.
*/
-#define ATOMIC_BITMAP_SIZE(num_bits) (1 + ((num_bits) - 1) / ATOMIC_BITS)
+#define ATOMIC_BITMAP_SIZE(num_bits) ((num_bits)<1 ? 0 : 1 + ((num_bits) - 1) / ATOMIC_BITS)

/**
* @brief Define an array of atomic variables.

because I believe there is still a signed/unsigned division problem when num_bits is equal to 0, and that used to show up in code analysis when macro ATOMIC_DEFINE was called from nrf_profiler.h code, due to the large positive number generated by (0-1)/ATOMIC_BITS. So, the error I guess is still there but doesn't show up because the large number is not quite as large? Or I am just wrong about the computer math.

Regards,
Burt

[Burt]

The packaged clang-tidy can be made to work on the command line by adding to compile_commands.json

-IC:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/lib/gcc/arm-zephyr-eabi/12.2.0/include -IC:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/lib/gcc/arm-zephyr-eabi/12.2.0/include-fixed

and if --extra-arg=--target=arm-zephyr-eabi-gcc is not present then also add

-IC:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include

Those paths are not needed under Linux. Even the sysroot doesn't seem to be needed in the Linux compile_commands.json.

However, I am not seeing the strcat warnings in either Linux or Windows VS Code at this time; only on the command line.

Another interesting thing: --target seems to know about only "arm" and "eabi" and it does not care whether it is written arm-eabi or eabi-arm. Doesn't seem to care about "zephyr" or "gcc". I read that you have to go to the source code to see which "triplets" are recognized. Ugh!

Burt

[Burt]

I made some good progress but I still need help. I made progress because I finally figured out where to find the OUTPUT for C/C++, so I can debug. All the appropriate warnings are showing up in the OUTPUT window. I did have to disable "-p build" (I had added it to match my command line usage.) And I do have to keep --system-headers in order to see the strcat warnings in the OUTPUT. Something seems wrong about that, and maybe that's the problem. Consider this snippet from app_module_event.c that has a few mods from the original code:

           

            size_t max_append_len = sizeof(data_types) - strlen(data_types) - 1;

            if (i < event->count - 1) {

                /* For every type except the last one, we also need space

                 * for a comma and a space character.

                 */

                //max_append_len -= 2;

            }

//          if (strlen(type2str(event->data_list[i])) > max_append_len) {

//              return;

//          }

            strcat(data_types, type2str(event->data_list[i]));

 

Unfortunately the formatting from VS Code didn't show up here, but clang-tidy warned me that max_append_len is not being read, but no warnings about strcat got transferred from the clang-tidy output to the editor window.

FYI, from settings.json

   "C_Cpp.codeAnalysis.clangTidy.args": [

        "--extra-arg=--target=arm-eabi",

        "--system-headers"/*

        "-p build",

        "-checks=-clang-d*"*/

      ],

Burt

[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.