On 20/05/2014 01:13, Hal Finkel wrote:
----- Original Message -----From: "Richard Smith" <rich...@metafoo.co.uk> To: "Hal Finkel" <hfin...@anl.gov> Cc: "Chandler Carruth" <chandl...@gmail.com>, reviews+d3806+public+8377178d9ac8d...@reviews.llvm.org, "John McCall" <rjmcc...@gmail.com>, "llvm cfe" <cfe-commits@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 < hfin...@anl.gov > wrote: ----- Original Message -----From: "Chandler Carruth" < chandl...@gmail.com > To: reviews+d3806+public+8377178d9ac8d...@reviews.llvm.org Cc: "John McCall" < rjmcc...@gmail.com >, "llvm cfe" < cfe-commits@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 < mcros...@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.
Alp.
Thanks again, Hal2. 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. -HalToo crazy? _______________________________________________ cfe-commits mailing list cfe-commits@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@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-- http://www.nuanti.com the browser experts _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
