Skip to content

[Draft]libs/instrument: add instrument level check to avoid nesting loops #15997

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[Draft]libs/instrument: add instrument level check to avoid nesting loops #15997

wants to merge 1 commit into from

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Mar 16, 2025

Summary

libs/instrument: add instrument level check to avoid nesting loops

Signed-off-by: chao an anchao.archer@bytedance.com

Impact

N/A

Testing

sim/nsh, enable vfs instrument functions

diff --git a/fs/vfs/Make.defs b/fs/vfs/Make.defs
index 30292d787f..e00b361407 100644
--- a/fs/vfs/Make.defs
+++ b/fs/vfs/Make.defs
@@ -22,6 +22,8 @@
 
 # Common file/socket descriptor support
 
+CFLAGS += -finstrument-functions
+
 CSRCS += fs_chstat.c fs_close.c fs_dup.c fs_dup2.c fs_fcntl.c fs_epoll.c
 CSRCS += fs_fchstat.c fs_fstat.c fs_fstatfs.c fs_ioctl.c fs_lseek.c
 CSRCS += fs_mkdir.c fs_open.c fs_poll.c fs_pread.c fs_pwrite.c fs_read.c

$ git diff .
diff --git a/boards/sim/sim/sim/configs/nsh/defconfig b/boards/sim/sim/sim/configs/nsh/defconfig
index 8517209444..496336d268 100644
--- a/boards/sim/sim/sim/configs/nsh/defconfig
+++ b/boards/sim/sim/sim/configs/nsh/defconfig
@@ -24,6 +24,7 @@ CONFIG_DEBUG_FEATURES=y
 CONFIG_DEBUG_SYMBOLS=y
 CONFIG_DEV_GPIO=y
 CONFIG_DEV_LOOP=y
+CONFIG_DRIVERS_NOTE=y
 CONFIG_ETC_FATDEVNO=2
 CONFIG_ETC_ROMFS=y
 CONFIG_ETC_ROMFSDEVNO=1
@@ -64,6 +65,14 @@ CONFIG_READLINE_TABCOMPLETION=y
 CONFIG_SCHED_BACKTRACE=y
 CONFIG_SCHED_EVENTS=y
 CONFIG_SCHED_HAVE_PARENT=y
+CONFIG_SCHED_INSTRUMENTATION=y
+CONFIG_SCHED_INSTRUMENTATION_DUMP=y
+CONFIG_SCHED_INSTRUMENTATION_FILTER=y
+CONFIG_SCHED_INSTRUMENTATION_FUNCTION=y
+CONFIG_SCHED_INSTRUMENTATION_HEAP=y
+CONFIG_SCHED_INSTRUMENTATION_PREEMPTION=y
+CONFIG_SCHED_INSTRUMENTATION_SWITCH=y
+CONFIG_SCHED_INSTRUMENTATION_WDOG=y
 CONFIG_SCHED_WAITPID=y
 CONFIG_SIM_HOSTFS=y
 CONFIG_SIM_WALLTIME_SIGNAL=y

@anchao
libs/instrument: add instrument level check to avoid nesting loops
d9c68d3 
Signed-off-by: chao an <anchao.archer@bytedance.com>
@anchao anchao marked this pull request as draft March 16, 2025 06:31
@anchao anchao requested a review from tmedicci March 16, 2025 06:32
@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Mar 16, 2025
@anchao anchao mentioned this pull request Mar 16, 2025
Merged
@nuttxpr
Copy link

nuttxpr commented Mar 16, 2025

[Experimental Bot, please feedback here]

This PR does NOT fully meet the NuttX requirements. Here's why:

  • Summary is insufficient: While it states what was changed, it doesn't explain why this change is necessary. What problem does the nesting of instrument loops cause? What are the symptoms? Why is this a bug that needs fixing? It also lacks a reference to a related NuttX issue.

  • Impact is incomplete: Simply stating "N/A" is not acceptable. While some sections might be N/A, the author needs to explicitly confirm that for each section. For example, the added compiler flag -finstrument-functions certainly has a build impact and needs to be documented. Furthermore, enabling various CONFIG_SCHED_INSTRUMENTATION_* options also impacts the build and runtime behavior. These need to be explained.

  • Testing is inadequate: The provided testing only shows the configuration changes, not the actual results of the testing. What were the observed problematic behaviors before the change? How do the logs after the change demonstrate that the issue is fixed? The provided diffs don't prove anything about functionality. The logs should showcase the issue and the resolution. Furthermore, testing only on sim/nsh with VFS instrumentation is likely insufficient. Ideally, testing should cover other scenarios where instrumentation might be used to ensure the fix doesn't introduce regressions elsewhere. Finally, the build host information is missing.

In short, the PR needs to provide more context, explain the impact thoroughly, and offer concrete evidence that the change works as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmedicci tmedicci Awaiting requested review from tmedicci

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anchao @nuttxpr