I'll attach the patch shortly once it applies to cleanly to cvs head.

Created attachment 2911 [details]
Printf Hooks extension patch

Here's a first pass.  There are some warnings from when I merged the debug
macros which I'll clean up in a forthcoming patch.

I'll also attach an example usage patch of how I used the hooks for Decimal
Floating Point types support.

Created attachment 2922 [details]
printf hooks patch revision 2 which cleans up debug and fixes val_ptr problem

This patch is a revision of the previous patch and fixes the bug related to
val_ptr.  It also makes the patch more readable by cleaning up the debug
macros.

Created attachment 2923 [details]
registration function for the dfp library

This is the registration function for the dfp library which gets called prior
to using printf for Decimal floating point types.

Created attachment 2924 [details]
register all of the decimal floating point types with the printf hooks mechanism

This file actually registers the decimal floating point library hooks with the
printf hooks mechanism.  This is where we describe which format codes are
overridden.

Created attachment 2925 [details]
Callback functions which recognize _Decimal* types and set length flags.

This file contains the callback functions which recognize the _Decimal* data
types and set the length flags.

Created attachment 2926 [details]
The printf function for the DFP library.

Function which recognizes the length flag passed as a user field in the printf
info struct and determines which version of the DFP printf length functions to
call.

Created attachment 2927 [details]
Individual length function which casts off the args data into a _Decimal* type

There is a length function for each _Decimal* type which casts off the printf
args data into a _Decimal* type.

Created attachment 3011 [details]
printf_hooks patch revision 3 which removes debugging messages, adds __builtin_expect, and conditional array creation.

Latest modification with:
Debugging messages removed.
__builtin_expect() used to indicate override is the uncommon branch.
conditional array creation for arginfo array only when user override is
provided.

We really have to start from the beginning.  The patch makes lots ooof
assumptions and implies to have the semantics wanted.  I don't agree with either.

Before any code is written we must nail down the desired semantics.  The
existing interface is nice because

- you can have multiple calls to the register functions and unless the
specifiers clash there is no problem

- the existing modifiers and specifiers are handled outside the callback
framework completely for speed.  I.e., if no specifier matches, no code through
modifiers is needed

These are both properties I want to preserve.

Therefore it is necessary to define the interface of the new register function
differently from what you have.  Some of what you propose makes sense:

- we want to be able to specify flags and length modifiers.  These have clearly
defined positions in the format specifier

- we obviously want new format specifiers

There should be no argument that all characters used for flags, length
specifiers, and format specifiers must be ASCII characters.  Otherwise the
formats wouldn't be available across domains.  This means no wchar_t needed and
only values from ASCII range 32 to 127 (all inclusive) are allowed.

Since the flags, length specifiers, and format specifiers have fixed positions
and to avoid unnecessary duplication in the code the user of the callbacks has
to provide, there should be a way to separate out the ways to introduce
extensions to each of these three types.  I.e., in the register function it
should be necessary to specify what is introduced (flag, length spec, format
spec).  The implementation can then have three different tables.  The extra flag
and length specification can be carried in a new word in the printf_info
structure (or its replacement).  Since only user-defined format specifiers can
handle the new flags etc the implementation can make sure that it calls into one
(or more, must be a list) of the user-defined format specifier callbacks if any
of the extra flag bits is set.  If no new flag bit is set and then format
specifier is known to the implementation, the standard formatting is performed.

I don't want to try to define the semantics here.  This is what should be done
in a design document which should have been the first step.

Some more specific things about the code (even though not really relevant right
now):

- I don't like that there is a 'free' function needed.  There shouldn't be a
need for this.  All the flags and length modifier functions should do is set
some bits etc.  And the format specifier callback should just perform its work
and be done.  It's easy enough to require that registration function tells the
implementation something about the size of the parameters it expects.

- in the fast path (which doesn't handle positional arguments) we should not
handle the tables either.  That code must remain as fast as before.  It is
unacceptable that just because of the stupidity of certain people which make
this complicated extension necessary sane programs gets slowed down.

There are certainly many other details I didn't touch here.  We;ll get to that
once there is a design document.

(In reply to comment #10)
> We really have to start from the beginning.  The patch makes lots ooof
> assumptions and implies to have the semantics wanted.  I don't agree with either.

The interface and semantics was the part that I was sure you'd have strong
opinions on.  I'll pitch a new design that I have ready and we can go from
there.  What format would you like this to take?  TeX? Glibc Wiki?

> Before any code is written we must nail down the desired semantics.  The
> existing interface is nice because
> - you can have multiple calls to the register functions and unless the
> specifiers clash there is no problem

In order to understand what was required I had to actually write some code.

> - the existing modifiers and specifiers are handled outside the callback
> framework completely for speed.  I.e., if no specifier matches, no code through
> modifiers is needed
> 
> These are both properties I want to preserve.

Absolutely.  There is one minor addition which will impact the existing
framework.  When there is a length modifier registration the internal framework
needs to check to see if there is a length modifier override, exactly like what
is done prior to processing conversion specifiers.

> Therefore it is necessary to define the interface of the new register function
> differently from what you have.  Some of what you propose makes sense:
> 
> - we want to be able to specify flags and length modifiers.  These have clearly
> defined positions in the format specifier
> 
> - we obviously want new format specifiers

> There should be no argument that all characters used for flags, length
> specifiers, and format specifiers must be ASCII characters.  Otherwise the
> formats wouldn't be available across domains.  This means no wchar_t needed and
> only values from ASCII range 32 to 127 (all inclusive) are allowed.

This was my thought as well but the existing printf.h struct printf_info defines
the 'spec' as wchar_t.  I know from experience that you're not a big fan of
changing structure definitions.

> Since the flags, length specifiers, and format specifiers have fixed positions
> and to avoid unnecessary duplication in the code the user of the callbacks has
> to provide, there should be a way to separate out the ways to introduce
> extensions to each of these three types.  I.e., in the register function it
> should be necessary to specify what is introduced (flag, length spec, format
> spec).  The implementation can then have three different tables.  The extra flag

Three different tables will definitely improve performance of the table
traversal/matching over what I have right now.

Are you inclined to introduce a new symbol name for the new registration
function and preserve an older compat interface for the old method?  Or do you
simply want to version the interface and change the parameter list for the new
version?

> and length specification can be carried in a new word in the printf_info
> structure (or its replacement).  Since only user-defined format specifiers can
> handle the new flags etc the implementation can make sure that it calls into one
> (or more, must be a list) of the user-defined format specifier callbacks if any
> of the extra flag bits is set.  If no new flag bit is set and then format
> specifier is known to the implementation, the standard formatting is performed.

I have a few ideas on how to present this that may not require redefining struct
printf_info.

> I don't want to try to define the semantics here.  This is what should be done
> in a design document which should have been the first step.

It was my intention to use code to get your attention/interest.

> Some more specific things about the code (even though not really relevant right
> now):
> 
> - I don't like that there is a 'free' function needed.  There shouldn't be a
> need for this.  All the flags and length modifier functions should do is set
> some bits etc.  And the format specifier callback should just perform its work
> and be done.  It's easy enough to require that registration function tells the
> implementation something about the size of the parameters it expects.

I originally did exactly what you suggested but then I ran into hard ABI issues.
 I'd hoped that I could have _Decimal128 types peeled off the va_arg list into
long double 128 types and then just deal with type punning in the callback function.

There are two reasons this doesn't work.  Firstly, on an architecture where long
double is double we'll lose half the _Decimal128 in the va_arg -> long double ->
_Decimal128 and the 'next' value will be peeled incorrectly as it will get the
second double remnant from the previous value.

Secondly, due to bizarre hardware requirements, _Decimal128 types must be passed
in an even-odd register pair.  Peeling a va_arg into a long double won't
guarantee this because the ABI makes no such requirements on long double 128.

> - in the fast path (which doesn't handle positional arguments) we should not
> handle the tables either.  That code must remain as fast as before.  It is
> unacceptable that just because of the stupidity of certain people which make
> this complicated extension necessary sane programs gets slowed down.

As I said earlier, the only infringement into the fast-path in my existing
design is to check for the availability of a length modifier override.

> There are certainly many other details I didn't touch here.  We;ll get to that
> once there is a design document.

Yes, for instance, the ability of format specifiers to consume more than one
argument.  Is there actually any sanctioned format specifier (dubious or not)
which defines such behavior?

Also, the existing definition of struct printf_info doesn't account for
multi-byte conversion specifiers.  As far as I can tell there aren't any
proposed C-spec extensions (AVX, VSX, DFP, Altivec) which define them so we're
currently covered.

Assigning to Ulrich so that he can see when I've made changes to the bugzilla.

(In reply to comment #11)
> (In reply to comment #10)
> > - I don't like that there is a 'free' function needed.  There shouldn't be a
> > need for this.  All the flags and length modifier functions should do is set
> > some bits etc.  And the format specifier callback should just perform its work
> > and be done.  It's easy enough to require that registration function tells the
> > implementation something about the size of the parameters it expects.
> 
> I originally did exactly what you suggested but then I ran into hard ABI issues.
>  I'd hoped that I could have _Decimal128 types peeled off the va_arg list into
> long double 128 types and then just deal with type punning in the callback
function.
> 
> There are two reasons this doesn't work.  Firstly, on an architecture where long
> double is double we'll lose half the _Decimal128 in the va_arg -> long double ->
> _Decimal128 and the 'next' value will be peeled incorrectly as it will get the
> second double remnant from the previous value.
> 
> Secondly, due to bizarre hardware requirements, _Decimal128 types must be passed
> in an even-odd register pair.  Peeling a va_arg into a long double won't
> guarantee this because the ABI makes no such requirements on long double 128.
> 

Oh I see what you mean.  We pass sizeof(_Decimal128) to the registration
function when we register 'e' as a conversion specification for DFP.  The printf
internals simply allocate the necessary storage and pass a reference to it to
the user's va_arg_fn callback.

This should definitely remove the need for a free function as long as the
internal code accounts for the fact that an argument (in some specifications)
may consume more than one argument.

Here's a first pass at a design document:

http://sources.redhat.com/glibc/wiki/PrintfHooksDesign

It needs a lot of cleanup to solidify some of the remaining implementation
questions.

Some of the important bits:

- Since the `__const struct printf_info *__info' parameter of
printf_arginfo_function is "const" the arginfo callback functions can't set
directly into it.  If they want to set any special flags then they have to set
flags in __argstype in the range of 0xffff0000.

- With this in mind `struct printf_info' only needs the addition of one
additional word-sized user flag, rather than two.  Once an override is detected
for a particular length-modifier the printf internals can simply copy the flags
in 0xffff0000 to `struct printf_info::user'.

- I've not thought of how to allow support for 'flag characters' yet since none
of the C-Spec proposals (VSX, Altivec, AVX, or DFP) define any overrides.

- The section `Design Preclusions::Questionable' should probably be answered.  I
was able to come up with a design which didn't modify the existing `struct
printf_info' definition at all and I can present that design if you'd like.

- There's a lingering issue of `struct printf_info::spec' being defined as a
single wchar_t.  This doesn't really work if we want to support multi-byte
'conversion specifications'.  As far as I can tell none of the existing C-Spec
proposals define any multi-byte conv specs.

Thanks for the review.
Ryan

Created attachment 3783 [details]
printf-hooks-2009-02-27.patch

This is getting closer to the design that Ulrich requested.

There's still a 'free()' callback but this will be replaced by a sizeof(type)
requirement on the registration function and the storage will be managed
internally by glibc.

Also, I currently have two separate registration functions because the length
modifier doesn't need all of the callbacks that the conversion specifier needs.
 These can certainly be unified if necessary.

Finally, I haven't implemented flag overrides because I'm not sure it is
necessary at this point.  (None of the draft specs include flag overrides).

Created attachment 3787 [details]
printf-hooks-2009-03-02.patch with full interface

Ulrich

This patch satisfies the interface requirements you laid out earlier and
completes the work of the previous patch, namely:

Unified function for creating a length modifier or conversion specifier
override.

Elimination of the free callback, and elimination of the requirement that a
user malloc storage.  GLIBC now takes a 'size' indicator when the length
modifier is registered and allocates storage for the user based on this.

I'd like comment on two factors.  Should I break apart the array of overrides
into a few single-type arrays rather than the unified array?

Also the current flag mechanism is limited since it uses unused bits in the
existing data_args_type flag to set user flags.  It only supports up to 16
override format codes this way.  I didn't want to extend any public structures
or modify and public functions without comment from you.

Thanks
Ryan

Created attachment 3788 [details]
example code registering the hooks and using them.

Created attachment 3796 [details]
printf-hooks-2009-03-06.patch

I believe this satisfies all of Ulrich's requirements.	It fixes the remaining
issues from the previous patch:

3 separate format specifier arrays
user flags field in the printf_info struct
Single API for registration
No free function necessary (auto allocation)

Please review

There are two severe design flaws I found so far:

1. There should be no fixed size be associated with a modifier.  With your code
a modifier identifies a specific format.  That's not how modifiers work. 
Instead, the size is only known by the combination of modifier and specifier.

2. Related: imagine what happens with your implementation if more than one place
inserts printf hooks and specifically for new modifiers.  The user bits use
define are up for grabs by everybody.  What instead should happen is that you
register a modifier (arbitrary length, BTW) and then get a bitmask back with one
bit set.  This you'll have to store and interpret appropriately when the
callbacks for the specifier is called.


I have to see where this leaves us.  I'll look at writing this code myself now,
so no need to revise your code.

Created attachment 3874 [details]
Example code for alternative implementation now in cvs

I have checked in a different implementation for the extended printf hooks. 
There are not many similarities with Ryan's implementation because it had so
many unnecessary limitations.

I've tested the implementation by implementing hooks to print x86 SSE
registers.  See the attached source file.  This should be sufficient for
Power's DFP as well.

Please try this out ASAP so that if necessary the code can be changed before an
implementation.

There is one little change I'll likely make at some point to allow more than
one installed handler per format specifier.  But that's an internal change.

I'm currently building & testing these hooks for _Decimal128.  Once I have that
working I'll add _Decimal32 and _Decimal64.  I have some concerns with the
implementation but I will withhold until I have some data.

Questions:

You left off flags?  You asked for them when you asked for the design document.

I notice you also discarded the single interface you asked for and went with two
separate interfaces.

Does the SSE definition for printf actually reuse currently defined specifiers,
e.g. 'f' for defining new modifiers, e.g. 'v4f'?  That's really screwy.

I haven't tried it yet, but I suspect that using the likes of the default
specifiers doesn't work after the specifier has been overridden?

I tested this with _Decimal128 and it seems to work.  I'm going to add in
_Decimal64 support now.

Indeed,

  mod_DD = register_printf_modifier (L"DD");
  mod_D = register_printf_modifier (L"H");
  mod_D = register_printf_modifier (L"D");
  register_printf_specifier ('f', printf_dfp, d128_ais);
  register_printf_specifier ('f', printf_dfp, d32_ais);
  register_printf_specifier ('f', printf_dfp, d64_ais);

Does not work.  The most recently registered specifier still works.  So
performing printf("%Df\n",d64); works, but performing printf("%DDf\n",d128) or
printf("%Hf\n",d32) result in undefined behavior even though they work when when
registered individually.

I also verified that the following works and that 'f' is printed in both cases,
i.e. when 'D' doesn't match.

printf("%f\n",f);
register_printf_specifier('f',printf_dfp,d64_ais);
printf("%Df\n",d64);
printf("%f\n",f);

I also noticed that a 'spec', i.e. a specifier is still bound to a single character?

(In reply to comment #23)
>   register_printf_specifier ('f', printf_dfp, d128_ais);
>   register_printf_specifier ('f', printf_dfp, d32_ais);
>   register_printf_specifier ('f', printf_dfp, d64_ais);

That's not how it's supposed to work.  Look at the code in comment #20, function
xmm_printf_x and xmm_printf_f. You differentiate the modifiers in the single
callback you install for 'f'.  I've mentioned at the end of comment #20 that
installing multiple callbacks for one format doesn't work yet.  This is
something I'll address at some point later.  It's no issue for this case.

Yes, I'm aware of this.  I just wanted to make sure that is what you meant and
I'll test when that facility is available.

Created attachment 3888 [details]
DFP types registration example

My callback 'printf_dfp' is quite capable of differentiating modifiers as you
directed.  Unless I'm missing something, handling both %Hf, %Df, and %DDf will
require the later extension.  Here's I've attached my code which accomplishes
what I think is necessary.

Full featured printf hooks support checked in with the following git commit:

commit 9d26efa90c6dcbcd6b3e586c9927b6058ef4d529
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Sat Apr 11 05:34:20 2009 +0000

    * stdio-common/printf.h (struct printf_info): Add user element.
    
        New types printf_arginfo_size_function, printf_va_arg_function.
        Declare register_printf_specifier, register_printf_modifier,
        register_printf_type.
        * stdio-common/printf-parse.h (struct printf_spec): Add size element.
        (union printf_arg): Add pa_user element.
        Adjust __printf_arginfo_table type.
        Add __printf_va_arg_table, __printf_modifier_table,
        __handle_registered_modifier_mb, and __handle_registered_modifier_wc
        declarations.
        * stdio-common/printf-parsemb.c: Recognize registered modifiers.
        If registered arginfo call failed try normal specifier.
        * stdio-common/printf-prs.c: Pass additional parameter to arginfo
        function.
        * stdio-common/Makefile (routines): Add reg-modifier and reg-type.
        * stdio-common/Versions: Export register_printf_modifier,
        register_printf_type, and register_printf_specifier for GLIBC_2.10.
        * stdio-common/reg-modifier.c: New file.
        * stdio-common/reg-type.c: New file.
        * stdio-common/reg-printf.c (__register_printf_specifier): New
        function.  Mostly the old __register_printf_function function but
        uses locking and type of third parameter changed.
        (__register_printf_function): Implement using
        __register_printf_specifier.
        * stdio-common/vfprintf.c (vfprintf): Collect argument sizes in
        calls to arginfo functions.  Allocate enough memory for user-defined
        types.  Call new va_arg functions to get user-defined types.
        Try installed handlers even for existing format specifiers first.