#3870 closed defect (fixed)

libc_reent set up is broken

Reported by: Chris Johns Owned by: Chris Johns <chrisj@…>
Priority: normal Milestone: 4.11.4
Component: score Version: 4.11
Severity: normal Keywords:
Cc: Blocked By:
Blocking: #3877

Description

The patch ...

https://git.rtems.org/rtems/commit/?id=69aa33490b1cd357519ab70b15ad150e11bb752e

... removes pieces of the initialisation of newlib's re-entrant struct. Are they are needed? I think they are. This breaks code such as telnet clients is subtle and difficult to find ways.

With this change you cannot assign anything to stdint or stdout unless a suitable call to newlib is made that has the internal CHECK_INIT() macro. Any assignment is wiped out as _sdidinit is false and that results in _sinit being called.

Is the lazy init in newlib on purpose? If so should __getreent in our libcsupport be returning a struct that is not initialised?

Is the change in the linked patch standards compliant?

A simple grep of code for an assignment of libc_reent did not show how this TCB field was initialised and it is not apparrent or easy to find. Why is the code written this way? There are no comments with _Thread_Control_add_ons and Thread_Configured_control explaining what are doing and how to use them and how they get loaded into the TCB.

Change History (19)

comment:1 Changed on 02/11/20 at 03:44:18 by Chris Johns

I see the reent struct is appended to the end of the TCB struct so a single allocation is made. This is nice but it is not easy to see.

The re-entrant struct is initialised the same way in the patch as before so the original initialisation does not allow access to stdin or stdout until after a suitable newlib call has been made.

There is no public interface to call __sinit, a call to newlib has to be made. The newlib code uses an internal macro called CHECK_INIT and it does:

     if ((_check_init_ptr) && !(_check_init_ptr)->__sdidinit)	\
	__sinit (_check_init_ptr);

I wonder if our __getreent could ...

struct _reent *__getreent(void)
{
  struct _reent *_reent_ptr = _Thread_Get_executing()->libc_reent;
  if (!_reent_ptr->__sdidinit)
    feof(_reent_ptr->stdout);
  return _reent_ptr;
}

?

Last edited on 02/11/20 at 03:45:02 by Chris Johns (previous) (diff)

comment:2 in reply to:  1 ; Changed on 02/11/20 at 06:08:31 by Sebastian Huber

Replying to Chris Johns:

I see the reent struct is appended to the end of the TCB struct so a single allocation is made. This is nice but it is not easy to see.

Yes, it is not easy to see, however, it is also not true that there are no comments in the code. For example:

https://docs.rtems.org/doxygen/branches/master/group__ScoreThread.html#ga60a80015d5ec442d6e0f42c1c7969d0a

comment:3 in reply to:  1 ; Changed on 02/11/20 at 06:17:39 by Sebastian Huber

Replying to Chris Johns:

The re-entrant struct is initialised the same way in the patch as before so the original initialisation does not allow access to stdin or stdout until after a suitable newlib call has been made.

The re-entrant struct is initialized by newlib_create_hook() and destroyed by newlib_terminate_hook(). There is a test case for this area: libtests/newlib01.

There is no public interface to call __sinit, a call to newlib has to be made. The newlib code uses an internal macro called CHECK_INIT and it does:

     if ((_check_init_ptr) && !(_check_init_ptr)->__sdidinit)	\
	__sinit (_check_init_ptr);

I wonder if our __getreent could ...

struct _reent *__getreent(void)
{
  struct _reent *_reent_ptr = _Thread_Get_executing()->libc_reent;
  if (!_reent_ptr->__sdidinit)
    feof(_reent_ptr->stdout);
  return _reent_ptr;
}

?

The __getreent() should not do any initialization steps.

Sorry, I don't know the problem you want to solve.

Last edited on 02/11/20 at 06:18:40 by Sebastian Huber (previous) (diff)

comment:4 in reply to:  3 ; Changed on 02/11/20 at 06:35:41 by Chris Johns

Replying to Sebastian Huber:

Replying to Chris Johns:
The re-entrant struct is initialized by newlib_create_hook() and destroyed by newlib_terminate_hook(). There is a test case for this area: libtests/newlib01.

Sure, that maybe a better place.

The __getreent() should not do any initialization steps.

Sure.

Sorry, I don't know the problem you want to solve.

Telnet is broken with joel script. This is compounded by the shell's main loop handling of stdout and stdin also being broken. See #3859. You see it when you create a new thread in the telnet shell thread, ie a joel command, and you want the IO to inherit the parent's stdin and stdout.

Create a thread and do not access any libc calls then do ...

  stdin = fopen("a-file", "r");
  stdout = my_stdout;
  setvbuf(stdout, NULL, _IONBF, 0);
  printf("hello will appear on the global stdout\n");

The stdin and stdout files pointers well be initialised to the globals. If you make a libc call before this type of logic, for example fflush(stdout); things work.

comment:5 in reply to:  2 Changed on 02/11/20 at 06:37:30 by Chris Johns

Replying to Sebastian Huber:

Replying to Chris Johns:

I see the reent struct is appended to the end of the TCB struct so a single allocation is made. This is nice but it is not easy to see.

Yes, it is not easy to see, however, it is also not true that there are no comments in the code. For example:

https://docs.rtems.org/doxygen/branches/master/group__ScoreThread.html#ga60a80015d5ec442d6e0f42c1c7969d0a

Thank you. It is not clear in confdefs.h what is going on and that code does not have a comment.

comment:6 in reply to:  4 ; Changed on 02/11/20 at 06:46:09 by Sebastian Huber

Replying to Chris Johns:

Replying to Sebastian Huber:

Replying to Chris Johns:
Sorry, I don't know the problem you want to solve.

Telnet is broken with joel script. This is compounded by the shell's main loop handling of stdout and stdin also being broken. See #3859. You see it when you create a new thread in the telnet shell thread, ie a joel command, and you want the IO to inherit the parent's stdin and stdout.

Create a thread and do not access any libc calls then do ...

  stdin = fopen("a-file", "r");
  stdout = my_stdout;
  setvbuf(stdout, NULL, _IONBF, 0);
  printf("hello will appear on the global stdout\n");

The stdin and stdout files pointers well be initialised to the globals. If you make a libc call before this type of logic, for example fflush(stdout); things work.

It would be good to have a test case that shows the problem. Maybe for libtests/newlib01. The Newlib re-entrant and in particular the standard file pointer handling is quite subtle and brittle. In the past it was helpful to me to work with test cases and use a debugger to figure out what is going on. To me it looks that you need reference counted FILE objects?

comment:7 in reply to:  6 Changed on 02/12/20 at 05:05:08 by Chris Johns

Replying to Sebastian Huber:

It would be good to have a test case that shows the problem. Maybe for libtests/newlib01.

Here is the patch (it is what happens in my shell patch)...

diff --git a/testsuites/libtests/newlib01/init.c b/testsuites/libtests/newlib01/init.c
index 74e648799e..c7eb5c4e81 100644
--- a/testsuites/libtests/newlib01/init.c
+++ b/testsuites/libtests/newlib01/init.c
@@ -69,6 +69,8 @@ static void worker_task(rtems_task_argument arg)
   stdout = fopen(&file_path[0], "r+");
   rtems_test_assert(stdout != NULL);
 
+  setvbuf(stdout,NULL,_IONBF,0);
+
   n = fwrite(&buf[0], sizeof(buf), 1, stdout);
   rtems_test_assert(n == 1);
 

It gives what I expected ...

*** BEGIN OF TEST NEWLIB 1 ***
*** TEST VERSION: 5.0.0.adf7e7538f3815db5ee5944daee82bb7053baf90-modified
*** TEST STATE: EXPECTED_PASS
*** TEST BUILD: RTEMS_NETWORKING RTEMS_POSIX_API
*** TEST TOOLS: 7.5.0 20191114 (RTEMS 5, RSB 5 (69dcab76853c modified), Newlib d14714c69)
/opt/work/chris/rtems/kernel/rtems.git/c/src/../../testsuites/libtests/newlib01/init.c: 77 ctx->current == OPEN

To me it looks that you need reference counted FILE objects?

It is not to do with reference counts, it is about initialisation of the struct __reent in our TCB. I cannot see a suitable interface in newlib to do this. I will play and see if I can get the test to pass.

comment:8 Changed on 02/12/20 at 06:20:05 by Chris Johns

Looking into the test failure it would seems stdout is not overwritten in RTEMS 5's newlib. I wonder if this issue has been fixed.

The following trace shows this...

]] 3 0x20162b0 0 0x2017f70
]] 4 0x20162b0 0 0x201b328
]] 5 0x20162b0 1 0x201b328

The columns are an id, the _reent pointer, _reent->__sdidinit and _reent->_stdout.
Id 3 is before the open in the test, 4 is after the open and 5 is after an fflush(stdout).
As you can see __sdidinit is true after the fflush call and stdout is still the new file pointer.

I change setvbuf to fflush and the test passed with the correct file pointer.

I have tested adding a call to __sinit in newlib_create_hook and it fixes the issue. I will reassign this bug to 4.11 and post a patch.

comment:9 Changed on 02/12/20 at 06:20:57 by Chris Johns

Milestone: 5.14.11.4
Priority: highnormal

comment:10 Changed on 02/12/20 at 06:21:08 by Chris Johns

Version: 54.11

comment:11 Changed on 02/12/20 at 07:00:11 by Sebastian Huber

Sorry, I still don't know what the problem is. Do you test on RTEMS 4.11 or 5? Your modified test case fails, because your setvbuf() call enabled the non-buffered mode and the fwrite() call directly writes to the file:

worker_task (arg=0) at /home/EB/sebastian_h/src/rtems/c/src/../../testsuites/libtests/newlib01/init.c:75
75        rtems_test_assert(n == 1);
(gdb) 
77        rtems_test_assert(ctx->current == OPEN);
(gdb) p ctx->current
$8 = WRITTEN

To me this looks fine.

comment:12 Changed on 02/12/20 at 07:06:05 by Sebastian Huber

The RTEMS 4.11 release was in 2016. I added the _REENT_GLOBAL_STDIO_STREAMS option to Newlib in 2017, so this could be an RTEMS 4.11 only problem.

comment:13 in reply to:  12 ; Changed on 02/13/20 at 22:10:28 by Chris Johns

Replying to Sebastian Huber:

The RTEMS 4.11 release was in 2016. I added the _REENT_GLOBAL_STDIO_STREAMS option to Newlib in 2017, so this could be an RTEMS 4.11 only problem.

Yes this is what I have discovered. I am preparing a patch to the newlib test to check assignements to stdout are working so we can spot any regressions. The check inspects the internal __sdidinit state but I feel this is worth doing because as you say it is fragile and difficult to spot.

I am preparing shell and test patches for 4.11 and 5 to fix joel scripts on both branches and a work round fix for 4.11. The 4.11 fix is a workable hack where _sinit is called in newlib_create_hook. Your solution is preferred however I do not think a new newlib on a release branch is a viable solution.

comment:14 in reply to:  13 ; Changed on 02/14/20 at 06:27:26 by Sebastian Huber

Replying to Chris Johns:

I am preparing shell and test patches for 4.11 and 5 to fix joel scripts on both branches and a work round fix for 4.11. The 4.11 fix is a workable hack where _sinit is called in newlib_create_hook. Your solution is preferred however I do not think a new newlib on a release branch is a viable solution.

Calling __sinit() in newlib_create_hook() should fix the problem with standard file pointer assignments in RTEMS 4.11. However, it may have the side-effect that now every thread delete closes the standard file descriptors. I think we need a test case that deletes a thread and then checks if the standard file pointers are still open.

comment:15 in reply to:  14 Changed on 02/14/20 at 09:12:40 by Chris Johns

Replying to Sebastian Huber:

Replying to Chris Johns:

I am preparing shell and test patches for 4.11 and 5 to fix joel scripts on both branches and a work round fix for 4.11. The 4.11 fix is a workable hack where _sinit is called in newlib_create_hook. Your solution is preferred however I do not think a new newlib on a release branch is a viable solution.

Calling __sinit() in newlib_create_hook() should fix the problem with standard file pointer assignments in RTEMS 4.11. However, it may have the side-effect that now every thread delete closes the standard file descriptors. I think we need a test case that deletes a thread and then checks if the standard file pointers are still open.

Ah ok. I will have a look at how the test can be extended. I think these tests are nice to have around.

comment:16 Changed on 02/18/20 at 02:54:45 by Chris Johns

Blocking: 3877 added

comment:17 Changed on 02/18/20 at 02:55:20 by Chris Johns

Blocking: 3859 removed

comment:18 Changed on 02/20/20 at 01:21:03 by Chris Johns <chrisj@…>

Owner: set to Chris Johns <chrisj@…>
Resolution: fixed
Status: newclosed

In fe09d8d/rtems:

libcsupport/newlib: Call newlib's sinit to force reent initialisation

  • Newlib overtites any FILE pointers set in stdin, stdout or stderr.

Closes #3870

comment:19 Changed on 02/20/20 at 04:00:33 by Chris Johns <chrisj@…>

In 3f7ebdd/rtems:

testsuite/newlib: Check newlib does not touch an assigned std FILE pointer

Update #3870

Note: See TracTickets for help on using tickets.