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

[Hal Finkel]

----- 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))");

 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