Don't diagnose non-modular includes when we are not compiling a module

[manmanren]

Summary

We used to have -fmodule-implementation-of and it was merged with -fmodule-name. But this causes some regression on our internal projects. We are now seeing non-modular include warnings.

This is triggered when we have relative includes to a VFS-mapped module, -I that points us to the real header and -F that points us to the vfs. The real header that is part of the umbrella directory will be classified as non-modular.

It seems like we can use CompilingModule to tell if we are compiling the interface of a module or the implementation file of a module. When compiling the implementation file, we don't diagnose non-modular includes. The non-modular includes will be diagnosed when building the interface and writing the pcm file.

Related changes:
http://llvm.org/viewvc/llvm-project?rev=213767&view=rev
http://llvm.org/viewvc/llvm-project?rev=261372&view=rev
http://llvm.org/viewvc/llvm-project?rev=263449&view=rev

[benlangmuir]

LGTM, but @rsmith should glance at this too.

[rsmith]

It seems to me like this is papering over an issue rather than fixing it.

diagnoseHeaderInclusion calls isHeaderInUmbrellaDirs here, which is presumably failing to determine that Nonmodular/A.h is in the umbrella directory for the Nonmodular module. My first guess would be that this code from ModuleMap::findHeaderInUmbrellaDirs may be responsible:

// Note: as an egregious but useful hack we use the real path here, because
// frameworks moving from top-level frameworks to embedded frameworks tend
// to be symlinked from the top-level location to the embedded location,
// and we need to resolve lookups as if we had found the embedded location.
StringRef DirName = SourceMgr.getFileManager().getCanonicalName(Dir);

lib/Lex/PPDirectives.cpp

749–750

I don't see why this should depend on whether we're compiling a module. If we're asked to diagnose non-modular #includes in modular headers, why should it matter whether we entered that header textually or by triggering precompilation of the corresponding module?

[manmanren]

In D23858#525647, @rsmith wrote:

It seems to me like this is papering over an issue rather than fixing it.

I guess that is why we introduced fmodule-implementation-of, to work around issues like "relative includes to a VFS-mapped module". Ben should know more about the background.

diagnoseHeaderInclusion calls isHeaderInUmbrellaDirs here, which is presumably failing to determine that Nonmodular/A.h is in the umbrella directory for the Nonmodular module.

Bruno and I looked at this together yesterday. With vfsoverlay, the header files in the module map are using the real path, while the umbrella directories in the module map are using the virtual path. We have issue figuring out the header file from the real path is in the umbrella directory. We have longer term goals on fixing up this. But this is a regression on our side and we are hoping to get back to our previous behavior.

Cheers,
Manman

lib/Lex/PPDirectives.cpp

749–750

I agree that here, we are actually including a non modular header from a modular header. But is it okay to not diagnose when we have specified fmodule-name and we are building the implementation of it? We should have diagnosed this when building the unit.

[rsmith]

OK, if this unblocks you and you're working towards properly handling umbrella headers in the VFS, I'm happy to go ahead with this.

Please either rename the variable or move that part of the check into diagnoseHeaderInclusion, though -- with this change, the variable means something quite different from its name.

lib/Lex/PPDirectives.cpp

749–750

Sure, if we're running in a mode where we actually build the modules, rather than just including them textually. Under -fmodules-local-submodule-visibility, we support providing modules semantics without doing separate compilation for module headers.

lib/Lex/PPDirectives.cpp

const FileEntry *Preprocessor::LookupFile(
     const DirectoryLookup *FromDir,
     const FileEntry *FromFile,
     const DirectoryLookup *&CurDir,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
     ModuleMap::KnownHeader *SuggestedModule,
     bool SkipCache) {
   Module *RequestingModule = getModuleForLocation(FilenameLoc);
-  bool RequestingModuleIsModuleInterface = !SourceMgr.isInMainFile(FilenameLoc);
+  bool RequestingModuleIsModuleInterface =
+      !SourceMgr.isInMainFile(FilenameLoc) && getLangOpts().CompilingModule;
 
   // If the header lookup mechanism may be relative to the current inclusion
   // stack, record the parent #includes.
   SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 16>
       Includers;
   bool BuildSystemModule = false;
   if (!FromDir && !FromFile) {
     FileID FID = getCurrentFileLexer()->getFileID();

test/VFS/Inputs/Nonmodular/A.h

+// A.h

test/VFS/Inputs/Nonmodular/Nonmodular.modulemap

+framework module Nonmodular [extern_c] {
+  umbrella header "umbrella.h"
+  export *
+  module * { export * }
+}

test/VFS/Inputs/Nonmodular/nonmodular-headers.yaml

+{
+  'version': 0,
+  'case-sensitive': 'false',
+  'ignore-non-existent-contents': 'true',
+  'roots': [
+    {
+      'type': 'directory',
+      'name': "VDIR/Nonmodular.framework/Headers",
+      'contents': [
+        {
+          'type': 'file',
+          'name': "umbrella.h",
+          'external-contents': "IN_DIR/Inputs/Nonmodular/umbrella.h"
+        },
+        {
+          'type': 'file',
+          'name': "A.h",
+          'external-contents': "IN_DIR/Inputs/Nonmodular/A.h"
+        }
+      ]
+    },
+    {
+      'type': 'directory',
+      'name': "VDIR/Nonmodular.framework/Modules",
+      'contents': [
+        {
+          'type': 'file',
+          'name': "module.modulemap",
+          'external-contents': "OUT_DIR/module.modulemap"
+        }
+      ]
+    }
+  ]
+}

test/VFS/Inputs/Nonmodular/test.c

+// expected-no-diagnostics
+
+#include "umbrella.h"

test/VFS/Inputs/Nonmodular/umbrella.h

+#ifndef __umbrella_h__
+#define __umbrella_h__
+
+#include <Nonmodular/A.h>
+#endif

test/VFS/test_nonmodular.c

+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/vdir %t/cache %t/outdir
+// We can't have module.map inside Inputs/Nonmodular.
+// RUN: cp %S/Inputs/Nonmodular/Nonmodular.modulemap %t/outdir/module.modulemap
+//
+// RUN: sed -e "s:VDIR:%t/vdir:g" -e "s:IN_DIR:%S:g" -e "s:OUT_DIR:%t/outdir:g" %S/Inputs/Nonmodular/nonmodular-headers.yaml > %t/vdir/nonmodular-headers.yaml
+// RUN: %clang_cc1 -fmodule-name=Nonmodular -fmodules -Wnon-modular-include-in-framework-module -verify -fimplicit-module-maps -fmodules-cache-path=%t/cache -ivfsoverlay %t/vdir/nonmodular-headers.yaml -I %S/Inputs -F %t/vdir -fsyntax-only %S/Inputs/Nonmodular/test.c
+
+// expected-no-diagnostics