Re: [patch] For BZ #17328, mark __errno_location with __attribute__((returns_nonnull)) for gcc >=4.9.0

[Rich Felker <dalias at libc dot org>]

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