Re: Some questions on reentrancy, __DYNAMIC_REENT__ and _impure_ptr

Thanks Jeff!

Have some follow up questions, esp. about _REENT as it seems to take on 
different expansion results depending on whether I try to print it 
during build or not, details below.

2021-04-01 12:26 に Jeff Johnston さんは書きました:
> On Thu, Apr 1, 2021 at 12:58 AM Nick <[email protected]> wrote:
> 
>> Hi,
>> 
>> I've been trying to enable reentrancy of newlib on a home brew
>> kernel
>> for the x86 platform and have some questions on how various pieces
>> all
>> fits together.
>> 
>> Implemented __getreent () to return a private copy of struct reent,
>> and
>> also hard coded __DYNAMIC_REENT__ and GETREENT_PROVIDED in
>> sys/config.h
>> to rule out any issue of passing in via build CFLAGS or the CFLAGS
>> in
>> configure.host. Things including errno seem to work but not totally
>> making sense.
>> 
>> As many library functions are still accessing the reent structure
>> using
>> _impure_ptr instead of calling my __getreent () function, for
>> example,
>> the CHECK_INIT (_REENT, fp) at the beginning of __swsetup_r (struct
>> _reent *ptr, register FILE * fp).
>> 
>> Questions:
>> 
>> 1. Are the library functions expected to still use _impure_ptr
>> instead
>> of calling __getreent () when both __DYNAMIC_REENT__ and
>> GETREENT_PROVIDED are hard coded in sys/config.h?
> 
> No, the dynamic reent system redefines the _REENT macro in sys/reent.h
> to call
> __getreent().  Something is going wrong in your build.  You can try
> compiling a file with -dD to see what is defined by the preprocessor
> to see
> what went wrong.  If you have specified --enable-newlib-multithread=no
> or
> --disable-newlib-multithread, then the dynamic reent system will not
> be used.


You are right! The build has somehow gone wrong. I added #error in the 
_impure_ptr branch in sys/reent.h like below:

#if defined(__DYNAMIC_REENT__) && !defined(__SINGLE_THREAD__)
#ifndef __getreent
   struct _reent * __getreent (void);
#endif
# define _REENT (__getreent())
#else /* __SINGLE_THREAD__ || !__DYNAMIC_REENT__ */

#error __getreent() is not being used!

# define _REENT _impure_ptr
#endif /* __SINGLE_THREAD__ || !__DYNAMIC_REENT__ */

I also broke the stub __getreent () implementation in getreent.c just in 
case it was unexpectedly pulled in instead of my implementation.

However, _REENT would still expand into to _impure_ptr. as observed by 
objdump the resultant binaries, without triggering either of the above.

What's more, if I add a #pragma message right above the _REENT inside 
library source files to print out what it would expand into, the message 
say that it has expanded to __getreent () as expected, and it really 
does, thus making that specific reference to _REENT working as expected. 
But all other _REENTs scattered throughout the library source still 
expended to _impure_ptr and fails at run time. This is despite the 
"#error __getreent() is not being used!" directive inside sys/reent.h 
never catching anything.

Now I'm quite confused and hopefully it wasn't due to today's date :(

Also --enable-newlib-multithread=no wasn't specified, and specifying 
--enable-newlib-multithread=yes to the configure script does not change 
the behavior either.

Any suggestions on what might be the issue here or where to look for it?

>> If so, how do they provide reentrancy? Since _impure_ptr is a global
>> 
>> pointer visible to all threads and threads can easily step on each
>> other's toes trying to change fields in the reent structure pointed
>> to
>> by _impure_ptr concurrently.
> 
>> If not, what other MACROs or changes should I make so that all the
>> library functions all use __getreent () instead of _impure_ptr? Is
>> it
>> okay to set _impure_ptr to a bad value such as NULL in this case, in
>> 
>> order to catch any unintended access?
> 
> Note there is a global impure ptr reference used for std I/O which is
> shared
> between threads.
> 
>> 2. in the documentation on https://sourceware.org/newlib/, the
>> following
>> is mentioned as needed for syscalls stubs to return errno:
>> 
>> #include <errno.h>
>> #undef errno
>> extern int errno;
>> 
>> If I do include this part, all the syscalls stubs seem to do when
>> they
>> assign values to errno is setting the global int errno; inside
>> reent.c.
>> As user code built against the library don’t read out that integer
>> but
>> instead calls __(), errno set by syscall stubs can't be read out by
>> user
>> code.
>> 
>> If on the other hand I don’t include this part before my syscall
>> stubs,
>> the errno set by them do seem to work as they also set the copy in
>> reent
>> structures. What might I have missed here?
> 
> Look at lib/include/reent.h for explanation of reentrancy and
> syscalls.  You need to specify true
> 
> _r versions of the syscalls that place the errno result into the
> reentrancy structure.  Otherwise,
> you can have a collision in writing to the global errno value before
> it gets transferred unless
> you use a lock mechanism to ensure syscalls don't happen at the same
> time.

I thought if I use errno inside syscall stubs, it is a MACRO rather than 
a global and expands to (*__errno()) which is already thread safe, and 
also the same value/address user code on the same thread can read out 
later?

"specify true _r versions" meaning provide syscall stubs that ends with 
_r instead?

>> 3. There were some old discussions about manually changing
>> _impure_ptr
>> at each context switch. But I’m wondering about the validity of
>> such a
>> method since it seems like a really clumsy maneuver for kernel code
>> at
>> CPL0 to reach into user space belonging to different binaries to
>> change
>> a global pointer. What's more, if manually changing _impure_ptr at
>> each
>> context switch is needed, then what would be the purpose of
>> __DYNAMIC_REENT__, GETREENT_PROVIDED and implementing a __getreent
>> () to
>> get a thread local version?
> 
> See Dave's response on that.
> 
>> 4. Is _global_impure_ptr thread safe? It is a bit concerning as it
>> seems
>> to be pointing to the same copy of impure_data that some libraries
>> calls
>> would access, and even if I try to change _impure_ptr at each
>> context
>> switch, some threads might still be accessing _global_impure_ptr
>> concurrently?
> 
> There are locks that are performed for various I/O functions.  You
> will need to provide
> the low-level lock support ... i.e. provide a lock.c implementation
> (see misc/lock.c, libc/include/sys/lock.h).
> 
>> 5. There were also old discussions about having to provide mutex for
>> 
>> malloc, is this still the case for newer versions of newlib like
>> 4.10?
> 
> There is a mallock lock system which again uses the low-level lock.c
> implementation which you need to provide
> as part of 4.

Makes sense, I'll add a lock.

>> Thanks!
>> Nick
> 
> -- Jeff J.

Thanks again,
Nick