[PATCH] Fix a printf hook problem
Jakub Jelinek
jakub@redhat.com
Sat Feb 12 15:30:00 GMT 2011
Hi!
Yesterday and today I've worked on printf hooks support for libquadmath
(i.e. support for printing __float128 IEEE754 quad values on
x86_64/i386/ia64). I'd like to use Q modifier for it, is anyone aware for
other uses of such modifier that would be incompatible with it?
I've used
http://www.eglibc.org/cgi-bin/viewcvs.cgi/*checkout*/libdfp/trunk/printf_dfp.c?content-type=text%2Fplain&rev=12594
as the source for this, but found a few issues in glibc and libdfp
printf_dfp.c. Current libquadmath patch is at http://gcc.gnu.org/PR47642
1) The comment in printf-parsemb.c suggests that arginfo function should
be able to return -1 to signal that it wants standard processing,
even the code looks like it was meant to handle that, but
as spec->ndata_args is unsigned (size_t), it is never < 0 and
*printf crashes badly when the arginfo routine returns -1.
Fixed by the first hunk in the patch below.
There is a workaround for this in the libraries, essentially instead
of returning -1 it can duplicate the stuff from __parse_one_specmb
instead, nevertheless it would be nice to see this fixed in glibc.
2) When testing fa_IR.UTF-8 %Ie support, I've noticed that often there
are garbage characters following the decimal point. This is because
decimal buffer (and thousands too) is not actually zero terminated,
so if stack doesn't happen to be zero, we get garbage. Fixed by the
remaining two hunks.
3) %Ie doesn't work in wide orientation at all. As COMPILE_WPRINTF
is not used by printf_fp.c, but instead decision whether it
is wide or not is based on wide variable, _i18n_number_rewrite
will always just rewrite the narrow string, but with info->wide
we actually print the wide string instead. Not fixed here.
4) I was surprised to see there is no way to unregister the hooks
(well, at least modifiers and types - specifiers can be
unregistered currently by calling register_printf_specifier
again with NULL, NULL and also that the implementation
allows only a single converter/arginfo pair to be registered
for each specifier letter. This makes it e.g. impossible
to handle both _Decimal* and __float128 printing at the same time.
I guess glibc could iterate through all the registered
arginfo functions as long as they return -1, and only the one
that will say to handle it would then be used together with the
corresponding converted function. Then unregistering wouldn't
work by calling it again with NULL, NULL though and thus
some unregister_* APIs would be needed.
And the libdfp bugs:
1) Related to 1) above, libdfp arginfo function currently returns
0 if it is not D/DD/H modifier, which means that whenever you
use e/E/f/F/g/G/a/A specifier, if no modifier or some other modifier
is used it assumes no argument needs to be consumed (similarly to %m),
i.e. when you register libdfp suddenly "%e %Lg %f" will not work
anymore.
2) __printf_dfp doesn't return -2 if modifier is not D/DD/H, which
means that say %e is printed as if it was _Decimal32 number
(and because of 1) above random garbage is printed as such number).
3) #ifndef OPTION_EGLIBC_LOCALE_CODE
char decimald;
#endif
const char *decimal;
...
#else
/* Hard-code values from 'C' locale. */
decimald = ".";
decimal = &decimald;
decimalwc.wc = L'.';
#endif
can't possible work right, assigning "." to a char? Surely
that was supposed to be decimal = "."; and kill decimald.
4) libdfp doesn't have a way to unregister, which means if
you say dlopen libdfp, call the register function and
then dlclose it, next printf will likely segfault.
5) Maybe the TR requires it, but I find it surprising that
a/A prints a decimal number instead of hexadecimal on
for _Decimal*.
Anyway, here is a patch to fix glibc 1) and 2) above.
2011-02-11 Jakub Jelinek <jakub@redhat.com>
* stdio-common/printf-parsemb.c (__parse_one_specmb): Handle
arginfo fn returning -1.
* stdio-common/_i18n_number.h (_i18n_number_rewrite): Ensure decimal
and thousands string is zero terminated.
--- libc/stdio-common/printf-parsemb.c.jj 2011-01-06 11:48:40.000000000 +0100
+++ libc/stdio-common/printf-parsemb.c 2011-02-11 16:47:33.034254929 +0100
@@ -295,9 +295,9 @@ __parse_one_specmb (const UCHAR_T *forma
/* We don't try to get the types for all arguments if the format
uses more than one. The normal case is covered though. If
the call returns -1 we continue with the normal specifiers. */
- || (spec->ndata_args = (*__printf_arginfo_table[spec->info.spec])
- (&spec->info, 1, &spec->data_arg_type,
- &spec->size)) < 0)
+ || (int) (spec->ndata_args = (*__printf_arginfo_table[spec->info.spec])
+ (&spec->info, 1, &spec->data_arg_type,
+ &spec->size)) < 0)
{
/* Find the data argument types of a built-in spec. */
spec->ndata_args = 1;
--- libc/stdio-common/_i18n_number.h.jj 2011-01-06 11:48:40.087542069 +0100
+++ libc/stdio-common/_i18n_number.h 2011-02-12 13:59:50.436254163 +0100
@@ -30,8 +30,8 @@ _i18n_number_rewrite (CHAR_T *w, CHAR_T
# define decimal NULL
# define thousands NULL
#else
- char decimal[MB_LEN_MAX];
- char thousands[MB_LEN_MAX];
+ char decimal[MB_LEN_MAX + 1];
+ char thousands[MB_LEN_MAX + 1];
#endif
/* "to_outpunct" is a map from ASCII decimal point and thousands-sep
@@ -47,13 +47,19 @@ _i18n_number_rewrite (CHAR_T *w, CHAR_T
mbstate_t state;
memset (&state, '\0', sizeof (state));
- if (__wcrtomb (decimal, wdecimal, &state) == (size_t) -1)
+ size_t n = __wcrtomb (decimal, wdecimal, &state);
+ if (n == (size_t) -1)
memcpy (decimal, ".", 2);
+ else
+ decimal[n] = '\0';
memset (&state, '\0', sizeof (state));
- if (__wcrtomb (thousands, wthousands, &state) == (size_t) -1)
+ n = __wcrtomb (thousands, wthousands, &state);
+ if (n == (size_t) -1)
memcpy (thousands, ",", 2);
+ else
+ thousands[n] = '\0';
}
#endif
Jakub
More information about the Libc-alpha
mailing list