HomePhabricator

[libc++] Move abs and div into stdlib.h to fix header cycle.

Authored by EricWF on Feb 15 2020, 3:55 PM.

Description

[libc++] Move abs and div into stdlib.h to fix header cycle.

libc++ is careful to not fracture overload sets. When one overload
is visible to a user, all of them should be. Anything less causes
subtle bugs and ODR violations.

Previously, in order to support ::abs and ::div being supplied by
both <cmath> and <cstdlib> we had to do awful things that make
<math.h> and <stdlib.h> have header cycles and be non-modular.
This really breaks with modules.

Specifically the problem was that in C++ ::abs introduces overloads
for floating point numbers, these overloads forward to ::fabs,
which are defined in math.h. Therefore ::abs needed to be in math.h
too. But this required stdlib.h to include math.h and math.h to
include stdlib.h.

To avoid these problems the definitions have been moved to stddef.h
(which math includes), and the floating point overloads of ::abs
have been changed to call __builtin_fabs, which both Clang and GCC
support.

Event Timeline

Somehow this commit is breaking that we get the non-std:: versions of some cstddef declarations such as abort or size_t in module builds on macOS with ToT Clang.

E.g. this code was compiling fine before, but now it no longer compiles:

#include <cstdlib>

int counter = 0;

void inc_counter() { ++counter; }

void do_abort() { abort(); }

int main() {
  return 0; // break here
}
/Users/teemperor/2llvm/rel/bin/clang-10  -fmodules -gmodules -fmodules-cache-path=/Users/teemperor/2llvm/rel/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules -std=c++11 -g -O0 -fno-builtin -isysroot "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk" -arch x86_64  -I/Users/teemperor/2llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include -I/Users/teemperor/2llvm/llvm-project/lldb/test/API/commands/expression/static-initializers -I/Users/teemperor/2llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make -include /Users/teemperor/2llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h  -fno-limit-debug-info   -fmodules -gmodules -fmodules-cache-path=/Users/teemperor/2llvm/rel/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules   --driver-mode=g++ -c -o main.o /Users/teemperor/2llvm/llvm-project/lldb/test/API/commands/expression/static-initializers/main.cpp
/Users/teemperor/2llvm/llvm-project/lldb/test/API/commands/expression/static-initializers/main.cpp:7:19: error: missing '#include <stdlib.h>'; declaration of 'abort' must be imported from module 'Darwin.C.stdlib' before it is required
void do_abort() { abort(); }
                  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/stdlib.h:131:7: note: previous declaration is here
void     abort(void) __cold __dead2;
         ^
1 error generated.

Same for size_t:

#include <cstdlib>

int side_effect = 0;

struct B { int dummy = 2324; };
struct C {
  void *operator new(std::size_t size) { C* r = ::new C; r->custom_new = true; return r; }
  void *operator new[](std::size_t size) { C* r = static_cast<C*>(std::malloc(size)); r->custom_new = true; return r; }
  void operator delete(void *p) { std::free(p); side_effect = 1; }
  void operator delete[](void *p) { std::free(p); side_effect = 2; }
};

with

/Users/teemperor/2llvm/rel/bin/clang-10  -std=c++11 -fmodules -gmodules -fmodules-cache-path=/Users/teemperor/2llvm/rel/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules -std=c++11 -g -O0 -fno-builtin -isysroot "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk" -arch x86_64  -I/Users/teemperor/2llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include -I/Users/teemperor/2llvm/rel/lldb-test-build.noindex/lang/cpp/operators/lldbsuite.test.lldbtest.test_gmodules -I/Users/teemperor/2llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make -include /Users/teemperor/2llvm/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h  -fno-limit-debug-info   -fmodules -gmodules -fmodules-cache-path=/Users/teemperor/2llvm/rel/lldb-test-build.noindex/module-cache-clang/lldb-api -gmodules -fcxx-modules   --driver-mode=g++ -c -o main.o /Users/teemperor/2llvm/llvm-project/lldb/test/API/lang/cpp/operators/main.cpp
/Users/teemperor/2llvm/llvm-project/lldb/test/API/lang/cpp/operators/main.cpp:7:22: error: missing '#include "/Users/teemperor/2llvm/rel/bin/../include/c++/v1/cstddef"'; declaration of 'size_t' must be imported from module 'std.compat.cstddef' before it is required
  void *operator new(size_t size) { C* r = ::new C; r->custom_new = true; return r; }

I assume this is a modules bug?

Seems to be a bug without -fmodules-local-submodule-visibility. Somehow in the following example we only mark std.compat.cstdlib as visible but not std.compat.cstddef which was imported

// RUN: clang++ -isysroot "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk" ~/test/size_t.cpp -fmodules -fcxx-modules -Rmodule-build
#include <cstdlib>

int main() {
  size_t i = 0;
  return i;
}

leads to

/Users/teemperor/test/size_t.cpp:4:3: error: missing '#include "/Users/teemperor/2llvm/rel/./bin/../include/c++/v1/cstddef"'; declaration of 'size_t' must be imported from module 'std.compat.cstddef' before it is required
  size_t i = 0;
  ^
/Users/teemperor/2llvm/rel/lib/clang/11.0.0/include/stddef.h:46:23: note: previous declaration is here
typedef __SIZE_TYPE__ size_t;
                      ^

Turning on submodule visibility fixes the issue (and so does using std::size_t instead of size_t).

No idea what's the right fix here. A simple solution would be to let Sema::BuildModuleInclude make the TopLevel module visible but I'm not sure if that is the right fix.

@EricWF Sorry, this seems to break the LLVM_ENABLE_MODULES builds on macOS (and probably every other system without -fmodules-local-submodule-visiblity by default). I'll revert this for now to get the bots green again until we can figure out why this is breaking all the builds. I'll reland this once we have a fix.

teemperor added a comment.EditedFeb 19 2020, 11:58 PM

I have a minimal reproducer for the problem here: https://github.com/Teemperor/reducer-cstddef-vis

The relevant parts of the std module AST dump are:

...
|-TypedefDecl 0x7fe29385d0b0 <line:46:1, col:23> col:23 in std.cstddef size_t 'unsigned long'
| `-BuiltinType 0x7fe293813bc0 'unsigned long'
|-TypedefDecl 0x7fe29385d128 <line:60:1, col:23> col:23 in std.cstddef rsize_t 'unsigned long'
| `-BuiltinType 0x7fe293813bc0 'unsigned long'
|-ImportDecl 0x7fe29385d188 <line:102:1> col:1 in std.cstddef implicit _Builtin_stddef_max_align_t
|-ImportDecl 0x7fe29385d1d0 <cxx/cstddef:6:27> col:27 in std.cstddef implicit std.cstddef
`-ImportDecl 0x7fe29385d218 <cxx/cstdlib:16:1> col:1 in std.cstdlib implicit std.cstdlib

The size_t from cstddef is what we return in the ASTReader as there is no size_t in cstdlib. I assume that the header guards prevent us from reparsing the stddef.h header again in cstdlib, but this is IIUC the way modules work without local submodule visibility.

With local-submodule visibility the module contains two decls and everything works fine:

...
|-TypedefDecl 0x7f9b9b05d0b0 <line:46:1, col:23> col:23 in std.cstddef hidden size_t 'unsigned long'
| `-BuiltinType 0x7f9b9b013bc0 'unsigned long'
|-TypedefDecl 0x7f9b9b05d128 <line:60:1, col:23> col:23 in std.cstddef hidden rsize_t 'unsigned long'
| `-BuiltinType 0x7f9b9b013bc0 'unsigned long'
|-ImportDecl 0x7f9b9b05d188 <line:102:1> col:1 in std.cstddef implicit _Builtin_stddef_max_align_t
|-ImportDecl 0x7f9b9b05d1d0 <cxx/cstddef:6:27> col:27 in std.cstddef implicit std.cstddef
|-TypedefDecl 0x7f9b9b05d248 </Users/teemperor/3llvm/rel/lib/clang/11.0.0/include/stddef.h:35:1, col:26> col:26 in std.cstdlib hidden ptrdiff_t 'long'
| `-BuiltinType 0x7f9b9b013b20 'long'
|-TypedefDecl 0x7f9b9b05d2d8 <line:46:1, col:23> col:23 in std.cstdlib hidden size_t 'unsigned long'
| `-BuiltinType 0x7f9b9b013bc0 'unsigned long'
|-TypedefDecl 0x7f9b9b05d368 <line:60:1, col:23> col:23 in std.cstdlib hidden rsize_t 'unsigned long'
| `-BuiltinType 0x7f9b9b013bc0 'unsigned long'
|-ImportDecl 0x7f9b9b05d3c8 <line:102:1> col:1 in std.cstdlib implicit _Builtin_stddef_max_align_t
`-ImportDecl 0x7f9b9b05d410 <cxx/cstdlib:16:1> col:1 in std.cstdlib implicit std.cstdlib

I'm genuinely surprised how broken this whole thing is. The reproducer is at this point just two submodules both including a non-module header and the main file is including the second module-header (in the order of the module map).

I tried bisecting but this has been broken since at least Clang 5. I couldn't build Clang 4 anymore on my system. I found Clang 3.4 on some ancient Linux and that seems to still support that code. I guess the addition of local-submodule-visibility broke this because that happened in that version range.

In any case I think the best way forward is to just also use -fmodules-local-submodule-visibility when building LLVM. The only build failure we get with that mode on macOS is due to one random ObjC++ file in LLDB's debugserver (that anyway shouldn't use modules). I'll fix that and then make a review for removing the special macOS case for modules on macOS from LLVM's CMake code.

The aforementioned review is at https://reviews.llvm.org/D74892. I'm currently verifying it fixes everything for us. I'll update you when you're good to recommit this, which should happen this week.

The aforementioned review is at https://reviews.llvm.org/D74892. I'm currently verifying it fixes everything for us. I'll update you when you're good to recommit this, which should happen this week.

Bruno actually mentioned that he doesn't want to use -fmodules-local-submodule-visibility on macOS so the we don't lose test coverage for that mode. So that's why I didn't move forward with that review. On the other hand we now have also another build failure that is most likely related to not using local submodule visibility (see D75091), so I'm inclined to just land a version of D74892 and then we add another bot that is building without that flag. That way we don't have to revert but still have test coverage for compilation without -fmodules-local-submodule-visibility

I remanded this commit now that this all works on macOS. Sorry for the ridiculously long delay.