[libcxx] r249738 - Split <ctype.h> out of <cctype>.

[Richard Smith via cfe-commits]

This was a thinko on my part: clang's builtin headers include_next the
system headers, not the other way around, so the system headers should be
implicitly textual, not clang's headers. This patch fixes the problem for
me with glibc. Does this help for Darwin too?

On Tue, Oct 25, 2016 at 2:01 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On Mon, Oct 24, 2016 at 4:58 PM, Bruno Cardoso Lopes via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> On Mon, Oct 24, 2016 at 4:17 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On Mon, Oct 24, 2016 at 3:30 PM, Bruno Cardoso Lopes
>> > <bruno.cardoso at gmail.com> wrote:
>> >>
>> >> > Sure, go ahead.
>> >>
>> >> I committed in r284797 and r284801 (libcxx). There's one minor issue
>> >> I've found: the changes for the builtins affecting non submodule local
>> >> visibility broke current users of plain "-fmodules" against our
>> >> frameworks in public SDKs, in 10.11 & 10.12. I've attached a patch to
>> >> work around that for the time being: make the new behavior dependent
>> >> on local vis. Can you take a look?
>> >
>> >
>> > What's the nature of the breakage? Generally I'd be fine with your
>> patch,
>> > but I wonder if there's something better we could do here.
>>
>> I haven't entirely isolated the problem, but they are all related to
>> definitions from stdint.h. In one example below, uint32_t doesn't
>> leak, requiring an explicit "#include <stdint.h>" to make it work.
>>
>> -- example.m
>> #import <IOKit/IODataQueueClient.h>
>> --
>> $ clang -arch x86_64 -isysroot
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.
>> platform/Developer/SDKs/MacOSX10.12.sdk
>> -fmodules-cache-path=tmpcache example.m -E -o /dev/null  -fmodules
>>
>> While building module 'IOKit' imported from example.m:1:
>> In file included from <module-includes>:2:
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.
>> platform/Developer/SDKs/MacOSX10.12.sdk/System/Library
>> /Frameworks/IOKit.framework/Headers/IODataQueueClient.h:62:71:
>> error: de
>>       'Darwin.POSIX._types._uint32_t' before it is required
>> IOReturn IODataQueueDequeue(IODataQueueMemory *dataQueue, void *data,
>> uint32_t *dataSize);
>>                                                                       ^
>> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.
>> platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/_
>> types/_uint32_t.h:31:22:
>> note: previous declaration is here
>> typedef unsigned int uint32_t;
>>                      ^
>> bot.m:1:9: fatal error: could not build module 'IOKit'
>> #import <IOKit/IODataQueueClient.h>
>>  ~~~~~~~^
>
>
> This change also broke local submodule visibility builds with modular
> glibc (see PR30778 for details). I have an idea for how to fix this;
> running it through bootstrap now.
>
> >> > Hmm. Ideally, we should try to pick something that captures the spirit
>> >> > of
>> >> > "only non-modular headers and headers from used modules". Something
>> like
>> >> > "ignore_modules_not_declared_used", but less wordy?
>> >>
>> >> Right. It's gonna be hard to shrink this to a meaningful short name.
>> >> What about a more generic "no_escape"?  "no_undeclared_headers"?
>> >
>> >
>> > Hmm. Maybe we could allow the existing [exhaustive] attribute to be
>> > specified on a use-declaration:
>> >
>> >   use [exhaustive] a, b, c
>>
>> I don't understand, the 'Darwin' module map doesn't use the 'use'
>> keyword in any of its modules, how do you suggest we would use that to
>> express the 'ignore_modules_not_declared_used' idea?
>
>
> Hah, right, this would only work if your module has dependencies. Maybe an
> [exhaustive_uses] attribute on the module itself then?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161025/92b703c0/attachment-0001.html>
-------------- next part --------------
Index: lib/Lex/ModuleMap.cpp
===================================================================
--- lib/Lex/ModuleMap.cpp	(revision 285117)
+++ lib/Lex/ModuleMap.cpp	(working copy)
@@ -1881,16 +1881,20 @@
       Module::Header H = {RelativePathName.str(), File};
       Map.excludeHeader(ActiveModule, H);
     } else {
-      // If there is a builtin counterpart to this file, add it now as a textual
-      // header, so it can be #include_next'd by the wrapper header, and can
-      // receive macros from the wrapper header.
+      // If there is a builtin counterpart to this file, add it now so it can
+      // wrap the system header.
       if (BuiltinFile) {
         // FIXME: Taking the name from the FileEntry is unstable and can give
         // different results depending on how we've previously named that file
         // in this build.
         Module::Header H = { BuiltinFile->getName(), BuiltinFile };
-        Map.addHeader(ActiveModule, H, ModuleMap::ModuleHeaderRole(
-                                           Role | ModuleMap::TextualHeader));
+        Map.addHeader(ActiveModule, H, Role);
+
+        // If we have both a builtin and system version of the file, the
+        // builtin version may want to inject macros into the system header, so
+        // force the system header to be treated as a textual header in this
+        // case.
+        Role = ModuleMap::ModuleHeaderRole(Role | ModuleMap::TextualHeader);
       }
 
       // Record this header.
Index: test/Modules/Inputs/System/usr/include/stdbool.h
===================================================================
--- test/Modules/Inputs/System/usr/include/stdbool.h	(revision 285117)
+++ test/Modules/Inputs/System/usr/include/stdbool.h	(working copy)
@@ -1 +1 @@
-#include_next <stdbool.h>
+// Testing hack: does not define bool/true/false.
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
===================================================================
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h	(revision 285117)
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h	(working copy)
@@ -1,2 +1 @@
 // stddef.h
-#include_next "stddef.h"