[RFC,v8,06/16] Provide backward compatibility for strftime family (bug 10871).

Message ID 1092431994.152319.1498644244955@poczta.nazwa.pl
State Dropped
Headers

Commit Message

Rafal Luzynski June 28, 2017, 10:04 a.m. UTC
  As %OB format specifier has been added to strftime/wcsftime
family of functions backward compatibility implementation must be
provided for older binaries which assume that %B returns
a month name in the nominative case.

[BZ #10871]
* include/time.h: Declare __strftime_l_common.
* include/wchar.h: Declare __wcsftime_l_common.
* time/Versions (libc: GLIBC_2.26):
  New (__)strftime(_l) and (__)wcsftime(_l) added.
* time/strftime.c: Provide backward compatible version.
* time/strftime_l.c: Likewise.
* time/wcsftime.c: Likewise.
* time/wcsftime_l.c: Likewise.
---
 include/time.h    |  9 +++++++++
 include/wchar.h   |  9 +++++++++
 time/Versions     |  4 ++++
 time/strftime.c   | 20 +++++++++++++++++---
 time/strftime_l.c | 56 +++++++++++++++++++++++++++++++++++++++++++++----------
 time/wcsftime.c   | 22 ++++++++++++++++++----
 time/wcsftime_l.c | 26 +++++++++++++++++++++++++-
 7 files changed, 128 insertions(+), 18 deletions(-)
  

Comments

Florian Weimer June 28, 2017, 10:10 a.m. UTC | #1
On 06/28/2017 12:04 PM, Rafal Luzynski wrote:
> As %OB format specifier has been added to strftime/wcsftime
> family of functions backward compatibility implementation must be
> provided for older binaries which assume that %B returns
> a month name in the nominative case.

I think this is a misuse of a compatibility symbols.

Either the reinterpretation of %B is okay, and then it should apply to
old binaries as well, or it is not, and then we should not do it and use
%OB for the new variant (avoiding the backwards compatibility issue
altogether).

I still think that the reinterpretation %B is quite wrong, needlessly
breaking backwards compatibility for most languages which benefit from
the change, but I won't stay in the way of consensus on this matter.

However, the compatibility symbol is wrong, and I'm sustaining my objection.

Thanks,
Florian
  
Rafal Luzynski June 28, 2017, 10:22 a.m. UTC | #2
28.06.2017 12:10 Florian Weimer <fweimer@redhat.com> wrote:
>
>
> On 06/28/2017 12:04 PM, Rafal Luzynski wrote:
> > As %OB format specifier has been added to strftime/wcsftime
> > family of functions backward compatibility implementation must be
> > provided for older binaries which assume that %B returns
> > a month name in the nominative case.
>
> I think this is a misuse of a compatibility symbols.
>
> Either the reinterpretation of %B is okay, and then it should apply to
> old binaries as well, or it is not, and then we should not do it and use
> %OB for the new variant (avoiding the backwards compatibility issue
> altogether).

I agree with that, especially I agree that the reinterpretation is okay
and should apply to old binaries as well.  I already suggested that the
backward compatibility part should be skipped.  I posted it only for
completeness, to avoid questions "where are the missing patches and please
post them" and because some people may have a different opinion.

> I still think that the reinterpretation %B is quite wrong, needlessly
> breaking backwards compatibility for most languages which benefit from
> the change, but I won't stay in the way of consensus on this matter.

Actually, most languages will not see any change at all.  If you mean most
languages which benefit from the change I think that most of them will
also benefit from the reinterpretation in most of the applications.  However,
I admit there will be a minority of applications and a minority (out of
the minority!) of languages where this will mean breaking the compatibility.
They will have to be fixed but since these are minorities the total sum
of changes will be smaller than the alternative solutions.

Also I keep in mind that this reinterpretation is the same as what BSD
family did about 20 years ago and what POSIX accepted as their future
change.

> However, the compatibility symbol is wrong, and I'm sustaining my objection.

Again, I agree here.

Regards,

Rafal
  
Florian Weimer July 27, 2017, 4:15 p.m. UTC | #3
On 06/28/2017 12:22 PM, Rafal Luzynski wrote:
> Also I keep in mind that this reinterpretation is the same as what BSD
> family did about 20 years ago and what POSIX accepted as their future
> change.

I looked at this last year (or earlier this year), and FreeBSD did not
make this change wholesale.  Their system wass in an inconsistent
transitional state.  So I don't FreeBSD is a good argument against or in
favor this change.

Florian
  
Rafal Luzynski July 29, 2017, 3:56 p.m. UTC | #4
27.07.2017 18:15 Florian Weimer <fweimer@redhat.com> wrote:
>
>
> On 06/28/2017 12:22 PM, Rafal Luzynski wrote:
> > Also I keep in mind that this reinterpretation is the same as what BSD
> > family did about 20 years ago and what POSIX accepted as their future
> > change.
>
> I looked at this last year (or earlier this year), and FreeBSD did not
> make this change wholesale. Their system wass in an inconsistent
> transitional state. So I don't FreeBSD is a good argument against or in
> favor this change.

I also checked FreeBSD earlier this year and indeed I noticed some
problems but I'm not sure if they were the same problems which you
noticed.  I think it's you who provided an output of cal command
line utility with Catalan locales.  I investigated this problem and
it is a bug in Catalan locale database which has been introduced
only recently.

I'm not sure what you mean by "inconsistent transitional state".
Do you mean that not all applications which should have adopted this
new %OB format have adopted it yet?  I usually hesitate from saying
this because it requires a proper survey but I'm pretty sure that
if we decide the other way (leave %B for nominative/standalone case
and introduce %OB to display genitive/full-date-context case)
the situation would be much worse because almost all applications
which display dates would have to be transitioned.

Since we are in the hard freeze period and 2.26 release is just
around the corner I think this will not be included in 2.26.
I wonder what else can I/we do to deliver my solution to some limited
but representative group of testers to see what impact to the real
applications would it bring.  I have my copr repo but I'm not aware of
any people actually using it.  Could it be included as a downstream
patch in Fedora Rawhide but not yet any GA release?  Sorry for being
Fedora-centric, seeing @redhat.com in your address I hope that's not
a big fault.

Regards,

Rafal
  

Patch

diff --git a/include/time.h b/include/time.h
index 9956b82..567c443 100644
--- a/include/time.h
+++ b/include/time.h
@@ -8,6 +8,15 @@  extern __typeof (strftime_l) __strftime_l;
 libc_hidden_proto (__strftime_l)
 extern __typeof (strptime_l) __strptime_l;
 
+/* Backward compatibility function: feature_OB argument specifies
+   whether or not %OB format specifier should be implemented.  */
+extern size_t __strftime_l_common (char *__restrict __s, size_t __maxsize,
+				   const char *__restrict __format,
+				   const struct tm *__restrict __tp,
+				   const int feature_OB,
+				   locale_t __loc) __THROW;
+libc_hidden_proto (__strftime_l_common)
+
 libc_hidden_proto (time)
 libc_hidden_proto (asctime)
 libc_hidden_proto (mktime)
diff --git a/include/wchar.h b/include/wchar.h
index 7bf042c..39cce2c 100644
--- a/include/wchar.h
+++ b/include/wchar.h
@@ -25,6 +25,15 @@  libc_hidden_proto (__wcstof_l)
 libc_hidden_proto (__wcstold_l)
 libc_hidden_proto (__wcsftime_l)
 
+/* Backward compatibility function: feature_OB argument specifies
+   whether or not %OB format specifier should be implemented.  */
+extern size_t __wcsftime_l_common (wchar_t *__restrict __s, size_t __maxsize,
+				   const wchar_t *__restrict __format,
+				   const struct tm *__restrict __tp,
+				   const int feature_OB,
+				   locale_t __loc) __THROW;
+libc_hidden_proto (__wcsftime_l_common)
+
 
 extern double __wcstod_internal (const wchar_t *__restrict __nptr,
 				 wchar_t **__restrict __endptr, int __group)
diff --git a/time/Versions b/time/Versions
index fd83818..da38335 100644
--- a/time/Versions
+++ b/time/Versions
@@ -65,4 +65,8 @@  libc {
   GLIBC_2.16 {
     timespec_get;
   }
+  GLIBC_2.26 {
+    __strftime_l; __wcsftime_l;
+    strftime; strftime_l; wcsftime; wcsftime_l;
+  }
 }
diff --git a/time/strftime.c b/time/strftime.c
index eeab20e..709046e 100644
--- a/time/strftime.c
+++ b/time/strftime.c
@@ -17,11 +17,25 @@ 
 
 #include <time.h>
 #include <locale/localeinfo.h>
+#include <shlib-compat.h>
 
 
 size_t
-strftime (char *s, size_t maxsize, const char *format, const struct tm *tp)
+__strftime (char *s, size_t maxsize, const char *format, const struct tm *tp)
 {
-  return __strftime_l (s, maxsize, format, tp, _NL_CURRENT_LOCALE);
+  return __strftime_l_common (s, maxsize, format, tp, 1, _NL_CURRENT_LOCALE);
 }
-libc_hidden_def (strftime)
+versioned_symbol (libc, __strftime, strftime, GLIBC_2_26);
+libc_hidden_ver (__strftime, strftime)
+
+
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_26)
+size_t
+attribute_compat_text_section
+__strftime_compat (char *s, size_t maxsize, const char *format,
+		   const struct tm *tp)
+{
+  return __strftime_l_common (s, maxsize, format, tp, 0, _NL_CURRENT_LOCALE);
+}
+compat_symbol (libc, __strftime_compat, strftime, GLIBC_2_0);
+#endif
diff --git a/time/strftime_l.c b/time/strftime_l.c
index 1c4bed8..1559a82 100644
--- a/time/strftime_l.c
+++ b/time/strftime_l.c
@@ -56,6 +56,8 @@ 
 extern char *tzname[];
 #endif
 
+#include <shlib-compat.h>
+
 /* Do multibyte processing if multibytes are supported, unless
    multibyte sequences are safe in formats.  Multibyte sequences are
    safe if they cannot contain byte sequences that look like format
@@ -279,15 +281,19 @@  static const CHAR_T zeroes[16] = /* "0000000000000000" */
    function gets as an additional argument the locale which has to be
    used.  To access the values we have to redefine the _NL_CURRENT
    macro.  */
-# define strftime		__strftime_l
-# define wcsftime		__wcsftime_l
+# define strftime		__strftime_l_common
+# define wcsftime		__wcsftime_l_common
 # undef _NL_CURRENT
 # define _NL_CURRENT(category, item) \
   (current->values[_NL_ITEM_INDEX (item)].string)
+# define FEATURE_OB_PARAM , int feature_OB
+# define FEATURE_OB_ARG , feature_OB
 # define LOCALE_PARAM , locale_t loc
 # define LOCALE_ARG , loc
 # define HELPER_LOCALE_ARG  , current
 #else
+# define FEATURE_OB_PARAM
+# define FEATURE_OB_ARG
 # define LOCALE_PARAM
 # define LOCALE_ARG
 # ifdef _LIBC
@@ -435,6 +441,7 @@  static CHAR_T const month_name[][10] =
 static size_t __strftime_internal (CHAR_T *, size_t, const CHAR_T *,
 				   const struct tm *, bool *
 				   ut_argument_spec
+				   FEATURE_OB_PARAM
 				   LOCALE_PARAM) __THROW;
 
 /* Write information from TP into S according to the format
@@ -446,7 +453,7 @@  static size_t __strftime_internal (CHAR_T *, size_t, const
CHAR_T *,
 
 size_t
 my_strftime (CHAR_T *s, size_t maxsize, const CHAR_T *format,
-	     const struct tm *tp ut_argument_spec LOCALE_PARAM)
+	     const struct tm *tp ut_argument_spec FEATURE_OB_PARAM LOCALE_PARAM)
 {
 #if !defined _LIBC && HAVE_TZNAME && HAVE_TZSET
   /* Solaris 2.5 tzset sometimes modifies the storage returned by localtime.
@@ -457,7 +464,7 @@  my_strftime (CHAR_T *s, size_t maxsize, const CHAR_T
*format,
 #endif
   bool tzset_called = false;
   return __strftime_internal (s, maxsize, format, tp, &tzset_called
-			      ut_argument LOCALE_ARG);
+			      ut_argument FEATURE_OB_ARG LOCALE_ARG);
 }
 #ifdef _LIBC
 libc_hidden_def (my_strftime)
@@ -466,10 +473,12 @@  libc_hidden_def (my_strftime)
 static size_t
 __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format,
 		     const struct tm *tp, bool *tzset_called
-		     ut_argument_spec LOCALE_PARAM)
+		     ut_argument_spec FEATURE_OB_PARAM LOCALE_PARAM)
 {
 #if defined _LIBC && defined USE_IN_EXTENDED_LOCALE_MODEL
   struct __locale_data *const current = loc->__locales[LC_TIME];
+#else
+# define feature_OB 1
 #endif
 
   int hour12 = tp->tm_hour;
@@ -791,13 +800,15 @@  __strftime_internal (CHAR_T *s, size_t maxsize, const
CHAR_T *format,
 	case L_('B'):
 	  if (modifier == L_('E'))
 	    goto bad_format;
+	  if (!feature_OB && modifier == L_('O'))
+	    goto bad_format;
 	  if (change_case)
 	    {
 	      to_uppcase = 1;
 	      to_lowcase = 0;
 	    }
 #if defined _NL_CURRENT || !HAVE_STRFTIME
-	  if (modifier == L_('O'))
+	  if (!feature_OB || modifier == L_('O'))
 	    cpy (STRLEN (f_altmonth), f_altmonth);
 	  else
 	    cpy (STRLEN (f_month), f_month);
@@ -829,10 +840,10 @@  __strftime_internal (CHAR_T *s, size_t maxsize, const
CHAR_T *format,
 	    CHAR_T *old_start = p;
 	    size_t len = __strftime_internal (NULL, (size_t) -1, subfmt,
 					      tp, tzset_called ut_argument
-					      LOCALE_ARG);
+					      FEATURE_OB_ARG LOCALE_ARG);
 	    add (len, __strftime_internal (p, maxsize - i, subfmt,
 					   tp, tzset_called ut_argument
-					   LOCALE_ARG));
+					   FEATURE_OB_ARG LOCALE_ARG));
 
 	    if (to_uppcase)
 	      while (old_start < p)
@@ -1433,10 +1444,35 @@  size_t
 emacs_strftime (char *s, size_t maxsize, const char *format,
 		const struct tm *tp)
 {
-  return my_strftime (s, maxsize, format, tp, 0);
+  return my_strftime (s, maxsize, format, tp, 1, 0);
 }
 #endif
 
 #if defined _LIBC && !defined COMPILE_WIDE
-weak_alias (__strftime_l, strftime_l)
+size_t
+__strftime_l_internal (char *s, size_t maxsize, const char *format,
+		       const struct tm *tp, locale_t loc)
+{
+  return my_strftime (s, maxsize, format, tp, 1, loc);
+}
+strong_alias (__strftime_l_internal, __strftime_l_internal_alias)
+versioned_symbol (libc, __strftime_l_internal_alias,
+		  __strftime_l, GLIBC_2_26);
+libc_hidden_ver (__strftime_l_internal_alias, __strftime_l)
+versioned_symbol (libc, __strftime_l_internal, strftime_l, GLIBC_2_26);
+libc_hidden_ver (__strftime_l_internal, strftime_l)
+
+# if SHLIB_COMPAT (libc, GLIBC_2_3, GLIBC_2_26)
+size_t
+attribute_compat_text_section
+__strftime_l_compat (char *s, size_t maxsize, const char *format,
+		     const struct tm *tp, locale_t loc)
+{
+  return my_strftime (s, maxsize, format, tp, 0, loc);
+}
+strong_alias (__strftime_l_compat, __strftime_l_compat_alias)
+compat_symbol (libc, __strftime_l_compat_alias, __strftime_l, GLIBC_2_3);
+compat_symbol (libc, __strftime_l_compat, strftime_l, GLIBC_2_3);
+# endif
+
 #endif
diff --git a/time/wcsftime.c b/time/wcsftime.c
index cbc4161..cf3371b 100644
--- a/time/wcsftime.c
+++ b/time/wcsftime.c
@@ -17,12 +17,26 @@ 
 
 #include <wchar.h>
 #include <locale/localeinfo.h>
+#include <shlib-compat.h>
 
 
 size_t
-wcsftime (wchar_t *s, size_t maxsize, const wchar_t *format,
-	  const struct tm *tp)
+__wcsftime (wchar_t *s, size_t maxsize, const wchar_t *format,
+	    const struct tm *tp)
 {
-  return __wcsftime_l (s, maxsize, format, tp, _NL_CURRENT_LOCALE);
+  return __wcsftime_l_common (s, maxsize, format, tp, 1, _NL_CURRENT_LOCALE);
 }
-libc_hidden_def (wcsftime)
+versioned_symbol (libc, __wcsftime, wcsftime, GLIBC_2_26);
+libc_hidden_ver (__wcsftime, wcsftime)
+
+
+#if SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_26)
+size_t
+attribute_compat_text_section
+__wcsftime_compat (wchar_t *s, size_t maxsize, const wchar_t *format,
+		   const struct tm *tp)
+{
+  return __wcsftime_l_common (s, maxsize, format, tp, 0, _NL_CURRENT_LOCALE);
+}
+compat_symbol (libc, __wcsftime_compat, wcsftime, GLIBC_2_2);
+#endif
diff --git a/time/wcsftime_l.c b/time/wcsftime_l.c
index 3210106..a748ccf 100644
--- a/time/wcsftime_l.c
+++ b/time/wcsftime_l.c
@@ -22,4 +22,28 @@ 
 #define COMPILE_WIDE	1
 #include "strftime_l.c"
 
-weak_alias (__wcsftime_l, wcsftime_l)
+size_t
+__wcsftime_l_internal (wchar_t *s, size_t maxsize, const wchar_t *format,
+		       const struct tm *tp, locale_t loc)
+{
+  return my_strftime (s, maxsize, format, tp, 1, loc);
+}
+strong_alias (__wcsftime_l_internal, __wcsftime_l_internal_alias)
+versioned_symbol (libc, __wcsftime_l_internal_alias,
+		  __wcsftime_l, GLIBC_2_26);
+libc_hidden_ver (__wcsftime_l_internal_alias, __wcsftime_l)
+versioned_symbol (libc, __wcsftime_l_internal, wcsftime_l, GLIBC_2_26);
+libc_hidden_ver (__wcsftime_l_internal, wcsftime_l)
+
+#if SHLIB_COMPAT (libc, GLIBC_2_3, GLIBC_2_26)
+size_t
+attribute_compat_text_section
+__wcsftime_l_compat (wchar_t *s, size_t maxsize, const wchar_t *format,
+		     const struct tm *tp, locale_t loc)
+{
+  return my_strftime (s, maxsize, format, tp, 0, loc);
+}
+strong_alias (__wcsftime_l_compat, __wcsftime_l_compat_alias)
+compat_symbol (libc, __wcsftime_l_compat_alias, __wcsftime_l, GLIBC_2_3);
+compat_symbol (libc, __wcsftime_l_compat, wcsftime_l, GLIBC_2_3);
+#endif