Giter Site home page Giter Site logo

Comments (23)

YuuichiNakamura avatar YuuichiNakamura commented on August 19, 2024

I have found a same problem which Sebastian pointed out in another processor.
Not only Renesus RX, for example, AVR has similar ABI spec and it cannot work
with the current code.

So, I have investigated the all callers of wd_start() in the latest NuttX kernel.
There are 206 wd_start() callers and 47 callers of them uses the type casting
from fixed argument function pointer to the variadic function pointer (wdentry_t).

Many of them have no need to fix because they are for the specific architecture
or specific device, but 8 callers in sched/ directory must be
fixed because they are common to all architectures.

7 callers pass the function pointer with only one argument.
They should be fixed just adding periods like:

static void nxsig_timeout(int argc, wdparm_t itcb, ...)

The only one exception is pthread_condtimedwait() which handles two arguments.
(This is only one use case in all NuttX kernel source which handle
multiple arguments for wd_start() callback.)

To make it portable, va_arg() must be used in pthread_condtimedout().

I will issue the PR for them.

from nuttx.

YuuichiNakamura avatar YuuichiNakamura commented on August 19, 2024

The minimum essential changes for the portability are included in the PR, but
if possible, I think that the API spec of wd_start() should be changed.
Because the current spec is likely to misuse which breaks the portability.

My proposal is, to change the API to:
int wd_start(WDOG_ID wdog, int32_t delay, wdentry_t wdentry, size_t size, wdparm_t arg);

And change the typedef of wdentry_t to:
typedef CODE void (*wdentry_t)(wdparm_t arg);

Instead of passing the variable arguments, pass the pointer to struct and its size
to wd_start() for multiple arguments.
For the almost all callers which passes only one argument, if size == 0,
arg is not treated as the pointer, but just a single value and it is given to
the callback as it is.

How about the proposal?

I think that it is difficult to change such API spec, but if acceptable,
I'll try to make the new PR for the API change.

from nuttx.

yamt avatar yamt commented on August 19, 2024

@YuuichiNakamura do you mean to make wd_start do memcpy with the given size to save the structure?

from nuttx.

YuuichiNakamura avatar YuuichiNakamura commented on August 19, 2024

Yes.

from nuttx.

yamt avatar yamt commented on August 19, 2024

the proposed api looks better than the current one to me.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

I prefer the existing interface. Not only is the proposed interface very ugly but does not seem to have any purpose other than added complexity. Let's not change this; we are better off without that change.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

Not only Renesus RX, for example, AVR has similar ABI spec and it cannot work
with the current code.

That is odd since the AVR architecture has been supported by NuttX for over ten years and has been thoroughly excersizes with no known, oustanding problems. I believe the existing interface works fine with AVR.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

I'll try to make the new PR for the API change.

Please don't. I will close it without merging. It is inappropriate and undesirable.

from nuttx.

YuuichiNakamura avatar YuuichiNakamura commented on August 19, 2024

@patacongo I understood. It's ok for me if the PR #741 is accepted.
Thanks.

from nuttx.

yamt avatar yamt commented on August 19, 2024

@patacongo the current api has at least two issues, besides the one pointed out here.

  1. wd_start is called with variable args with various types. like uint32_t. on the other hand, wd_start assumes that they are all wdparm_t.

  2. wd_expiration calls wdog->func with variable args, all wdparm_t.
    many of actual callback functions assume different types like uint32_t.

from nuttx.

yamt avatar yamt commented on August 19, 2024

i agree the proposed api is a bit ugly. but it's definitely better than the current api.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

The minimum essential changes for the portability are included in the PR, but
if possible, I think that the API spec of wd_start() should be changed.
Because the current spec is likely to misuse which breaks the portability.

My proposal is, to change the API to:
int wd_start(WDOG_ID wdog, int32_t delay, wdentry_t wdentry, size_t size, wdparm_t arg);

And change the typedef of wdentry_t to:
typedef CODE void (*wdentry_t)(wdparm_t arg);

Instead of passing the variable arguments, pass the pointer to struct and its size
to wd_start() for multiple arguments.

What controls the "life" of structure pointed to by "arg". It cannot be in a stack and it can become stale if it is in allocated memory. In general, it cannot be a static variable either. If allocated, what frees the structure.

Because of these complexities, I think this is a bad idea. Let's not do this. I vote -1. Why not just:

int wd_start(WDOG_ID wdog, int32_t delay, wdentry_t wdentry, size_t size,wdparm_t arg1, wdparm_t arg2);`
typedef CODE void (*wdentry_t)(wdparm_t arg1, wdparm_t arg2);`

Since those are passed directly be value rather then through a container structure, the interface is simplified and there is no concern for management of the life of an allocated structure.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

What controls the "life" of structure pointed to by "arg". It cannot be in a stack and it can become stale if it is in allocated memory. In general, it cannot be a static variable either. If allocated, what frees the structure.

This is not a problem, however, if you copy the data out of the structure and into the wdog_s structure immediately in the wd_start call as is done now. The original structure itself must be assumed to be volatile.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

The inefficiency in the proposal should be considered as well. You once told me that all calls to wd_start() but one provided only a single parameter. it sems wasteful to set up a structure containing that one parameter and passing it to to wd_start().

What is the size argument? There can be no more than CONFIG_WDOG_MAXPARMS arguments and each must be size wdparm_t. A size_t is used to hold the largest possible memory object (in bytes). A count of arguments makes more since and should be just an int.

So If I wanted to pass one argument I would have to do:

wdparm_t wdarg = (wdparm_t)arg;
ret = wd_start(&wdog, delay, entry, sizeof(wdarg), &wdarg);

arg would have to copied to a wdparm_t variable because sizeof(arg) may not be compatible with sizeof(wdparm_t) so you cannnot just pass a pointer to arg.

Then wd_start() would have to divide size by sizeof(wdparm_t) to get the number of arguments?

Versus my suggestion:

ret = wd_start(&wdog, delay, entry, arg, 0);

Since we are passing by value, not reference, arg will be automatically converted to type wdparm_t.

Which looks less "ugly" to you. (We should not be using ugliness as a value for making engineering decisions unless all other things are equal. I think we say ugly because we can't verbal the reasons for preferring one thing over an another.)

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

The old saying applies here: "If it is not broken, then don't fix it."

Nothing is broken, no change is necessary. Achieving code stability is really more important than making a change to wd_start().

from nuttx.

xiaoxiang781216 avatar xiaoxiang781216 commented on August 19, 2024

@papatience after reviewing all caller of wd_start, all place just need one argument so, the prototype can be simple as:

typedef CODE void (*wdentry_t)(FAR void *arg);

just like what work_s done. If out of tree usage need pass more context info, he/she can always allocate the memory as need. So here is the patch: #1565

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

@papatience after reviewing all caller of wd_start, all place just need one argument so, the prototype can be simple as:

typedef CODE void (*wdentry_t)(FAR void *arg);

just like what work_s done. If out of tree usage need pass more context info, he/she can always allocate the memory as need. So here is the patch: #1565

If arg is the actual, single argument and NOT a pointer to an array of arguments, then this seems okay. But don't use type void *. There are problems using type void * with certain architctures (AVR I think) that was fixed by created wdparm_t. Please continue to use wdparm_t.

Again, I think it is bad engineering judgement to make this change. Nothing is broken, this fixes nothing, and just adds turmoil and additional incompatibility to the documented OS internal interfaces. This is NOT responsible engineering.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

But don't use type void *. There are problems using type void * with certain architctures (AVR I think) that was fixed by created wdparm_t. Please continue to use wdparm_t.

The argument was originally a uint32_t. I was changed to a union with commit b8f3bd8 because there are many architectures where sizeof(void *) < sizeof(uint32_t), MCS51, ez80, etc. So void * is insufficient for passing a uint32_t.

This was changed again to the current wdparm_t form with commit 3adcae8 because there were issues in passing a union with certain compilers (SDCC at the time).

So, please don't consider using void * instead of wdparm_t. You will definitely break things!

from nuttx.

xiaoxiang781216 avatar xiaoxiang781216 commented on August 19, 2024

But don't use type void *. There are problems using type void * with certain architctures (AVR I think) that was fixed by created wdparm_t. Please continue to use wdparm_t.

The argument was originally a uint32_t. I was changed to a union with commit b8f3bd8 because there are many architectures where sizeof(void *) < sizeof(uint32_t), MCS51, ez80, etc. So void * is insufficient for passing a uint32_t.

Yes, sizeof(void *) may small than sizeof(uint32_t), but I am wondering that is it possible that sizeof(FAR void*) < sizeof(uint32_t).

This was changed again to the current wdparm_t form with commit 3adcae8 because there were issues in passing a union with certain compilers (SDCC at the time).

So, please don't consider using void * instead of wdparm_t. You will definitely break things!

And if FAR void * will break things, there are many basic components will break too, e.g.:

/* This struct defines the form of an interrupt service routine */

typedef CODE int (*xcpt_t)(int irq, FAR void *context, FAR void *arg);

/* Defines the work callback */

typedef CODE void (*worker_t)(FAR void *arg);

#ifndef __PTHREAD_ADDR_T_DEFINED
typedef FAR void *pthread_addr_t;
#define __PTHREAD_ADDR_T_DEFINED 1
#endif

typedef CODE pthread_addr_t (*pthread_startroutine_t)(pthread_addr_t);
typedef pthread_startroutine_t pthread_func_t;

/* Non-standard convenience definition of signal handling function types.
 * These should be used only internally within the NuttX signal logic.
 */

typedef CODE void (*_sa_handler_t)(int signo);
typedef CODE void (*_sa_sigaction_t)(int signo, FAR siginfo_t *siginfo,
                                     FAR void *context);

int       on_exit(CODE void (*func)(int, FAR void *), FAR void *arg);

/* thrd_start_t: function pointer type passed to thrd_create */

typedef CODE int (*thrd_start_t)(FAR void *arg);

/* tss_dtor_t: function pointer type used for TSS destructor */

typedef CODE void (*tss_dtor_t)(FAR void *);

I don't think without irq/signal/work especially irq, the system can work as expect.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

And if FAR void * will break things, there are many basic components will break too

No, in general no. FAR void * is only a fatal decision if you intend to use it as a "container" to hold variables of other sizes. sizeof(uint32_t) is always 32 bits, but sizeof(void *) is variable on different MCUs from 16 to 64 bits.

from nuttx.

xiaoxiang781216 avatar xiaoxiang781216 commented on August 19, 2024

And if FAR void * will break things, there are many basic components will break too

No, in general no. FAR void * is only a fatal decision if you intend to use it as a "container" to hold variables of other sizes. sizeof(uint32_t) is always 32 bits, but sizeof(void *) is variable on different MCUs from 16 to 64 bits.

Yes, sizeof(void *) may become 16bits due to the small memory model on some arch, but can we assume that sizeof(FAR void *) is always equals to sizeof(uintptr_t) and >= sizeof(uint32_t)?
Since for wdog_s case, most caller pass 'FAR void *', and all other pass pid_t(int16_t)/uint8_t, so it's safe.
I decide to select FAR void *' because all other similar callbacks use FAR void *`. Is it reasonable?

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

but can we assume that sizeof(FAR void *) is always equals to sizeof(uintptr_t) and >= sizeof(uint32_t)?

No. that is a false assumption. sizeof(uintptr) can be less than sizeof(uint32_t).. On most 8-bit machines, sizeof(uintptr_t) is or should be 2 bytes. On ez80 it is either 2 or 3 bytes depending on mode. For ez80, you can see:

In include/stdint.h:

/* Integer types capable of holding object pointers
 * As a general rule, the size of size_t should be the same as the size of
 * uintptr_t: 32-bits on a machine with 32-bit addressing but 64-bits on a
 * machine with 64-bit addressing.
 */

typedef _ssize_t            intptr_t;
typedef _size_t             uintptr_t;

And in arch/z80/include/ez80/types.h:

/* These are the sizes of the standard integer types.  NOTE that these type
 * names have a leading underscore character.  This file will be included
 * (indirectly) by include/stdint.h and typedef'ed to the final name without
 * the underscore character.  This roundabout way of doings things allows
 * the stdint.h to be removed from the include/ directory in the event that
 * the user prefers to use the definitions provided by their toolchain header
 * files
 *
 * These are the sizes of the types supported by the ZiLOG compiler:
 *
 *   int    - 24-bits
 *   short  - 16-bits
 *   long   - 32-bits
 *   char   - 8-bits
 *   float  - 32-bits
 */
...
/* A pointer is 2 or 3 bytes, depending upon if the ez80 is in z80
 * compatibility mode or not
 *
 *   Z80 mode - 16 bits
 *   ADL mode - 24 bits
 */
...
#elif defined(CONFIG_EZ80_Z80MODE)
typedef signed short       _ssize_t;
typedef unsigned short     _size_t;
#else
typedef signed int         _ssize_t;
typedef unsigned int       _size_t;
#endif

And in limits.h:

/* A pointer is 2 or 3 bytes, depending upon if the ez80 is in z80
 * compatibility mode or not
 *
 *   Z80 mode - 16 bits
 *   ADL mode - 24 bits
 */

#define PTR_MIN     (-PTR_MAX - 1)
#ifdef CONFIG_EZ80_Z80MODE
#define PTR_MAX     32767
#define UPTR_MAX    65535U
#else
#define PTR_MAX     8388607
#define UPTR_MAX    16777215U
#endif

For AVR, the size of a pointer is different on the I-space bus and on the D-space bus. As I recall, sizeof(FAR void *) is usualy 16-bits but might be as much as 24-bits).

In fact sizeof(uinptr_t) < sizeof(uint32_t) is very common among the support architecures. You must not use FAR void * as a container for arbitrary values. You will break MANY architectures and we cannot permit you to do that.

from nuttx.

patacongo avatar patacongo commented on August 19, 2024

Is it reasonable?

Absolutely NOT! wdparm_t was added to work around the kind of errors that you are tying to re-introduce. What you are proposing will work on most 32- and 64- bit machines but almost no where else and, hence, cannot be permitted. I will close an PR that attempts to pass watchdog paramters using FAR void * when the correct solution is already in place. What you are proposing is just wrong!

If a memory storage location is allocated as a FAR void * it will be 16, 24, 32, or 64 bits wide and in many cases cannot hold a uint32_t. If it is allocated as a wdparm_t it will be 32 or 64 bits in width and is guaranteed to hold a uint32_t or a pointer in ALL cases. You proposal will not work and will break code.

This problem has already been resolved and the correct type is available, please don't be stubborn and harm the system.

from nuttx.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.