nuttxpr commented on PR #15199: URL: https://github.com/apache/nuttx/pull/15199#issuecomment-2544410017
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to mostly meet the NuttX requirements, but has some areas that could be improved: **Strengths:** * **Summary:** Clearly states the *what* (using CFI in IRQ ASM) and *why* (improve debugging/backtraces). The *how* could be slightly more explicit (e.g., "by adding debug frame information"). * **Testing:** Provides testing logs for before and after the change across multiple architectures (qemu mps3-an547, qemu armv7a, qemu risc-v). The test case that triggers a fault is helpful. The backtraces demonstrate an improvement in debug information. **Weaknesses:** * **Impact:** The impact section is too brief. While it mentions debugging, it needs to be more structured and answer all the questions explicitly with YES/NO and descriptions. For example: * **Is new feature added? Is existing feature changed?** (Likely changing an existing feature - interrupt handling) * **Impact on user:** YES (Improved debugability for fault analysis). * **Impact on build:** Potentially YES (if CFI changes build flags/linker scripts. This needs to be stated explicitly). * **Impact on hardware:** NO (unless the CFI has specific hardware dependencies). * **Impact on documentation:** Possibly YES (if the build process changes or if there are new debugging considerations). * **Impact on security:** Unlikely, but state NO explicitly. * **Impact on compatibility:** Hopefully NO, but needs to be stated explicitly. * **Testing:** While logs are provided, there's a lack of information on the *Build Host*. Specify OS, CPU architecture, and compiler version. Also, "hello_main" is vague. Provide the full path to the test application within the `nuttx-apps` repo (if applicable). Clarify how the exception is triggered. "Triggering an exception" isn't descriptive enough. How is it triggered? Is it deliberate, or a side-effect of the code? Explain the observed improvement in the backtraces in more detail. What specifically is better in the "after" backtraces? **Recommendations for Improvement:** 1. **Expand the Impact section.** Address all the points with YES/NO and provide specific details. 2. **Provide Build Host information.** Be explicit about your development environment. 3. **Clarify the test case.** Give the full path to `hello_main` and precisely describe how the exception is triggered. 4. **Explain the backtrace improvement.** Instead of just showing the backtraces, highlight the specific differences and how they represent an improvement in debug information due to the PR. For example, "After the change, the backtrace correctly shows frame `#13 0x00027a8c in hello_main ...` which was missing before, demonstrating the effectiveness of the added CFI." By addressing these weaknesses, the PR will be much stronger and easier for reviewers to assess. -- 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]
