[PATCH v2] c-user: Document new clock manager directives

[Chris Johns]

On 16/11/21 4:27 am, Sebastian Huber wrote:
> On 11/11/2021 08:02, Sebastian Huber wrote:> On 09/11/2021 13:06, Sebastian
> Huber wrote:
>>> On 09/11/2021 08:50, Sebastian Huber wrote:
>>>> On 09/11/2021 08:41, Chris Johns wrote:
>>>>>> We could also use something like this:
>>>>>>
>>>>>> static inline struct timespec rtems_clock_get_realtime(void)
>>>>>> {
>>>>>>    struct timespec time_snapshot;
>>>>>>
>>>>>>    _Timecounter_Nanotime( &time_snapshot );
>>>>>>
>>>>>>    return time_snapshot;
>>>>>> }
>>>>>>
>>>>>> Unfortunately GCC is not able to optimize this.
>>>>>>
>>>>> Ah OK. This can be fixed and the performance improved but once the API is
>>>>> set it
>>>>> cannot change or do you think we can add a check later and not break the API?
>>>>
>>>> I filed a GCC bug for this:
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103150
>>>>
>>>> It seems I was not the only one noticing issues related to structure returns:
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101926
>>>>
>>>> However, if we want a foolproof API, then I would prefer the structure
>>>> return over the return status and pointer argument. Compilers may get better
>>>> in the future. clang has similar issues, so this is not only a GCC problem.
>>>
>>> We have at least three options for the API:
>>>
>>> 1. Use the existing FreeBSD implementation as is:
>>>
>>> void rtems_clock_get_realtime(struct timespec *);
>>>
>>> This is the easiest and most efficient approach.
>>>
>>> 2. Check for NULL and return a status:
>>>
>>> rtems_status_code rtems_clock_get_realtime(struct timespec *);
>>>
>>> This requires a wrapper function which is a bit less efficient and needs more
>>> code/testing:
>>>
>>> rtems_status_code
>>> rtems_clock_get_realtime(struct timespec *time_snapshot)
>>> {
>>>    if ( time_snapshot == NULL ) {
>>>      return RTEMS_INVALID_ADDRESS;
>>>    }
>>>
>>>    _Timecounter_Nanotime( time_snapshot );
>>>    return RTEMS_SUCCESSFUL;
>>> }
>>>
>>> 3. Return the structure and use the existing implementation:
>>>
>>> static inline struct timespec rtems_clock_get_realtime(void)
>>> {
>>>    struct timespec time_snapshot;
>>>
>>>    _Timecounter_Nanotime( &time_snapshot );
>>>
>>>    return time_snapshot;
>>> }
>>>
>>> This is currently not well supported by existing compilers:
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103150
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101926
>>>
>>
>> 4. Do nothing for a NULL pointer:
>>
>> void rtems_clock_get_realtime(struct timespec *);
>>
>> This requires a wrapper function which can use a tail call optimization:
>>
>> void
>> rtems_clock_get_realtime(struct timespec *time_snapshot)
>> {
>>    if ( time_snapshot == NULL ) {
>>      return;
>>    }
>>
>>    _Timecounter_Nanotime( time_snapshot );
>> }
> 
> How do we want to proceed with this? We ship the high performance and very
> useful clock routines from FreeBSD since 2015. I just would like to add an RTEMS
> signature for them and document them in the Clock Manager. Currently, these
> routines are the most efficient way to get clock values in RTEMS. Developers
> afraid of unchecked NULL pointers may use existing RTEMS directives or
> clock_get(). It would be nice if we can decide on a way forward.

I will leave this for Joel to decide.

Chris