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

[Trond Einar Snekvik]

 Are there situations in which people are developing an nRF Connect application but it would be inappropriate to have this setting?

I don't think so, but we have a policy of not altering version controlled files without the user initiating the change, as we think it's incredibly annoying when tools dirty the git index on its own. We could consider adding it to the initial settings when generating new workspaces though.

Ideally, I think this should be picked up automatically by the C/C++ extension, so I made an issue in their repo: https://github.com/microsoft/vscode-cpptools/issues/11749

I think that issue with ATOMIC_BITMAP_SIZE no longer generating a warning might be a correct but unfortunate side effect of the default integer size changing. When invoked with 0, that expression becomes -1, which is 0xFFFFFFFFU on 32 bit platforms (resulting in an array size of 134217728). This is well within the maximum array size of PTRDIFF_MAX (2147483647), which is the upper boundary for a single array according to the C standard. On a 64 bit platform, however, -1 is 0xFFFFFFFFFFFFFFFFU, and for some reason, PTRDIFF_MAX is only 288230376151711744.

I think this is a real bug, and your fix is valid, although (1 + MAX(0, (num_bits) - 1) / ATOMIC_BITS) might be more appropriate. Would you consider contributing this change to Zephyr?

[Burt]

Hi Trond,

I like your feature request 11749. It makes a lot of sense to me. Hopefully the C/C++ extension folks will implement the feature.

Indeed, my expression for ATOMIC_BITMAP_SIZE ended up bloated. I will switch to your definition. I will attempt to contribute the change to Zephyr. It will be my first attempt to contribute; I'll follow their documented recommendations.

Thank you again for the thought and effort you have put into all of this.

Burt

[Burt]

I have another baffling situation: Using nRF Connect for VS CodeI "Created a new application" from a sample, in other words, I created asset_tracker_v2 outside of the SDK tree, whereas previously I was working in the tree. And here everything seems to behave except that Code Analysis will not run at all. I don't even know how to debug this. 

[Burt]

Now in the "in tree", the extra-arg fix is working BUT in the file app_event.c whereas I used to see Code Analysis warnings due to strcat, I no longer see them. Here is some stuff from the command line: I get different results using clang-tidy from 

C:\Users\Burt>where clang-tidy
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\bin\clang-tidy.exe

than I do using the built in version:

C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2>clang-tidy -p build src\events\app_module_event.c -checks=-*,clang-analyzer-security.insecureAPI.strcpy --system-headers
3 warnings and 2 errors generated.
Error while processing C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2\src\events\app_module_event.c.
error: unknown argument: '-fno-reorder-functions' [clang-diagnostic-error]
error: unknown argument: '-mfp16-format=ieee' [clang-diagnostic-error]
C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2\src\events\app_module_event.c:82:4: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcat(data_types, type2str(event->data_list[i])); AND ON AND ON AND ON...

and here is builtin

C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2>\Users\Burt\.vscode\extensions\ms-vscode.cpptools-1.19.1-win32-x64\LLVM\bin\clang-tidy -p build src\events\app_module_event.c -checks=-*,clang-analyzer-security.insecureAPI.strcpy --system-headers --extra-arg=--target=arm-zephyr-eabi-gcc
1 warning and 3 errors generated.
Error while processing C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2\src\events\app_module_event.c.
error: unknown argument: '-fno-reorder-functions' [clang-diagnostic-error]
error: unknown argument: '-mfp16-format=ieee' [clang-diagnostic-error]
C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi\include\sys/cdefs.h:47:10: error: 'stddef.h' file not found [clang-diagnostic-error]
47 | #include <stddef.h>
| ^~~~~~~~~~
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).

C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2>

Everything is crazy with clang-tidy!!!!! Note although I was playing around with -checks here I did not have that in my settings.json originally (and most of the time).

Burt