Differential D3806
These builtin functions set errno. Mark them accordingly. Authored by mcrosier on May 16 2014, 9:23 AM.
Details
A number of library calls that set errno were not behaving correctly with -fmath-errno. Specifically, they were being lowered to native instruction and not setting errno. The changes are based on grepping through the man pages. Please have a look and let me know if I've missed something. Chad
Diff Detail Event TimelineComment Actions I'm not really a fan of this. I don't know why it is reasonable to expect the builtin to set errno. If you want the libc behavior of a libc function, you should call the libc function. My impression was that one of the primary reasons for the existence of these builtins was to *avoid* calling a libc function. Comment Actions And just for reference, yes, GCC does call the libc function. But that doesn't seem like useful behavior to me, and we have survived quite some time without this.
Comment Actions 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 input, 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.
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.
Comment Actions We should perhaps reopen this in light of PR21635 (or consider some variant of it). The problem, as pointed out in the bug report, is that libstdc++ implements std::X in terms of __builtin_X: inline float
exp(float __x)
{ return __builtin_expf(__x); }and these C++ functions are *required* to follow the same math error handling protocol as the underlying C functions. So we actually don't have much of a choice here (at least for those functions used to implement libstdc++) as far as I can tell. As a result, this seems like the right approach after all (although this is somewhat unfortunate). Comment Actions This is only non-conforming because math_errhandling is defined to a value that includes MATH_ERRNO. Another possible approach would be to subvert that macro instead of providing an errno guarantee in our builtins. Is there some reason we want to provide a guarantee that we set errno on domain error if the underlying libc does? Comment Actions Correct.
Interesting idea. Are the calls allowed to set errno if math_errhandling does not include MATH_ERRNO? And do we introduce a usability problem by having the behavior differ from the C functions? Part of the problem here is that we don't really track errno, and if we think the calls don't set errno, we mark them as const. In doing so, they can be reordered with calls that do set errno (like open()), and can corrupt the errno value coming from those calls. That having been said, we have this problem in fast-math mode too. Comment Actions Richard, as much as I don't like this, I'd like to move forward with this. While intercepting math_errhandling might allow us to pretend that the functions don't set errno (and it is not clear to me that we can really make the "C++" math_errhandling differ from the "C" math_errhandling), it does not address the fact that they actually do (and, thus, can cause miscompiles if they're reordered with calls to open(), etc.). I also recognize that this is somewhat self-inducing, because if we mark the functions as non-const, then they'll become real functions which are non-const, but if we mark them as const, then some of them are directly lowered, and thus really are const. The problem is marking calls as const which really aren't -- and some of these functions are never directly handled by the backends. Thanks! We, however, need a test case. Comment Actions A few comments, then this seems OK with accompanying testcases. All the functions other than the ones I commented on are lowered directly to libm calls, so it's correct to mark them "e". (Separately, I wonder why we lower these directly to libcalls rather than to intrinsic calls, and conversely why LLVM has intrinsics that are exactly equivalent to libm functions.)
| |||||||||||||||||||||||||||||||
This is wrong. We lower these to an frem instruction, which does not set errno.