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

  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
  

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
  

+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!
  

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
  

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 '('.
  

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
  

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
  

> ... 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
  

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
  

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).
  

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
  

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
  

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.
  

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