diff mbox

For BZ #17328, mark __errno_location with __attribute__((returns_nonnull)) for gcc >=4.9.0

Message ID CALoOobOuAEpw+zxRrrDyHB7UVbAZMzreXqpujzZOWNLS7+aRUA@mail.gmail.com
State Changes Requested
Headers show

Commit Message

Paul Pluzhnikov Feb. 28, 2015, 10:03 p.m. UTC
Greetings,

Tested on Linux/x86_64 with gcc-4.8 and 5.0.


2015-02-28  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #17328]
        * include/errno.h (__errno_locaiton): Mark returns_nonnull.
        * sysdeps/mach/hurd/bits/errno.h (__errno_location): Likewise.
        * sysdeps/unix/sysv/linux/alpha/bits/errno.h (__errno_location):
        Likewise.
        * sysdeps/unix/sysv/linux/bits/errno.h (__errno_location): Likewise
        * sysdeps/unix/sysv/linux/hppa/bits/errno.h (__errno_location):
        Likewise.
        * sysdeps/unix/sysv/linux/mips/bits/errno.h (__errno_location):
        Likewise.
        * sysdeps/unix/sysv/linux/sparc/bits/errno.h (__errno_location):
        Likewise.

--
Paul Pluzhnikov

Comments

Rich Felker March 1, 2015, 1:17 a.m. UTC | #1
On Sat, Feb 28, 2015 at 02:03:11PM -0800, Paul Pluzhnikov wrote:
> Greetings,
> 
> Tested on Linux/x86_64 with gcc-4.8 and 5.0.

No objection, but I'm curious if there's any practical benefit of
this. I can't think of many situations where knowledge that
&errno!=NULL would assist the compiler in optimizing or diagnostics.

Rich
Paul Pluzhnikov March 1, 2015, 1:54 a.m. UTC | #2
+cc zackw@panix.com

On Sat, Feb 28, 2015 at 5:17 PM, Rich Felker <dalias@libc.org> wrote:
> On Sat, Feb 28, 2015 at 02:03:11PM -0800, Paul Pluzhnikov wrote:
>> Greetings,
>>
>> Tested on Linux/x86_64 with gcc-4.8 and 5.0.
>
> No objection, but I'm curious if there's any practical benefit of
> this. I can't think of many situations where knowledge that
> &errno!=NULL would assist the compiler in optimizing or diagnostics.

Hmm, maybe you are right.

I was reading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58689 and
threads linked from it, and it really looks like not much is gained
inside (or outside) GLIBC -- it's not like anyone ever checks whether
__errno_location() returned NULL.

Zack,

Do you have any additional motivation for filing
https://sourceware.org/bugzilla/show_bug.cgi?id=17328

Thanks!
Mike Frysinger March 1, 2015, 7:10 p.m. UTC | #3
On 28 Feb 2015 14:03, Paul Pluzhnikov wrote:
> +#  if __GNUC_PREREQ(4,9)
> +     __attribute__((returns_nonnull))
> +#  endif

repeating this everywhere is ugly.  update misc/sys/cdefs.h instead to create a 
dedicated attribute define based on version, and then apply that define to all 
the headers w/out any version checks.
-mike
Joseph Myers March 1, 2015, 10:22 p.m. UTC | #4
On Sun, 1 Mar 2015, Mike Frysinger wrote:

> On 28 Feb 2015 14:03, Paul Pluzhnikov wrote:
> > +#  if __GNUC_PREREQ(4,9)
> > +     __attribute__((returns_nonnull))
> > +#  endif
> 
> repeating this everywhere is ugly.  update misc/sys/cdefs.h instead to create a 
> dedicated attribute define based on version, and then apply that define to all 
> the headers w/out any version checks.

Also, returns_nonnull is in the user namespace - this should have shown up 
in regression testing as conform/ test failures.  You need to use 
__returns_nonnull__.  And remember the space before the first '('.
Zack Weinberg March 2, 2015, 10:23 p.m. UTC | #5
On Sat, Feb 28, 2015 at 8:54 PM, Paul Pluzhnikov <ppluzhnikov@gmail.com> wrote:
> On Sat, Feb 28, 2015 at 5:17 PM, Rich Felker <dalias@libc.org> wrote:
>>
>> No objection, but I'm curious if there's any practical benefit of
>> this. I can't think of many situations where knowledge that
>> &errno!=NULL would assist the compiler in optimizing or diagnostics.
>
> Hmm, maybe you are right.
>
> I was reading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58689 and
> threads linked from it, and it really looks like not much is gained
> inside (or outside) GLIBC -- it's not like anyone ever checks whether
> __errno_location() returned NULL.
>
> Zack, do you have any additional motivation for filing
> https://sourceware.org/bugzilla/show_bug.cgi?id=17328

My original motivation was to improve code generation with
-fsanitize=undefined, which, among other things, instruments *every
use of errno* with a check to ensure that the pointer returned by
__errno_location is non-null.  For instance, the admittedly silly code

    #include <errno.h>
    int get_errno (void) { return errno; }

with GCC 4.9 and glibc 2.19, preprocesses to

    extern int *__errno_location (void) __attribute__ ((__nothrow__ ,
__leaf__)) __attribute__ ((__const__));
    int get_errno (void) { return (*__errno_location ()); }

and compiling with -O2 -fsanitize=undefined produces (.cfi chatter elided):

get_errno:
        subq    $24, %rsp
        call    __errno_location
        testq   %rax, %rax
        je      .L5
.L2:
        movl    (%rax), %eax
        addq    $24, %rsp
        ret
.L5:
        xorl    %esi, %esi
        movl    $.Lubsan_data0, %edi
        movq    %rax, 8(%rsp)
        call    __ubsan_handle_type_mismatch
        movq    8(%rsp), %rax
        jmp     .L2

clang 3.5 does essentially the same thing.  However, as is, adding
__attribute__((returns_nonnull)) doesn't improve matters, because
neither gcc nor clang will optimize out the checks based on that.  I
did file a GCC bug for that, but people were reluctant to make it
smarter for fear of losing necessary checks:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62307

... so I gotta wonder if it mightn't be a better use of people's time
to make errno a proper thread-local variable instead.  What's blocking
that?

zw
Rich Felker March 2, 2015, 10:39 p.m. UTC | #6
On Mon, Mar 02, 2015 at 05:23:58PM -0500, Zack Weinberg wrote:
> On Sat, Feb 28, 2015 at 8:54 PM, Paul Pluzhnikov <ppluzhnikov@gmail.com> wrote:
> > On Sat, Feb 28, 2015 at 5:17 PM, Rich Felker <dalias@libc.org> wrote:
> >>
> >> No objection, but I'm curious if there's any practical benefit of
> >> this. I can't think of many situations where knowledge that
> >> &errno!=NULL would assist the compiler in optimizing or diagnostics.
> >
> > Hmm, maybe you are right.
> >
> > I was reading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58689 and
> > threads linked from it, and it really looks like not much is gained
> > inside (or outside) GLIBC -- it's not like anyone ever checks whether
> > __errno_location() returned NULL.
> >
> > Zack, do you have any additional motivation for filing
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17328
> 
> My original motivation was to improve code generation with
> -fsanitize=undefined, which, among other things, instruments *every
> use of errno* with a check to ensure that the pointer returned by
> __errno_location is non-null.  For instance, the admittedly silly code

This is indeed ugly, but I'm not necessarily convinced that improving
code optimization with ubsan is a worthwhile use of human effort or
code complexity. Anyone else have opinions on this?

> clang 3.5 does essentially the same thing.  However, as is, adding
> __attribute__((returns_nonnull)) doesn't improve matters, because
> neither gcc nor clang will optimize out the checks based on that.  I
> did file a GCC bug for that, but people were reluctant to make it
> smarter for fear of losing necessary checks:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62307

This seems reasonable if ubsan is actually intended only for debugging
and/or hardcode expensive hardening, which is my impression.

> .... so I gotta wonder if it mightn't be a better use of people's time
> to make errno a proper thread-local variable instead.  What's blocking
> that?

I seem to recall there was some issue with buggy programs having
conflicting symbol definitions of errno as a non-TLS symbol. But I
don't see why something like the following wouldn't work:

extern __thread int __errno_tls;
#define errno __errno_tls

Presumably IE-model could even be used for references since there
would always be a definition. I don't know if there are any cases
where this would pessimize code generation or not (versus the current
__errno_location approach).

Rich
Roland McGrath March 2, 2015, 10:46 p.m. UTC | #7
> ... so I gotta wonder if it mightn't be a better use of people's time
> to make errno a proper thread-local variable instead.  What's blocking
> that?

That's what it already is internally.  It's never been part of the public
API or ABI.  I think that's primarily because we started using TLS
internally for errno when TLS use and support was still quite new, and we
did not want to complicate matters for users.  

Nowadays with some appropriate compiler-version checks it would probably be
fine to let users see the errno symbol.  But I'm still hesitant to make the
change by default, because it would make approximately every single
application everywhere a participant in the ELF TLS ABI.  I tend to think
it's better that a program where no TLS variables were used explicitly does
not use any of the TLS ABI at all.

Something that we could consider as a middle road is exporting errno with a
proper symbol version (it's currently GLIBC_PRIVATE, which nothing residing
outside the libc source tree should ever use) and then having errno.h do:

	extern __thread int errno;
	#define errno (*__errno_location ())

Then a savvy user could '#undef errno' to use the TLS version.  We could
also make the macro conditional on some predefine that -fsanitize=undefined
provides.  I would be fine with doing something like this now, though for
at least the next release cycle I would still argue against making the TLS
declaration be what programs compiled in normal ways see by default.


Thanks,
Roland
Zack Weinberg March 2, 2015, 10:51 p.m. UTC | #8
On Mon, Mar 2, 2015 at 5:39 PM, Rich Felker <dalias@libc.org> wrote:
> On Mon, Mar 02, 2015 at 05:23:58PM -0500, Zack Weinberg wrote:
>> My original motivation was to improve code generation with
>> -fsanitize=undefined, which, among other things, instruments *every
>> use of errno* with a check to ensure that the pointer returned by
>> __errno_location is non-null.  For instance, the admittedly silly code
>
> This is indeed ugly, but I'm not necessarily convinced that improving
> code optimization with ubsan is a worthwhile use of human effort or
> code complexity. Anyone else have opinions on this?

For what it's worth, the hope was that ubsan would be sufficiently
accurate about its instrumentation that I could use it as a poor man's
tool for identifying places where the program couldn't be proven not
to have undefined behavior.  In a real program that did a lot of
low-level work and therefore had to tweak errno all the time, though,
the noise level was just too high.  I should probably suck it up and
learn how to use a proper correctness prover.

zw
Joseph Myers March 3, 2015, 3:42 a.m. UTC | #9
On Mon, 2 Mar 2015, Roland McGrath wrote:

> Then a savvy user could '#undef errno' to use the TLS version.  We could

I don't think we should encourage users to #undef errno (something 
undefined in ISO C, and that wouldn't work with current glibc).
Mike Frysinger March 3, 2015, 5:07 a.m. UTC | #10
On 02 Mar 2015 17:39, Rich Felker wrote:
> On Mon, Mar 02, 2015 at 05:23:58PM -0500, Zack Weinberg wrote:
> > .... so I gotta wonder if it mightn't be a better use of people's time
> > to make errno a proper thread-local variable instead.  What's blocking
> > that?
> 
> I seem to recall there was some issue with buggy programs having
> conflicting symbol definitions of errno as a non-TLS symbol. But I
> don't see why something like the following wouldn't work:
> 
> extern __thread int __errno_tls;
> #define errno __errno_tls

this problem already came & went.  when glibc was rolling out tls support and 
started marking errno as such, there were packages that failed like so:

/usr/lib/gcc/i686-pc-linux-gnu/3.4.5/../../../../i686-pc-linux-gnu/bin/ld: 
errno: TLS definition in /lib/libc.so.6 section .tbss mismatches non-TLS reference in libqemu.a(helper2.o)

and they've all been fixed since -- we migrated packages to errno.h instead of 
open coding their own extern.
-mike
Rich Felker March 3, 2015, 5:13 a.m. UTC | #11
On Tue, Mar 03, 2015 at 03:42:48AM +0000, Joseph Myers wrote:
> On Mon, 2 Mar 2015, Roland McGrath wrote:
> 
> > Then a savvy user could '#undef errno' to use the TLS version.  We could
> 
> I don't think we should encourage users to #undef errno (something 
> undefined in ISO C, and that wouldn't work with current glibc).

Agree. This is a really nasty idea. If such a feature is to be
exposed, it should be via a feature test macro, not UB hacks.

Rich
Florian Weimer March 3, 2015, 10:48 a.m. UTC | #12
On 03/02/2015 11:46 PM, Roland McGrath wrote:

> Something that we could consider as a middle road is exporting errno with a
> proper symbol version (it's currently GLIBC_PRIVATE, which nothing residing
> outside the libc source tree should ever use) and then having errno.h do:
> 
> 	extern __thread int errno;

Can we make the TLS offset part of the ABI instead and somehow teach GCC
about it?  Then there would at least be an unquestionable improvement.

Right now, we have (in PIC code):

	call	__errno_location@PLT
	movl	(%rax), %eax

This turns into:

	data32
	leaq	errcode@tlsgd(%rip), %rdi
	data32
	data32
	rex64
	call	__tls_get_addr@PLT
	movl	(%rax), %eax

errno is mostly used on error paths, so this just bloats the code and is
probably even slower, too.
Rich Felker March 3, 2015, 2:54 p.m. UTC | #13
On Tue, Mar 03, 2015 at 11:48:43AM +0100, Florian Weimer wrote:
> On 03/02/2015 11:46 PM, Roland McGrath wrote:
> 
> > Something that we could consider as a middle road is exporting errno with a
> > proper symbol version (it's currently GLIBC_PRIVATE, which nothing residing
> > outside the libc source tree should ever use) and then having errno.h do:
> > 
> > 	extern __thread int errno;
> 
> Can we make the TLS offset part of the ABI instead and somehow teach GCC
> about it?  Then there would at least be an unquestionable improvement.

I would really advise against this. Fixed offsets are really nasty
lock-in and don't save much. For the non-PIE main program they save
basically nothing versus a runtime-variable offset. For shared
libraries or PIE, there is some minor saving especially on archs
(32-bit x86) that need to load a GOT register to load the offset;
however, right now such archs are already loading a GOT pointer to
make the function call to __errno_location, so this is not a new cost.

> Right now, we have (in PIC code):
> 
> 	call	__errno_location@PLT
> 	movl	(%rax), %eax
> 
> This turns into:
> 
> 	data32
> 	leaq	errcode@tlsgd(%rip), %rdi
> 	data32
> 	data32
> 	rex64
> 	call	__tls_get_addr@PLT
> 	movl	(%rax), %eax
> 
> errno is mostly used on error paths, so this just bloats the code and is
> probably even slower, too.

__tls_get_addr is not needed if you use the initial-exec model. And if
you're using GD with TLSDESC rather than the old GD model, it's also
not this ugly/bloated.

However I still prefer just leaving things alone with
__errno_location. Like you say it's tiny and its internals can be
optimized heavily if needed (e.g. using a constant offset like it is
now). In this case the constant is encoded in libc.so so it can change
from one version to another rather than being permanent ABI lock-in.

Rich
diff mbox

Patch

diff --git a/include/errno.h b/include/errno.h
index 7df41df..a7615a4 100644
--- a/include/errno.h
+++ b/include/errno.h
@@ -40,6 +40,9 @@  extern __thread int errno attribute_tls_model_ie;
 
 # ifndef __ASSEMBLER__
 extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#  if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#  endif
 #  if RTLD_PRIVATE_ERRNO
      attribute_hidden
 #  endif
diff --git a/sysdeps/mach/hurd/bits/errno.h b/sysdeps/mach/hurd/bits/errno.h
index d20ffe6..5e84174 100644
--- a/sysdeps/mach/hurd/bits/errno.h
+++ b/sysdeps/mach/hurd/bits/errno.h
@@ -317,7 +317,11 @@  typedef enum __error_t_codes error_t;
 
 /* Return the current thread's location for `errno'.
    The syntax of this function allows redeclarations like `int errno'.  */
-extern int *__errno_location (void) __THROW __attribute__ ((__const__));
+extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#endif
+;
 
 #define errno			(*__errno_location ())
 
diff --git a/sysdeps/unix/sysv/linux/alpha/bits/errno.h b/sysdeps/unix/sysv/linux/alpha/bits/errno.h
index 6e8c5cd..02dd301 100644
--- a/sysdeps/unix/sysv/linux/alpha/bits/errno.h
+++ b/sysdeps/unix/sysv/linux/alpha/bits/errno.h
@@ -46,7 +46,11 @@ 
 
 # ifndef __ASSEMBLER__
 /* Function to get address of global `errno' variable.  */
-extern int *__errno_location (void) __THROW __attribute__ ((__const__));
+extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#  if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#  endif
+;
 
 #  if !defined _LIBC || defined _LIBC_REENTRANT
 /* When using threads, errno is a per-thread value.  */
diff --git a/sysdeps/unix/sysv/linux/bits/errno.h b/sysdeps/unix/sysv/linux/bits/errno.h
index b00c621..a982669 100644
--- a/sysdeps/unix/sysv/linux/bits/errno.h
+++ b/sysdeps/unix/sysv/linux/bits/errno.h
@@ -47,7 +47,11 @@ 
 
 # ifndef __ASSEMBLER__
 /* Function to get address of global `errno' variable.  */
-extern int *__errno_location (void) __THROW __attribute__ ((__const__));
+extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#  if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#  endif
+;
 
 #  if !defined _LIBC || defined _LIBC_REENTRANT
 /* When using threads, errno is a per-thread value.  */
diff --git a/sysdeps/unix/sysv/linux/hppa/bits/errno.h b/sysdeps/unix/sysv/linux/hppa/bits/errno.h
index ad8f0b2..a46c752 100644
--- a/sysdeps/unix/sysv/linux/hppa/bits/errno.h
+++ b/sysdeps/unix/sysv/linux/hppa/bits/errno.h
@@ -47,7 +47,11 @@ 
 
 # ifndef __ASSEMBLER__
 /* Function to get address of global `errno' variable.  */
-extern int *__errno_location (void) __THROW __attribute__ ((__const__));
+extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#  if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#  endif
+;
 
 #  if !defined _LIBC || defined _LIBC_REENTRANT
 /* When using threads, errno is a per-thread value.  */
diff --git a/sysdeps/unix/sysv/linux/mips/bits/errno.h b/sysdeps/unix/sysv/linux/mips/bits/errno.h
index 26e5a77..9da9bed 100644
--- a/sysdeps/unix/sysv/linux/mips/bits/errno.h
+++ b/sysdeps/unix/sysv/linux/mips/bits/errno.h
@@ -46,7 +46,11 @@ 
 
 # ifndef __ASSEMBLER__
 /* Function to get address of global `errno' variable.  */
-extern int *__errno_location (void) __THROW __attribute__ ((__const__));
+extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#  if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#  endif
+;
 
 #  if !defined _LIBC || defined _LIBC_REENTRANT
 /* When using threads, errno is a per-thread value.  */
diff --git a/sysdeps/unix/sysv/linux/sparc/bits/errno.h b/sysdeps/unix/sysv/linux/sparc/bits/errno.h
index 2fbafac..ce2d32e 100644
--- a/sysdeps/unix/sysv/linux/sparc/bits/errno.h
+++ b/sysdeps/unix/sysv/linux/sparc/bits/errno.h
@@ -46,7 +46,11 @@ 
 
 # ifndef __ASSEMBLER__
 /* Function to get address of global `errno' variable.  */
-extern int *__errno_location (void) __THROW __attribute__ ((__const__));
+extern int *__errno_location (void) __THROW __attribute__ ((__const__))
+#  if __GNUC_PREREQ(4,9)
+     __attribute__((returns_nonnull))
+#  endif
+;
 
 #  if !defined _LIBC || defined _LIBC_REENTRANT
 /* When using threads, errno is a per-thread value.  */