RFC: first stab at getting rid of libc_hidden_proto() forest

[Khem Raj]

-----BEGIN PGP SIGNED MESSAGE-----

Hash: SHA1



Denys Vlasenko wrote:
> Hi,
> 
> I want to make clear that I do not intend to apply this patch
> without getting a positive review. I don't want people to
> start thinking "omg, vda can commit this %&&$ and mess up
> the tree"!
> 
> I won't. It's really an RFC.
> 
> What it does:
> 
> I find the need of having libc_hidden_proto(fprintf)
> anywhere you want to use fprintf efficiently (i.e.
> to call it directly, not thru GOT/PLT) to be unmaintainable.
> 
> First, it clogs up the source.
> 
> Second, it's too easy to forget to do it, and you get
> suboptimal code, silently.
> 
> Third, sometimes it gets even more ugly: you need to know
> _implementation details_ and dance with #defines like this:
> 
> #ifdef __UCLIBC_HAS_XLOCALE__
> libc_hidden_proto(__ctype_b_loc)
> #elif __UCLIBC_HAS_CTYPE_TABLES__
> libc_hidden_proto(__ctype_b)
> #endif
> 
> Fifth: it needs to be repeated in every .c file where you want
> to use e.g. ctype.h functions!
> 
> Obvious solution of adding libc_hidden_proto's to .h files
> does not work so well since we'd like to not pollute them with
> machinery which doesn't belong to public interface.
> 
> In the attached patch, I chose to create a "master include file"
> which includes almost all other headers; and then marks
> all exported functions(variables) with libc_hidden_proto.
> The file is called uClibc_libc.h.
> 
> Currently it does all #includes first, and all libc_hidden_proto's
> next. Maybe it makes sense to group them by header instead:
> #include <stdio.h>
> libc_hidden_proto(printf)
> libc_hidden_proto(sprintf)
> ...
> #include <stdlib.h>
> ...
> 
> Anyway. It is meant to be used this way: you include only this file,
> and you are getting (almost) all headers included for you,
> and all exported functions libc_hidden_proto'ed.
> You don't need to do it again and again in each .c file.
> Lots and lots of #includes and libc_hidden_proto's can be removed;
> and a lot of churn in adding/removing thme in future is eliminated
> too.
> 
> The upside is less crowded .c files.
> 
> The downside is a somewhat longer build time.
> 
> The patch demonstrates this approach by performing this surgery on
> a handful of files, namely:
> libc/inet/getaddrinfo.c
> libc/inet/ifaddrs.c
> libc/inet/resolv.c
> libc/inet/rpc/pmap_rmt.c
> libc/inet/rpc/rcmd.c
> libc/misc/locale/locale.c
> libc/misc/syslog/syslog.c
> libc/misc/time/time.c
> libc/misc/wchar/wchar.c
> libc/misc/wordexp/wordexp.c
> libc/stdio/_scanf.c
> libc/stdio/_vfprintf.c
> 
> Of these, only libc/misc/locale/locale.c required a little more
> work to make it build, and a small fix in libc/inet/netlinkaccess.h
> was needed too.
> 
> I tried building uclibc with several .configs and fixed all fallout;
> then I compared object files one-by-one before and after the patch.
> 
> They seem to be identical except for libc/misc/time/tzset.os.
> I tracked the difference to this code:
> 
> void tzset(void)
> {
>     _time_tzset((time(NULL)) < new_rule_starts);
> }
> 
> Before the patch, we were forgetting to treat time()
> with libc_hidden_proto(time) macro, and it was called
> thru indirection machinery:
> 
>  00000000 <__GI_tzset>:
> -   0:  53                      push   %ebx
> -   1:  e8 00 00 00 00          call   6 <__GI_tzset+0x6>
> -   6:  5b                      pop    %ebx
> -   7:  81 c3 03 00 00 00       add    $0x3,%ebx
> -                       9: R_386_GOTPC  _GLOBAL_OFFSET_TABLE_
> -   d:  6a 00                   push   $0x0
> -   f:  e8 fc ff ff ff          call   10 <__GI_tzset+0x10>
> -                       10: R_386_PLT32 time
> 
> +   0:  6a 00                   push   $0x0
> +   2:  e8 fc ff ff ff          call   3 <__GI_tzset+0x3>
> +                       3: R_386_PC32   __GI_time
> 
> As you see, after the patch it was automatically using
> faster calling method.
> 
> So I found one missed optimization by editing 12 files only.
> There are probably more of these.
> 
> Please let me know what do you think about this approach.

inet_ntoa_r() has been visible to users and making it static will break applications. One for sure I know is curl which uses this function.

- -- 
Khem Raj
MontaVista Software Inc.
-----BEGIN PGP SIGNATURE-----

Version: GnuPG v1.4.7 (MingW32)

Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org



iD8DBQFIECQrUjbQJxVzeZQRAso3AJ9fm4uCLkIUN5Dy0jSyep0SFcbiVQCeJzTK

PYZLjUixBP7gKrQQRLOLEZA=

=foRJ

-----END PGP SIGNATURE-----