#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 follow-ups: 2 3 Changed on 02/11/20 at 03:44:18 by Chris Johns
comment:2 follow-up: 5 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:
comment:3 follow-up: 4 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
orstdout
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 calledCHECK_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.
comment:4 follow-up: 6 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 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:
Thank you. It is not clear in confdefs.h what is going on and that code does not have a comment.
comment:6 follow-up: 7 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 ofstdout
andstdin
also being broken. See #3859. You see it when you create a new thread in the telnet shell thread, ie ajoel
command, and you want the IO to inherit the parent'sstdin
andstdout
.
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
andstdout
files pointers well be initialised to the globals. If you make a libc call before this type of logic, for examplefflush(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 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.1 → 4.11.4 |
---|---|
Priority: | high → normal |
comment:10 Changed on 02/12/20 at 06:21:08 by Chris Johns
Version: | 5 → 4.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 follow-up: 13 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 follow-up: 14 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 follow-up: 15 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 innewlib_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 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 innewlib_create_hook
. Your solution is preferred however I do not think a new newlib on a release branch is a viable solution.
Calling
__sinit()
innewlib_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: | new → closed |
In fe09d8d/rtems:
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
orstdout
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 calledCHECK_INIT
and it does:I wonder if our
__getreent
could ...?