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

nxgdb/memdump: add free command #15744

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Conversation

XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Feb 2, 2025

Note: Please adhere to Contributing Guidelines.

Summary

Similar to nsh command free.

(gdb) free
name            total      used       free       maxused    maxfree    nused      nfree
GImageCache     10485756   10411964   73792      10602184   62328      217        7
uncache         10485756   127812     10357944   270512     10350560   4          2
Umem            26879516   17458732   9420784    20957888   9260768    1211       234
(gdb)

Impact

New feature.

Testing

Tested locally with a coredump.

(gdb) free
name            total      used       free       maxused    maxfree    nused      nfree
GImageCache     10485756   10411964   73792      10602184   62328      217        7
uncache         10485756   127812     10357944   270512     10350560   4          2
Umem            26879516   17458732   9420784    20957888   9260768    1211       234
(gdb)

Signed-off-by: xuxingliang <xuxingliang@xiaomi.com>
@github-actions github-actions bot added Area: Tooling Size: S The size of the change in this PR is small labels Feb 2, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 2, 2025

[Experimental Bot, please feedback here]

No, this PR description does not meet the NuttX requirements. Here's why and how to fix it:

  • Insufficient Summary: The summary is far too brief. It needs to explain why a free command within gdb is necessary. Is it for debugging memory leaks? Is the existing free command in nsh insufficient in some way? How does the gdb command differ/improve things? What code was changed to implement this (e.g., files modified)?

  • Missing Impact Details: While it notes a "New feature," the impact section needs to be much more detailed. Consider these questions:

    • Impact on user: How will users utilize this new command? Will they need new build flags or configuration options?
    • Impact on build: Does adding this command change the build process at all? Does it add any dependencies?
    • Impact on documentation: This new feature definitely requires documentation updates. Where will that documentation be added/updated?
    • Other Impacts: The other impact sections (hardware, security, compatibility) are marked "NO" by omission. Explicitly state "NO" for each to be clear.
  • Inadequate Testing: "Tested locally with a coredump" is not enough.

    • Build Host(s): Specify the OS, CPU architecture, and compiler version used for the build host.
    • Target(s): Be specific about the target architecture and board configuration. Was this tested on a simulator (e.g., qemu-system-arm) or actual hardware?
    • Testing logs: The sections for "Testing logs before change" and "Testing logs after change" are empty. You must provide example output of the command before the change (presumably nothing or an error) and after the change (showing the memory information). Ideally, include a concise test case that demonstrates the command's functionality and how it addresses the problem it solves.

Example of an improved summary and testing section:

## Summary

This PR adds a `free` command to the NuttX gdb stub, similar to the nsh `free` command. This addresses the need for a convenient way to inspect heap memory usage directly within the gdb debugger, which is particularly helpful for identifying and diagnosing memory leaks during development. This enhancement allows developers to quickly assess memory allocation without exiting the debugging session, streamlining the debugging workflow. The changes primarily modify the `nuttx/debug/gdb/gdb_*.c` files to implement the command parsing and output formatting within the gdb stub.

Related NuttX Issue: #1234 (if applicable)


## Impact

* Is new feature added? YES
* Impact on user (will user need to adapt to change)? YES. Users will have a new gdb command available. Documentation will be updated to guide usage.
* Impact on build (will build process change)? NO
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
* Impact on documentation (is update required / provided)? YES. The gdb stub documentation will be updated to include the `free` command and its usage.
* Impact on security (any sort of implications)? NO
* Impact on compatibility (backward/forward/interoperability)? NO
* Anything else to consider? NO


## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.3.0
* Target(s): sim:nsh


Testing logs before change:

(gdb) free
Undefined command: "free". Try "help".


Testing logs after change:

(gdb) free
name total used free maxused maxfree nused nfree
GImageCache 1048576 524288 524288 786432 786432 2 2
uncache 1048576 16384 1032192 32768 1032192 1 1
Umem 2097152 1048576 1048576 1572864 1572864 4 4
(gdb)

By providing more context, details, and evidence of testing, your PR will be much more likely to be accepted. Remember to also follow the other guidelines in the CONTRIBUTING.md document.

@acassis acassis merged commit f9d8cdc into apache:master Feb 4, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Tooling 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.

6 participants