nuttxpr commented on PR #14771: URL: https://github.com/apache/nuttx/pull/14771#issuecomment-2475318182
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. The summary and impact sections are insufficient. The testing section, while including some information, lacks crucial detail. Here's a breakdown of the deficiencies: * **Summary:** * **Missing "Why":** The summary doesn't explain *why* these changes are necessary. What problem are they solving? What benefit do they provide? Simply stating what files were changed is not enough. Is this for performance analysis, debugging a specific issue, or general profiling support? * **Missing "How":** While it mentions turning off KASAN and turning on gprof, it doesn't explain *how* these changes are implemented within the code. Did you add a config option? Modify existing code? * **Impact:** * **Too brief:** Stating "None" is almost never accurate. At a minimum, this change impacts the build (gprof is now included) and potentially performance (due to profiling overhead). It also impacts the build by disabling KASAN, which has important safety implications. The impact of disabling KASAN should be explicitly called out and justified. If there are no other impacts, explicitly state *why* there are no impacts on users, hardware, documentation, security, or compatibility. * **Testing:** * **Insufficient detail on build host:** Simply stating "arm64/qemu/nsh defconfig" is not sufficient. Provide the host operating system, compiler version (GCC/Clang and specific version number). * **Missing "before" logs:** There are no "before" logs to demonstrate the behavior prior to the change. This makes it impossible to assess the impact of the change. * **Unclear "after" logs:** What do the "after" logs show? Do they demonstrate that gprof is working correctly? Provide specific examples from the logs that demonstrate the expected behavior. What commands were run in nsh? * **Lack of specific test cases:** What specific functions/features were profiled? What were the expected results? Did the profiling data make sense? **To make this PR acceptable, you need to:** 1. **Expand the Summary:** Explain the motivation for these changes. Detail the specific code modifications made (e.g., "Added a Kconfig option to enable gprof," "Modified the build system to include the gprof library," "Added calls to mcount in arm64 code"). 2. **Detail the Impact:** Be explicit about *all* impacts, even if they seem minor. Justify why disabling KASAN is necessary and acceptable. If you believe there's truly no user impact, explain why. 3. **Provide complete testing information:** Include the full build host details. Provide "before" logs showing the absence of gprof output. Provide "after" logs showing specific gprof output demonstrating that it's working as expected. Explain the steps taken to generate the logs (e.g., "Ran the following commands in nsh: ..."). Show specific examples from the profiling output that confirm the correct behavior. By addressing these points, you will significantly improve the quality and reviewability of your PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
