Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libs: add gcov framework support #14838

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Gary-Hobson
Copy link
Contributor

Summary

libs: add gcov framework support

In devices without storage media, you can export data to the
command line and then generate the corresponding gcda file

It can save the result output by calling __gcov_info_to_gcda
The usage is similar to:
https://gcc.gnu.org/onlinedocs/gcc/Freestanding-Environments.html#Profiling-and-Test-Coverage-in-Freestanding-Environments

Usage:
./tools/configure.sh qemu-armv7a:nsh
Modify the configuration

+CONFIG_COVERAGE_ALL=y
+CONFIG_COVERAGE_MINI=y
+CONFIG_SYSTEM_GCOV=y

Run:

qemu-system-arm -cpu cortex-a7 -nographic -smp 4 \
     -machine virt,virtualization=off,gic-version=2 \
     -net none -chardev stdio,id=con,mux=on -serial chardev:con \
     -mon chardev=con,mode=readline -kernel ./nuttx/nuttx -semihosting -s | tee gcov.txt
./nuttx/tools/gcov_convert.py -i ./gcov.txt
./nuttx/tools/gcov.sh -t arm-none-eabi-gcov

Impact

Testing

qemu

profile-generate is used to generate compilation feedback optimization data, not just code coverage data

It will rely on the toolchain library:
nuttx/libs/libc/misc/lib_utsname.c:94:(.text.uname+0x2c): undefined reference to `__gcov_indirect_call_profiler_v4'
arm-none-eabi-ld: nuttx/libs/libc/misc/lib_utsname.c:113:(.text.uname+0x178): undefined reference to `__gcov_indirect_call'
arm-none-eabi-ld: nuttx/libs/libc/misc/lib_utsname.c:113:(.text.uname+0x188): undefined reference to `__gcov_time_profiler_counter'
arm-none-eabi-ld: nuttx/staging/libc.a(lib_utsname.o):(.data..LPBX0+0x30): undefined reference to `__gcov_merge_time_profile'

Signed-off-by: yinshengkai <yinshengkai@xiaomi.com>
@github-actions github-actions bot added Area: Tooling Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86_64 Issues related to the x86_64 architecture Area: OS Components OS Components issues Board: simulator Size: L The size of the change in this PR is large labels Nov 18, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 18, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is lacking in key details. Here's a breakdown:

Strengths:

  • Clear Summary of the "What": The summary explains the purpose of the change (adding gcov support), the affected area (libs), and the mechanism (exporting data to the command line and generating gcda files). The usage example and configuration instructions are helpful.
  • Provides Testing Information: It specifies the target (qemu) and gives basic usage instructions. The inclusion of gcov.txt processing is good.

Weaknesses:

  • Missing Issue References: If this relates to any existing NuttX issues, those should be linked.
  • Incomplete Impact Assessment: The "Impact" section is entirely empty. This is a critical part of the PR. At a minimum, address all the listed impact points (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO", state it explicitly. For example: "Impact on user: NO". If the impact is "YES", explain the impact.
  • Insufficient Testing Detail: While testing information is present, it's minimal.
    • Build Host: Provide specifics about the build host OS, CPU architecture, and compiler version used.
    • Target Details: Be more specific about the qemu configuration (e.g., qemu-system-arm -cpu cortex-a7 ...).
    • Missing "Before" and "After" Logs: The code blocks for "Testing logs before change" and "Testing logs after change" are empty. Include relevant log snippets that demonstrate the functionality before and after your changes. This is crucial for reviewers to understand the impact of the changes. Show that the gcov output is as expected.

Recommendations for Improvement:

  1. Fill out the Impact section completely. This is the most significant deficiency. Consider these points:

    • Impact on build: Does enabling gcov require any build system changes or new dependencies?
    • Impact on hardware: Is this specific to certain architectures?
    • Impact on documentation: Does this require documentation updates? If so, were those updates included in the PR?
    • Impact on security: Does enabling code coverage introduce any security considerations? (Probably not, but it's good to explicitly state this).
    • Impact on compatibility: Does this change break any existing functionality?
  2. Provide detailed build host information. (OS, CPU, Compiler version).

  3. Include relevant "before" and "after" test logs. Demonstrate that gcov is not working before the change and is working after the change. Show some example gcov output.

  4. Link any related NuttX issues.

By addressing these points, the PR will be much more complete and easier for reviewers to evaluate.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice @Gary-Hobson your commit log message included all needed information to let people start to using it.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Gary-Hobson :-)

libs/libbuiltin/compiler-rt/Make.defs Show resolved Hide resolved
endif

ifeq ($(CONFIG_COVERAGE_MINI)$(CONFIG_ARCH_TOOLCHAIN_GCC),yy)
CSRCS += gcov.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coverage.c

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot have the same name as coverage.c in compiler-rt. When compiling using makefile, it will search for coverage.c in the compiler-rt directory, causing it to not find the correct symbol when linking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the full path

libs/libbuiltin/libgcc/CMakeLists.txt Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
tools/gcov_convert.py Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
{
}

void __gcov_dump(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not implement __gcov_dump

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toolchain already has an implementation of __gcov_dump. I think you can use the default one most of the time. If necessary, we can reimplement __gcov_dump

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is mini version, how can we enable toolchain and mini version at the same time?

In devices without storage media, you can export data to the
command line and then generate the corresponding gcda file

It can save the result output by calling __gcov_info_to_gcda
The usage is similar to:
https://gcc.gnu.org/onlinedocs/gcc/Freestanding-Environments.html#Profiling-and-Test-Coverage-in-Freestanding-Environments

Usage:
 ./tools/configure.sh qemu-armv7a:nsh
Modify the configuration
+CONFIG_COVERAGE_ALL=y
+CONFIG_COVERAGE_MINI=y
+CONFIG_SYSTEM_GCOV=y
Run:
qemu-system-arm -cpu cortex-a7 -nographic -smp 4 \
     -machine virt,virtualization=off,gic-version=2 \
     -net none -chardev stdio,id=con,mux=on -serial chardev:con \
     -mon chardev=con,mode=readline -kernel ./nuttx/nuttx -semihosting -s | tee gcov.txt
./nuttx/tools/gcov_convert.py -i ./gcov.txt
./nuttx/tools/gcov.sh -t arm-none-eabi-gcov

Signed-off-by: yinshengkai <yinshengkai@xiaomi.com>
@acassis
Copy link
Contributor

acassis commented Nov 21, 2024

@Gary-Hobson please consider creating later a Guide (https://nuttx.apache.org/docs/latest/guides/index.html) to use GCOV with NuttX using these commands you included in the commit log.

@Gary-Hobson
Copy link
Contributor Author

@Gary-Hobson please consider creating later a Guide (https://nuttx.apache.org/docs/latest/guides/index.html) to use GCOV with NuttX using these commands you included in the commit log.

Yes, we will upload the documentation for checking code coverage in different environments later

Currently, we support gcc, clang, simulator, device, it will take some time to verify all environments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86_64 Issues related to the x86_64 architecture Area: OS Components OS Components issues Area: Tooling Board: simulator Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants