This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH]Introducing small foot-print implementation for non-wide-char formatted IO


On Wed, Jul 2, 2014 at 7:44 PM, Jeff Johnston <jjohnstn@redhat.com> wrote:
> Hi Bin,
>
> Sorry for the long delay in replying.  Here in Canada, we had a holiday on Tuesday so I have been on vacation since Friday.
>
> A few comments on the latest patch:
>
> 1. You have files that have the Berkeley advertising clause in them.  You need to remove those clauses.
>    See: ftp://ftp.cs.berkeley.edu/pub/4bsd/README.Impt.License.Change

Hi Jeff, thanks for correcting the mistake.
There are different kinds of modification in files from
newlib/stdio/libc.  Files like vfwprintf.c just delete the 3rd clause,
while files like vfwscanf.c make bigger changes, so which kind of
statement should I follow?

Thanks,
bin

>
> 2. You have a typo in the README regarding long double.  I took the liberty of rewording a bit as well:
>
>         The only exception is the configuration option provides limited support for
>         long double.  Internally, the nano formatted I/O functions use double so accuracy is only guaranteed
>         to double precision.
>
> 3. Regarding the discussion on the newlib mailing list about the standard supported, I have no issue with you just stating that you only support the C89 Standard.
>
> -- Jeff J.
>
>
> ----- Original Message -----
> From: "Bin Cheng" <bin.cheng@arm.com>
> To: "Jeff Johnston" <jjohnstn@redhat.com>
> Cc: newlib@sourceware.org
> Sent: Thursday, June 26, 2014 3:36:37 AM
> Subject: RE: [PATCH]Introducing small foot-print implementation for non-wide-char formatted IO
>
> Hi Jeff,
> Thanks very much for rephrasing the wording.  Here is the revised patch incorporating your comments, please review.
>
> Thanks,
> bin
>
> -----Original Message-----
> From: newlib-owner@sourceware.org [mailto:newlib-owner@sourceware.org] On Behalf Of Jeff Johnston
> Sent: Thursday, June 26, 2014 7:31 AM
> To: Bin Cheng
> Cc: newlib@sourceware.org
> Subject: Re: [PATCH]Introducing small foot-print implementation for non-wide-char formatted IO
>
> Hi Bin,
>
> Looks interesting.  I have a few minor comments/questions:
>
> 1. Your README section could be clarified.  Perhaps something like this:
>
> +`--enable-newlib-nano-formatted-io'
> +     This builds NEWLIB with a special implementation of formatted I/O functions, designed to
> +     lower the size of applications on small systems with size constraint issues.  This option does not affect
> +     wide-char formatted I/O functions.  Some notes about the feature:
> +
> +      1) The non-wide-char formatted I/O functions only support the C89
> + standard
> +
> +      2) Floating-point support is split out of the formatted I/O code into weak functions which are not linked
> +         by default.  Programs that need floating-point I/O support must explicitly
> +         request linking of one or both of the floating-point functions: _printf_float or _scanf_float.  This can be done
> +         at link time using the -u option which can be passed to either gcc or ld.  The -u option forces the link to
> +         resolve those function references.
> +
> +      3) Integer-only versions of the formatted I/O functions (the iprint/iscanf family) simply alias their regular counter-parts.
> +         The affected functions are:
> +
> +         diprintf vdiprintf
> +
> +         siprintf fiprintf iprintf sniprintf asiprintf asniprintf
> +
> +         siscanf fiscanf iscanf
> +
> +         viprintf vfiprintf vsiprintf vsniprintf vasiprintf vasniprintf
> +
> +         viscanf vfiscanf vsiscanf
> +
> +         _diprintf_r _vdiprintf_r
> +
> +         _siprintf_r _fiprintf_r _iprintf_r _sniprintf_r _asiprintf_r
> +         _asniprintf_r
> +
> +         _siscanf_r _fiscanf_r _iscanf_r
> +
> +         _viprintf_r _vfiprintf_r _vsiprintf_r _asniprintf_r _vasiprintf_r
> +         _vasniprintf_r
> +
> +         _viscanf_r _vfiscanf_r _vsiscanf_r
> +
> +
> +      4) As mentioned, the option does not affect wide-char formatted I/O.  The following configuration options are ignored for non-wide-char
> +         formatted I/O functions:
> +
> +         enable-newlib-io-pos-args
> +         enable-newlib-io-c99-formats
> +         enable-newlib-io-long-long
> +         enable-newlib-io-long-double
> +
>
> 2. In your original README you mention:
>
> +     Additionally, "enable-newlib-io-float" is no longer needed because of
> +     changes to the handling of floating-point formatted IO.
>
> but you have checks for the FLOATING_POINT flag in your code.  I did not address this comment in the README edit above.
>
> The FLOATING_POINT flag is tied to the NO_FLOATING_POINT flag being undefined.  This flag is tied to the --enable-newlib-io-float option (see configure.host).  So, if the end-user performs a --disable-newlib-io-float, it removes a bit of code, some of which does nothing meaningful unless the end-user has explicitly asked for floating-point support.
>
> Would it not be clearer to state something like: "Floating-point format specifiers are, by default, recognized, but if the floating-point functions are not linked in, this may result in undefined behavior.  If the application does not support floating-point, one can use the --disable-newlib-io-float configuration option and floating-point specifiers will not be recognized.  Use of this option further reduces code size."
>
> 3. There should be some note about long double support.  It appears that you partially support long double values by converting them to double precision internally.
>
> 4. For attribute flags, please use _ATTRIBUTE(__weak__) and _ATTRIBUTE(__alias__) though this isn't done everywhere, it would be nice to start to be consistent.
>
> -- Jeff J.
>
> ----- Original Message -----
> From: "Bin Cheng" <bin.cheng@arm.com>
> To: newlib@sourceware.org
> Sent: Friday, June 20, 2014 1:44:17 AM
> Subject: [PATCH]Introducing small foot-print implementation for non-wide-char formatted IO
>
> Hi,
>
> This is the second try to merge our work of formatted IO for small systems originated from Newlib-nano library, in project "GNU Tools for ARM Embedded Processors".  For the first time try, please refer to archived message at:
> https://sourceware.org/ml/newlib/2013/msg00435.html
>
> Since Newlib-nano in our project has been adopted/verified by many ARM embedded developers and is relatively stable as far as I can see, here I am trying again to upstream it.  In this patch I rebased/refactored code against the latest trunk.  I also introduced a new configuration option "--enable-newlib-nano-formatted-io" to control this small foot-print implementation of formatted IO.  The new option can be used just like other code size saving ones, and there is some straightforward description I added for it in newlib/README, which is quoted here:
>
> +`--enable-newlib-nano-formatted-io'
> +     NEWLIB has a special implementation for formatted IO functions, it has
> +     below features:
> +      1) It only supports C89 standard and would never include other
> features.
> +      2) It removes direct dependency on floating-point IO handling code.
> +        Programs that need to handle floating-point IO must now explicitly
> +        request the feature by providing options "-u _printf_float" or
> +        "-u _scanf_float" at linking command line.
> +      3) It removes now redundant integer-only implementation of the
> +        printf/scanf family of routines (iprintf/iscanf, etc.).  These
> +        functions now alias the standard routines.  This avoids the risk
> +        of getting duplicated functionality when these operations are
> +        needed.  The affected functions are:
> +
> +         diprintf vdiprintf
> +
> +         siprintf fiprintf iprintf sniprintf asiprintf asniprintf
> +
> +         siscanf fiscanf iscanf
> +
> +         viprintf vfiprintf vsiprintf vsniprintf vasiprintf vasniprintf
> +
> +         viscanf vfiscanf vsiscanf
> +
> +         _diprintf_r _vdiprintf_r
> +
> +         _siprintf_r _fiprintf_r _iprintf_r _sniprintf_r _asiprintf_r
> +         _asniprintf_r
> +
> +         _siscanf_r _fiscanf_r _iscanf_r
> +
> +         _viprintf_r _vfiprintf_r _vsiprintf_r _asniprintf_r _vasiprintf_r
> +         _vasniprintf_r
> +
> +         _viscanf_r _vfiscanf_r _vsiscanf_r
> +
> +
> +     With this implementation, the following newlib configuration options
> +     are no longer available for non-wide-char formatted IO:
> +
> +       enable-newlib-io-pos-args
> +       enable-newlib-io-c99-formats
> +       enable-newlib-io-long-long
> +       enable-newlib-io-long-double
> +
> +     Additionally, "enable-newlib-io-float" is no longer needed because of
> +     changes to the handling of floating-point formatted IO.
> +
> +     This option enables the nano-formatted-IO implementation, and should
> +     be used for small systems with very limited memory.
> +     Disabled by default.
> +
>
> I know there are concerns about maintenance burden of keeping two implementations of formatted IO, which I hope can be addressed by below
> comments:
> 1) The two implementations are totally independent to each other, in other words, there is no shared code which need to be dealt with in future patches.
> 2) The nano version of implementation is designed for small systems with very limited resources only, so it is restricted to C89 standard.  I think it's reasonable to setup a rule after merging that we will never include features outside of C89.
> 3) The nano version of implementation is quite modularized, with most codes
> added by several new files.   Other than that, it's just like other code
> size saving options.
> 4) The nano version of implementation has been widely used since it was first introduced in 2012.  Because it's very simple, there is not many issues reported so far, and I don't expect there will be many issues after merging.
>
> With these considerations, I guess it's not inappropriate for it to be included in Newlib.  So any comments?
>
> BTW, I compressed the patch since it exceeds size limitation of newlib mailing list.
>
> Thanks,
> bin
>
> 2014-06-20  Bin Cheng  <bin.cheng@arm.com>
>
>         * README (--enable-newlib-nano-formatted-io): Describe.
>         * acconfig.h (_NANO_FORMATTED_IO): Undef.
>         * newlib.hin (_NANO_FORMATTED_IO): Undef.
>         * configure.in (--enable-newlib-nano-formatted-io): New option.
>         * configure: Regenerated.
>         * libc/configure.in (--enable-newlib-nano-formatted-io): New option.
>         * libc/configure: Regenerated.
>         * libc/stdio/Makefile.am (NEWLIB_NANO_FORMATTED_IO): Support new
>         configuration option.
>         * libc/stdio/Makefile.in: Regenerated.
>         * libc/stdio/asnprintf.c (_asniprintf_r, asniprintf): Use
>         _NANO_FORMATTED_IO to declare alias prototypes.
>         * libc/stdio/asprintf.c (_asiprintf_r, asiprintf): Ditto.
>         * libc/stdio/dprintf.c (_diprintf_r, diprintf): Ditto.
>         * libc/stdio/fprintf.c (_fiprintf_r, fiprintf): Ditto.
>         * libc/stdio/fscanf.c (fiscanf, _fiscanf_r): Ditto.
>         * libc/stdio/printf.c (_iprintf_r, iprintf): Ditto.
>         * libc/stdio/scanf.c (iscanf, _iscanf_r): Ditto.
>         * libc/stdio/snprintf.c (_sniprintf_r, sniprintf): Ditto.
>         * libc/stdio/sprintf.c (_siprintf_r, siprintf): Ditto.
>         * libc/stdio/sscanf.c (siscanf, _siscanf_r): Ditto.
>         * libc/stdio/vasnprintf.c (_vasniprintf_r, vasniprintf): Ditto.
>         * libc/stdio/vasprintf.c (vasiprintf, _vasiprintf_r): Ditto.
>         * libc/stdio/vdprintf.c (_vdiprintf_r, vdiprintf): Ditto.
>         * libc/stdio/vprintf.c (viprintf, _viprintf_r): Ditto.
>         * libc/stdio/vscanf.c (viscanf, _viscanf_r): Ditto.
>         * libc/stdio/vsnprintf.c (vsniprintf, _vsniprintf_r): Ditto.
>         * libc/stdio/vsprintf.c (vsiprintf, _vsiprintf_r): Ditto.
>         * libc/stdio/vsscanf.c (vsiscanf, _vsiscanf_r): Ditto.
>         * libc/stdio/nano-vfprintf.c: New file.
>         * libc/stdio/nano-vfprintf_float.c: New file.
>         * libc/stdio/nano-vfprintf_i.c: New file.
>         * libc/stdio/nano-vfprintf_local.h: New file.
>         * libc/stdio/nano-vfscanf.c: New file.
>         * libc/stdio/nano-vfscanf_float.c: New file.
>         * libc/stdio/nano-vfscanf_i.c: New file.
>         * libc/stdio/nano-vfscanf_local.h: New file.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]