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

mempool backtrace fix and de-init fix #14370

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

Conversation

XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Oct 16, 2024

Note: Please adhere to Contributing Guidelines.

Summary

Fix memory pool backtrace minor issues.

  1. Should use backtrace_format to format backtrace now.
  2. Default to turn of recording backtrace if CONFIG_MM_BACKTRACE_DEFAULT is disabled.

Fix memory pool de-init.
Mark dict pointer as NULL after free, in case misuse of it.

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Minor fixes, should have no impact.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

CI build test pass.

Signed-off-by: anjiahao <anjiahao@xiaomi.com>
@github-actions github-actions bot added Area: Memory Management Memory Management issues Size: S The size of the change in this PR is small labels Oct 16, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 16, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

This PR appears to mostly meet the NuttX requirements, but could use some improvements in the "Testing" section.

Here's a breakdown:

✅ Summary:

  • Clear explanation of the changes: The summary clearly outlines the two main issues being addressed (memory pool backtrace and de-init) and provides a brief description of the solutions.
  • Links to relevant issues: While the summary doesn't explicitly link to any specific issues, it's not strictly required if no directly related issues exist.

Could be improved 🤔 Impact:

  • Generic statement: The impact section states "Minor fixes, should have no impact," which is too vague.
  • Be specific: Even if the impact is minimal, explicitly state which aspects are unaffected (e.g., "No impact on users, build process, or hardware support.").
  • Address potential changes: Mention if the backtrace changes could lead to different log outputs or require any configuration adjustments.

Needs Improvement ❌ Testing:

  • Lack of details: The testing section only mentions "CI build test pass," which is insufficient.
  • Provide specifics: Include:
    • Host OS, CPU architecture, compiler used for building.
    • Target architecture, board, and configuration tested on.
    • Ideally, provide snippets of the relevant log output before and after the change to demonstrate the fix.

Recommendations:

  1. Expand the "Impact" section: Provide specific details about the areas affected (or unaffected) by the changes.
  2. Enhance the "Testing" section: Include concrete information about your testing environment (host and target) and demonstrate the fix's effectiveness with relevant log snippets.

By addressing these points, you can make your PR even stronger and easier for reviewers to assess.

XuNeo and others added 2 commits October 17, 2024 00:18
There's no need to manually format the backtrace any more.

Signed-off-by: Neo Xu <neo.xu1990@gmail.com>
Need to initialize procfs.backtrace to false if it's disabled.

Signed-off-by: yinshengkai <yinshengkai@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Memory Management Memory Management issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants