[PATCH] These builtin functions set errno. Mark them accordingly.
Hal Finkel
hfinkel at anl.gov
Mon May 19 15:13:22 PDT 2014
----- 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?
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
>
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the cfe-commits
mailing list