Page MenuHomePhabricator

[CodeGen] change const-ness of complex calls
ClosedPublic

Authored by spatel on Nov 3 2017, 12:11 PM.

Details

Summary

The lack of errno setting is allowed by the C standard with 7.3.2:
"An implementation may set errno but is not required to."

The POSIX docs all have language like this for complex calls:
"No errors are defined."
http://pubs.opengroup.org/onlinepubs/9699919799/functions/ccos.html

But by default, we must allow that an implementation could set errno.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Nov 3 2017, 12:11 PM
spatel edited the summary of this revision. (Show Details)Nov 3 2017, 12:12 PM
efriedma edited edge metadata.Nov 3 2017, 1:32 PM

The POSIX docs all have language like this for complex calls:
"No errors are defined."

I would not trust the POSIX documentation.

carg() is an alias for atan2(), and cabs() is an alias for hypot(), so they should be marked the same way. (On glibc, cabs/hypot will set errno; not sure about carg/atan2.)

For the others transcendental functions... they can raise exceptions in some cases, so they could in theory set errno, but it looks like they don't on glibc? Not sure if that's documented anywhere, or something we should depend on.

efriedma added inline comments.Nov 3 2017, 1:35 PM
include/clang/Basic/Builtins.def
169 ↗(On Diff #121521)

Not sure adding this comment specifically for cbrt adds anything; the same could be said of a bunch of other functions, like ceil().

hfinkel edited edge metadata.Nov 3 2017, 5:32 PM

The POSIX docs all have language like this for complex calls:
"No errors are defined."

I would not trust the POSIX documentation.

carg() is an alias for atan2(), and cabs() is an alias for hypot(), so they should be marked the same way. (On glibc, cabs/hypot will set errno; not sure about carg/atan2.)

That seems like a bug if they set errno. The complex versions shouldn't set errno. glibc's docs say that neither hypot or set errno (although the POSIX spec allows for it in theory).

For the others transcendental functions... they can raise exceptions in some cases, so they could in theory set errno, but it looks like they don't on glibc? Not sure if that's documented anywhere, or something we should depend on.

In general, because all of these functions are well defined over the entire complex domain, they don't have errors. The "no errors defined" is likely correct.

In general, because all of these functions are well defined over the entire complex domain, they don't have errors. The "no errors defined" is likely correct.

One, it is not true that all these functions are well-defined over the entire complex domain. For example, according to the C standard catanh(1) raises a divide-by-zero error.

Two, even for the functions which are defined over the entire complex domain, the result can still overflow and/or underflow, and therefore they could set errno to ERANGE. cabs() can overflow, cexp() can overflow, etc.

Three, even if we only care about the behavior of the current version of glibc, cabs() actually does modify errno. (Try cabs(1.7e308+I*1.7e308)).

In general, because all of these functions are well defined over the entire complex domain, they don't have errors. The "no errors defined" is likely correct.

One, it is not true that all these functions are well-defined over the entire complex domain. For example, according to the C standard catanh(1) raises a divide-by-zero error.

True. That's why I said that the statement was true in general. There are a few exceptions.

Two, even for the functions which are defined over the entire complex domain, the result can still overflow and/or underflow, and therefore they could set errno to ERANGE. cabs() can overflow, cexp() can overflow, etc.

Three, even if we only care about the behavior of the current version of glibc, cabs() actually does modify errno. (Try cabs(1.7e308+I*1.7e308)).

As I understand things currently, that's a bug:

In the C specification, 7.12 specifies the requirements for functions in math.h. For those functions, 7.12.1 (Treatment of error conditions) says that overflows do set ERANGE, and that it's implementation defined if the same is true for underflows. That's true for functions in math.h in general. 7.3 specifies the requirements for functions in complex.h. Here, 7.3.2 says, "An implementation may set errno but is not required to." That, as I understand it, applies to all functions in complex.h (unless otherwise noted, I presume). However, because setting errno is not required by C for functions in complex.h, when POSIX says "No errors are defined." that constrains the choice (POSIX is constraining the implementation choice allowed by C; this is not a contraction with the C specification). As a result, under POSIX, the functions in complex.h don't set errno (some, as you've noted, may raise FP exceptions, but that's another matter).

Regarding glibc-specific behavior, I do want us to be careful only to model this behavior when the target triple has -gnu. Otherwise, we should stick to the standard behavior.

spatel added a comment.Nov 5 2017, 6:33 AM

In the C specification, 7.12 specifies the requirements for functions in math.h. For those functions, 7.12.1 (Treatment of error conditions) says that overflows do set ERANGE, and that it's implementation defined if the same is true for underflows. That's true for functions in math.h in general. 7.3 specifies the requirements for functions in complex.h. Here, 7.3.2 says, "An implementation may set errno but is not required to." That, as I understand it, applies to all functions in complex.h (unless otherwise noted, I presume). However, because setting errno is not required by C for functions in complex.h, when POSIX says "No errors are defined." that constrains the choice (POSIX is constraining the implementation choice allowed by C; this is not a contraction with the C specification). As a result, under POSIX, the functions in complex.h don't set errno (some, as you've noted, may raise FP exceptions, but that's another matter).

Regarding glibc-specific behavior, I do want us to be careful only to model this behavior when the target triple has -gnu. Otherwise, we should stick to the standard behavior.

I'll split 'complex' off into its own patch then. We'll need a new attribute code for those functions. Something like:
// E -> const when -fmath-errno=0 or in a GNU (or a POSIX-compliant?) environment

I think we can detect GNU-ness with:
Context.getTargetInfo().getTriple().isGNUEnvironment()

But is that correct? If this is actually a POSIX-compliance-based constraint of the choice to not set errno, then GNU is not wide enough to cover other platforms like macOS or AIX that are (mostly?) POSIX-compliant. Also, does GNU guarantee POSIX-compliance?

Currently, the clang driver supports three platforms which have errno-setting libc implementations: GNU/Linux (with glibc), Solaris, and Windows (with Visual Studio CRT). (BSD-based systems, including OS X, never set errno in libm, so they aren't relevant here.) As long as our markings are consistent with those targets, we should be fine.

Both MSVC and GNU libraries never set errno for cbrt() and fma(); we want to keep those assumptions in clang.

We can ignore Visual Studio for the discussion of complex.h, I guess, because MSVC doesn't support _Complex. (It has a header called complex.h, but the types aren't right, so clang won't recognize any of the functions. For reference, though, its cabs() and catanh() set errno).

I have no idea how Solaris and other Unix targets handle these functions; it's probably a bad idea to add special cases for them without testing.

spatel updated this revision to Diff 121949.Nov 7 2017, 12:05 PM
spatel retitled this revision from [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant to [CodeGen] change const-ness of complex calls.
spatel edited the summary of this revision. (Show Details)

Patch updated:
I don't know if we have agreement on the behavior that we want yet, but I'm updating the patch/description/title to only deal with <complex> in this patch and taking a shot at one interpretation, so we can see what that might look like.

There are really 2 changes here:

  1. We had all of <complex> marked constant all the time ('c'). I think there's agreement that can't be true by default (a platform could set errno on any of these calls based on the language in the C standard).
  2. We make an exception for a GNU environment - here, the calls are always constant because the standard allows - and POSIX constrains - the errno setting behavior. This is despite the fact that glibc is known to set errno in some cases.

Patch updated:
I don't know if we have agreement on the behavior that we want yet,

I just sent a note to the Austin group mailing list to see if the POSIX folks agree with my reading. I'll follow up.

but I'm updating the patch/description/title to only deal with <complex> in this patch and taking a shot at one interpretation, so we can see what that might look like.

There are really 2 changes here:

  1. We had all of <complex> marked constant all the time ('c'). I think there's agreement that can't be true by default (a platform could set errno on any of these calls based on the language in the C standard).
  2. We make an exception for a GNU environment - here, the calls are always constant because the standard allows - and POSIX constrains - the errno setting behavior. This is despite the fact that glibc is known to set errno in some cases.

Patch updated:
I don't know if we have agreement on the behavior that we want yet,

I just sent a note to the Austin group mailing list to see if the POSIX folks agree with my reading. I'll follow up.

They don't, so here's a summary...

  1. It is not the intent of POSIX to restrict implementation choices here made available by the C specification. Oversights aside, places where POSIX intentionally restricts implementation choices allowed by C are given CX or XSI shading.

Moreover, 2.3 Error Numbers, says, "Implementations may... contain extensions or limitations that prevent some errors from occurring. The ERRORS section on each reference page specifies whether an error shall be returned, or whether it may be returned. Implementations shall not generate a different error number from the ones described here for error conditions described in this volume of IEEE Std 1003.1-2001, but may generate additional errors unless explicitly disallowed for a particular function." Because no errors are defined, and POSIX contains no specific prohibition on the complex.h functions returning errors via errno, no restriction on these functions setting errno is implied.

Thus, if C says that an implementation can set errno for functions in complex.h, POSIX does not restrict that.

  1. 7.12 in C99 says that setting ERANGE on overflow is implementation-defined (same as in C90), but C99 TC3 and POSIX.1-2001 have math_errhandling, and there, when (math_errhandling & MATH_ERRNO) != 0, all overflows for functions in <math.h> set errno to ERANGE.
  1. We can assume, for consistency, that the intent of the C standard is to allow math_errhandling, and thus associated requirements (e.g., the overflow requirements), to also apply to function in complex.h. G6p4 does say that there is an equivalence for floating-point exceptions (e.g., when (math_errhandling & MATH_ERREXCEPT) != 0), and so while errno is not explicitly discussed in this context, it is not unreasonable to assume this to be an oversight. It is also noted that in C99 Annex G was informative, not normative. So anything derived from Annex G is not a requirement until C11.
  1. C11 DR#442 clarifies that, when Annex F is relevant, the definition of floating-point overflow in Annex F takes precedence over it being generically undefined in the absence of Annex F. Moreover, as clarified by C90 DR#025, where the type as infinities, all values are within the represented range, and so just the fact that an infinity is generated as a result implies that an overflow had occurred. Overflow only occurs for cases where a value is generated outside of the represented range for its type, such as when doing floating-point-to-integer conversions.

But, C99 has an updated definition of overflow, and it says, 7.12.1p4, "A floating result overflows if the magnitude of the mathematical result is finite but so large that the mathematical result cannot be represented without extraordinary roundoff error in an object of the specified type." Moreover, C99's Annex F says, "The ‘‘overflow’’ floating-point exception is raised whenever an infinity - or, because of
rounding direction, a maximal-magnitude finite number - is returned in lieu of a value whose magnitude is too large." While the Annex F text replaces the overflow definition, and does not mention errno explicitly, it is reasonable to assume it does not eliminate the errno-associated requirements from 7.12.1p4 intentionally.

Thus, cabs is fine to set errno, as can essentially all of the other complex.h functions.

but I'm updating the patch/description/title to only deal with <complex> in this patch and taking a shot at one interpretation, so we can see what that might look like.

There are really 2 changes here:

  1. We had all of <complex> marked constant all the time ('c'). I think there's agreement that can't be true by default (a platform could set errno on any of these calls based on the language in the C standard).
  2. We make an exception for a GNU environment - here, the calls are always constant because the standard allows - and POSIX constrains - the errno setting behavior. This is despite the fact that glibc is known to set errno in some cases.

Thanks for the clarification!

If I'm reading this properly, we should make the same kind of change as in D39481 ('c' -> 'e') for most of complex.h. Ie, the standard allows errno-setting, and it's (unfortunately for optimization) even more clearly stated in the newer additions to the standards.

We can leave these functions as always constant ('c') because they don't actually do any math and therefore won't set errno:
cimag ( http://en.cppreference.com/w/c/numeric/complex/cimag )
creal ( http://en.cppreference.com/w/c/numeric/complex/creal )
cproj ( http://en.cppreference.com/w/c/numeric/complex/cproj )
conj (http://en.cppreference.com/w/c/numeric/complex/conj )

Thanks for the clarification!

If I'm reading this properly, we should make the same kind of change as in D39481 ('c' -> 'e') for most of complex.h. Ie, the standard allows errno-setting, and it's (unfortunately for optimization) even more clearly stated in the newer additions to the standards.

We can leave these functions as always constant ('c') because they don't actually do any math and therefore won't set errno:
cimag ( http://en.cppreference.com/w/c/numeric/complex/cimag )
creal ( http://en.cppreference.com/w/c/numeric/complex/creal )
cproj ( http://en.cppreference.com/w/c/numeric/complex/cproj )
conj (http://en.cppreference.com/w/c/numeric/complex/conj )

Sounds right to me.

spatel updated this revision to Diff 122474.Nov 10 2017, 10:40 AM

Patch updated:
Except for cimag, cproj, conj, and creal, everything in <complex> is marked 'e' - the functions are not const if we might set errno.

This revision is now accepted and ready to land.Nov 17 2017, 12:23 PM
This revision was automatically updated to reflect the committed changes.