Give preprocessor definitions to cppcheck

[rotu]

cppcheck is unable to correctly analyze source code because it does not know the preprocessor definitions that the project will be compiled with. This may be responsible for the CI-breaking performance seen in #263 (since in the absence of precompiler definitions, cppcheck analyzes all preprocessor paths, which can be excessive, with each preprocessor conditional doubling the number of code paths to analyze).

Limiting the headers to reclaim manageable performance causes numerous spurious downstream false failures for missing macros ros2/ros2#942

Ideally, the list of include paths and compile definitions should be correct on the file level, possibly with the help of compile_commands.json, which can be generated with CMake.

Although it would not fully resolve this issue, telling cppcheck the include paths and compile definitions at the target or directory level might be a good enough stopgap. Compile options can be specified per build file, but the target level is usually as specific as these get. (I attempted this approach in #269, but unfortunately was not able to finish).

[ivanpauno]

In almost all the packages, there are almost no compile definitions we need to pass.
The only one I can think of is the visibility macros (I expect cppcheck to infer things like _WIN32 from the platform is running, I'm not sure if that's the case).

[rotu]

@ivanpauno Easy enough to check:

int main() {
#if _WIN32
(*0) = 1;
#else
(*0) = 2;
#endif

#ifdef __unix__
(*0) = 3;
#else
(*0) = 4;
#endif

#ifdef _MSC_VER
(*0) = 5;
#else
(*0) = 6;
#endif
}

$cppcheck example.cpp
Checking example.cpp ...
example.cpp:5:2: error: Null pointer dereference [nullPointer]
(*0) = 2;
 ^
example.cpp:11:2: error: Null pointer dereference [nullPointer]
(*0) = 4;
 ^
example.cpp:17:2: error: Null pointer dereference [nullPointer]
(*0) = 6;
 ^
Checking example.cpp: _MSC_VER...
example.cpp:15:2: error: Null pointer dereference [nullPointer]
(*0) = 5;
 ^
Checking example.cpp: _WIN32...
example.cpp:3:2: error: Null pointer dereference [nullPointer]
(*0) = 1;
 ^
Checking example.cpp: __unix__...
example.cpp:9:2: error: Null pointer dereference [nullPointer]
(*0) = 3;
 ^

[rotu]

Even if we do manage to pass all platform and compiler-specific flags, cppcheck needs help to reason about, e.g. RCUTILS_BUILDING_DLL, which is dependent on CMake logic.

[ivanpauno]

Even if we do manage to pass all platform and compiler-specific flags, cppcheck needs help to reason about, e.g. RCUTILS_BUILDING_DLL, which is dependent on CMake logic.

That was what I meant with "visibility macros".

The problem is that we need to configure them for the current package and for all the dependencies.
The second part is tricky.

[rotu]

The second part does not seem that tricky:

target_compile_definitions(rcutils
  PRIVATE RCUTILS_BUILDING_DLL=1
  INTERFACE RCUTILS_BUILDING_DLL=0
)

and replace #ifdef RCUTILS_BUILDING_DLL with #if RCUTILS_BUILDING_DLL

[ivanpauno]

Ok. We should collect all exported compile definitions in ament_cppcheck and pass them to cppcheck, right?
That sounds doable.

[rotu]

We shouldn't have to even collect them, since CMake populates from link interface dependencies.
As a bonus I added another possible implementation below using generator expressions:

cmake_minimum_required(VERSION 3.1216
project(test)

add_library(Foo foo.cpp)
target_compile_definitions(Foo
PRIVATE FOO_BUILDING_DLL=1
INTERFACE FOO_BUILDING_DLL=0
PUBLIC FOO_ALT_BUILDING_DLL=$<STREQUAL:$<TARGET_PROPERTY:NAME>,Foo>
)
add_library(Bar bar.cpp)
target_link_libraries(Bar Foo)
add_dependencies(Bar Foo)

file(GENERATE
OUTPUT defs.txt
CONTENT "Foo: $<TARGET_PROPERTY:Foo,COMPILE_DEFINITIONS>\nBar: $<TARGET_PROPERTY:Bar,COMPILE_DEFINITIONS>")

 touch foo.cpp bar.cpp; mkdir build; cmake build -B build -S .; cat build/defs.txt
-- The C compiler identification is GNU 9.3.0
...
-- Generating done
-- Build files have been written to: /home/dan/Desktop/build
Foo: FOO_BUILDING_DLL=1;FOO_ALT_BUILDING_DLL=1
Bar: FOO_BUILDING_DLL=0;FOO_ALT_BUILDING_DLL=0

[ivanpauno]

We shouldn't have to even collect them, since CMake populates from link interface dependencies.

Yeah, what I mean is that we still have to collect the targets in the project, take the definitions, and pass them to cppcheck.

[rotu]

Yeah, what I mean is that we still have to collect the targets in the project, take the definitions, and pass them to cppcheck.

There's some subtlety here. Because cppcheck is currently done on a per-directory basis, naively adding definitions with -D will cause the definitions to leak out of their target scope.

If we want definitions (and includes) to be correct, we need either to invoke cppcheck separately per source file or to use the --project command-line option.

[ivanpauno]

@clalancette the suggestion in this comment is what I mentioned today: #270 (comment)

[mm318]

Let's concretize this to "give compile_commands.json to all ament_linters".

[mm318]

cppcheck has support for compile_commands.json: http://cppcheck.sourceforge.net/manual.html#cmake

Not sure to what extent. It should be able to find all headers in this case, but not sure if it'll take into consideration preprocessor definitions flags (gcc -DBLAH=1, which definitely are present in compile_commands.json though).

[johan-boule]

You certainly want to extract anything that's default to a real compiler and then give that explicitly to cppcheck's preprocessor.
> "$build"/sonar-force-include-predefined-macros.h "$cxx" -E -dM $cxx_std_version -xc++ /dev/null
preprocessor_include_path=$("$cxx" -E -Wp,-v $cxx_std_version -xc++ /dev/null 2>&1 | sed -n 's,^ ,,p' | paste -sd,)
If you don't do that, your best bet is probably to send the preprocessed output to cppcheck.
Also note that cppcheck's parser is not going to be maintained forever it's too hard a task with C++20 and C++23 crazy features. I think cppcheck's author will probably migrate its checks to something like clang-tidy/clang's static analyser infrastructure.