Comments (23)
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.
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.
@YuuichiNakamura do you mean to make wd_start do memcpy with the given size to save the structure?
from nuttx.
Yes.
from nuttx.
the proposed api looks better than the current one to me.
from nuttx.
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.
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.
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.
@patacongo I understood. It's ok for me if the PR #741 is accepted.
Thanks.
from nuttx.
@patacongo the current api has at least two issues, besides the one pointed out here.
-
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.
-
wd_expiration calls wdog->func with variable args, all wdparm_t.
many of actual callback functions assume different types like uint32_t.
from nuttx.
i agree the proposed api is a bit ugly. but it's definitely better than the current api.
from nuttx.
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.
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.
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.
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.
@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.
@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.
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.
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.
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.
And if
FAR void *
will break things, there are many basic components will break tooNo, 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.
but can we assume that
sizeof(FAR void *)
is always equals tosizeof(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.
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)
- Add esp32c6 power manager feature HOT 3
- [BUG] Just a test HOT 1
- [BUG] kasan read access error in umm_initialize HOT 9
- [BUG] Running lm3s6965-ek:qemu-protected with gdb-multiarch is crashing HOT 5
- [HELP] ESP32 chip revision ERROR HOT 2
- [BUG] ESP32C6 DevKitM spi issue HOT 17
- [FEATURE] Extensible PL011 UART driver HOT 3
- [HELP] Cannot debug esp32c6 using open ocd HOT 8
- [HELP] prompts "undefined referenc to `_impure_prt`" HOT 6
- [FEATURE] Add support for better pre-commit HOT 2
- [HELP] Register setting during booting the OS on ARM A53 FVP HOT 2
- [HELP] Recreating symlinks/switching directory (configure) HOT 3
- [HELP] FFI compatiblity issues HOT 7
- [BUG] sim:citest doesn't compile HOT 17
- [BUG] is it typo in esp_wifi_adapter.c ? (ESP32C3) HOT 4
- [BUG] Nucleo-L432KC NSH (USART2) no RX (wrong pin map?) HOT 6
- [HELP] How to print MMU related information on ARM FVP? HOT 16
- [HELP] Why does libm not have nextafter? HOT 3
- [BUG] ESP32C6 SPI Read Issue HOT 15
- [BUG] imxrt:lpi2c kconfig and source code discrepancy HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
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.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from nuttx.