Queries concerning RTOS protection in newllib

[Antony KING]

Hi Corinna,

Which part of the patch was causing the problem ? Was it just the 
introduction of _global_impure_ptr ? I recently discovered a problem 
which was exposed by this patch when the _cleanup_r function was changed 
from walking the FILE object list with fflush() to fclose(). I wonder if 
this is the cause of the unexpected truncation.

Note that I have also submitted another patch, 
http://sources.redhat.com/ml/newlib/2004/msg00302.html, which changes 
the effect of _fwalk (called via _reclaim_reent) to only walk the FILE 
objects in current task's reent structure and not the global reentrancy 
structure (aka _GLOBAL_REENT), which is handled via the exit() function.

Since the problem seems to be with the capture of stdout, I wonder if 
the problem is related to the fact that the standard i/o handles (0, 1, 
2) are shared by all tasks and so when _reclaim_reent is called it will 
be closing these shared handles and thus, unless protected in some way, 
i/o on these handles by other tasks will be lost. In our system the 
implementation of _close (actually _close_r) prevents the closure of the 
standard handles, e.g.:

int
_close_r (ptr, fd)
      struct _reent *ptr;
      int fd;
{
   /*
    * Since the stdin, stdout & stderr file handles
    * are shared by all tasks, we do not want to allow
    * any one task to close them - hence the following
    * filter.
    */
   if ((fd != __sfileno(_stdin_r(ptr))) &&
       (fd != __sfileno(_stdout_r(ptr))) &&
       (fd != __sfileno(_stderr_r(ptr))))
   {
     return _SH_posix_Close (fd);
   }
   else
   {
     return -1;
   }
}

This is not very subtle but at least it is safe. I suspect that this 
solution is not applicable to Cygwin since it will prevent the re-use of 
the standard handles via open/dup.

If your problems are related to "walking" with fclose instead of fflush 
then perhaps what is needed is a method by which the implementation of 
_cleanup_r can be configured to select the function to "walk" over the 
FILE object list. This option could be set by a config file or through 
configure, e.g. (in libc/stdio/findfp.c):

#ifndef _STDIO_CLEANUP_FUNCTION
#define _STDIO_CLEANUP_FUNCTION fflush	/* `cheating' */
#endif

_VOID
_DEFUN(_cleanup_r, (ptr),
        struct _reent *ptr)
{
   _CAST_VOID _fwalk (ptr, _STDIO_CLEANUP_FUNCTION);
}

The above approach would restore the original behaviour (i.e. flushing 
the stdio streams) and would also allow other systems to select their 
desired behaviour.

I have also had a quick look in the cygwin sources (1.5.9-1) and the use 
of _REENT, _GLOBAL_REENT and _impure_ptr is a little tricky to follow 
given what Cygwin is doing when handling forking, shared libraries and 
thread local storage. If Cygwin is overriding libc/reent/impure.c (which 
it seems to be doing in winsup/cygwin/lib/_cygwin_crt0_common.cc) then 
in addition to the above change (in _cleanup_r) perhaps the definition 
of _GLOBAL_REENT can also be conditional on whether newlib is being 
targeted for Cygwin, e.g. (in libc/include/sys/reent.h):

#ifdef __CYGWIN__
#define _GLOBAL_REENT _impure_ptr
#else
#define _GLOBAL_REENT _global_impure_ptr
#endif

which may restore the behaviour you need. Alternatively as well as 
manipulating _impure_ptr Cygwin can manipulate _global_impure_ptr as well.

Cheers,

Antony.

Corinna Vinschen wrote:
> I just found that the below patch broke Cygwin.  While running a build
> in bison, I got an error message that I'm using an old version of m4.
> Which wasn't true.  The configure script asks for a specific string in
> the --help output of m4, roughly
> 
>   case `m4 --help` in
>   *reload-state*) ...
>   esac
> 
> The reason that this doesn't work anymore is that the output of the
> backquoted command is truncated.  I found that Anthony's patch is the
> culprit.  If I back it out, Cygwin works fine again.
> 
> The reason seem to be the invention of the _global_impure_ptr which
> then in turn is used in _GLOBAL_REENT.  Cygwin already implements
> changing _impure_ptr in case of context switches and it relies on
> this functionality.
> 
> The below described patch apparently breaks backward compatibility.
> 
> However, I'm not fluent enough in the impure_ptr business to offer
> the "right" solution except for backing out this patch again.
> 
> 
> Corinna
> 
> 
> On Jun 11 16:44, Jeff Johnston wrote:
> 
>>Thanks Antony.  Patch checked in.  A patch was also required to get 
>>x86-linux building with these changes so I added that as well.  In the 
>>future, it would be helpful to me if you could add a ChangeLog entry for 
>>any changes you make.
>>
>>-- Jeff J.
>>
>>Antony KING wrote:
>>
>>>Hi,
>>>
>>>I am currently in the process of adapting newlib co-operate with an ST
>>>developed embedded RTOS. FYI this RTOS is pretty much standard in terms
>>>of capability (e.g. tasks, mutexes, semaphores, interrupt handlers) and
>>>an application is RTOS enabled by simply linking in a static library and
>>>using the API. The target is an SH4 based device.
>>>
>>>My goal in adapting newlib for this RTOS is that the same static C
>>>library can be used in applications which either use the RTOS library or
>>>do not. Also, I would like to reduce the impact of changes I need to
>>>make to newlib. As a result I have provided my own implementation of
>>>sys/lock.h which implements the locking API as stub functions (instead
>>>of empty macros) which will be overridden when a application is linked
>>>against the RTOS static library. Also, I am aiming to get the RTOS to
>>>switch the task re-entrancy structures by modifying _impure_ptr at each
>>>context switch (instead of using a __getreent() function).
>>>
>>>Enough of the background, now my queries and solutions:
>>>
>>>1) There is a macro _GLOBAL_REENT which is used to obtain the global
>>>re-entrancy structure used for managing the atexit handlers and FILE
>>>objects. This is currently defined to be _impure_ptr which of course is
>>>incompatible with changing _impure_ptr on each context switch.
>>>
>>>My solution is to add a new variable, _global_impure_ptr, to impure.c
>>>which initially has the same value as _impure_ptr and is the value
>>>returned by _GLOBAL_REENT. The value of this variable should never change.
>>>
>>>Files affected: impure.c, sys/reent.h
>>>
>>>2) Given that I am providing an alternative implementation of sys/lock.h
>>>the current type definition for the FILE lock (_flock_t) is incorrect,
>>>especially in light of the explicit casting (and de-referencing) to
>>>_LOCK_RECURSIVE_T in a number of files when invoking the locking API.
>>>The reason is that the definition of _LOCK_RECURSIVE_T may not be the
>>>same size as the current type (int) for _flock_t.
>>>
>>>I think _flock_t should be redefined to be of type _LOCK_RECURSIVE_T
>>>(instead of int) and all the explicit casting be removed.
>>>
>>>Files affected: sys/_types.h, fclose.h findfp.c fopen.c, freopen.c,
>>>fopen64.c, vfprintf.c
>>>
>>>3) It would be useful if the stub implementations of the specialised
>>>locking APIs __malloc_lock/unlock and __env_lock/unlock use the locking
>>>API defined in sys/lock.h. This would then mean that these functions
>>>would not need overriding by an RTOS if sys/lock.h is RTOS ready.
>>>(Actually I do have to override the malloc lock API since I need to
>>>pre-allocate the malloc lock object under the control of the RTOS since
>>>the type is opaque and cannot be fully initialised statically).
>>>
>>>Files affected: envlock.c, mlock.c.
>>>
>>>Similarly it would also be useful for the stdio file locking API
>>>_flockfile/_funlockfile to be modified in the same way.
>>>
>>>Files affected: sys/stdio.h.
>>>
>>>I have attached a patch file incorporating all the changes I have made
>>>to the newlib library (against a CVS snapshot dated 03 June 2004). The
>>>implementation of the locking API (lock.h and lock.c) is also attached
>>>for reference.
>>>
>>>Please note that the patch includes some additional changes in the file
>>>sys/_reent.h where the re-entrancy structure initialisation macros have
>>>been slightly re-worked to make them more readable (for me :-), complete
>>>(I hope) and efficient (greater use of memset). Also the implementation
>>>of _cleanup_r has been altered so that fclose is called instead of
>>>fflush when walking the FILE object list.
>>>
>>>Comments please (and sorry for the overlong email).
>>>
>>>Cheers,
>>>
>>>Antony.

-- 
-----------------------------------------------------------------
Antony King                        | Email: antony.king@st.com
STMicroelectronics (R&D) Ltd.      |
Bristol, BS32 4SQ. United Kingdom. | Tel: +44 (0)1454 462646