RE: problem with nxsig_timeout (sig_timedwait.c)

["Nakamura, Yuuichi (Sony)" <Yu...@sony.com>]

Hi all,

I have found a same problem which Sebastian pointed out in another processor.
Not only Renesus RX, for example, AVR has similar ABI spec.

To fix the problem, I have investigated the callers of wd_start().
Please let me discuss about the problem in github issue #740.

Thanks,
Yuuichi Nakamura

> From: Sebastian Perta <Se...@renesas.com>
> Sent: Friday, March 20, 2020 1:38 AM
> To: dev@nuttx.apache.org
> Subject: problem with nxsig_timeout (sig_timedwait.c)
> 
> 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