Message ID | 20180307193205.4751-7-zackw@panix.com |
---|---|
State | New |
Headers | show |
Series | Use more flags parameters instead of global bits in stdio | expand |
On 03/07/2018 08:32 PM, Zack Weinberg wrote: > +/* __vsyslog_internal uses the same mode_flags bits as > + __v*printf_internal; see libio/libioP.h. */ > +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list ap, > + unsigned int mode_flags) > + __attribute__ ((__format__ (__printf__, 2, 0))); I'm surprised that this doesn't need attribute_hidden or libc_hidden_proto to avoid new PLT calls. Rest looks okay. Thanks, Florian
On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 03/07/2018 08:32 PM, Zack Weinberg wrote: > >> +/* __vsyslog_internal uses the same mode_flags bits as >> + __v*printf_internal; see libio/libioP.h. */ >> +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list >> ap, >> + unsigned int mode_flags) >> + __attribute__ ((__format__ (__printf__, 2, 0))); > > > I'm surprised that this doesn't need attribute_hidden or libc_hidden_proto > to avoid new PLT calls. That's only needed for functions that will be called _both_ from inside and outside glibc. This function is only ever called from inside glibc, so it doesn't appear in any Versions files and it's hidden by default. zw
On 03/13/2018 01:39 PM, Zack Weinberg wrote: > On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 03/07/2018 08:32 PM, Zack Weinberg wrote: >> >>> +/* __vsyslog_internal uses the same mode_flags bits as >>> + __v*printf_internal; see libio/libioP.h. */ >>> +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list >>> ap, >>> + unsigned int mode_flags) >>> + __attribute__ ((__format__ (__printf__, 2, 0))); >> >> >> I'm surprised that this doesn't need attribute_hidden or libc_hidden_proto >> to avoid new PLT calls. > > That's only needed for functions that will be called _both_ from > inside and outside glibc. This function is only ever called from > inside glibc, so it doesn't appear in any Versions files and it's > hidden by default. Some architectures will still use indirect calls without attribute_hidden, so please add it. The existing tests do not catch this reliably unfortunately. Thanks, Florian
On Tue, Mar 13, 2018 at 8:43 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 03/13/2018 01:39 PM, Zack Weinberg wrote: >> >> On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com> >> wrote: >>> >>> On 03/07/2018 08:32 PM, Zack Weinberg wrote: >>> >>>> +/* __vsyslog_internal uses the same mode_flags bits as >>>> + __v*printf_internal; see libio/libioP.h. */ >>>> +extern void __vsyslog_internal (int pri, const char *fmt, >>>> __gnuc_va_list >>>> ap, >>>> + unsigned int mode_flags) >>>> + __attribute__ ((__format__ (__printf__, 2, 0))); >>> >>> >>> >>> I'm surprised that this doesn't need attribute_hidden or >>> libc_hidden_proto >>> to avoid new PLT calls. >> >> >> That's only needed for functions that will be called _both_ from >> inside and outside glibc. This function is only ever called from >> inside glibc, so it doesn't appear in any Versions files and it's >> hidden by default. > > > Some architectures will still use indirect calls without attribute_hidden, > so please add it. The existing tests do not catch this reliably > unfortunately. Can you be more specific? This will affect all of the other new __*_internal functions added in this patchset, so I need to know how to be sure I got it right. Also, this seems like something we should find a way to automate if at all possible. zw
On 03/13/2018 02:37 PM, Zack Weinberg wrote: > On Tue, Mar 13, 2018 at 8:43 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 03/13/2018 01:39 PM, Zack Weinberg wrote: >>> >>> On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com> >>> wrote: >>>> >>>> On 03/07/2018 08:32 PM, Zack Weinberg wrote: >>>> >>>>> +/* __vsyslog_internal uses the same mode_flags bits as >>>>> + __v*printf_internal; see libio/libioP.h. */ >>>>> +extern void __vsyslog_internal (int pri, const char *fmt, >>>>> __gnuc_va_list >>>>> ap, >>>>> + unsigned int mode_flags) >>>>> + __attribute__ ((__format__ (__printf__, 2, 0))); >>>> >>>> >>>> >>>> I'm surprised that this doesn't need attribute_hidden or >>>> libc_hidden_proto >>>> to avoid new PLT calls. >>> >>> >>> That's only needed for functions that will be called _both_ from >>> inside and outside glibc. This function is only ever called from >>> inside glibc, so it doesn't appear in any Versions files and it's >>> hidden by default. >> >> >> Some architectures will still use indirect calls without attribute_hidden, >> so please add it. The existing tests do not catch this reliably >> unfortunately. > > Can you be more specific? This will affect all of the other new > __*_internal functions added in this patchset, so I need to know how > to be sure I got it right. Also, this seems like something we should > find a way to automate if at all possible. Consider this code: $ cat call.c int external (void) ATTR; int call (void) { return external () + 1; } With default visibility, GCC 7 produces: $ ppc64-linux-gnu-gcc -m32 -fPIC -c -O2 -DATTR= call.c && ppc64-linux-gnu-objdump -d --reloc call.o call.o: file format elf32-powerpc Disassembly of section .text: 00000000 <call>: 0: 94 21 ff e0 stwu r1,-32(r1) 4: 7c 08 02 a6 mflr r0 8: 42 9f 00 05 bcl 20,4*cr7+so,c <call+0xc> c: 93 c1 00 18 stw r30,24(r1) 10: 7f c8 02 a6 mflr r30 14: 90 01 00 24 stw r0,36(r1) 18: 3f de 00 00 addis r30,r30,0 1a: R_PPC_REL16_HA .got2+0x800e 1c: 3b de 00 00 addi r30,r30,0 1e: R_PPC_REL16_LO .got2+0x8012 20: 48 00 00 01 bl 20 <call+0x20> 20: R_PPC_PLTREL24 external+0x8000 24: 80 01 00 24 lwz r0,36(r1) 28: 83 c1 00 18 lwz r30,24(r1) 2c: 38 21 00 20 addi r1,r1,32 30: 38 63 00 01 addi r3,r3,1 34: 7c 08 03 a6 mtlr r0 38: 4e 80 00 20 blr With hidden visibility, we get instead: $ ppc64-linux-gnu-gcc -m32 -fPIC -c -O2 -DATTR='__attribute ((visibility ("hidden")))' call.c && ppc64-linux-gnu-objdump -d --reloc call.o call.o: file format elf32-powerpc Disassembly of section .text: 00000000 <call>: 0: 94 21 ff e0 stwu r1,-32(r1) 4: 7c 08 02 a6 mflr r0 8: 93 c1 00 18 stw r30,24(r1) c: 90 01 00 24 stw r0,36(r1) 10: 48 00 00 01 bl 10 <call+0x10> 10: R_PPC_LOCAL24PC external 14: 80 01 00 24 lwz r0,36(r1) 18: 83 c1 00 18 lwz r30,24(r1) 1c: 38 21 00 20 addi r1,r1,32 20: 38 63 00 01 addi r3,r3,1 24: 7c 08 03 a6 mtlr r0 28: 4e 80 00 20 blr The linker may have some optimization to eliminate the PLT indirection (blinding the localplt test), but it cannot get rid of the other unnecessary instructions. Does this example help? Thanks, Florian
On Tue, Mar 13, 2018 at 9:50 AM, Florian Weimer <fweimer@redhat.com> wrote: >>> Some architectures will still use indirect calls without >>> attribute_hidden, >>> so please add it. The existing tests do not catch this reliably >>> unfortunately. >> >> >> Can you be more specific? This will affect all of the other new >> __*_internal functions added in this patchset, so I need to know how >> to be sure I got it right. Also, this seems like something we should >> find a way to automate if at all possible. ... > The linker may have some optimization to eliminate the PLT indirection > (blinding the localplt test), but it cannot get rid of the other unnecessary > instructions. > > Does this example help? I believe I understand the problem now, thank you. And we can't/don't use -fvisibility=hidden because then we would have to annotate all of the _public_ symbols with visibility default, yes? zw
On 03/13/2018 03:11 PM, Zack Weinberg wrote: > I believe I understand the problem now, thank you. And we can't/don't > use -fvisibility=hidden because then we would have to annotate all of > the_public_ symbols with visibility default, yes? That, and on some architectures, it is impossible to call IFUNCs with hidden visibility declared to the compiler. Thanks, Florian
diff --git a/include/sys/syslog.h b/include/sys/syslog.h index 3be3189ed1..459ca70746 100644 --- a/include/sys/syslog.h +++ b/include/sys/syslog.h @@ -1,11 +1,20 @@ +#ifndef _LIBC_SYS_SYSLOG_H +#define _LIBC_SYS_SYSLOG_H 1 #include <misc/sys/syslog.h> - #ifndef _ISOMAC + libc_hidden_proto (syslog) libc_hidden_proto (vsyslog) +/* __vsyslog_internal uses the same mode_flags bits as + __v*printf_internal; see libio/libioP.h. */ +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list ap, + unsigned int mode_flags) + __attribute__ ((__format__ (__printf__, 2, 0))); + extern void __vsyslog_chk (int __pri, int __flag, const char *__fmt, __gnuc_va_list __ap) __attribute__ ((__format__ (__printf__, 3, 0))); -libc_hidden_proto (__vsyslog_chk) -#endif + +#endif /* _ISOMAC */ +#endif /* syslog.h */ diff --git a/misc/syslog.c b/misc/syslog.c index 644dbe80ec..eb1283a604 100644 --- a/misc/syslog.c +++ b/misc/syslog.c @@ -53,7 +53,7 @@ static char sccsid[] = "@(#)syslog.c 8.4 (Berkeley) 3/18/94"; #include <stdarg.h> -#include <libio/iolibio.h> +#include <libio/libioP.h> #include <math_ldbl_opt.h> #include <kernel-features.h> @@ -114,24 +114,39 @@ __syslog(int pri, const char *fmt, ...) va_list ap; va_start(ap, fmt); - __vsyslog_chk(pri, -1, fmt, ap); + __vsyslog_internal(pri, fmt, ap, 0); va_end(ap); } ldbl_hidden_def (__syslog, syslog) ldbl_strong_alias (__syslog, syslog) +void +__vsyslog(int pri, const char *fmt, va_list ap) +{ + __vsyslog_internal(pri, fmt, ap, 0); +} +ldbl_hidden_def (__vsyslog, vsyslog) +ldbl_weak_alias (__vsyslog, vsyslog) + void __syslog_chk(int pri, int flag, const char *fmt, ...) { va_list ap; va_start(ap, fmt); - __vsyslog_chk(pri, flag, fmt, ap); + __vsyslog_internal(pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0); va_end(ap); } void __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap) +{ + __vsyslog_internal(pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0); +} + +void +__vsyslog_internal(int pri, const char *fmt, va_list ap, + unsigned int mode_flags) { struct tm now_tm; time_t now; @@ -216,10 +231,7 @@ __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap) /* We have the header. Print the user's format into the buffer. */ - if (flag == -1) - vfprintf (f, fmt, ap); - else - __vfprintf_chk (f, flag, fmt, ap); + __vfprintf_internal (f, fmt, ap, mode_flags); /* Close the memory stream; this will finalize the data into a malloc'd buffer in BUF. */ @@ -316,15 +328,6 @@ __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap) if (buf != failbuf) free (buf); } -libc_hidden_def (__vsyslog_chk) - -void -__vsyslog(int pri, const char *fmt, va_list ap) -{ - __vsyslog_chk (pri, -1, fmt, ap); -} -ldbl_hidden_def (__vsyslog, vsyslog) -ldbl_weak_alias (__vsyslog, vsyslog) static struct sockaddr_un SyslogAddr; /* AF_UNIX address of local logger */ diff --git a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c index 7e8af80d63..f00eb55eb5 100644 --- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c +++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c @@ -842,7 +842,7 @@ attribute_compat_text_section __nldbl___vsyslog_chk (int pri, int flag, const char *fmt, va_list ap) { set_no_long_double (); - __vsyslog_chk (pri, flag, fmt, ap); + __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0); clear_no_long_double (); } libc_hidden_def (__nldbl___vsyslog_chk)