[PATCH] These builtin functions set errno. Mark them accordingly.
Richard Smith
richard at metafoo.co.uk
Thu May 22 18:15:32 PDT 2014
On Mon, May 19, 2014 at 10:05 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
> > From: "Alp Toker" <alp at nuanti.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>, "Richard Smith" <
> richard at metafoo.co.uk>
> > Cc: reviews+D3806+public+8377178d9ac8d89d at reviews.llvm.org, "John
> McCall" <rjmccall at gmail.com>, "llvm cfe"
> > <cfe-commits at cs.uiuc.edu>
> > Sent: Monday, May 19, 2014 7:13:26 PM
> > Subject: Re: [PATCH] These builtin functions set errno. Mark them
> accordingly.
> >
> >
> > On 20/05/2014 01:13, Hal Finkel wrote:
> > > ----- Original Message -----
> > >> From: "Richard Smith" <richard at metafoo.co.uk>
> > >> To: "Hal Finkel" <hfinkel at anl.gov>
> > >> Cc: "Chandler Carruth" <chandlerc at gmail.com>,
> > >> reviews+D3806+public+8377178d9ac8d89d at reviews.llvm.org, "John
> > >> McCall"
> > >> <rjmccall at gmail.com>, "llvm cfe" <cfe-commits at cs.uiuc.edu>
> > >> Sent: Monday, May 19, 2014 3:57:34 PM
> > >> Subject: Re: [PATCH] These builtin functions set errno. Mark them
> > >> accordingly.
> > >>
> > >>
> > >>
> > >>
> > >> On Mon, May 19, 2014 at 11:28 AM, Hal Finkel < hfinkel at anl.gov >
> > >> wrote:
> > >>
> > >>
> > >>
> > >>
> > >>
> > >> ----- Original Message -----
> > >>> From: "Chandler Carruth" < chandlerc at gmail.com >
> > >>> To: reviews+D3806+public+8377178d9ac8d89d at reviews.llvm.org
> > >>> Cc: "John McCall" < rjmccall at gmail.com >, "llvm cfe" <
> > >>> cfe-commits at cs.uiuc.edu >
> > >>> Sent: Monday, May 19, 2014 12:35:47 PM
> > >>> Subject: Re: [PATCH] These builtin functions set errno. Mark them
> > >>> accordingly.
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>> On Mon, May 19, 2014 at 10:26 AM, Chad Rosier <
> > >>> mcrosier at codeaurora.org > wrote:
> > >>>
> > >>>
> > >>>
> > >>> Hal and I discussed this on IRC and here's the synopsis (mostly
> > >>> in
> > >>> Hal's words):
> > >>>
> > >>> Many of these builtins turn into libc calls, and those do set
> > >>> errno.
> > >>> Marking them as readnone is a problem, because we can
> > >>> reorder/eliminate writes to errno. For example, if we assume
> > >>> readnone for something like __builtin_sqrt(); write(); perror();
> > >>> it
> > >>> may be reordered to write(); __builtin_sqrt(); perror(); and
> > >>> perror
> > >>> may provide junk. However, if we don't mark the builtins as
> > >>> readnone, then they won't get lowered into ISD nodes (for those
> > >>> nodes that are relevant), and that means that they won't get
> > >>> instruction selected into native instructions on targets where
> > >>> that's possible. So the __builtin_sqrt is kind of an escape hatch
> > >>> to
> > >>> force the direct lowering behavior even when we can't do it for
> > >>> sqrt
> > >>> because sqrt might set errno. But, as Hal said, this behavior
> > >>> *is*
> > >>> broken, technically. And, in fact, it is not hard to create test
> > >>> cases where you can see buggy behavior. Hak's just not sure what
> > >>> the
> > >>> right way of handling it is. Hal thinks we really need some kind
> > >>> of
> > >>> target inp!
> > >>> ut, so that we can know ahead of time whether the builtin will
> > >>> *really* set errno or not after isel. Hal thinks there are two
> > >>> solutions possible.
> > >>>
> > >>> 1. Like with inline asm registers/constraints; each target will
> > >>> provide a table describing what builtins will really not set
> > >>> errno.
> > >>> 2. Let Clang essentially query TTI for those builtins that are
> > >>> relevant. This second method will require a bit of infrastructure
> > >>> work, but is probably not too difficult.
> > >>>
> > >>> I'm not going to push this patch further because it's not the
> > >>> right
> > >>> solution. In theory, I believe it is correct, but it breaks the
> > >>> __buitlin escape hatch, which would likely result in serious
> > >>> performance degradations.
> > >>> Crazy idea:
> > >>>
> > >>>
> > >>> When we lower one of these to a call to a libc function, save and
> > >>> restore errno around it.
> > >> Actually, I really like this idea. It will take some work,
> > >> however,
> > >> because errno is not a particular symbol (it is almost always a
> > >> macro that expands to something else), and it changes depending on
> > >> what OS/libc is being used. Clang has, at least in theory, all of
> > >> the information necessary to generate a save/restore function for
> > >> errno (although it may need to force the inclusion of errno.h).
> > >>
> > >> For example, we could do something like this:
> > >>
> > >> 1. In clang::InitializePreprocessor, add something like:
> > >> Builder.append("#include <errno.h>
> > >> static int __read_errno() { return errno; }
> > >> __attribute__((always_inline))
> > >> static void __write_errno(int e) { errno = e; }
> > >> __attribute__((always_inline))");
> > >>
> > >>
> > >>
> > >> I'm strongly opposed to this particular approach.
> > >>
> > > The thing that I don't like about it is that it would affect the
> > > AST. We could, however, compile a small artificial source file
> > > containing just those definitions (using the same preprocessor
> > > defines, etc.), and just "link" the IR with the IR module for
> > > which we're actually generating code before we optimize (and we
> > > could do this only if some __builtin_<foo> was used). Do you feel
> > > differently about that?
> > >
> > >> Is it possible to teach LLVM's target library info about errno, or
> > >> does it vary too much (based on preprocessor defines / ...)?
> > > The problem, I think, really shows up on Linux systems where there
> > > is a choice of libc implementations (especially on the embedded
> > > side of things). Here's a quick overview of errno on different
> > > systems:
> > >
> > > - Traditional POSIX (not thread safe):
> > >
> > > extern int errno;
> > >
> > > - DragonFly libc:
> > >
> > > extern __thread int errno;
> > >
> > > - Android libc:
> > >
> > > extern volatile int* __errno(void);
> > > #define errno (*__errno())
> > >
> > > - Darwin libc:
> > >
> > > int * __error(void);
> > > #define errno (*__error())
> > >
> > > - musl libc:
> > >
> > > extern int *__errno_location(void);
> > > #define errno (*__errno_location())
> > >
> > > - glibc:
> > >
> > > extern int *__errno_location (void) __THROW __attribute__
> > > ((__const__));
> > > # if !defined _LIBC || defined _LIBC_REENTRANT
> > > define errno (*__errno_location ())
> > > # endif
> > >
> > > #ifndef errno
> > > extern int errno;
> > > #endif
> > >
> > > - newlib:
> > >
> > > #define errno (*__errno())
> > > extern int *__errno _PARAMS ((void));
> > >
> > > - diet libc
> > >
> > > #ifndef _REENTRANT
> > > extern int errno;
> > > #else
> > > #define errno (*__errno_location())
> > > #endif
> > >
> > > extern int *__errno_location(void);
> > >
> > > - uClibc
> > >
> > > #ifdef _LIBC
> > > #ifdef IS_IN_rtld
> > > # undef errno
> > > # define errno _dl_errno
> > > extern int _dl_errno; /* attribute_hidden */
> > > #elif defined __UCLIBC_HAS_TLS__
> > > # if !defined NOT_IN_libc || defined IS_IN_libpthread
> > > # undef errno
> > > # ifndef NOT_IN_libc
> > > # define errno __libc_errno
> > > # else
> > > # define errno errno /* For #ifndef errno tests.
> > > */
> > > # endif
> > > extern __thread int errno attribute_tls_model_ie;
> > > # endif
> > > #endif
> > >
> > > I'm somewhat afraid of introducing a dependence on the libc
> > > implementation at the LLVM target layer, and I'm somewhat afraid
> > > that it will break things specifically when Clang is being used to
> > > compile libc itself (or other system software). Having Clang
> > > provide the information seems like the "technically correct" way
> > > of doing it. Thoughts?
> >
> > Perhaps we can adapt your __read_errno() / __write_errno() technique
> > to
> > make it less magic and by placing it it in a header like
> > lib/Headers/fastmath.h
> >
> > User code including fastmath.h would become eligible for the goodies,
> > along with the errno.h or whatever include it took to achieve that.
>
> The problem is that we're talking about how to generate code for
> __builtin_sqrt and friends. Having the behavior of __builtin_sqrt change
> when some header is included seems strange (although certainly possible),
> and certainly going to be different from gcc's behavior. Fundamentally,
> however, it takes something that is our problem (figuring out how to
> interact with errno), which we *can* solve within the compiler in a
> straightforward low-overhead way, and makes it the user's problem.
> Fundamentally, we should not do that.
>
If we need to fall back to a libcall for a __builtin_blah function, we
could perhaps fall back to something provided by compiler-rt (that never
sets errno), rather than a libm function. The compiler-rt implementation
could be as simple as saving errno across a call to libm.
Thoughts? What does GCC do? (If there's no libgcc equivalent, this is
probably dead in the water since it won't work for people who don't use
compiler-rt.)
> Thanks again,
> Hal
>
> >
> > Alp.
> >
> >
> > >
> > > Thanks again,
> > > Hal
> > >
> > >>
> > >> 2. When we lower these __builtin_<foo> functions, add code around
> > >> them using __read_errno and __write_errno so that errno is not
> > >> clobbered even if the builtin is lowered to a libm call.
> > >>
> > >> 3. Make sure that DAGCombine and/or DeadMachineInstructionElim
> > >> removes the save/restore code when it is dead (because the builtin
> > >> was lowered to an instruction) for common platforms.
> > >>
> > >> -Hal
> > >>
> > >>>
> > >>> Too crazy?
> > >>> _______________________________________________
> > >>> cfe-commits mailing list
> > >>> cfe-commits at cs.uiuc.edu
> > >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > >>>
> > >>
> > >> --
> > >> Hal Finkel
> > >> Assistant Computational Scientist
> > >> Leadership Computing Facility
> > >> Argonne National Laboratory
> > >>
> > >>
> > >> _______________________________________________
> > >> cfe-commits mailing list
> > >> cfe-commits at cs.uiuc.edu
> > >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > >>
> > >>
> >
> > --
> > http://www.nuanti.com
> > the browser experts
> >
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140522/e535607b/attachment.html>
More information about the cfe-commits
mailing list