Skip to content

Conversation

@marcmo
Copy link
Contributor

@marcmo marcmo commented Sep 9, 2025

originates from CR #134850

- Updated Dockerfile to include Valgrind installation
- Enhanced code-coverage.py to run tests with Valgrind
- Uploaded coverage reports as artifacts on code-coverage.yml

Change-Id: I8be2777baa59dceccfb130344e5a8aaf24939d97
@marcmo
Copy link
Contributor Author

marcmo commented Sep 23, 2025

@shamitha-shashidhara can you please briefly describe how the output can then be used? is it uploaded somewhere?

@shamitha-shashidhara
Copy link
Contributor

Hi @marcmo,
I ran Valgrind for memory checks on my local machine and found the following results:

Potential Memory Leaks: 1885
Invalid Syscall Parameters: 2
Uninitialized Memory Conditionals: 55
Uninitialized Memory Reads: 1

Instead of failing the CI pipeline immediately, we’ve chosen to upload the Valgrind output folder as an artifact, which can be downloaded from the Actions tab or you can access it directly via the URL link available in the CI console. This approach allows us to begin addressing the memory issues incrementally and track improvements over time.

Valgrind performs deep analysis, and as a result, our unit test build time has increased from 5 minutes to 15 minutes.

Screenshot 2025-09-23 141011

@rolandreichweinbmw
Copy link
Contributor

rolandreichweinbmw commented Sep 24, 2025

The file memory-check.zip that you can download from this location, in turn contains another memory-check.zip archive. Maybe worth to have a look at?

Further, looking at the contained results, the valgrind findings are primarily related to unit test code which is expected to use dynamic memory in contrast to OpenBSW production code.

OpenBSW production code is not supposed to have relevant amounts of dynamic memory at all. So I wonder if you could identify any relevant findings for our main use case?

An example for a processed output would be interesting.

@shamitha-shashidhara
Copy link
Contributor

shamitha-shashidhara commented Oct 1, 2025

Hi @rolandreichweinbmw , thank you for the feedback!

Yes, I noticed that there are two .zip files—memory-check.zip contains another memory-check.zip archive. I am currently uploading the outer memory-check.zip file as an artifact. However, when downloading artifacts from GitHub Actions, GitHub wraps the uploaded file inside another .zip named after the artifact itself. To avoid this double-wrapping, we can simply avoid uploading a pre-zipped memory-check.zip file.

Regarding the Valgrind results: you're right that the findings are primarily related to unit test code, which is expected to use dynamic memory. In contrast, the OpenBSW production code is designed to avoid dynamic memory usage altogether.
As per your suggestion, I tried running Valgrind on our binary files. When I ran it on the S32K binary, I received an error indicating that Valgrind does not support the ARM architecture. When I ran it on the POSIX binary, Valgrind entered an infinite loop and produced repeated warnings such as:

==45601== Warning: invalid file descriptor -1 in syscall write()
==45601== Warning: invalid file descriptor -1 in syscall read()

@rolandreichweinbmw
Copy link
Contributor

Does this mean we are still searching for a useful application of Valgrind for OpenBSW?

@shamitha-shashidhara
Copy link
Contributor

yes, we are still searching for a useful application of Valgrind for OpenBSW.

@johannes-esr
Copy link
Contributor

@shamitha-shashidhara, could you also summarize the difference between the sanitizers and valgrind? Thanks!

@shamitha-shashidhara
Copy link
Contributor

shamitha-shashidhara commented Oct 7, 2025

I've put together a brief summary based on my findings about sanitizers and Valgrind. Here's a beginner’s perspective:

  • Valgrind

Memory leaks: Excellent (Memcheck very reliable)

Thread issues: Limited detection

Use-after-free: Detected

Uninitialized memory: Detected reliably (Memcheck)

Recompilation: Not required — works with existing binaries

Ease of use: Can be installed via package manager and run directly with:

ctest -T memcheck

Performance: Very slow (tests can run 10–15 minutes or more, depending on machine speed)

Best for:

Deep memory analysis

Debugging legacy binaries (no need to rebuild)

Situations where performance isn’t critical

  • Sanitizers (ASan, TSan, MSan, UBSan)

Memory leaks: Good (AddressSanitizer catches many, but not all leaks)

Thread issues: Very strong (ThreadSanitizer is excellent)

Use-after-free: Detected

Uninitialized memory: Partially detected (MemorySanitizer)

Recompilation: Required — must

Ease of use: add -fsanitize=address,undefined to Cmakelists.txt

Performance: Faster than Valgrind, but still take longer time.

Best for:

Faster feedback during development

Detecting multithreading issues

When you control the build and can recompile

Sanitizer Type

address (ASan) - Detects memory corruption like buffer overflows and use-after-free

leak - Detects memory leaks

undefined (UBSan) - Detects undefined behavior like integer overflows

thread (TSan) - Detects data races in multithreaded programs

MSan (MemorySanitizer) Not Supported in GCC

MSan is not available in GCC. The -fsanitize=memory flag is not recognized by GCC.

MSan is only supported in Clang.

What happens if we use both of them at same time?

If you build with Sanitizers enabled in CMake (e.g. -fsanitize=address): That binary is already instrumented. Running Valgrind memcheck on it usually causes conflicts (both tools hook into memory management). You’ll get crashes, “false positives,” or meaningless output.
Workaround:
We should add an option in CMake to enable or disable sanitizers. This way, developers who want to use sanitizers can enable them, while those who prefer using Valgrind can disable sanitizers and use Valgrind explicitly.

@christian-schilling christian-schilling marked this pull request as draft October 28, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants