You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "jasonbu (via GitHub)" <gi...@apache.org> on 2025/01/05 12:35:16 UTC
[PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
jasonbu opened a new pull request, #15425:
URL: https://github.com/apache/nuttx/pull/15425
## Summary
we are mixed using inline and inline_funcion in irq.h
also this kind of declaration is a little bit confuzed,
```C
static inline uint8_t getprimask(void) always_inline_function;
static inline uint8_t getprimask(void)
```
a further in-development issue is:
in arm-v6/7/8m when **exception_direct**, the running_regs in xcp, will not match with the arm auto saved register.
that will mostly cause the backtrace fail as regs pointed to the random value in stack.
we should make a difference between arm-m and other arm-series. make up_getusrsp isolate by each arh should prefer.
## Impact
in arm up_getusrsp from arm-common.c to irq.h each arch.
all other arch move up_getusrsp to irq.h also.
no other side effect.
should possible fix the psp & exception_direct caused unwind failure in next patch.
## Testing
CI-test, local mps2 qemu.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
Posted by "lupyuen (via GitHub)" <gi...@apache.org>.
lupyuen commented on PR #15425:
URL: https://github.com/apache/nuttx/pull/15425#issuecomment-2574214966
Sorry @jasonbu could you check if the PR above is causing this `up_getusrsp` build error? Thanks :-)
https://github.com/NuttX/nuttx/actions/runs/12637024742/job/35210486919#step:7:266
```text
Configuration/Tool: c5471evm/httpd,CONFIG_ARM_TOOLCHAIN_GNU_EABI
misc/assert.c: In function 'dump_running_task':
Error: misc/assert.c:659:21: error: implicit declaration of function 'up_getusrsp'; did you mean 'up_getusrpc'? [-Werror=implicit-function-declaration]
659 | dump_stacks(rtcb, up_getusrsp(regs));
| ^~~~~~~~~~~
| up_getusrpc
```
More errors at nuttx-dashboard.org
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
Posted by "jasonbu (via GitHub)" <gi...@apache.org>.
jasonbu commented on code in PR #15425:
URL: https://github.com/apache/nuttx/pull/15425#discussion_r1903563977
##########
arch/tricore/include/irq.h:
##########
@@ -186,6 +186,13 @@ static inline_function bool up_interrupt_context(void)
return ret;
}
+
+/****************************************************************************
+ * Name: up_getusrsp
+ ****************************************************************************/
+
+uintptr_t up_getusrsp(void *regs);
Review Comment:
the current up_getusrsp method in tricore is not good enough, first patch keep the implementation in tricore_registerdump.c,
and if we want to move up_getusrsp to irq.h, should move the tricore_csa2addr from internal.h to public header
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
Posted by "jasonbu (via GitHub)" <gi...@apache.org>.
jasonbu commented on PR #15425:
URL: https://github.com/apache/nuttx/pull/15425#issuecomment-2574394223
> Sorry @jasonbu could you check if the PR above is causing this `up_getusrsp` build error? Thanks :-) https://github.com/NuttX/nuttx/actions/runs/12637024742/job/35210486919#step:7:266
>
> ```
> Configuration/Tool: c5471evm/httpd,CONFIG_ARM_TOOLCHAIN_GNU_EABI
> misc/assert.c: In function 'dump_running_task':
> Error: misc/assert.c:659:21: error: implicit declaration of function 'up_getusrsp'; did you mean 'up_getusrpc'? [-Werror=implicit-function-declaration]
> 659 | dump_stacks(rtcb, up_getusrsp(regs));
> | ^~~~~~~~~~~
> | up_getusrpc
> ```
>
> More errors at nuttx-dashboard.org 
watching, can re-produce in my environment, looks like the ARCH_CHIP_C5471 missing some header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #15425:
URL: https://github.com/apache/nuttx/pull/15425
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #15425:
URL: https://github.com/apache/nuttx/pull/15425#discussion_r1903450693
##########
arch/x86/include/i486/irq.h:
##########
@@ -293,15 +293,15 @@ static inline irqstate_t up_irq_save(void)
/* Conditionally disable interrupts */
-static inline void up_irq_restore(irqstate_t flags)
+static inline_function void up_irq_restore(irqstate_t flags)
{
if (up_irq_enabled(flags))
{
up_irq_enable();
}
}
-static inline void system_call3(unsigned int nbr, uintptr_t parm1,
+static inline_function void system_call3(unsigned int nbr, uintptr_t parm1,
uintptr_t parm2, uintptr_t parm3)
Review Comment:
align
##########
arch/tricore/include/irq.h:
##########
@@ -186,6 +186,13 @@ static inline_function bool up_interrupt_context(void)
return ret;
}
+
+/****************************************************************************
+ * Name: up_getusrsp
+ ****************************************************************************/
+
+uintptr_t up_getusrsp(void *regs);
Review Comment:
where the implementation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] merge inline & inline_funcion to inline_funcion in irq.h, move up_getusrsp to irq.h per arch [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #15425:
URL: https://github.com/apache/nuttx/pull/15425#issuecomment-2571614402
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
This PR appears to **mostly** meet the NuttX requirements but is missing some key information and clarity. Here's a breakdown:
**Strengths:**
* **Clear problem statement:** The summary explains the issue with inconsistent inline declarations and hints at a larger problem with `up_getusrsp` and exception handling.
* **Impact description touches on key areas:** It mentions changes to `up_getusrsp` across architectures and claims no other side effects.
* **Testing mentions CI and a specific target:** This shows some level of testing was performed.
**Weaknesses:**
* **Summary lacks detail on *how* the change works:** While the problem is stated, the summary doesn't explain *how* the inconsistent declarations are being fixed. What is the proposed solution? More specifics are needed. e.g., "All declarations will use `static inline` consistently" or "The redundant declaration will be removed".
* **Impact is vague:** "no other side effect" is too general. Be explicit. For example:
* **Impact on user:** NO
* **Impact on build:** NO (assuming no build system changes are needed)
* **Impact on hardware:** YES (affects all architectures due to `up_getusrsp` changes - specify which architectures were tested).
* **Impact on documentation:** NO (or YES if documentation needs updating due to these changes)
* **Impact on security:** NO (unless there are security implications, justify why)
* **Impact on compatibility:** NO (or YES if there are compatibility concerns - be specific)
* **Testing is insufficient:** "CI-test, local mps2 qemu" is not enough detail.
* **Build Host(s):** Provide details about the build host OS, CPU architecture, and compiler version used.
* **Target(s):** Be more specific about the target architecture and board configuration used for testing beyond "mps2 qemu." List *all* tested targets.
* **Logs:** The template specifically requests "Testing logs before change" and "Testing logs after change". These are missing. Even if the logs are voluminous, a snippet demonstrating the corrected behavior is essential.
**Recommendations for improvement:**
1. **Elaborate on the solution:** Explain precisely how the code is changed to address the inline declaration inconsistencies.
2. **Provide specific impact details:** Replace vague statements like "no other side effect" with explicit YES/NO answers for each impact category and provide justifications.
3. **Significantly improve the testing section:** Include details about the build host environment, all tested target configurations, and *most importantly*, provide relevant snippets of testing logs demonstrating the issue before the change and the corrected behavior after the change. If the full logs are too large, consider linking to a CI run or a separate file containing them. Even a concise summary of the test results is better than nothing.
By addressing these weaknesses, the PR will better meet the NuttX requirements and be easier for reviewers to evaluate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org