[6/9] Add __vsyslog_internal, with same flags as __v*printf_internal.
Commit Message
__nldbl___vsyslog_chk will ultimately want to pass PRINTF_LDBL_IS_DBL
down to __vfprintf_internal *as well as* possibly setting PRINTF_FORTIFY.
To make that possible, we need a __vsyslog_internal that takes the
same flags as printf. The code in misc/syslog.c does also get a
little simpler.
* misc/syslog.c: Include libioP.h, not iolibio.h.
(__vsyslog_internal): New function with the former body of
__vsyslog_chk; takes mode_flags argument same as
__v*printf_internal. Call __vfprintf_internal directly.
(__vsyslog_chk): Now a wrapper around __vsyslog_internal.
Remove libc_hidden_def.
(__syslog, __syslog_chk): Use __vsyslog_internal.
(__vsyslog): Move to just below __syslog. Use __vsyslog_internal.
* include/sys/syslog.h: Add multiple inclusion guard.
Add prototype for __vsyslog_internal.
Remove libc_hidden_proto for __vsyslog_chk.
* sysdeps/ieee754/ldbl-opt/nldbl-compat.c (__nldbl___vsyslog_chk):
Use __vsyslog_internal.
---
include/sys/syslog.h | 15 +++++++++++---
misc/syslog.c | 35 ++++++++++++++++++---------------
sysdeps/ieee754/ldbl-opt/nldbl-compat.c | 2 +-
3 files changed, 32 insertions(+), 20 deletions(-)
Comments
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
@@ -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 */
@@ -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 */
@@ -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)