Maybe we need find all callers of wd_start to ensure that the callback's prototype hasn't the similar issue.
On Fri, Mar 20, 2020 at 12:36 PM Gregory Nutt <spudan...@gmail.com> wrote: > > The best thing would be if you could submit a verified PR at > https://github.com/apache/incubator-nuttx > > Greg > > On 3/19/2020 10:38 AM, Sebastian Perta wrote: > > Hello, > > > > I have no experience and I'm not directly involved with NuttX (I maintain > > one of the compilers used to build NuttX), > > so hopefully I did the right thing in reporting the following issue here. > > Initially the was reported to me as a problem in the compiler however when > > I investigated I found there no problem > > In the compiler, the problem was in nuttx/sched/signal/sig_timedwait.c at > > line 359-360: > > > > wd_start(rtcb->waitdog, waitticks, > > (wdentry_t)nxsig_timeout, 1, wdparm.pvarg); > > > > Looking at the declarations of nxsig_timeout and wdentry_t: > > > > static void nxsig_timeout(int argc, wdparm_t itcb) > > typedef CODE void (*wdentry_t)(int argc, uint32_t arg1, ...); > > > > We can see those declarations are incompatible, if we remove the cast the > > most compilers (GCC for example) will return > > an warning (other compilers will go as far as to return an error). > > ... warning: passing argument ... from incompatible pointer type > > [-Wincompatible-pointer-types] > > ... note: expected 'wdentry_t' {aka 'void (*)(int, int, ...)'} but > > argument is of type 'void (*)(int, int)' > > > > One might think this warning can be safely ignored, especially since it > > works in many cases, however this is not > > generally true it can fail on some targets depending on ABI specification. > > This is made clear in the > > ISO/IEC standard chapter 7.6 more exactly the following statement: > > "The rightmost parameter plays a special role in the access mechanism, and > > will be designated parmN in this description." > > > > What this means is that "itcb" from nxsig_timeout can be at a different > > location from where "arg1" from wdentry_t is expected it to be. > > > > The target for which I've see this problem is Renesas RX. In case of RX > > itcb is placed in register R2 while arg1 is placed on the stack. > > > > The majority of functions in the code used in this way (casted to > > wdentry_t) are variadic as well so this is not a problem in general, > > just in a few cases (nxsig_timeout is the only one I found so far there > > might be a few others) > > In order to make the code ANSI/ISO compliant nxsig_timeout needs to be made > > variadic as well: > > static void nxsig_timeout(int argc, wdparm_t itcb, ...) > > > > I hope you agree with my finding and fix it as this will make the code more > > robust (incompatible pointer assignment is unsafe) > > and more portable (other targets besides Renesas RX in which I have a > > vested interest). > > > > Thank you, > > Sebastian > > > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten > > Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse > > 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: > > Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 > > WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
