Feature/fix wd start casts #741
Conversation
|
i'm not sure if this is an improvement. |
|
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. |
|
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? |
|
"13.What registers are used by the C compiler?" describes the AVR calling convention and the document describes that:
I have confirmed by avr-gcc that the description is correct. As I wrote in issue #740, this PR only fixes architecture independent codes. |
|
Originally the problem is pointed out by Sebastian Perta in: I don't have a information about Renesas RX ABI, but I think that it have similar calling conventions. |
|
@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. |
| * 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); |
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
In general the change looks good. I would like you to consider the change that I suggested in the comments:
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. 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.
|
@xiaoxiang781216 I got it. I'll add the commits to remove all casts. |
|
@patacongo 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. |
|
@YuuichiNakamura could you merge "Fix types of the arguments for wd_start() callback" into "Remove type casting in pthread_condtimedwait()"? You can try: |
|
@xiaoxiang781216 Thanks. I'll arrange the commits and force push again. |
YuuichiNakamura
force-pushed
the
feature/fix-wd_start-casts
branch
from
e57bdee
to
680a649
Compare
|
LGTM. |
|
LGTM |
This is a PR of issue #740.