You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/04/06 04:57:02 UTC
[GitHub] [incubator-nuttx] YuuichiNakamura opened a new pull request #741:
Feature/fix wd start casts
YuuichiNakamura opened a new pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741
This is a PR of issue #740.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404108953
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Actual type is wdparm_t. It will be a 64-bit uintpr_t on 64-bit machines, and 32-bits on others. I don't think you can guarantee that uint32_t will always be correct. Depending on endian-ness, this could grab the wrong 32-bits of the 32-bit value.
My guess is that this will work on the 64-bit simulation, but only because it is little endian and you will probably get the least significant 32-bits which hold the correct signal number.
The existing code has a similar issue since the parameter was received as a uint32_t but does work fine on a 64-bit simulator. I think that was only lucky.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] YuuichiNakamura commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
YuuichiNakamura commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-609632119
Originally the problem is pointed out by Sebastian Perta in:
http://mail-archives.apache.org/mod_mbox/nuttx-dev/202003.mbox/%3CTYAPR01MB2797027AA22C5187BD5EEAAAE0F40%40TYAPR01MB2797.jpnprd01.prod.outlook.com%3E
I don't have a information about Renesas RX ABI, but I think that it have similar calling conventions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404108953
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Actual type is wdparm_t. It will be a 64-bit uintpr_t on 64-bit machines, and 32-bits on others. I don't think you can guarantee that uint32_t will always be correct. Depending on endian-ness, this could grab the wrong 32-bits of the 32-bit value.
My guess is that this will work on the 64-bit simulation, but only because it is little endian and you will probably get the least significant 32-bits which hold the correct signal number.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-610130151
@YuuichiNakamura could you merge "Fix types of the arguments for wd_start() callback" into "Remove type casting in pthread_condtimedwait()"? You can try:
1.git rebase --interactive
2.git push -f
And add new patch to fix nxstyle issue as much as possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-610328770
LGTM.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404111591
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Wouldn't this make more sense:
int signo;
...
signo = (int)va_arg(ap, wdparm_t);
Since the width of the argument is wdparm_t, not necessarily 32-bits, and because the signal number started out as an integer to begin with.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] YuuichiNakamura commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
YuuichiNakamura commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-609578399
It is related to the calling convention of some processors.
In many RISC processors, some arguments of the function call are passed by the CPU registers and the remaining arguments by the stack. It is same in both fixed and variadic functions.
But, some processors such as AVR have the different calling convention for the variadic function.
It passes *all* arguments by the stack, not using the registers.
So if the pointer to the fixed argument function casts to the pointer to the variadic function, the argument cannot be passed correctly.
This PR removes such casts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on issue #741: Feature/fix wd
start casts
Posted by GitBox <gi...@apache.org>.
yamt commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-609572765
i'm not sure if this is an improvement.
wd_start/wd_expiration still treat these arguments as of wdparm_t.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #741: Feature/fix
wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] yamt commented on issue #741: Feature/fix wd
start casts
Posted by GitBox <gi...@apache.org>.
yamt commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-609588057
does it conform to C? how can functions without prototypes work?
can you give me a pointer to the AVR calling convention doc? just curious.
are you going to fix all such casts in the tree?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] YuuichiNakamura commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
YuuichiNakamura commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-610116061
@patacongo
I think that it also work fine even on 64-bit big endian processor because the argument smaller than 64-bit is promoted to 64-bit in argument passing.
But, in this case, wdparm_t should be used as your suggestion because wd_start() treats the all arguments as wdparm_t. I'll fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] YuuichiNakamura commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
YuuichiNakamura commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-610142064
@xiaoxiang781216 Thanks. I'll arrange the commits and force push again.
Now I'm fixing the styles...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-609708470
@YuuichiNakamura, how about we remove the all cast? Many people add the new feature by coping the similar code. The bad usage will spread again if we don't completely remove them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404111591
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Wouldn't this may more sense:
uint32_t signo;
...
signo = (int)va_arg(ap, wdpart_t);
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] jerpelea commented on issue #741: Feature/fix wd
start casts
Posted by GitBox <gi...@apache.org>.
jerpelea commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-610332341
LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] YuuichiNakamura commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
YuuichiNakamura commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-610115664
@xiaoxiang781216 I got it. I'll add the commits to remove all casts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404108953
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Actual type is wdparm_t. It will be a 64-bit uintpr_t on 64-bit machines, and 32-bits on others. I don't think you can guarantee that uint32_t will always be correct. Depending on endian-ness, this could grab the wrong 32-bits of the 32-bit value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] YuuichiNakamura commented on issue #741:
Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
YuuichiNakamura commented on issue #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#issuecomment-609630052
https://onlinedocs.microchip.com/pr/GUID-317042D4-BCCE-4065-BB05-AC4312DBC2C4-en-US-1/index.html?GUID-D7A8D3E7-DF5F-4CD6-8764-B5039BC9E5FE
"13.What registers are used by the C compiler?" describes the AVR calling convention and the document describes that:
> Arguments to functions with variable argument lists (printf etc.) are all passed on stack,
I have confirmed by avr-gcc that the description is correct.
As I wrote in issue #740, this PR only fixes architecture independent codes.
There are many type casting in arch/, boards/, and drivers/. But because almost casts are in the code only for arm processor, they doesn't occur the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404111591
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Wouldn't this make more sense:
uint32_t signo;
...
signo = (int)va_arg(ap, wdpart_t);
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#741: Feature/fix wd start casts
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #741: Feature/fix wd start casts
URL: https://github.com/apache/incubator-nuttx/pull/741#discussion_r404111591
##########
File path: sched/pthread/pthread_condtimedwait.c
##########
@@ -67,61 +68,73 @@
*
****************************************************************************/
-static void pthread_condtimedout(int argc, uint32_t pid, uint32_t signo)
+static void pthread_condtimedout(int argc, wdparm_t arg1, ...)
{
-#ifdef HAVE_GROUP_MEMBERS
+ uint32_t pid = (uint32_t)arg1;
+ uint32_t signo;
+ va_list ap;
- FAR struct tcb_s *tcb;
- siginfo_t info;
+ /* Retrieve the variadic argument */
- /* The logic below if equivalent to nxsig_queue(), but uses
- * nxsig_tcbdispatch() instead of nxsig_dispatch(). This avoids the group
- * signal deliver logic and assures, instead, that the signal is delivered
- * specifically to this thread that is known to be waiting on the signal.
- */
+ va_start(ap, arg1);
+ signo = (uint32_t)va_arg(ap, uint32_t);
Review comment:
Wouldn't this make more sense:
uint32_t signo;
...
signo = (int)va_arg(ap, wdparm_t);
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services