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

coredump: fix issue that nvic region overlapped by board memory region #15676

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Jan 23, 2025

Note: Please adhere to Contributing Guidelines.

Summary

Firstly call arm_coredump_add_region in up_initialize to add nvic region, then call coredump_set_memory_region to add board mem region, at this moment, an overlap occurs.

Impact

API coredump_set_memory_region has been removed, memory region is initialized with CONFIG_BOARD_MEMORY_RANGE.

Testing

Tested with CONFIG_ARM_COREDUMP_REGION.

Firstly call arm_coredump_add_region in up_initialize to add nvic region, then call
coredump_set_memory_region to add board mem region, at this moment, an overlap occurs.

Signed-off-by: wanggang26 <[email protected]>
@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 Jan 23, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 23, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial details.

Missing Information:

  • Insufficient Summary: The summary explains the problem but doesn't clearly state the solution. How is the overlap being resolved? Just removing coredump_set_memory_region isn't enough explanation. How are memory regions managed now?
  • Incomplete Impact Assessment: The Impact section is severely lacking. It only addresses one aspect (API removal). All other impact categories (user, build, hardware, documentation, security, compatibility) are unaddressed. Even for the API removal, there's no mention of whether users will need to adapt (which they likely will).
  • Vague Testing Description: "Tested with CONFIG_ARM_COREDUMP_REGION" is inadequate. What architecture, board, and configuration were used? What were the results of the tests? The required "Testing logs before change" and "Testing logs after change" sections are completely empty. There's no evidence provided that the changes actually work.
  • No Issue Reference: There's no mention of a related NuttX issue, even though the template specifically requests it.

To meet the requirements, the PR needs to:

  • Clarify the Solution: Explain how the memory region initialization is handled after the API removal.
  • Complete the Impact Assessment: Address all impact categories. Explain the consequences of the API removal for users.
  • Provide Detailed Testing Information: Specify the exact test setup (host and target details) and include actual testing logs (output) from both before and after the change, demonstrating the fix.
  • Reference a Related Issue (if applicable): If this PR addresses a reported issue, link to it.

Without these improvements, the PR is incomplete and difficult to review.

@xiaoxiang781216 xiaoxiang781216 merged commit f6b9a8f into apache:master Jan 24, 2025
20 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components 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