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


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

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]