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

xtensa: initial support for debugpoint api #15907

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chirping78
Copy link
Contributor

Summary

Implement up_debugpoint_add/up_debugpoint_remove for xtensa.

Impact

Can use debugpoint program to test code and data debugpoint.

Currently the int handler here will call panic when the debugpoint hit, will improve the int handler with following PR.

Testing

enable CONFIG_LIB_GDBSTUB CONFIG_SYSTEM_DEBUGPOINT and disable CONFIG_NSH_DISABLE_MW to test.

Implement up_debugpoint_add/up_debugpoint_remove for xtensa.

Signed-off-by: chenxiaoyi <[email protected]>
@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Feb 25, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 25, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it's incomplete. Here's a breakdown:

Strengths:

  • Clear Summary: The summary adequately describes the what and why of the change. The how could be slightly more explicit, perhaps mentioning the specific implementation details (e.g., register manipulation, interrupt handling).
  • Impact Description: The impact section identifies the affected area (debugpoint functionality) and notes a planned future improvement for the interrupt handler.
  • Testing Instructions: It provides basic testing instructions, including necessary configuration options.

Weaknesses:

  • Missing Links: The summary mentions a future PR for improving the interrupt handler but doesn't provide a link or issue number.
  • Incomplete Testing: The "Testing logs before change" and "Testing logs after change" sections are empty. This is critical and must be filled in with actual logs demonstrating the change's functionality and verifying it works as expected.
  • Lack of Detail in Impact: While the impact section mentions changes to the user experience (ability to use the debugpoint program) and build process (config options), it lacks specifics. For instance, it doesn't mention which Xtensa architectures or boards were tested. The impact on documentation is also unclear — will documentation need to be updated to reflect this new functionality?
  • Missing Build/Host Information: The testing section lacks details about the build host and target used for verification. Provide the requested OS, CPU, compiler, Xtensa architecture, board, and configuration.
  • Security/Compatibility Impacts: The PR mentions no considerations for security or compatibility, which should be addressed, even if the impact is "NO". Explicitly stating "NO" with a brief justification (e.g., "NO - This change only affects debugging functionality and doesn't introduce new security risks.") is better than leaving it blank.

Conclusion:

While the PR presents a good starting point, it needs further work to fully meet the NuttX requirements. Specifically, it needs to include actual testing logs, complete information about the testing environment, and address the missing details in the Impact section. Adding the missing link to the planned interrupt handler PR would also be beneficial.

#if defined(XCHAL_NUM_DBREAK) && XCHAL_NUM_DBREAK > 0
g_trigger_count += XCHAL_NUM_DBREAK;
#endif
#if defined(XCHAL_NUM_IBREAK) && XCHAL_NUM_IBREAK > 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's change to fix array and remove allocation at line 232

for (; i < XCHAL_NUM_DBREAK; i++)
{
g_trigger_map[i].trigger_type = TRIGGER_TYPE_DATA;
g_trigger_map[i].index = index++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need save index, which is same as i

****************************************************************************/

static int g_trigger_count = 0;
static struct xtensa_debug_trigger *g_trigger_map;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the separated array for code and data to simplify the initializition

: "r"(address), "r"(ibreakenable)
);
}
else if (index == 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about other index

: "r"(address), "r"(dbreakc)
);
}
else if (index == 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

static struct xtensa_debug_trigger *
xtensa_debug_find_slot(int trigger_type, int type, void *address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use different find function for code and data?

* Private Functions
****************************************************************************/

static void xtensa_enable_ibreak(int index, uintptr_t address)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include function Description

}
}

static void xtensa_disable_ibreak(int index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

);
}

static void xtensa_enable_dbreak(int index, int type, uintptr_t address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

}
}

static void xtensa_disable_dbreak(int index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

}

static struct xtensa_debug_trigger *
xtensa_debug_find_slot(int trigger_type, int type, void *address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

return NULL;
}

static int xtensa_debug_init(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

* Name: up_debugpoint_add
****************************************************************************/

int up_debugpoint_add(int type, void *addr, size_t size,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

* Name: up_debugpoint_remove
****************************************************************************/

int up_debugpoint_remove(int type, void *addr, size_t size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Contributor

@tmedicci tmedicci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chirping78 , nice job!

Can you please improve the testing section on how to run the gdbstub and how to test it using a real HW? Also, being a new feature, can you please provide the related documentation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants