[0/3] Reentrancy

Message ID 4b19412f4ec22c719c7d0effe9e0e096@sourceware.mail.kapsi.fi
Headers
Series Reentrancy |

Message

Pekka Seppänen Aug. 30, 2023, 9:16 a.m. UTC
  Hi,

The following patch series provides small (one line) fixes for 
newlib/libc
reentrancy when using --enable-newlib-reent-thread-local and uses the 
existing
infrastructure to do that:

   - There were a few locations on which _errno member was directly 
accessed.
As struct _reent is not available if _REENT_THREAD_LOCAL is defined, any
accesses to errno shall be done using the corresponding _REENT_ERRNO() 
macro.
Should be self-explanatory.

   - __getreent() did not check that struct _reent and _impure_ptr were
available.  Both <sys/reent.h> and impure.c, that declare and define
_impure_ptr, use similar #ifdef/ifndef gate.  If thread-local storage is 
used
the allocated objects are not related to each other.  Therefore 
__getreent()
does not exist even at a concept level, hence should not be provided at 
all.

   - As _Thread_local might not be available (as in that particular 
keyword),
<sys/reent.h> shall include <sys/cdefs.h> that will provide a correct 
mapping,
should the compiler actually support thread-local storage.  A prime 
example is
libstdc++ configure, which uses #include <math.h> in some of the 
generated
checks.  Essentially any C++ target is affected and possibly C targets 
in
near future as C23 uses thread_local, not _Thread_local.

It should be noted that libgloss is affected by similar reentrancy 
issues, but
I decided not to touch those.

-- Pekka
  

Comments

Corinna Vinschen Aug. 31, 2023, 9:23 a.m. UTC | #1
Hi Pekka,

your patches don't apply cleanly because your MUA added unwanted
line breaks.

Can you please tell your MUA not to do that and send the patches again?
If that's not possible for some reason, add the patches as plaintext
attachments to a single mail.


Thanks,
Corinna


On Aug 30 12:16, Pekka Seppänen wrote:
> Hi,
> 
> The following patch series provides small (one line) fixes for newlib/libc
> reentrancy when using --enable-newlib-reent-thread-local and uses the
> existing
> infrastructure to do that:
> 
>   - There were a few locations on which _errno member was directly accessed.
> As struct _reent is not available if _REENT_THREAD_LOCAL is defined, any
> accesses to errno shall be done using the corresponding _REENT_ERRNO()
> macro.
> Should be self-explanatory.
> 
>   - __getreent() did not check that struct _reent and _impure_ptr were
> available.  Both <sys/reent.h> and impure.c, that declare and define
> _impure_ptr, use similar #ifdef/ifndef gate.  If thread-local storage is
> used
> the allocated objects are not related to each other.  Therefore __getreent()
> does not exist even at a concept level, hence should not be provided at all.
> 
>   - As _Thread_local might not be available (as in that particular keyword),
> <sys/reent.h> shall include <sys/cdefs.h> that will provide a correct
> mapping,
> should the compiler actually support thread-local storage.  A prime example
> is
> libstdc++ configure, which uses #include <math.h> in some of the generated
> checks.  Essentially any C++ target is affected and possibly C targets in
> near future as C23 uses thread_local, not _Thread_local.
> 
> It should be noted that libgloss is affected by similar reentrancy issues,
> but
> I decided not to touch those.
> 
> -- Pekka
  
Corinna Vinschen Aug. 31, 2023, 9:24 a.m. UTC | #2
Hi Sebastian,

given you're the one pushing the thread_local change in the first
place, would you mind to review this, too?  Looks good to me, but
I'd like to have your input on this.

If you think the patches are ok, feel free to push them.


Thanks,
Corinna


On Aug 30 12:16, Pekka Seppänen wrote:
> Hi,
> 
> The following patch series provides small (one line) fixes for newlib/libc
> reentrancy when using --enable-newlib-reent-thread-local and uses the
> existing
> infrastructure to do that:
> 
>   - There were a few locations on which _errno member was directly accessed.
> As struct _reent is not available if _REENT_THREAD_LOCAL is defined, any
> accesses to errno shall be done using the corresponding _REENT_ERRNO()
> macro.
> Should be self-explanatory.
> 
>   - __getreent() did not check that struct _reent and _impure_ptr were
> available.  Both <sys/reent.h> and impure.c, that declare and define
> _impure_ptr, use similar #ifdef/ifndef gate.  If thread-local storage is
> used
> the allocated objects are not related to each other.  Therefore __getreent()
> does not exist even at a concept level, hence should not be provided at all.
> 
>   - As _Thread_local might not be available (as in that particular keyword),
> <sys/reent.h> shall include <sys/cdefs.h> that will provide a correct
> mapping,
> should the compiler actually support thread-local storage.  A prime example
> is
> libstdc++ configure, which uses #include <math.h> in some of the generated
> checks.  Essentially any C++ target is affected and possibly C targets in
> near future as C23 uses thread_local, not _Thread_local.
> 
> It should be noted that libgloss is affected by similar reentrancy issues,
> but
> I decided not to touch those.
> 
> -- Pekka
  
Pekka Seppänen Aug. 31, 2023, 10:40 a.m. UTC | #3
On 31.8.2023 12:23, Corinna Vinschen wrote:
> Hi Pekka,
> 
> your patches don't apply cleanly because your MUA added unwanted
> line breaks.
> 
> Can you please tell your MUA not to do that and send the patches again?
> If that's not possible for some reason, add the patches as plaintext
> attachments to a single mail.
> 

My apologies, appears not to be possible until the next distro stable 
version rolls out.  As that might take a while, here are the patches as 
plain text attachments.

-- Pekka
  
Sebastian Huber Sept. 11, 2023, 8:09 a.m. UTC | #4
Hello Pekka,

thanks for the fixes. I reviewed the changes and checked them in.