[PATCH 5/9] posix: Convert to inline function

[Peter Dufault]

On Jul 18, 2013, at 14:29 , Ralf Corsepius <ralf.corsepius at rtems.org> wrote:

> On 07/18/2013 07:48 PM, Rempel, Cynthia wrote:
>>> ________________________________________
>>> From: rtems-devel-bounces at rtems.org [rtems-devel-bounces at rtems.org] on behalf of Gedare Bloom [gedare at rtems.org]
>>> Sent: Thursday, July 18, 2013 9:47 AM
>>> To: Ralf Corsepius
>>> Cc: RTEMS Devel
>>> Subject: Re: [PATCH 5/9] posix: Convert to inline function
>>> 
>>> On Thu, Jul 18, 2013 at 12:43 PM, Ralf Corsepius
>>> <ralf.corsepius at rtems.org> wrote:
>>>> On 07/17/2013 04:18 PM, Sebastian Huber wrote:
>>>>> 
>>>>> ---
>>>>>   cpukit/posix/include/rtems/posix/muteximpl.h |   20 ++++++++++++++++----
>>>>>   cpukit/posix/src/mutextranslatereturncode.c  |   22 +---------------------
>>>>>   2 files changed, 17 insertions(+), 25 deletions(-)
>>>>> 
>>>>> diff --git a/cpukit/posix/include/rtems/posix/muteximpl.h
>>>>> b/cpukit/posix/include/rtems/posix/muteximpl.h
>>>>> index d4673aa..29e93c2 100644
>>>>> --- a/cpukit/posix/include/rtems/posix/muteximpl.h
>>>>> +++ b/cpukit/posix/include/rtems/posix/muteximpl.h
>>>>> @@ -38,6 +40,8 @@ POSIX_EXTERN Objects_Information
>>>>> _POSIX_Mutex_Information;
>>>>> 
>>>>>   POSIX_EXTERN pthread_mutexattr_t _POSIX_Mutex_Default_attributes;
>>>>> 
>>>>> +extern const int _POSIX_Mutex_Return_codes[CORE_MUTEX_STATUS_LAST + 1];
>>>>> +
>>>>>   /*
>>>>>    *  @brief POSIX Mutex Manager Initialization
>>>>>    *
>>>>> @@ -144,11 +148,19 @@ int _POSIX_Mutex_Lock_support(
>>>>>    * willing to block but the operation was unable to complete within the
>>>>> time
>>>>>    * allotted because the resource never became available.
>>>>>    */
>>>>> -
>>>>> -int _POSIX_Mutex_Translate_core_mutex_return_code(
>>>>> +RTEMS_INLINE_ROUTINE int _POSIX_Mutex_Translate_core_mutex_return_code(
>>>>>     CORE_mutex_Status  the_mutex_status
>>>>> -);
>>>>> -
>>>>> +)
>>>>> +{
>>>>> +  /*
>>>>> +   *  Internal consistency check for bad status from SuperCore
>>>>> +   */
>>>>> +  #if defined(RTEMS_DEBUG)
>>>>> +    if ( the_mutex_status > CORE_MUTEX_STATUS_LAST )
>>>>> +      return EINVAL;
>>>>> +  #endif
>>>>> +  return _POSIX_Mutex_Return_codes[the_mutex_status];
>>>>> +}
>>>>> 
>>>>>   /*
>>>>>    *  _POSIX_Mutex_Get
>>>> 
>>>> I don't like this kind of changes, because they contradict to the working
>>>> principles of data abstraction and encapsulation.
>>>> 
>>>> They expose internal implementation details and symbols, no user should see.
>>>> 
>>> I think this file and it's structures/functions are not meant to be
>>> part of the user API.
>> That's correct...
> I can't check ATM and don't know off-head
> 
>> muteximpl.h is supposed to be a private header...
>> we should probably verify the header doesn't get installed (see patch 1/9), if it does, we need to modify the patch-sets so the private headers don't get installed...
>> probably a Makefile.am change...
>>>> Many people, comprising me, consider this to be very bad coding style.
>> Hmm... is that because it's defining a function in (what is intended to be) a private header?
>> Interesting catch... We'll have to try this patch-set out to see if the private headers are being installed...
>> Could you give us pointers to a coding style guide?
>> We could probably incorporate it into our Coding Style, http://www.rtems.org/wiki/index.php/Coding_Conventions
> The problem with global vars is them being global, which allows nasty 
> tricks (direct access and casts).
> 
> Global arrays are one magnitude more evil (range checking).
> 
> The worst feature about both is them being very hard to debug, because 
> you can't set breakpoints on them.
> 
> One way to work around all this is to systematically make all globals 
> static and to access them only though accessors-function. Naturally, 
> this comes at a performance penalty, but this is the price for 
> code-safety and debug-abilty.
> 
> I know, in RTEMS this approach is disliked, because this contradicts the 
> "use C as macro-asm" school some people around here prefer.
> 
> Ralf


I thread this needle by implementing it both ways.  You do sometimes want a compliant, high-performance, low-memory in-line implementation of (especially) primitives like mutex locking and semaphores.  You also don't want users to cheat.  What I do is (this is a dramatization, I don't really have code with _RTEMS_HIDE_IMPLEMENTATION):

- Add a feature test macro e.g. _RTEMS_HIDE_IMPLEMENTATION that forces the implementation to be hidden.
- When that feature test is in effect only global interfaces compliant with the POSIX spec appear in any header.
- Implement a C language implementation that uses the same implementation that shows up when _RTEMS_HIDE_IMPLEMENTATION is not defined to provide a linkable fallback.

When you generate a release you can verify it builds and runs properly with _RTEMS_HIDE_IMPLEMENTATION for all targets.  Anyone using the hidden implementation, given that it is in the "leading underscore" name space reserved for the OS, will hopefully either know what they are getting into or will only make the mistake once.

Peter
-----------------
Peter Dufault
HD Associates, Inc.      Software and System Engineering