[PATCH] Delete default __getreent() implementation

[PATCH] Delete default __getreent() implementation

[Sebastian Huber]

Systems defining  __DYNAMIC_REENT__ have to provide a proper
implementation of __getreent().  Do not include a default implementation
to avoid a potential link time issue (real vs. default implementation of
__getreent()).

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 newlib/libc/reent/Makefile.am |  1 -
 newlib/libc/reent/Makefile.in | 30 +++++++++++-------------------
 newlib/libc/reent/getreent.c  | 14 --------------
 newlib/libc/sys/rtems/crt0.c  |  2 +-
 4 files changed, 12 insertions(+), 35 deletions(-)
 delete mode 100644 newlib/libc/reent/getreent.c

diff --git a/newlib/libc/reent/Makefile.am b/newlib/libc/reent/Makefile.am
index ee11e864a..cc6041027 100644
--- a/newlib/libc/reent/Makefile.am
+++ b/newlib/libc/reent/Makefile.am
@@ -37,7 +37,6 @@ GENERAL_SOURCES = \
 	impure.c \
 	fcntlr.c \
 	fstatr.c \
-	getreent.c \
 	gettimeofdayr.c \
 	isattyr.c \
 	linkr.c \
diff --git a/newlib/libc/reent/Makefile.in b/newlib/libc/reent/Makefile.in
index 62b8500d4..9555b7fe7 100644
--- a/newlib/libc/reent/Makefile.in
+++ b/newlib/libc/reent/Makefile.in
@@ -74,15 +74,14 @@ lib_a_AR = $(AR) $(ARFLAGS)
 lib_a_LIBADD =
 am__objects_1 = lib_a-closer.$(OBJEXT) lib_a-reent.$(OBJEXT) \
 	lib_a-impure.$(OBJEXT) lib_a-fcntlr.$(OBJEXT) \
-	lib_a-fstatr.$(OBJEXT) lib_a-getreent.$(OBJEXT) \
-	lib_a-gettimeofdayr.$(OBJEXT) lib_a-isattyr.$(OBJEXT) \
-	lib_a-linkr.$(OBJEXT) lib_a-lseekr.$(OBJEXT) \
-	lib_a-mkdirr.$(OBJEXT) lib_a-openr.$(OBJEXT) \
-	lib_a-readr.$(OBJEXT) lib_a-renamer.$(OBJEXT) \
-	lib_a-signalr.$(OBJEXT) lib_a-signgam.$(OBJEXT) \
-	lib_a-sbrkr.$(OBJEXT) lib_a-statr.$(OBJEXT) \
-	lib_a-timesr.$(OBJEXT) lib_a-unlinkr.$(OBJEXT) \
-	lib_a-writer.$(OBJEXT)
+	lib_a-fstatr.$(OBJEXT) lib_a-gettimeofdayr.$(OBJEXT) \
+	lib_a-isattyr.$(OBJEXT) lib_a-linkr.$(OBJEXT) \
+	lib_a-lseekr.$(OBJEXT) lib_a-mkdirr.$(OBJEXT) \
+	lib_a-openr.$(OBJEXT) lib_a-readr.$(OBJEXT) \
+	lib_a-renamer.$(OBJEXT) lib_a-signalr.$(OBJEXT) \
+	lib_a-signgam.$(OBJEXT) lib_a-sbrkr.$(OBJEXT) \
+	lib_a-statr.$(OBJEXT) lib_a-timesr.$(OBJEXT) \
+	lib_a-unlinkr.$(OBJEXT) lib_a-writer.$(OBJEXT)
 @HAVE_STDIO64_DIR_TRUE@am__objects_2 = lib_a-fstat64r.$(OBJEXT) \
 @HAVE_STDIO64_DIR_TRUE@	lib_a-lseek64r.$(OBJEXT) \
 @HAVE_STDIO64_DIR_TRUE@	lib_a-stat64r.$(OBJEXT) \
@@ -100,9 +99,9 @@ lib_a_OBJECTS = $(am_lib_a_OBJECTS)
 LTLIBRARIES = $(noinst_LTLIBRARIES)
 libreent_la_LIBADD =
 am__objects_6 = closer.lo reent.lo impure.lo fcntlr.lo fstatr.lo \
-	getreent.lo gettimeofdayr.lo isattyr.lo linkr.lo lseekr.lo \
-	mkdirr.lo openr.lo readr.lo renamer.lo signalr.lo signgam.lo \
-	sbrkr.lo statr.lo timesr.lo unlinkr.lo writer.lo
+	gettimeofdayr.lo isattyr.lo linkr.lo lseekr.lo mkdirr.lo \
+	openr.lo readr.lo renamer.lo signalr.lo signgam.lo sbrkr.lo \
+	statr.lo timesr.lo unlinkr.lo writer.lo
 @HAVE_STDIO64_DIR_TRUE@am__objects_7 = fstat64r.lo lseek64r.lo \
 @HAVE_STDIO64_DIR_TRUE@	stat64r.lo open64r.lo
 am__objects_8 = $(am__objects_7)
@@ -309,7 +308,6 @@ GENERAL_SOURCES = \
 	impure.c \
 	fcntlr.c \
 	fstatr.c \
-	getreent.c \
 	gettimeofdayr.c \
 	isattyr.c \
 	linkr.c \
@@ -465,12 +463,6 @@ lib_a-fstatr.o: fstatr.c
 lib_a-fstatr.obj: fstatr.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-fstatr.obj `if test -f 'fstatr.c'; then $(CYGPATH_W) 'fstatr.c'; else $(CYGPATH_W) '$(srcdir)/fstatr.c'; fi`
 
-lib_a-getreent.o: getreent.c
-	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-getreent.o `test -f 'getreent.c' || echo '$(srcdir)/'`getreent.c
-
-lib_a-getreent.obj: getreent.c
-	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-getreent.obj `if test -f 'getreent.c'; then $(CYGPATH_W) 'getreent.c'; else $(CYGPATH_W) '$(srcdir)/getreent.c'; fi`
-
 lib_a-gettimeofdayr.o: gettimeofdayr.c
 	$(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(lib_a_CFLAGS) $(CFLAGS) -c -o lib_a-gettimeofdayr.o `test -f 'gettimeofdayr.c' || echo '$(srcdir)/'`gettimeofdayr.c
 
diff --git a/newlib/libc/reent/getreent.c b/newlib/libc/reent/getreent.c
deleted file mode 100644
index 60ae6fbb7..000000000
--- a/newlib/libc/reent/getreent.c
+++ /dev/null
@@ -1,14 +0,0 @@
-/* default reentrant pointer when multithread enabled */
-
-#include <_ansi.h>
-#include <reent.h>
-
-#ifdef __getreent
-#undef __getreent
-#endif
-
-struct _reent *
-_DEFUN_VOID(__getreent)
-{
-  return _impure_ptr;
-}
diff --git a/newlib/libc/sys/rtems/crt0.c b/newlib/libc/sys/rtems/crt0.c
index 0e9d426ec..6538f7d72 100644
--- a/newlib/libc/sys/rtems/crt0.c
+++ b/newlib/libc/sys/rtems/crt0.c
@@ -146,7 +146,7 @@ RTEMS_STUB(pid_t, getppid (), { return -1; })
 RTEMS_STUB(pid_t, _getpid_r (struct _reent *r), { return -1; })
 RTEMS_STUB(int, _gettimeofday_r(struct _reent *r, struct timeval *tp, void *tzp), { return 0; })
 RTEMS_STUB(int, _isatty_r (struct _reent *r, int fd), { return isatty( fd ); })
-RTEMS_STUB(int, _kill_r (struct _reent *r, int pid, int sig ), { return -1; })
+RTEMS_STUB(struct _reent *, __getreent (void), { return 0; })
 #if !defined(REENTRANT_SYSCALLS_PROVIDED)
 /* cf. newlib/libc/reent/linkr.c */
 RTEMS_STUB(int, _link_r (struct _reent *r, const char *oldpath, const char *newpath), { return -1; })
-- 
2.12.3

Re: [PATCH] Delete default __getreent() implementation

[Sebastian Huber]

On 07/11/17 12:53, Sebastian Huber wrote:
> diff --git a/newlib/libc/sys/rtems/crt0.c b/newlib/libc/sys/rtems/crt0.c
> index 0e9d426ec..6538f7d72 100644
> --- a/newlib/libc/sys/rtems/crt0.c
> +++ b/newlib/libc/sys/rtems/crt0.c
> @@ -146,7 +146,7 @@ RTEMS_STUB(pid_t, getppid (), { return -1; })
>   RTEMS_STUB(pid_t, _getpid_r (struct _reent *r), { return -1; })
>   RTEMS_STUB(int, _gettimeofday_r(struct _reent *r, struct timeval *tp, void *tzp), { return 0; })
>   RTEMS_STUB(int, _isatty_r (struct _reent *r, int fd), { return isatty( fd ); })
> -RTEMS_STUB(int, _kill_r (struct _reent *r, int pid, int sig ), { return -1; })
> +RTEMS_STUB(struct _reent *, __getreent (void), { return 0; })
>   #if !defined(REENTRANT_SYSCALLS_PROVIDED)
>   /* cf. newlib/libc/reent/linkr.c */
>   RTEMS_STUB(int, _link_r (struct _reent *r, const char *oldpath, const char *newpath), { return -1; })

The _kill_r() removal is a mistake.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [PATCH] Delete default __getreent() implementation

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Nov  7 12:53, Sebastian Huber wrote:
> Systems defining  __DYNAMIC_REENT__ have to provide a proper
> implementation of __getreent().  Do not include a default implementation
> to avoid a potential link time issue (real vs. default implementation of
> __getreent()).

Ok, please push (without the kill_r removal).


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Delete default __getreent() implementation

[Craig Howland]

On 11/07/2017 10:03 AM, Corinna Vinschen wrote:
> On Nov  7 12:53, Sebastian Huber wrote:
>> Systems defining  __DYNAMIC_REENT__ have to provide a proper
>> implementation of __getreent().  Do not include a default implementation
>> to avoid a potential link time issue (real vs. default implementation of
>> __getreent()).
> Ok, please push (without the kill_r removal).
>
>
> Thanks,
> Corinna
>
Hold on a minute!  This patch is making assumptions that are not necessarily 
true, and will break backwards compatibility.  Quoting from reent.h:

  "Dynamic reentrancy is specified by the __DYNAMIC_REENT__ flag. This
    flag denotes setting up a macro to replace _REENT with a function call
    to __getreent().  This function needs to be implemented by the platform
    and it is meant to return the reentrancy structure for the current
    thread."

While this text has an implied expectation that the platform is intended to 
supply the function, this implication is not true because there is a default 
implementation--one that really does work.  And given it is there by default, 
people have likely just used it.  In fact, I have an implementation from 10 
years ago that does use the default implementation.  (The task switch is very 
simple, just re-assigning the value of impure_ptr.)  While I do not presently 
have plans to change from the old Newlib used in that project, it is an obvious 
example of breakage if I were to need to update that application to a Newlib 
version in which this change had been made.  It is easy to envision that others 
have done the same thing.

I suggest that at a minimum the contents of getreent.c should be kept.  This 
keeps a starting point for people who are needing to supply such a function.  
(Leaving it out of the library stops the link problem which is the stated goal 
of the patch.)  But I think it needs to go further, and this patch should not be 
done in this form because of the cited backwards-compatibility reasons.  If 
something were to be done it should be a configure flag to leave it out of the 
library.

Craig

Re: [PATCH] Delete default __getreent() implementation

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

On Nov  7 11:12, Craig Howland wrote:
> On 11/07/2017 10:03 AM, Corinna Vinschen wrote:
> > On Nov  7 12:53, Sebastian Huber wrote:
> > > Systems defining  __DYNAMIC_REENT__ have to provide a proper
> > > implementation of __getreent().  Do not include a default implementation
> > > to avoid a potential link time issue (real vs. default implementation of
> > > __getreent()).
> > Ok, please push (without the kill_r removal).
> > 
> > 
> > Thanks,
> > Corinna
> > 
> Hold on a minute!  This patch is making assumptions that are not necessarily
> true, and will break backwards compatibility.  Quoting from reent.h:
> 
>  "Dynamic reentrancy is specified by the __DYNAMIC_REENT__ flag. This
>    flag denotes setting up a macro to replace _REENT with a function call
>    to __getreent().  This function needs to be implemented by the platform
>    and it is meant to return the reentrancy structure for the current
>    thread."
> 
> While this text has an implied expectation that the platform is intended to
> supply the function, this implication is not true because there is a default
> implementation--one that really does work.  And given it is there by
> default, people have likely just used it.  In fact, I have an implementation
> from 10 years ago that does use the default implementation.  (The task
> switch is very simple, just re-assigning the value of impure_ptr.)  While I
> do not presently have plans to change from the old Newlib used in that
> project, it is an obvious example of breakage if I were to need to update
> that application to a Newlib version in which this change had been made.  It
> is easy to envision that others have done the same thing.
> 
> I suggest that at a minimum the contents of getreent.c should be kept.  This
> keeps a starting point for people who are needing to supply such a
> function.  (Leaving it out of the library stops the link problem which is
> the stated goal of the patch.)  But I think it needs to go further, and this
> patch should not be done in this form because of the cited
> backwards-compatibility reasons.  If something were to be done it should be
> a configure flag to leave it out of the library.
> 
> Craig

Good point.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Delete default __getreent() implementation

[Sebastian Huber]

----- Am 7. Nov 2017 um 17:12 schrieb Craig Howland howland@LGSInnovations.com:

> On 11/07/2017 10:03 AM, Corinna Vinschen wrote:
>> On Nov  7 12:53, Sebastian Huber wrote:
[...] 
> I suggest that at a minimum the contents of getreent.c should be kept.  This
> keeps a starting point for people who are needing to supply such a function.

The default implementation is a bad example.  It makes no sense to use __DYNAMIC_REENT__ with this implementation, e.g. you get the same thing without a function call overhead:

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

> (Leaving it out of the library stops the link problem which is the stated goal
> of the patch.)  But I think it needs to go further, and this patch should not be
> done in this form because of the cited backwards-compatibility reasons.  If
> something were to be done it should be a configure flag to leave it out of the
> library.

Then we are back to Joel's patch:

https://sourceware.org/ml/newlib/2017/msg01026.html

I don't know why Joel added this _dummy_getreent, but something similar is used elsewhere:

libc/reent/linkr.c:int _dummy_link_syscalls = 1;
libc/reent/fstat64r.c:int _dummy_fstat_syscalls = 1;
libc/reent/timesr.c:int _dummy_times_syscalls = 1;
libc/reent/sbrkr.c:int _dummy_sbrk_syscalls = 1;
libc/reent/stat64r.c:int _dummy_stat64_syscalls = 1;
libc/reent/gettimeofdayr.c:int _dummy_gettimeofday_syscalls = 1;
libc/reent/signalr.c:int _dummy_link_syscalls = 1;
libc/reent/execr.c:int _dummy_exec_syscalls = 1;
libc/reent/statr.c:int _dummy_stat_syscalls = 1;
libc/reent/fstatr.c:int _dummy_fstat_syscalls = 1;
libc/reent/isattyr.c:int _dummy_isatty_syscalls = 1;
libc/signal/signal.c:int _dummy_simulated_signal;
libc/signal/raise.c:int _dummy_raise;
libc/sys/netware/crt0.c:int _dummy_crt0 = 1;
libc/stdlib/abort.c:int _dummy_abort = 1;
libc/stdlib/mstats.c:int _dummy_mstats = 1;
libc/stdlib/malloc.c:int _dummy_malloc = 1;
libc/stdlib/mallocr.c:int _dummy_mallocr = 1;
libc/stdlib/realloc.c:int _dummy_calloc = 1;
libc/stdlib/calloc.c:int _dummy_calloc = 1;
libc/time/clock.c:int _dummy_clock = 1;

Re: [PATCH] Delete default __getreent() implementation

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Nov  7 20:18, Sebastian Huber wrote:
> 
> 
> ----- Am 7. Nov 2017 um 17:12 schrieb Craig Howland howland@LGSInnovations.com:
> 
> > On 11/07/2017 10:03 AM, Corinna Vinschen wrote:
> >> On Nov  7 12:53, Sebastian Huber wrote:
> [...] 
> > I suggest that at a minimum the contents of getreent.c should be kept.  This
> > keeps a starting point for people who are needing to supply such a function.
> 
> The default implementation is a bad example.  It makes no sense to use __DYNAMIC_REENT__ with this implementation, e.g. you get the same thing without a function call overhead:
> 
> #if defined(__DYNAMIC_REENT__) && !defined(__SINGLE_THREAD__)
> #ifndef __getreent
>   struct _reent * _EXFUN(__getreent, (void));
> #endif
> # define _REENT (__getreent())
> #else /* __SINGLE_THREAD__ || !__DYNAMIC_REENT__ */
> # define _REENT _impure_ptr
> #endif /* __SINGLE_THREAD__ || !__DYNAMIC_REENT__ *
> 
> > (Leaving it out of the library stops the link problem which is the stated goal
> > of the patch.)  But I think it needs to go further, and this patch should not be
> > done in this form because of the cited backwards-compatibility reasons.  If
> > something were to be done it should be a configure flag to leave it out of the
> > library.
> 
> Then we are back to Joel's patch:
> 
> https://sourceware.org/ml/newlib/2017/msg01026.html
> 
> I don't know why Joel added this _dummy_getreent, but something similar is used elsewhere:

The "I don't know" is the key here.  And as long as Joel doesn't
answer this question, this is on hold.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]