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

Fix xtensa formatting #1694

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

xobs
Copy link
Contributor

@xobs xobs commented Nov 30, 2023

Detailed description

Debug print statements were added that cause build breakage on ESP32 Xtensa targets.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

There's one small tweak we spotted in review but otherwise this looks great. With that one item addressed, we're happy to merge this.

@@ -967,13 +967,12 @@ static void cortexr_mem_handle_fault(
#if ENABLE_DEBUG == 1
const uint32_t fault_status = cortexar_coproc_read(target, CORTEXAR_DFSR);
const uint32_t fault_addr = cortexar_coproc_read(target, CORTEXAR_DFAR);
DEBUG_WARN("%s: Failed at 0x%08" PRIx32 " (%08" PRIx32 ")\n", func, fault_addr, fault_status);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than moving this, it should be gated by #ifndef DEBUG_WARN_IS_NOOP in place of #if ENABLE_DEBUG == 1 - this will stop the variable definitions doing the wrong thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I've updated this gate.

Copy link
Member

Choose a reason for hiding this comment

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

We guess it's fine to have the DEBUG_WARN() inside the guard block, just.. not ideal as we should really complete the IO operations before reporting the failure. That said, this nominally works so we'll go with it and merge.

This function has a debug print statement that doesn't use `PRIx32`,
resulting in build breakage on Farpatch.

Signed-off-by: Sean Cross <[email protected]>
This function needs to use PRIu32 when printing values. This fixes the
build on Farpatch.

Signed-off-by: Sean Cross <[email protected]>
Using printf formatters fixes the build on ESP32 Xtensa targets.

Signed-off-by: Sean Cross <[email protected]>
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼, merging. Thank you for the contribution!

@@ -967,13 +967,12 @@ static void cortexr_mem_handle_fault(
#if ENABLE_DEBUG == 1
const uint32_t fault_status = cortexar_coproc_read(target, CORTEXAR_DFSR);
const uint32_t fault_addr = cortexar_coproc_read(target, CORTEXAR_DFAR);
DEBUG_WARN("%s: Failed at 0x%08" PRIx32 " (%08" PRIx32 ")\n", func, fault_addr, fault_status);
Copy link
Member

Choose a reason for hiding this comment

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

We guess it's fine to have the DEBUG_WARN() inside the guard block, just.. not ideal as we should really complete the IO operations before reporting the failure. That said, this nominally works so we'll go with it and merge.

@dragonmux dragonmux merged commit ef32a54 into blackmagic-debug:main Dec 1, 2023
6 checks passed
@xobs xobs deleted the fix-xtensa-formatting branch December 1, 2023 01:18
@dragonmux dragonmux added this to the v2.0 release milestone Dec 20, 2023
@dragonmux dragonmux added the Bug Confirmed bug label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants