[clang] Don't create import decls without -fmodules

[kadircet]

Summary

When modules are disabled, there's no loaded module for these import
decls to point at. This results in crashes when there are modulemap
files but no -fmodules flag (this configuration is used for layering
check violations).

This patch makes sure import declarations are introduced only when
modules are enabled, which makes this case similar to textual headers
(no import decls are created for #include of textual headers from a
modulemap).

[kadircet]

@dblaikie this also updates getSourceDescriptor-crash.cpp, which was making use of generation of implicit import decl. I assumed that test is trying to make sure ImportDecl is represented properly, and incidentally relied on implicit generation of it. LMK if that isn't the case.

[kadircet]

• Test layering check violation diagnostics

[ChuanqiXu]

clang/test/Modules/getSourceDescriptor-crash.cpp

2

It is better to use split-file. You can find the examples by searching in the same dir.

[sammccall]

The internal state when modules are off but layering check is on is really counterintuitive, and clearly not all configurations have been tested :-(
I'm pretty sure this is the right behavior, though.

Just readability nits.

clang/lib/Sema/SemaModule.cpp

638–639

It's been a couple of weeks since we looked at this together, and I already had trouble following the logic here.

I think it would be slightly clearer by:

• flipping the order (LangOpts.Modules is "more fundanmental"/a prerequisite)
• moving this var *after* the comment that explains it
• adding a brief comment for the extra clause.

Something like

// If we are really importing a module (not just checking layering)
// due to an #include in the main file, synthesize an ImportDecl.
bool ShouldAddImport = getLangOpts().Modules && !IsInModuleIncludes;
if (ShouldAddImport) {

I'd also consider inlining+dropping the ShouldAddImport variable, since the comment explains pretty well what the condition represents

all up to you though

clang/lib/Sema/SemaModule.cpp

void Sema::BuildModuleInclude(SourceLocation DirectiveLoc, Module *Mod) {
   // Determine whether we're in the #include buffer for a module. The #includes
   // in that buffer do not qualify as module imports; they're just an
   // implementation detail of us building the module.
   //
   // FIXME: Should we even get ActOnModuleInclude calls for those?
   bool IsInModuleIncludes =
       TUKind == TU_Module &&
       getSourceManager().isWrittenInMainFile(DirectiveLoc);
 
-  bool ShouldAddImport = !IsInModuleIncludes;
-
-  // If this module import was due to an inclusion directive, create an
-  // implicit import declaration to capture it in the AST.
-  if (ShouldAddImport) {
+  // If we are really importing a module (not just checking layering) due to an
+  // #include in the main file, synthesize an ImportDecl.
+  if (getLangOpts().Modules && !IsInModuleIncludes) {
     TranslationUnitDecl *TU = getASTContext().getTranslationUnitDecl();
     ImportDecl *ImportD = ImportDecl::CreateImplicit(getASTContext(), TU,
                                                      DirectiveLoc, Mod,
                                                      DirectiveLoc);
     if (!ModuleScopes.empty())
       Context.addModuleInitializer(ModuleScopes.back().Module, ImportD);
     TU->addDecl(ImportD);
     Consumer.HandleImplicitImportDecl(ImportD);

clang/test/Modules/Inputs/modulemaps-nomodules/module.modulemap

+module M {
+  private header "header.h"
+}

clang/test/Modules/getSourceDescriptor-crash.cpp

-// RUN: %clang_cc1 -I %S/Inputs/getSourceDescriptor-crash -S -emit-llvm -debug-info-kind=limited -debugger-tuning=lldb -fimplicit-module-maps %s -o - | FileCheck %s
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -I %S/Inputs/getSourceDescriptor-crash -S -emit-llvm -debug-info-kind=limited -debugger-tuning=lldb -fmodules -fmodules-cache-path=%t -fimplicit-module-maps %s -o - | FileCheck %s
 
 #include "h1.h"
 #include "h1.h"
 
 // CHECK: DIImportedEntity
 // CHECK-SAME: entity: ![[ENTITY:[0-9]+]]
 // CHECK: ![[ENTITY]] = !DIModule
 // CHECK-SAME: name: "foo"

clang/test/Modules/modulemaps-nomodules.cpp

+// Make sure we treat includes that are part of modulemaps the same as textual
+// headers when modules are not enabled (e.g do't generate import decls, but
+// still perform layering checks).
+// No need to pass -fno-modules explicitly, absence implies negation for cc1.
+// RUN: %clang_cc1 -I %S/Inputs/modulemaps-nomodules -fmodule-map-file=%S/Inputs/modulemaps-nomodules/module.modulemap %s -verify -ast-dump | FileCheck %s
+
+#include "header.h" // expected-error{{use of private header from outside its module: 'header.h'}}
+// CHECK-NOT: ImportDecl

clang/test/PCH/Inputs/modulemaps-nomodules/module.modulemap

+module M {
+  header "header.h"
+}

clang/test/PCH/modulemaps-nomodules.cpp

+// Make sure we don't crash when serializing a PCH with an include from a
+// modulemap file in nomodules mode.
+// No need to pass -fno-modules explicitly, absence implies negation for cc1.
+// RUN: %clang_cc1 -I %S/Inputs/modulemaps-nomodules -fmodule-map-file=%S/Inputs/modulemaps-nomodules/module.modulemap %s -emit-pch -o /dev/null
+
+#include "header.h"