Skip to content

arch/xtensa: add crt0 to initialize environment #16208

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

anjiahao1
Copy link
Contributor

@anjiahao1 anjiahao1 commented Apr 15, 2025

Summary

add crt0 to initialize environment for xtensa arch

we use crt0 inside of start hook in pr #16154, so xtensa also need add it.

Impact

fix xtensa elf build

Testing

build esp32-devkitc:elf

@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 Apr 15, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 15, 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.

Here's a breakdown of what's missing:

  • Summary: While it states WHAT was changed (added crt0), it doesn't explain WHY (what problem did this solve? Was there a bug? Missing functionality?). It also lacks HOW it works (what does this crt0 initialization entail?). Issue references are missing.

  • Impact: The impact section is too vague. "fix xtensa elf build" is insufficient. Be specific. Does it fix a build failure? Does it change the output binaries in some way? All the other impact categories (user, build process, hardware, documentation, security, compatibility) need to be addressed, even if the answer is "NO". Explicitly stating "NO" for each category demonstrates that these aspects were considered.

  • Testing: While it mentions a target, the information is inadequate. What host OS/compiler was used? Crucially, the "Testing logs before change" and "Testing logs after change" sections are empty. Provide concrete evidence that the change works as intended. Show the build failure before the change and the successful build after. Or, if it's not a build failure, show how the behavior differs before and after the change. What specific tests were run?

In short, the PR needs more detail and evidence to demonstrate the change's validity and impact. It needs to convince a reviewer that the change is necessary, works correctly, and doesn't introduce unintended consequences.

@xiaoxiang781216 xiaoxiang781216 linked an issue Apr 15, 2025 that may be closed by this pull request
1 task
@anjiahao1 anjiahao1 force-pushed the 20250415_xtensa_elf branch from 8fdaa00 to a1c934d Compare April 15, 2025 06:21
Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please update the title to
arch/xtensa: add crt0 to initialize environment

and add a commit description which describes the change

@jerpelea jerpelea changed the title xtensa:add crt0 to initialize environment arch/xtensa: add crt0 to initialize environment Apr 15, 2025
we use crt0 inside of start hook in pr apache#16154, so xtensa also need add it.

Signed-off-by: anjiahao <[email protected]>
@anjiahao1 anjiahao1 force-pushed the 20250415_xtensa_elf branch from a1c934d to b2e39ae Compare April 15, 2025 08:28
@anjiahao1
Copy link
Contributor Author

@jerpelea thanks, i add some description for this change

@tmedicci
Copy link
Contributor

Testing on our internal CI...

@tmedicci
Copy link
Contributor

Testing on our internal CI...

Everything is fine with this PR. It solves #16201 . Thanks, @anjiahao1 !

@xiaoxiang781216 xiaoxiang781216 merged commit 28ad852 into apache:master Apr 15, 2025
13 checks passed
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.

[BUG] esp32-devkitc:elf broken after #16154
5 participants