This is an archive of the discontinued LLVM Phabricator instance.

Fix implementation of ::abs and std::abs LWG 2192.
ClosedPublic

Authored by EricWF on Apr 1 2019, 1:48 PM.

Details

Summary

All overloads of ::abs and std::abs must be present in both <cmath> and <cstdlib>. This is problematic to implement because C defines fabs in math.h and labs in stdlib.h. This introduces a circular dependency between the two headers.

This patch implements that requirement by moving abs into math.h and making stdlib.h include math.h. In order to get the underlying C declarations from the "real" stdlib.h inside our math.h we need some trickery. Specifically we need to make stdlib.h include next itself.

Suggestions for a cleaner implementation are welcome.

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF created this revision.Apr 1 2019, 1:48 PM

I like the tests. Still looking at the code.

test/std/depr/depr.c.headers/stdlib_h.pass.cpp
92 ↗(On Diff #193174)

same as below re: signed char and size_t

test/std/language.support/support.runtime/cstdlib.pass.cpp
67 ↗(On Diff #193174)

How about signed char here? I don't think that there are any others we need to check.

test/std/numerics/c.math/cmath.pass.cpp
137 ↗(On Diff #193174)

How about size_t here?

EricWF updated this revision to Diff 193230.Apr 1 2019, 9:16 PM
EricWF marked 4 inline comments as done.

Address review comments.

test/std/language.support/support.runtime/cstdlib.pass.cpp
67 ↗(On Diff #193174)

Sure. Done.

Suggestions for a cleaner implementation are welcome.

Why don't we move abs to a third header, say __abs.h, and then include that from both math.h and stdlib.h?

include/math.h
800 ↗(On Diff #193230)

We were not using the builtin before -- why the change?

ldionne requested changes to this revision.Apr 2 2019, 9:46 AM

Requesting change to clear up my review queue.

This revision now requires changes to proceed.Apr 2 2019, 9:46 AM
mclow.lists added inline comments.Apr 2 2019, 10:01 AM
include/math.h
800 ↗(On Diff #193230)

I agree. We can't use the intrinsics here w/o a check to see if the compiler supports them.

We'll probably want to use them in the future, when SG6 plasters constexpr all over cmath, but not right now.

ldionne added inline comments.Apr 2 2019, 10:41 AM
include/math.h
800 ↗(On Diff #193230)

I agree. We can't use the intrinsics here w/o a check to see if the compiler supports them.

Just to be clear -- this is not what I'm claiming (although you might be right), but I just wanted to understand the reason for this drive-by change.

EricWF marked an inline comment as done.Apr 3 2019, 1:53 PM

Suggestions for a cleaner implementation are welcome.

Why don't we move abs to a third header, say __abs.h, and then include that from both math.h and stdlib.h?

Because __abs.h will need to #include_next both math.h and stdlib.h to get the required declarations for abs and fabs anyway.
Which means we can't avoid the _LIBCPP_STDLIB_INCLUDE_NEXT trick.

And having stdlib.h drag in the C library math.h header but not our math.h header leaves us in a worse place. For example:

#include <stdlib.h> // causes inclusion of /usr/include/math.h but not our math.h

auto x = fabs(0.0f); // calls ::fabs(double)

#include <math.h>

auto y = fabs(0.0f); // calls ::fabs(float)

So I think adding an additional header doesn't solve any of our problems.

include/math.h
800 ↗(On Diff #193230)

I think that's a remnant from an older version of this patch.

I'll remove the __builtin_fabs stuff.

EricWF updated this revision to Diff 193587.Apr 3 2019, 1:54 PM
  • Revert use of builtins.
  • Fix configuration error.
ldionne accepted this revision.Apr 9 2019, 7:09 AM
This revision is now accepted and ready to land.Apr 9 2019, 7:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2019, 10:59 AM
raj.khem added inline comments.
libcxx/trunk/include/stdlib.h
100

Should this be #include_next <math.h> here ? this changes causes failures when an app defines a math.h in its own sources and adds -I path to that location, any source code then having include <stdlib.h> ends up including apps math.h via system provided stdlib.h you can see this happening with gnu groff utility

EricWF marked an inline comment as done.Aug 4 2019, 2:01 PM
EricWF added inline comments.
libcxx/trunk/include/stdlib.h
100

If I'm not mistaken, it's undefined behavior to define your own math.h and add it to the <...> include path.

And yes, this was intentional. I need to include libc++'s version of math.h to get the correct set of symbols.

raj.khem added inline comments.Aug 4 2019, 2:40 PM
libcxx/trunk/include/stdlib.h
100

if a system header is included then whatever that includes should remain within system headers search path, user is doing #include <stdlib.h>
and includes local math.h via include "math.h" not <math.h>, however the problem is clang searching for math.h requested by stdlib.h then goes into user header search paths specified via -I

raj.khem added inline comments.Aug 4 2019, 4:09 PM
libcxx/trunk/include/stdlib.h
100

to add a bit more, here I think the perceived problem is indirect inclusion of user defined header via a system header

EricWF marked an inline comment as done.Aug 4 2019, 6:18 PM
EricWF added inline comments.
libcxx/trunk/include/stdlib.h
100

The problem is that the user is doing something they shouldn't. From the C standard

C11 7.1.2/3:
"If a file with the same name as one of the [standard headers] is placed in any of the standard places that are searched for included source files, the behavior is undefined."

This change breaks compilation of firefox with clang and libc++. See https://bugzilla.mozilla.org/show_bug.cgi?id=1594027

I understand from the discussion between Raj and Eric above that that is up to mozilla developers to fix, but the problem is pretty hard to grok, so any suggestions for a fix?