[PATCH] These builtin functions set errno. Mark them accordingly.

[Richard Smith]

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.

Is it possible to teach LLVM's target library info about errno, or does it
vary too much (based on preprocessor defines / ...)?


>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140519/30755183/attachment.html>