[v2,2/8] Add __vfscanf_internal and __vfwscanf_internal with flags arguments.

Message ID c3cd9217-c14b-7e63-5d0c-b7304fa328e7@linaro.org
State New
Headers show
Series
  • Untitled series #16139
Related show

Commit Message

Adhemerval Zanella Nov. 7, 2018, 6:42 p.m.
On 29/10/2018 09:16, Gabriel F. T. Gomes wrote:
> From: Zack Weinberg <zackw@panix.com>

> 

> Changed since v1:

> 

>   - Added one line comment to new files.

>   - In nldbl-compat.c, added explanation for the need to use

>     '#if SHLIB_COMPAT' within 'if LONG_DOUBLE_COMPAT'.  Also moved the

>     block further down, for stylistic reasons [1].

>   - Added attribute_hidden to the internal (libioP.h) declarations of

>     __vfscanf_internal and __vfwscanf_internal [2].

>   - Added comment explaining the reason for the use of SHLIB_COMPAT on

>     stdio-common/iovfscanf.c and stdio-common/iovfwscanf.c.

>   - Converted the definition of ldbl_compat_symbol from an expansion of

>     compat_symbol to '...', because ldbl_compat_symbol is also not

>     supposed to be used outside of '#if SHLIB_COMPAT' statements.  In my

>     opinion, it's better to make this clear in the definition of

>     ldbl_compat_symbol itself, rather than having to go to the

>     definition of compat_symbol to learn this (if people think that this

>     is not the best option, I can revert this change (In that case, the

>     definition of ldbl_compat_symbol could be moved outside the '#if

>     SHARED' block).  Added a comment with this explanation.

>   - Added signed-off-by statements.

>   - Replaced 2.28 with 2.29 to adjust for the next release.

> 

> Not changed since v1:

> 

>   - Florian suggested that the need for ldbl_compat_symbol is

>     questionable, because we could define LONG_DOUBLE_COMPAT_VERSION to

>     GLIBC_2_0 by default (pretending that the long double transition

>     happened for all other platforms), then use it in compat_symbol

>     calls, which would create the compat symbols for _IO_vfscanf and

>     _IO_vfwscanf with that version.

>     That would work, because all the functions that will use

>     ldbl_compat_symbol after this patch set were actually introduced

>     before GLIBC_2_0.  However, for newer functions, such as swscanf,

>     that wouldn't work, if we ever need to do something similar.

> 

> Additional note for review:

> 

>   - Reviewing the changes from vfscanf.c to vfscanf-internal.c in the

>     original patch would be vey hard, because git doesn't detect the

>     filename change.  To make review a little easier, I did as Zack did

>     and manually edited the diff.  I'll reply to this thread and attach

>     the original patch if someone wants to apply it.

>     (ping me if I forget it)


I would advise to avoid it, it is error-prone and we do have tools to 
handle it.  One option is to use -C and -M git options with a higher s
imilarity index, for instance on this patch using:

git format-patch -M90% -C -1

it generates the expected result:

[...]

> 

> [1] https://sourceware.org/ml/libc-alpha/2018-03/msg00309.html

> 

> [2] As a result, internal calls to __vfscanf_internal, on powerpc, do

>     not use the PLT, the following blocks show the difference:

> 

>     Without __attribute__ (hidden):

>       $ objdump -d --reloc VFSCANF-VISIBLE-glibc/libc.so | grep "<__nldbl___vfscanf>" -A 17

>       0014ea00 <__nldbl___vfscanf>:

>         14ea00:       94 21 ff f0     stwu    r1,-16(r1)

>         14ea04:       38 c0 00 01     li      r6,1

>         14ea08:       7c 08 02 a6     mflr    r0

>         14ea0c:       42 9f 00 05     bcl     20,4*cr7+so,14ea10 <__nldbl___vfscanf+0x10>

>         14ea10:       93 c1 00 08     stw     r30,8(r1)

>         14ea14:       90 01 00 14     stw     r0,20(r1)

>         14ea18:       7f c8 02 a6     mflr    r30

>         14ea1c:       3f de 00 05     addis   r30,r30,5

>         14ea20:       3b de 15 e4     addi    r30,r30,5604

>         14ea24:       4b f0 b8 0d     bl      5a230 <__vfscanf_internal>

>         14ea28:       80 01 00 14     lwz     r0,20(r1)

>         14ea2c:       83 c1 00 08     lwz     r30,8(r1)

>         14ea30:       38 21 00 10     addi    r1,r1,16

>         14ea34:       7c 08 03 a6     mtlr    r0

>         14ea38:       4e 80 00 20     blr

>         14ea3c:       60 00 00 00     nop

> 

>     With __attribute__ (hidden):

>       $ objdump -d --reloc VFSCANF-HIDDEN-glibc/libc.so | grep "<__nldbl___vfscanf>" -A 5

>       0014e8b0 <__nldbl___vfscanf>:

>         14e8b0:       38 c0 00 01     li      r6,1

>         14e8b4:       4b f0 b8 bc     b       5a170 <__vfscanf_internal>

>         14e8b8:       60 00 00 00     nop

>         14e8bc:       60 00 00 00     nop

> 

> -- 8< --

> There are two flags currently defined: SCANF_LDBL_IS_DBL is the mode

> used by __nldbl_ scanf variants, and SCANF_ISOC99_A is the mode used

> by __isoc99_ scanf variants.  In this patch, the new functions honor

> these flag bits if they're set, but they still also look at the

> corresponding bits of environmental state, and callers all pass zero.

> 

> The new functions do *not* have the "errp" argument possessed by

> _IO_vfscanf and _IO_vfwscanf.  All internal callers passed NULL for

> that argument.  External callers could theoretically exist, so I

> preserved wrappers, but they are flagged as compat symbols and they

> don't preserve the three-way distinction among types of errors that

> was formerly exposed.  These functions probably should have been in

> the list of deprecated _IO_ symbols in 2.27 NEWS -- they're not just

> aliases for vfscanf and vfwscanf.

> 

> (It was necessary to introduce ldbl_compat_symbol for _IO_vfscanf.

> Please check that part of the patch very carefully, I am still not

> confident I understand all of the details of ldbl-opt.)

> 

> This patch also introduces helper inlines in libio/strfile.h that

> encapsulate the process of initializing an _IO_strfile object for

> reading.  This allows us to call __vfscanf_internal directly from

> sscanf, and __vfwscanf_internal directly from swscanf, without

> duplicating the initialization code.  (Previously, they called their

> v-counterparts, but that won't work if we want to control *both* C99

> mode and ldbl-is-dbl mode using the flags argument to__vfscanf_internal.)

> It's still a little awkward, especially for wide strfiles, but it's

> much better than what we had.


Look good in general with some nits below.

> 

> Tested for powerpc and powerpc64le.

> 

> 2018-10-16  Zack Weinberg  <zackw@panix.com>

> 	    Gabriel F. T. Gomes  <gabriel@inconstante.eti.br>

> 

> 	* libio/libioP.h (SCANF_LDBL_IS_DBL, SCANF_ISOC99_A): New constants.

> 	(__vfscanf_internal, __vfwscanf_internal): New function prototypes.

> 	* libio/libio.h: Remove libc_hidden_proto for _IO_vfscanf.

> 	* libio/strfile.h: Add multiple inclusion guard.

> 	(_IO_strfile_read, _IO_strfile_readw): New inline functions.

> 

> 	* sysdeps/generic/math_ldbl_opt.h: Include shlib-compat.h, for

> 	consistency with the other version of this file.

> 	(ldbl_compat_symbol): New macro.

> 	* sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h (ldbl_compat_symbol):

> 	New macro.

> 

> 	* stdio-common/vfscanf-internal.c: Rename from vfscanf.c.

> 	Define __vfscanf_internal or __vfwscanf_internal, depending on

> 	COMPILE_WSCANF; don't define any other public symbols.

> 	Remove errval and code to set errp.

> 	Temporarily check __ldbl_is_dbl and _IO_FLAGS2_SCANF_STD as well

> 	as the mode_flags argument.

> 	(encode_error, conv_error, input_error): Don't set errval.

> 	* stdio-common/vfwscanf-internal.c: Rename from vfwscanf.c.

> 	Include vfscanf-internal.c.

> 	* stdio-common/vfscanf.c: New file defining the public entry

> 	point vfscanf, which calls __vfscanf_internal.

> 	* stdio-common/vfwscanf.c: New file defining the public entry

> 	point vfwscanf, which calls __vfwscanf_internal.

> 

> 	* stdio-common/iovfscanf.c: New file.

> 	* stdio-common/iovfwscanf.c: Likewise.

> 

> 	* stdio-common/Makefile (routines): Add vfscanf-internal,

> 	vfwscanf-internal, iovfscanf, iovfwscanf.

> 	* stdio-common/Versions: Mention GLIBC_2.29, so that

> 	it can be used in SHLIB_COMPAT expressions.

> 	* sysdeps/ieee754/ldbl-opt/nldbl-compat.c (__nldbl__IO_vfscanf):

> 	Wrap definition and compat_symbol line in #if SHLIB_COMPAT.

> 	Call __vfscanf_internal, instead of _IO_vfscanf.

> 	(__nldbl___vfscanf): Call __vfscanf_internal, instead of

> 	_IO_vfscanf.

> 	(__nldbl_vfwscanf): Call __vfwscanf_internal, instead of

> 	_IO_vfwscanf.

> 

> 	* libio/iovsscanf.c: Clean up includes, when possible.  Use

> 	_IO_strfile_read or _IO_strfile_readw, when needed.  Call

> 	__vfscanf_internal or __vfwscanf_internal directly.

> 	* libio/iovswscanf.c: Likewise.

> 	* libio/swscanf.c: Likewise.

> 	* libio/vscanf.c: Likewise.

> 	* libio/vwscanf.c: Likewise.

> 	* libio/wscanf.c: Likewise.

> 	* stdio-common/isoc99_fscanf.c: Likewise.

> 	* stdio-common/isoc99_scanf.c: Likewise.

> 	* stdio-common/isoc99_sscanf.c: Likewise.

> 	* stdio-common/isoc99_vfscanf.c: Likewise.

> 	* stdio-common/isoc99_vscanf.c: Likewise.

> 	* stdio-common/isoc99_vsscanf.c: Likewise.

> 	* stdio-common/scanf.c: Likewise.

> 	* stdio-common/sscanf.c: Likewise.

> 	* wcsmbs/isoc99_fwscanf.c: Likewise.

> 	* wcsmbs/isoc99_swscanf.c: Likewise.

> 	* wcsmbs/isoc99_vfwscanf.c: Likewise.

> 	* wcsmbs/isoc99_vswscanf.c: Likewise.

> 	* wcsmbs/isoc99_vwscanf.c: Likewise.

> 	* wcsmbs/isoc99_wscanf.c: Likewise.

> 

> Signed-off-by: Zack Weinberg <zackw@panix.com>

> Signed-off-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br>


As Florian has said, we don't use DCO, but copyright assignments.


> ---

>  libio/iovsscanf.c                        |   12 +-

>  libio/iovswscanf.c                       |   14 +-

>  libio/libio.h                            |    1 -

>  libio/libioP.h                           |   11 +

>  libio/strfile.h                          |   33 +-

>  libio/swscanf.c                          |   10 +-

>  libio/vscanf.c                           |    2 +-

>  libio/vwscanf.c                          |    2 +-

>  libio/wscanf.c                           |    2 +-

>  stdio-common/Makefile                    |    3 +-

>  stdio-common/Versions                    |    3 +

>  stdio-common/iovfscanf.c                 |   38 +

>  stdio-common/iovfwscanf.c                |   38 +

>  stdio-common/isoc99_fscanf.c             |    2 +-

>  stdio-common/isoc99_scanf.c              |    2 +-

>  stdio-common/isoc99_sscanf.c             |    9 +-

>  stdio-common/isoc99_vfscanf.c            |    2 +-

>  stdio-common/isoc99_vscanf.c             |    2 +-

>  stdio-common/isoc99_vsscanf.c            |   17 +-

>  stdio-common/scanf.c                     |    2 +-

>  stdio-common/sscanf.c                    |   12 +-

>  stdio-common/vfscanf-internal.c          | 3050 ++++++++++++++++++++++++++++++

>  stdio-common/vfscanf.c                   | 3042 +----------------------------

>  stdio-common/vfwscanf-internal.c         |    2 +

>  stdio-common/vfwscanf.c                  |   28 +-

>  sysdeps/generic/math_ldbl_opt.h          |    4 +

>  sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h |    5 +

>  sysdeps/ieee754/ldbl-opt/nldbl-compat.c  |   17 +-

>  wcsmbs/isoc99_fwscanf.c                  |    2 +-

>  wcsmbs/isoc99_swscanf.c                  |   12 +-

>  wcsmbs/isoc99_vfwscanf.c                 |    2 +-

>  wcsmbs/isoc99_vswscanf.c                 |   16 +-

>  wcsmbs/isoc99_vwscanf.c                  |    2 +-

>  wcsmbs/isoc99_wscanf.c                   |    2 +-

>  34 files changed, 3273 insertions(+), 3128 deletions(-)

>  create mode 100644 stdio-common/iovfscanf.c

>  create mode 100644 stdio-common/iovfwscanf.c

>  create mode 100644 stdio-common/vfscanf-internal.c

>  create mode 100644 stdio-common/vfwscanf-internal.c

> 

> diff --git a/libio/iovsscanf.c b/libio/iovsscanf.c

> index e56ab8bd7d..ee6a99ec6a 100644

> --- a/libio/iovsscanf.c

> +++ b/libio/iovsscanf.c

> @@ -24,22 +24,14 @@

>     This exception applies to code released by its copyright holders

>     in files containing the exception.  */

>  

> -#include "libioP.h"

>  #include "strfile.h"

>  

>  int

>  _IO_vsscanf (const char *string, const char *format, va_list args)

>  {

> -  int ret;

>    _IO_strfile sf;

> -#ifdef _IO_MTSAFE_IO

> -  sf._sbf._f._lock = NULL;

> -#endif

> -  _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);

> -  _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;

> -  _IO_str_init_static_internal (&sf, (char*)string, 0, NULL);

> -  ret = _IO_vfscanf (&sf._sbf._f, format, args, NULL);

> -  return ret;

> +  FILE *f = _IO_strfile_read (&sf, string);

> +  return __vfscanf_internal (f, format, args, 0);

>  }

>  ldbl_weak_alias (_IO_vsscanf, __vsscanf)

>  ldbl_weak_alias (_IO_vsscanf, vsscanf)


Ok.

> diff --git a/libio/iovswscanf.c b/libio/iovswscanf.c

> index 5bd1c88412..cb9cbe15cc 100644

> --- a/libio/iovswscanf.c

> +++ b/libio/iovswscanf.c

> @@ -24,24 +24,16 @@

>     This exception applies to code released by its copyright holders

>     in files containing the exception.  */

>  

> -#include "libioP.h"

> -#include "strfile.h"

>  #include <wchar.h>

> +#include "strfile.h"

>  

>  int

>  __vswscanf (const wchar_t *string, const wchar_t *format, va_list args)

>  {

> -  int ret;

>    _IO_strfile sf;

>    struct _IO_wide_data wd;

> -#ifdef _IO_MTSAFE_IO

> -  sf._sbf._f._lock = NULL;

> -#endif

> -  _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, 0, &wd, &_IO_wstr_jumps);

> -  _IO_fwide (&sf._sbf._f, 1);

> -  _IO_wstr_init_static (&sf._sbf._f, (wchar_t *)string, 0, NULL);

> -  ret = _IO_vfwscanf ((FILE *) &sf._sbf, format, args, NULL);

> -  return ret;

> +  FILE *f = _IO_strfile_readw (&sf, &wd, string);

> +  return __vfwscanf_internal (f, format, args, 0);

>  }

>  libc_hidden_def (__vswscanf)

>  ldbl_hidden_def (__vswscanf, vswscanf)


Ok.

> diff --git a/libio/libio.h b/libio/libio.h

> index 00f9169613..d4eba2df54 100644

> --- a/libio/libio.h

> +++ b/libio/libio.h

> @@ -321,7 +321,6 @@ libc_hidden_proto (_IO_padn)

>  libc_hidden_proto (_IO_putc)

>  libc_hidden_proto (_IO_sgetn)

>  libc_hidden_proto (_IO_vfprintf)

> -libc_hidden_proto (_IO_vfscanf)

>  

>  #ifdef _IO_MTSAFE_IO

>  # undef _IO_peekc

> diff --git a/libio/libioP.h b/libio/libioP.h

> index df2633d858..fa34a628fe 100644

> --- a/libio/libioP.h

> +++ b/libio/libioP.h

> @@ -704,6 +704,17 @@ extern off64_t _IO_seekpos_unlocked (FILE *, off64_t, int)

>  

>  #endif /* _G_HAVE_MMAP */

>  

> +/* Flags for __vfscanf_internal and __vfwscanf_internal.  */

> +#define SCANF_LDBL_IS_DBL 0x0001

> +#define SCANF_ISOC99_A    0x0002


As before, please describe what actually the flag does.

> +

> +extern int __vfscanf_internal (FILE *fp, const char *format, va_list argp,

> +			       unsigned int flags)

> +  attribute_hidden;

> +extern int __vfwscanf_internal (FILE *fp, const wchar_t *format, va_list argp,

> +				unsigned int flags)

> +  attribute_hidden;

> +

>  extern int _IO_vscanf (const char *, va_list) __THROW;

>  

>  #ifdef _IO_MTSAFE_IO


Ok.

> diff --git a/libio/strfile.h b/libio/strfile.h

> index 75caac2af5..62900a7128 100644

> --- a/libio/strfile.h

> +++ b/libio/strfile.h

> @@ -24,7 +24,9 @@

>     This exception applies to code released by its copyright holders

>     in files containing the exception.  */

>  

> -#include <stdio.h>

> +#ifndef STRFILE_H_

> +#define STRFILE_H_

> +

>  #include "libioP.h"

>  

>  typedef void *(*_IO_alloc_type) (size_t);

> @@ -80,3 +82,32 @@ typedef struct

>  } _IO_wstrnfile;

>  

>  extern const struct _IO_jump_t _IO_wstrn_jumps attribute_hidden;

> +

> +/* Initialize an _IO_strfile SF to read from narrow string STRING, and

> +   return the corresponding FILE object.  It is not necessary to fclose

> +   the FILE when it is no longer needed.  */

> +static inline FILE *

> +_IO_strfile_read (_IO_strfile *sf, const char *string)

> +{

> +  sf->_sbf._f._lock = NULL;

> +  _IO_no_init (&sf->_sbf._f, _IO_USER_LOCK, -1, NULL, NULL);

> +  _IO_JUMPS (&sf->_sbf) = &_IO_str_jumps;

> +  _IO_str_init_static_internal (sf, (char*)string, 0, NULL);

> +  return &sf->_sbf._f;

> +}

> +

> +/* Initialize an _IO_strfile SF and _IO_wide_data WD to read from wide

> +   string STRING, and return the corresponding FILE object.  It is not

> +   necessary to fclose the FILE when it is no longer needed.  */

> +static inline FILE *

> +_IO_strfile_readw (_IO_strfile *sf, struct _IO_wide_data *wd,

> +                   const wchar_t *string)

> +{

> +  sf->_sbf._f._lock = NULL;

> +  _IO_no_init (&sf->_sbf._f, _IO_USER_LOCK, 0, wd, &_IO_wstr_jumps);

> +  _IO_fwide (&sf->_sbf._f, 1);

> +  _IO_wstr_init_static (&sf->_sbf._f, (wchar_t *)string, 0, NULL);

> +  return &sf->_sbf._f;

> +}

> +

> +#endif /* strfile.h.  */


Ok.

> diff --git a/libio/swscanf.c b/libio/swscanf.c

> index c8686bcbaf..90f721cc51 100644

> --- a/libio/swscanf.c

> +++ b/libio/swscanf.c

> @@ -15,20 +15,22 @@

>     License along with the GNU C Library; if not, see

>     <http://www.gnu.org/licenses/>.  */

>  

> -#include <libioP.h>

>  #include <stdarg.h>

> -#include <wchar.h>

> +#include "strfile.h"

>  

>  /* Read formatted input from S, according to the format string FORMAT.  */

> -/* VARARGS2 */

> +

>  int

>  __swscanf (const wchar_t *s, const wchar_t *format, ...)

>  {

>    va_list arg;

>    int done;

> +  _IO_strfile sf;

> +  struct _IO_wide_data wd;

> +  FILE *f = _IO_strfile_readw (&sf, &wd, s);

>  

>    va_start (arg, format);

> -  done = __vswscanf (s, format, arg);

> +  done = __vfwscanf_internal (f, format, arg, 0);

>    va_end (arg);

>  

>    return done;


Ok.

> diff --git a/libio/vscanf.c b/libio/vscanf.c

> index 9c27122c27..a3e2dd43f2 100644

> --- a/libio/vscanf.c

> +++ b/libio/vscanf.c

> @@ -32,6 +32,6 @@

>  int

>  _IO_vscanf (const char *format, va_list args)

>  {

> -  return _IO_vfscanf (_IO_stdin, format, args, NULL);

> +  return __vfscanf_internal (_IO_stdin, format, args, 0);

>  }

>  ldbl_weak_alias (_IO_vscanf, vscanf)


Ok.

> diff --git a/libio/vwscanf.c b/libio/vwscanf.c

> index 0d5f558758..7af770c8c3 100644

> --- a/libio/vwscanf.c

> +++ b/libio/vwscanf.c

> @@ -30,6 +30,6 @@

>  int

>  __vwscanf (const wchar_t *format, va_list args)

>  {

> -  return _IO_vfwscanf (_IO_stdin, format, args, NULL);

> +  return __vfwscanf_internal (_IO_stdin, format, args, 0);

>  }

>  ldbl_strong_alias (__vwscanf, vwscanf)


Ok.

> diff --git a/libio/wscanf.c b/libio/wscanf.c

> index c8cdad0acd..fe27ff6fa6 100644

> --- a/libio/wscanf.c

> +++ b/libio/wscanf.c

> @@ -30,7 +30,7 @@ __wscanf (const wchar_t *format, ...)

>    int done;

>  

>    va_start (arg, format);

> -  done = _IO_vfwscanf (stdin, format, arg, NULL);

> +  done = __vfwscanf_internal (stdin, format, arg, 0);

>    va_end (arg);

>  

>    return done;


Ok.

> diff --git a/stdio-common/Makefile b/stdio-common/Makefile

> index a10f12ab3c..f3b3ceddbd 100644

> --- a/stdio-common/Makefile

> +++ b/stdio-common/Makefile

> @@ -39,7 +39,8 @@ routines	:=							      \

>  	flockfile ftrylockfile funlockfile				      \

>  	isoc99_scanf isoc99_vscanf isoc99_fscanf isoc99_vfscanf isoc99_sscanf \

>  	isoc99_vsscanf							      \

> -	psiginfo gentempfd

> +	psiginfo gentempfd						      \

> +	vfscanf-internal vfwscanf-internal iovfscanf iovfwscanf

>  

>  aux	:= errlist siglist printf-parsemb printf-parsewc fxprintf

>  


Ok.

> diff --git a/stdio-common/Versions b/stdio-common/Versions

> index b8217578c8..522f302198 100644

> --- a/stdio-common/Versions

> +++ b/stdio-common/Versions

> @@ -60,6 +60,9 @@ libc {

>    GLIBC_2.28 {

>      renameat2;

>    }

> +  GLIBC_2.29 {

> +    # SHLIB_COMPAT(GLIBC_2_0, GLIBC_2_29) used in iovfscanf.c etc.

> +  }

>    GLIBC_PRIVATE {

>      # global variables

>      _itoa_lower_digits;


Ok.

> diff --git a/stdio-common/iovfscanf.c b/stdio-common/iovfscanf.c

> new file mode 100644

> index 0000000000..77e698f665

> --- /dev/null

> +++ b/stdio-common/iovfscanf.c

> @@ -0,0 +1,38 @@

> +/* Implementation and symbols for _IO_vfscanf.

> +   Copyright (C) 2018 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#include <libioP.h>

> +#include <shlib-compat.h>

> +

> +/* This function is provided for ports older than GLIBC 2.29 because

> +   external callers could theoretically exist.  Newer ports do not need,

> +   since it is not part of the API.  */

> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_29)

> +

> +int

> +attribute_compat_text_section

> +__IO_vfscanf (FILE *fp, const char *format, va_list ap, int *errp)

> +{

> +  int rv = __vfscanf_internal (fp, format, ap, 0);

> +  if (__glibc_unlikely (errp != 0))

> +    *errp = (rv == -1);

> +  return rv;

> +}

> +ldbl_compat_symbol (libc, __IO_vfscanf, _IO_vfscanf, GLIBC_2_0);

> +

> +#endif


Ok.

> diff --git a/stdio-common/iovfwscanf.c b/stdio-common/iovfwscanf.c

> new file mode 100644

> index 0000000000..26a57788cb

> --- /dev/null

> +++ b/stdio-common/iovfwscanf.c

> @@ -0,0 +1,38 @@

> +/* Implementation and symbols for _IO_vfwscanf.

> +   Copyright (C) 1991-2018 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#include <libioP.h>

> +#include <shlib-compat.h>

> +

> +/* This function is provided for ports older than GLIBC 2.29 because

> +   external callers could theoretically exist.  Newer ports do not need,

> +   since it is not part of the API.  */

> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_29)

> +

> +int

> +attribute_compat_text_section

> +__IO_vfwscanf (FILE *fp, const wchar_t *format, va_list ap, int *errp)

> +{

> +  int rv = __vfwscanf_internal (fp, format, ap, 0);

> +  if (__glibc_unlikely (errp != 0))

> +    *errp = (rv == -1);

> +  return rv;

> +}

> +compat_symbol (libc, __IO_vfwscanf, _IO_vfwscanf, GLIBC_2_0);

> +

> +#endif


Ok.

> diff --git a/stdio-common/isoc99_fscanf.c b/stdio-common/isoc99_fscanf.c

> index 9cdf85e679..4210d11f2b 100644

> --- a/stdio-common/isoc99_fscanf.c

> +++ b/stdio-common/isoc99_fscanf.c

> @@ -31,7 +31,7 @@ __isoc99_fscanf (FILE *stream, const char *format, ...)

>    stream->_flags2 |= _IO_FLAGS2_SCANF_STD;

>  

>    va_start (arg, format);

> -  done = _IO_vfscanf (stream, format, arg, NULL);

> +  done = __vfscanf_internal (stream, format, arg, 0);

>    va_end (arg);

>  

>    _IO_release_lock (stream);


Ok.

> diff --git a/stdio-common/isoc99_scanf.c b/stdio-common/isoc99_scanf.c

> index bf7dbe86bb..64c873eed9 100644

> --- a/stdio-common/isoc99_scanf.c

> +++ b/stdio-common/isoc99_scanf.c

> @@ -34,7 +34,7 @@ __isoc99_scanf (const char *format, ...)

>    stdin->_flags2 |= _IO_FLAGS2_SCANF_STD;

>  

>    va_start (arg, format);

> -  done = _IO_vfscanf (stdin, format, arg, NULL);

> +  done = __vfscanf_internal (stdin, format, arg, 0);

>    va_end (arg);

>  

>  #ifdef _IO_MTSAFE_IO


Ok.

> diff --git a/stdio-common/isoc99_sscanf.c b/stdio-common/isoc99_sscanf.c

> index 56a60a2c05..2c89a03fe9 100644

> --- a/stdio-common/isoc99_sscanf.c

> +++ b/stdio-common/isoc99_sscanf.c

> @@ -16,19 +16,20 @@

>     <http://www.gnu.org/licenses/>.  */

>  

>  #include <stdarg.h>

> -#include <stdio.h>

> -#include <libioP.h>

> +#include <libio/strfile.h>

>  

>  /* Read formatted input from S, according to the format string FORMAT.  */

> -/* VARARGS2 */

>  int

>  __isoc99_sscanf (const char *s, const char *format, ...)

>  {

>    va_list arg;

>    int done;

> +  _IO_strfile sf;

> +  FILE *f = _IO_strfile_read (&sf, s);

> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;

>  

>    va_start (arg, format);

> -  done = __isoc99_vsscanf (s, format, arg);

> +  done = __vfscanf_internal (f, format, arg, 0);

>    va_end (arg);

>  

>    return done;


Ok.

> diff --git a/stdio-common/isoc99_vfscanf.c b/stdio-common/isoc99_vfscanf.c

> index b80e05f8db..c96ca831ae 100644

> --- a/stdio-common/isoc99_vfscanf.c

> +++ b/stdio-common/isoc99_vfscanf.c

> @@ -27,7 +27,7 @@ __isoc99_vfscanf (FILE *stream, const char *format, va_list args)

>  

>    _IO_acquire_lock_clear_flags2 (stream);

>    stream->_flags2 |= _IO_FLAGS2_SCANF_STD;

> -  done = _IO_vfscanf (stream, format, args, NULL);

> +  done = __vfscanf_internal (stream, format, args, 0);

>    _IO_release_lock (stream);

>    return done;

>  }


Ok.

> diff --git a/stdio-common/isoc99_vscanf.c b/stdio-common/isoc99_vscanf.c

> index 0b747f85ba..72ae72ddee 100644

> --- a/stdio-common/isoc99_vscanf.c

> +++ b/stdio-common/isoc99_vscanf.c

> @@ -27,7 +27,7 @@ __isoc99_vscanf (const char *format, va_list args)

>  

>    _IO_acquire_lock_clear_flags2 (stdin);

>    stdin->_flags2 |= _IO_FLAGS2_SCANF_STD;

> -  done = _IO_vfscanf (stdin, format, args, NULL);

> +  done = __vfscanf_internal (stdin, format, args, 0);

>    _IO_release_lock (stdin);

>    return done;

>  }


Ok.

> diff --git a/stdio-common/isoc99_vsscanf.c b/stdio-common/isoc99_vsscanf.c

> index ac85ef2d0d..02bc0f50e6 100644

> --- a/stdio-common/isoc99_vsscanf.c

> +++ b/stdio-common/isoc99_vsscanf.c

> @@ -24,23 +24,14 @@

>     This exception applies to code released by its copyright holders

>     in files containing the exception.  */

>  

> -#include <libioP.h>

> -#include <stdio.h>

> -#include "../libio/strfile.h"

> +#include <libio/strfile.h>

>  

>  int

>  __isoc99_vsscanf (const char *string, const char *format, va_list args)

>  {

> -  int ret;

>    _IO_strfile sf;

> -#ifdef _IO_MTSAFE_IO

> -  sf._sbf._f._lock = NULL;

> -#endif

> -  _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);

> -  _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;

> -  _IO_str_init_static_internal (&sf, (char*)string, 0, NULL);

> -  sf._sbf._f._flags2 |= _IO_FLAGS2_SCANF_STD;

> -  ret = _IO_vfscanf (&sf._sbf._f, format, args, NULL);

> -  return ret;

> +  FILE *f = _IO_strfile_read (&sf, string);

> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;

> +  return __vfscanf_internal (f, format, args, 0);

>  }

>  libc_hidden_def (__isoc99_vsscanf)


Ok.

> diff --git a/stdio-common/scanf.c b/stdio-common/scanf.c

> index e61b5f1ad3..de38d70353 100644

> --- a/stdio-common/scanf.c

> +++ b/stdio-common/scanf.c

> @@ -30,7 +30,7 @@ __scanf (const char *format, ...)

>    int done;

>  

>    va_start (arg, format);

> -  done = _IO_vfscanf (stdin, format, arg, NULL);

> +  done = __vfscanf_internal (stdin, format, arg, 0);

>    va_end (arg);

>  

>    return done;


Ok.

> diff --git a/stdio-common/sscanf.c b/stdio-common/sscanf.c

> index 88cd641798..e25e9c27a5 100644

> --- a/stdio-common/sscanf.c

> +++ b/stdio-common/sscanf.c

> @@ -16,26 +16,24 @@

>     <http://www.gnu.org/licenses/>.  */

>  

>  #include <stdarg.h>

> -#include <stdio.h>

> -#include <libioP.h>

> -#define __vsscanf(s, f, a) _IO_vsscanf (s, f, a)

> +#include <libio/strfile.h>

>  

>  /* Read formatted input from S, according to the format string FORMAT.  */

> -/* VARARGS2 */

> +

>  int

>  __sscanf (const char *s, const char *format, ...)

>  {

>    va_list arg;

>    int done;

> +  _IO_strfile sf;

> +  FILE *f = _IO_strfile_read (&sf, s);

>  

>    va_start (arg, format);

> -  done = __vsscanf (s, format, arg);

> +  done = __vfscanf_internal (f, format, arg, 0);

>    va_end (arg);

>  

>    return done;

>  }

>  ldbl_hidden_def (__sscanf, sscanf)

>  ldbl_strong_alias (__sscanf, sscanf)

> -#undef _IO_sscanf

> -/* This is for libg++.  */

>  ldbl_strong_alias (__sscanf, _IO_sscanf)


Ok.

> diff -u stdio-common/vfscanf.c.old stdio-common/vfscanf-internal.c.new

> --- stdio-common/vfscanf.c.old	2018-10-25 17:21:09.232711752 -0300

> +++ stdio-common/vfscanf-internal.c.new	2018-10-25 17:21:24.143739981 -0300

> @@ -1,4 +1,5 @@

> -/* Copyright (C) 1991-2018 Free Software Foundation, Inc.

> +/* Internal functions for the *scanf* implementation.

> +   Copyright (C) 1991-2018 Free Software Foundation, Inc.

>     This file is part of the GNU C Library.

>  

>     The GNU C Library is free software; you can redistribute it and/or

> @@ -132,16 +133,13 @@

>  #include "printf-parse.h" /* Use read_int.  */

>  

>  #define encode_error() do {						      \

> -			  errval = 4;					      \

>  			  __set_errno (EILSEQ);				      \

>  			  goto errout;					      \

>  			} while (0)

>  #define conv_error()	do {						      \

> -			  errval = 2;					      \

>  			  goto errout;					      \

>  			} while (0)

>  #define input_error()	do {						      \

> -			  errval = 1;					      \

>  			  if (done == 0) done = EOF;			      \

>  			  goto errout;					      \

>  			} while (0)

> @@ -267,12 +265,12 @@

>     Return the number of assignments made, or -1 for an input error.  */

>  #ifdef COMPILE_WSCANF

>  int

> -_IO_vfwscanf (FILE *s, const wchar_t *format, va_list argptr,

> -	      int *errp)

> +__vfwscanf_internal (FILE *s, const wchar_t *format, va_list argptr,

> +                     unsigned int mode_flags)

>  #else

>  int

> -_IO_vfscanf_internal (FILE *s, const char *format, va_list argptr,

> -		      int *errp)

> +__vfscanf_internal (FILE *s, const char *format, va_list argptr,

> +                    unsigned int mode_flags)

>  #endif

>  {

>    va_list arg;

> @@ -283,7 +281,6 @@

>    WINT_T c = 0;	/* Last char read.  */

>    int width;		/* Maximum field width.  */

>    int flags;		/* Modifiers for current format element.  */

> -  int errval = 0;

>  #ifndef COMPILE_WSCANF

>    locale_t loc = _NL_CURRENT_LOCALE;

>    struct __locale_data *const curctype = loc->__locales[LC_CTYPE];

> @@ -335,6 +332,14 @@

>    struct char_buffer charbuf;

>    scratch_buffer_init (&charbuf.scratch);

>  

> +#define LDBL_DISTINCT (__glibc_likely ((mode_flags & SCANF_LDBL_IS_DBL) == 0))

> +#define USE_ISOC99_A  (__glibc_likely (mode_flags & SCANF_ISOC99_A))


Do we really gain anything using these defines? I tend to frown on macros
that use internal named variables instead of set them as arguments. Also
this is quite short, I think it is more readable to just use the comparison
directly.

> +  /* Temporarily honor the environmental mode bits.  */

> +  if (__ldbl_is_dbl)

> +    mode_flags |= SCANF_LDBL_IS_DBL;

> +  if (s->_flags2 & _IO_FLAGS2_SCANF_STD)

> +    mode_flags |= SCANF_ISOC99_A;

> +

>  #ifdef __va_copy

>    __va_copy (arg, argptr);

>  #else

> @@ -566,7 +571,7 @@

>  	    }

>  	  /* In __isoc99_*scanf %as, %aS and %a[ extension is not

>  	     supported at all.  */

> -	  if (s->_flags2 & _IO_FLAGS2_SCANF_STD)

> +	  if (USE_ISOC99_A)

>  	    {

>  	      --f;

>  	      break;

> @@ -2423,7 +2428,7 @@

>  	      done = EOF;

>  	      goto errout;

>  	    }

> -	  if ((flags & LONGDBL) && !__ldbl_is_dbl)

> +	  if ((flags & LONGDBL) && LDBL_DISTINCT)

>  	    {

>  	      long double d = __strtold_internal

>  		(char_buffer_start (&charbuf), &tw, flags & GROUP);

> @@ -3018,8 +3023,6 @@

>    UNLOCK_STREAM (s);

>  

>    scratch_buffer_free (&charbuf.scratch);

> -  if (errp != NULL)

> -    *errp |= errval;

>  

>    if (__glibc_unlikely (done == EOF))

>      {

> @@ -3045,23 +3048,3 @@

>      }

>    return done;

>  }

> -

> -#ifdef COMPILE_WSCANF

> -int

> -__vfwscanf (FILE *s, const wchar_t *format, va_list argptr)

> -{

> -  return _IO_vfwscanf (s, format, argptr, NULL);

> -}

> -ldbl_weak_alias (__vfwscanf, vfwscanf)

> -#else

> -int

> -___vfscanf (FILE *s, const char *format, va_list argptr)

> -{

> -  return _IO_vfscanf_internal (s, format, argptr, NULL);

> -}

> -ldbl_strong_alias (_IO_vfscanf_internal, _IO_vfscanf)

> -ldbl_hidden_def (_IO_vfscanf_internal, _IO_vfscanf)

> -ldbl_strong_alias (___vfscanf, __vfscanf)

> -ldbl_hidden_def (___vfscanf, __vfscanf)

> -ldbl_weak_alias (___vfscanf, vfscanf)

> -#endif


Ok.

> diff -u /dev/null stdio-common/vfscanf.c.new

> --- /dev/null	2018-10-15 19:30:16.914999855 -0300

> +++ stdio-common/vfscanf.c.new	2018-10-25 17:21:39.355768779 -0300


This manually change diff is confusing, since most of the original patch
is code removal.

> @@ -0,0 +1,27 @@

> +/* Copyright (C) 1991-2018 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#include <libioP.h>

> +

> +int

> +___vfscanf (FILE *s, const char *format, va_list argptr)

> +{

> +  return __vfscanf_internal (s, format, argptr, 0);

> +}

> +ldbl_strong_alias (___vfscanf, __vfscanf)

> +ldbl_hidden_def (___vfscanf, __vfscanf)

> +ldbl_weak_alias (___vfscanf, vfscanf)


Ok.

> diff --git a/stdio-common/vfwscanf-internal.c b/stdio-common/vfwscanf-internal.c

> new file mode 100644

> index 0000000000..26c89270b7

> --- /dev/null

> +++ b/stdio-common/vfwscanf-internal.c

> @@ -0,0 +1,2 @@

> +#define COMPILE_WSCANF	1

> +#include "vfscanf-internal.c"


Ok.

> diff --git a/stdio-common/vfwscanf.c b/stdio-common/vfwscanf.c

> index 26b1a66608..f1c70ad6b3 100644

> --- a/stdio-common/vfwscanf.c

> +++ b/stdio-common/vfwscanf.c

> @@ -1,2 +1,26 @@

> -#define COMPILE_WSCANF	1

> -#include "vfscanf.c"

> +/* Implementation and symbols for vfwscanf.

> +   Copyright (C) 1991-2018 Free Software Foundation, Inc.


I think you should use just 2018 here.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#include <libioP.h>

> +

> +int

> +__vfwscanf (FILE *s, const wchar_t *format, va_list argptr)

> +{

> +  return __vfwscanf_internal (s, format, argptr, 0);

> +}

> +ldbl_weak_alias (__vfwscanf, vfwscanf)


Ok.

> diff --git a/sysdeps/generic/math_ldbl_opt.h b/sysdeps/generic/math_ldbl_opt.h

> index 8a5d8ba107..92f670dff7 100644

> --- a/sysdeps/generic/math_ldbl_opt.h

> +++ b/sysdeps/generic/math_ldbl_opt.h

> @@ -6,9 +6,13 @@

>     for platforms where compatibility symbols are required for a previous

>     ABI that defined long double functions as aliases for the double code.  */

>  

> +#include <shlib-compat.h>

> +

>  #define LONG_DOUBLE_COMPAT(lib, introduced) 0

>  #define long_double_symbol(lib, local, symbol)

>  #define ldbl_hidden_def(local, name) libc_hidden_def (name)

>  #define ldbl_strong_alias(name, aliasname) strong_alias (name, aliasname)

>  #define ldbl_weak_alias(name, aliasname) weak_alias (name, aliasname)

> +#define ldbl_compat_symbol(lib, local, symbol, version) \

> +  compat_symbol (lib, local, symbol, version)

>  #define __ldbl_is_dbl 0


Ok.

> diff --git a/sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h b/sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h

> index 61ba784f86..4d2f3c7be2 100644

> --- a/sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h

> +++ b/sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h

> @@ -20,10 +20,15 @@

>    long_double_symbol (libc, __GL_##name##_##aliasname, aliasname);

>  # define long_double_symbol_1(lib, local, symbol, version) \

>    versioned_symbol (lib, local, symbol, version)

> +# define ldbl_compat_symbol(lib, local, symbol, version) \

> +  compat_symbol (lib, local, symbol, LONG_DOUBLE_COMPAT_VERSION)

>  #else

>  # define ldbl_hidden_def(local, name) libc_hidden_def (name)

>  # define ldbl_strong_alias(name, aliasname) strong_alias (name, aliasname)

>  # define ldbl_weak_alias(name, aliasname) weak_alias (name, aliasname)

> +/* Same as compat_symbol, ldbl_compat_symbol is not to be used outside

> +   '#if SHLIB_COMPAT' statement and should fail if it is.  */

> +# define ldbl_compat_symbol(lib, local, symbol, version) ...


Why not use an _Static_assert(0, "ldbl_compat_symbol should be used inside SHLIB_COMPAT")?

>  # ifndef __ASSEMBLER__

>  /* Note that weak_alias cannot be used - it is defined to nothing

>     in most of the C files.  */

> diff --git a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c

> index 7a1e89c1a3..91ea27a423 100644

> --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c

> +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c

> @@ -330,16 +330,20 @@ __nldbl_wprintf (const wchar_t *fmt, ...)

>    return done;

>  }

>  

> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_29)

>  int

>  attribute_compat_text_section

>  __nldbl__IO_vfscanf (FILE *s, const char *fmt, va_list ap, int *errp)

>  {

>    int res;

>    set_no_long_double ();

> -  res = _IO_vfscanf (s, fmt, ap, errp);

> +  res = __vfscanf_internal (s, fmt, ap, 0);

>    clear_no_long_double ();

> +  if (__glibc_unlikely (errp != 0))

> +    *errp = (res == -1);

>    return res;

>  }

> +#endif

>  

>  int

>  attribute_compat_text_section

> @@ -347,7 +351,7 @@ __nldbl___vfscanf (FILE *s, const char *fmt, va_list ap)

>  {

>    int res;

>    set_no_long_double ();

> -  res = _IO_vfscanf (s, fmt, ap, NULL);

> +  res = __vfscanf_internal (s, fmt, ap, 0);

>    clear_no_long_double ();

>    return res;

>  }

> @@ -423,7 +427,7 @@ __nldbl_vfwscanf (FILE *s, const wchar_t *fmt, va_list ap)

>  {

>    int res;

>    set_no_long_double ();

> -  res = _IO_vfwscanf (s, fmt, ap, NULL);

> +  res = __vfwscanf_internal (s, fmt, ap, 0);

>    clear_no_long_double ();

>    return res;

>  }

> @@ -1027,7 +1031,6 @@ compat_symbol (libc, __nldbl_vdprintf, vdprintf, GLIBC_2_0);

>  compat_symbol (libc, __nldbl_vsnprintf, vsnprintf, GLIBC_2_0);

>  compat_symbol (libc, __nldbl_vsprintf, vsprintf, GLIBC_2_0);

>  compat_symbol (libc, __nldbl__IO_sscanf, _IO_sscanf, GLIBC_2_0);

> -compat_symbol (libc, __nldbl__IO_vfscanf, _IO_vfscanf, GLIBC_2_0);

>  compat_symbol (libc, __nldbl___vfscanf, __vfscanf, GLIBC_2_0);

>  compat_symbol (libc, __nldbl___vsscanf, __vsscanf, GLIBC_2_0);

>  compat_symbol (libc, __nldbl_fscanf, fscanf, GLIBC_2_0);

> @@ -1040,6 +1043,12 @@ compat_symbol (libc, __nldbl___printf_fp, __printf_fp, GLIBC_2_0);

>  compat_symbol (libc, __nldbl_strfmon, strfmon, GLIBC_2_0);

>  compat_symbol (libc, __nldbl_syslog, syslog, GLIBC_2_0);

>  compat_symbol (libc, __nldbl_vsyslog, vsyslog, GLIBC_2_0);

> +/* This function is not in public headers, but was exported until

> +   version 2.29.  For platforms that are newer than that, there's no

> +   need to expose the symbol.  */

> +# if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_29)

> +compat_symbol (libc, __nldbl__IO_vfscanf, _IO_vfscanf, GLIBC_2_0);

> +# endif

>  #endif

>  #if LONG_DOUBLE_COMPAT(libc, GLIBC_2_1)

>  compat_symbol (libc, __nldbl___asprintf, __asprintf, GLIBC_2_1);


Ok.

> diff --git a/wcsmbs/isoc99_fwscanf.c b/wcsmbs/isoc99_fwscanf.c

> index 0c6a2c47ac..00b07dd48e 100644

> --- a/wcsmbs/isoc99_fwscanf.c

> +++ b/wcsmbs/isoc99_fwscanf.c

> @@ -32,7 +32,7 @@ __isoc99_fwscanf (FILE *stream, const wchar_t *format, ...)

>    stream->_flags2 |= _IO_FLAGS2_SCANF_STD;

>  

>    va_start (arg, format);

> -  done = _IO_vfwscanf (stream, format, arg, NULL);

> +  done = __vfwscanf_internal (stream, format, arg, 0);

>    va_end (arg);

>  

>    _IO_release_lock (stream);


Ok.

> diff --git a/wcsmbs/isoc99_swscanf.c b/wcsmbs/isoc99_swscanf.c

> index ff523db706..40401d0aa1 100644

> --- a/wcsmbs/isoc99_swscanf.c

> +++ b/wcsmbs/isoc99_swscanf.c

> @@ -16,20 +16,22 @@

>     <http://www.gnu.org/licenses/>.  */

>  

>  #include <stdarg.h>

> -#include <stdio.h>

> -#include <libioP.h>

> -#include <wchar.h>

> +#include <libio/strfile.h>

>  

>  /* Read formatted input from S, according to the format string FORMAT.  */

> -/* VARARGS2 */

> +

>  int

>  __isoc99_swscanf (const wchar_t *s, const wchar_t *format, ...)

>  {

>    va_list arg;

>    int done;

> +  _IO_strfile sf;

> +  struct _IO_wide_data wd;

> +  FILE *f = _IO_strfile_readw (&sf, &wd, s);

> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;

>  

>    va_start (arg, format);

> -  done = __isoc99_vswscanf (s, format, arg);

> +  done = __vfwscanf_internal (f, format, arg, 0);

>    va_end (arg);

>  

>    return done;


Ok.

> diff --git a/wcsmbs/isoc99_vfwscanf.c b/wcsmbs/isoc99_vfwscanf.c

> index 7beb45b4d3..f70c6b596d 100644

> --- a/wcsmbs/isoc99_vfwscanf.c

> +++ b/wcsmbs/isoc99_vfwscanf.c

> @@ -28,7 +28,7 @@ __isoc99_vfwscanf (FILE *stream, const wchar_t *format, va_list args)

>  

>    _IO_acquire_lock_clear_flags2 (stream);

>    stream->_flags2 |= _IO_FLAGS2_SCANF_STD;

> -  done = _IO_vfwscanf (stream, format, args, NULL);

> +  done = __vfwscanf_internal (stream, format, args, 0);

>    _IO_release_lock (stream);

>    return done;

>  }


Ok.

> diff --git a/wcsmbs/isoc99_vswscanf.c b/wcsmbs/isoc99_vswscanf.c

> index 130769154d..b91eb651a3 100644

> --- a/wcsmbs/isoc99_vswscanf.c

> +++ b/wcsmbs/isoc99_vswscanf.c

> @@ -24,24 +24,16 @@

>     This exception applies to code released by its copyright holders

>     in files containing the exception.  */

>  

> -#include <libioP.h>

>  #include <wchar.h>

> -#include "../libio/strfile.h"

> +#include <libio/strfile.h>

>  

>  int

>  __isoc99_vswscanf (const wchar_t *string, const wchar_t *format, va_list args)

>  {

> -  int ret;

>    _IO_strfile sf;

>    struct _IO_wide_data wd;

> -#ifdef _IO_MTSAFE_IO

> -  sf._sbf._f._lock = NULL;

> -#endif

> -  _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, 0, &wd, &_IO_wstr_jumps);

> -  _IO_fwide (&sf._sbf._f, 1);

> -  _IO_wstr_init_static (&sf._sbf._f, (wchar_t *)string, 0, NULL);

> -  sf._sbf._f._flags2 |= _IO_FLAGS2_SCANF_STD;

> -  ret = _IO_vfwscanf ((FILE *) &sf._sbf, format, args, NULL);

> -  return ret;

> +  FILE *f = _IO_strfile_readw (&sf, &wd, string);

> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;

> +  return __vfwscanf_internal (f, format, args, 0);

>  }

>  libc_hidden_def (__isoc99_vswscanf)


Ok.

> diff --git a/wcsmbs/isoc99_vwscanf.c b/wcsmbs/isoc99_vwscanf.c

> index 049521b964..eb22c8acae 100644

> --- a/wcsmbs/isoc99_vwscanf.c

> +++ b/wcsmbs/isoc99_vwscanf.c

> @@ -28,7 +28,7 @@ __isoc99_vwscanf (const wchar_t *format, va_list args)

>  

>    _IO_acquire_lock_clear_flags2 (stdin);

>    stdin->_flags2 |= _IO_FLAGS2_SCANF_STD;

> -  done = _IO_vfwscanf (stdin, format, args, NULL);

> +  done = __vfwscanf_internal (stdin, format, args, 0);

>    _IO_release_lock (stdin);

>    return done;

>  }


Ok.

> diff --git a/wcsmbs/isoc99_wscanf.c b/wcsmbs/isoc99_wscanf.c

> index abfbd50c11..59f80d78fb 100644

> --- a/wcsmbs/isoc99_wscanf.c

> +++ b/wcsmbs/isoc99_wscanf.c

> @@ -33,7 +33,7 @@ __isoc99_wscanf (const wchar_t *format, ...)

>    stdin->_flags2 |= _IO_FLAGS2_SCANF_STD;

>  

>    va_start (arg, format);

> -  done = _IO_vfwscanf (stdin, format, arg, NULL);

> +  done = __vfwscanf_internal (stdin, format, arg, 0);

>    va_end (arg);

>  

>    _IO_release_lock (stdin);

> 


Ok.

Comments

Gabriel F. T. Gomes Nov. 14, 2018, 3:18 p.m. | #1
On Wed, 07 Nov 2018, Adhemerval Zanella wrote:

>On 29/10/2018 09:16, Gabriel F. T. Gomes wrote:

>

>> Additional note for review:

>> 

>>   - Reviewing the changes from vfscanf.c to vfscanf-internal.c in the

>>     original patch would be vey hard, because git doesn't detect the

>>     filename change.  To make review a little easier, I did as Zack did

>>     and manually edited the diff.  I'll reply to this thread and attach

>>     the original patch if someone wants to apply it.

>>     (ping me if I forget it)  

>

>I would advise to avoid it, it is error-prone and we do have tools to 

>handle it.  One option is to use -C and -M git options with a higher s

>imilarity index, for instance on this patch using:

>

>git format-patch -M90% -C -1

>

>it generates the expected result:


It does, indeed.  Thanks for the explanation.  I'll use it from now on.

>> Signed-off-by: Zack Weinberg <zackw@panix.com>

>> Signed-off-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br>  

>

>As Florian has said, we don't use DCO, but copyright assignments.


Ack, removed.

>> +/* Flags for __vfscanf_internal and __vfwscanf_internal.  */

>> +#define SCANF_LDBL_IS_DBL 0x0001

>> +#define SCANF_ISOC99_A    0x0002  

>

>As before, please describe what actually the flag does.


Ack, how about the following text?

+/* Flags for __vfscanf_internal and __vfwscanf_internal.
+
+   SCANF_LDBL_IS_DBL indicates whether long double values are to be
+   handled as having the same format as double, in which case the flag
+   should be set to one, or as another format, otherwise.
+
+   SCANF_ISOC99_A, when set to one, indicates that the ISO C99 or POSIX
+   behavior of the scanf functions is to be used, i.e. automatic
+   allocation for input strings with %as, %aS and %a[, a GNU extension,
+   is disabled. This is the behavior that the __isoc99_scanf family of
+   functions use.  When the flag is set to zero, automatic allocation is
+   enabled.  */
+#define SCANF_LDBL_IS_DBL 0x0001
+#define SCANF_ISOC99_A    0x0002

>> +#define LDBL_DISTINCT (__glibc_likely ((mode_flags & SCANF_LDBL_IS_DBL) == 0))

>> +#define USE_ISOC99_A  (__glibc_likely (mode_flags & SCANF_ISOC99_A))  

>

>Do we really gain anything using these defines? I tend to frown on macros

>that use internal named variables instead of set them as arguments. Also

>this is quite short, I think it is more readable to just use the comparison

>directly.


I don't have a strong opinion about it, but it does look good with the
comparison directly where it is used.

Fixed as suggested.

>> +/* Same as compat_symbol, ldbl_compat_symbol is not to be used outside

>> +   '#if SHLIB_COMPAT' statement and should fail if it is.  */

>> +# define ldbl_compat_symbol(lib, local, symbol, version) ...  

>

>Why not use an _Static_assert(0, "ldbl_compat_symbol should be used inside SHLIB_COMPAT")?


No reason other than I wasn't aware of _Static_assert.

Fixed as suggested.


I'll send a new version when I finish reviewing the whole patch set.

Thanks for the careful review.

Patch

diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf-internal.c
similarity index 98%
copy from stdio-common/vfscanf.c
copy to stdio-common/vfscanf-internal.c
index 1ce836a324..1ae37a2a11 100644
--- a/stdio-common/vfscanf.c
+++ b/stdio-common/vfscanf-internal.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 1991-2018 Free Software Foundation, Inc.
+/* Internal functions for the *scanf* implementation.
+   Copyright (C) 1991-2018 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
[...]