[cxxmodules] Error when building ROOT dict implicitly

rootcling should never implicitly build a module that is associated
with a ROOT dict as such a module needs the rootcling transformations
(e.g. to preserve the source code comments for class member as
annotated attributes). The only implicitly built modules should be
system libraries that don't need those transformations.

To inform the user that this shouldn't happen we add a new clang
diagnostic client that listens for the -Rmodule-build remarks and
verifies that every module built this way is a system module.

The reason for choosing a diag client is that clang is building
those modules in a very isolated environment that doesn't leak any
other information out that we could use to diagnose this. The only
supported way clang is informing clients about implicit module builds
seems to be the -Rmodule-build remarks.

Author: Teemperor

build/unix/module.modulemap

@@ -3,12 +3,12 @@
 // FIXME: This should be separated because it introduces header dependency
 // libThread->libCore->libThread. We should figure out a way to implement it in
 // a more reasonable way.
-module ThreadLocalStorage {
+module ThreadLocalStorage [system] {
   header "ThreadLocalStorage.h"
   export *
 }
 // These can be used in C mode.
-module ROOT_Types {
+module ROOT_Types [system] {
   module "RtypesCore.h" { header "RtypesCore.h" export * }
   module "ESTLType.h" { header "ESTLType.h" export * }
   // FIXME: This module should contain only header files with types.
@@ -34,7 +34,7 @@ module ROOT_Types {
 // Rtypes.h however has an optional dependency on rtti unless one defines
 // R__NO_INLINE_CLASSDEF. We therefore keep two versions of this module: One
 // with rtti and one without rtti.
-module ROOT_Core_Config_C {
+module ROOT_Core_Config_C [system] {
  //requires cplusplus
 
  exclude header "RtypesImp.h"
@@ -63,9 +63,9 @@ module ROOT_Core_Config_C {
 // when there is no folder GL or contents in it.
 // module ROOT_Glew {
   // Depending on the platform we get some of these three installed.
-  module "glew.h" { header "GL/glew.h" export * }
-  module "wglew.h" { header "GL/wglew.h" export * }
-  module "glxew.h" { header "GL/glxew.h" export * }
+  module "glew.h" [system] { header "GL/glew.h" export * }
+  module "wglew.h" [system] { header "GL/wglew.h" export * }
+  module "glxew.h" [system] { header "GL/glxew.h" export * }
 //  link "lib/libGLEW.so"
 //}
 

core/dictgen/src/rootcling_impl.cxx

@@ -225,6 +225,7 @@ const char *rootClingHelp =
 #include "clang/Basic/MemoryBufferCache.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/ModuleMap.h"
@@ -3853,6 +3854,105 @@ static bool FileExists(const char *file)
    return (stat(file, &buf) == 0);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// Custom diag client for clang that verifies that each implicitly build module
+/// is a system module. If not, it will let the current rootcling invocation
+/// fail with an error. All other diags beside module build remarks will be
+/// forwarded to the passed child diag client.
+///
+/// The reason why we need this is that if we built implicitly a C++ module
+/// that belongs to a ROOT dictionary, then we will miss information generated
+/// by rootcling in this file (e.g. the source code comments to annotation
+/// attributes transformation will be missing in the module file).
+class CheckModuleBuildClient : public clang::DiagnosticConsumer {
+   clang::DiagnosticConsumer *fChild;
+   bool fOwnsChild;
+   clang::ModuleMap &fMap;
+
+public:
+   CheckModuleBuildClient(clang::DiagnosticConsumer *Child, bool OwnsChild, clang::ModuleMap &Map)
+      : fChild(Child), fOwnsChild(OwnsChild), fMap(Map)
+   {
+   }
+
+   ~CheckModuleBuildClient()
+   {
+      if (fOwnsChild)
+         delete fChild;
+   }
+
+   virtual void HandleDiagnostic(clang::DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) override
+   {
+      using namespace clang::diag;
+
+      // This method catches the module_build remark from clang and checks if
+      // the implicitly built module is a system module or not. We only support
+      // building system modules implicitly.
+
+      std::string moduleName;
+      const clang::Module *module = nullptr;
+
+      // Extract the module from the diag argument with index 0.
+      const auto &ID = Info.getID();
+      if (ID == remark_module_build || ID == remark_module_build_done) {
+         moduleName = Info.getArgStdStr(0);
+         module = fMap.findModule(moduleName);
+         // We should never be able to build a module without having it in the
+         // modulemap. Still, let's print a warning that we at least tell the
+         // user that this could lead to problems.
+         if (!module) {
+            ROOT::TMetaUtils::Warning(0,
+                                      "Couldn't find module %s in the available modulemaps. This"
+                                      "prevents us from correctly diagnosing wrongly built modules.\n",
+                                      moduleName.c_str());
+         }
+      }
+
+      // Skip the diag only if we build a system module. We still print the diag
+      // when building a non-system module as we will print an error below and the
+      // user should see the detailed default clang diagnostic.
+      bool isSystemModuleDiag = module && module->IsSystem;
+      if (!isSystemModuleDiag)
+         fChild->HandleDiagnostic(DiagLevel, Info);
+
+      if (ID == remark_module_build && !isSystemModuleDiag) {
+         ROOT::TMetaUtils::Error(0,
+                                 "Had to build non-system module %s implicitly. You first need to\n"
+                                 "generate the dictionary for %s or mark the C++ module as a system\n"
+                                 "module if you provided your own system modulemap file:\n"
+                                 "%s [system] { ... }\n",
+                                 moduleName.c_str(), moduleName.c_str(), moduleName.c_str());
+      }
+   }
+
+   // All methods below just forward to the child and the default method.
+   virtual void clear() override
+   {
+      fChild->clear();
+      DiagnosticConsumer::clear();
+   }
+
+   virtual void BeginSourceFile(const clang::LangOptions &LangOpts, const clang::Preprocessor *PP) override
+   {
+      fChild->BeginSourceFile(LangOpts, PP);
+      DiagnosticConsumer::BeginSourceFile(LangOpts, PP);
+   }
+
+   virtual void EndSourceFile() override
+   {
+      fChild->EndSourceFile();
+      DiagnosticConsumer::EndSourceFile();
+   }
+
+   virtual void finish() override
+   {
+      fChild->finish();
+      DiagnosticConsumer::finish();
+   }
+
+   virtual bool IncludeInDiagnosticCounts() const override { return fChild->IncludeInDiagnosticCounts(); }
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 
 int RootClingMain(int argc,
@@ -4292,10 +4392,6 @@ int RootClingMain(int argc,
       // the shared library.
       clingArgsInterpreter.push_back("-fmodules-cache-path=" +
                                      llvm::sys::path::parent_path(sharedLibraryPathName).str());
-
-      // We enable remarks in clang for building on-demand modules. This is
-      // useful to figure out when and why on-demand modules are built by clang.
-      clingArgsInterpreter.push_back("-Rmodule-build");
    }
 
    // Convert arguments to a C array and check if they are sane
@@ -4342,9 +4438,27 @@ int RootClingMain(int argc,
       interpPtr = owningInterpPtr.get();
    }
    cling::Interpreter &interp = *interpPtr;
+   clang::CompilerInstance *CI = interp.getCI();
    // FIXME: Remove this once we switch cling to use the driver. This would handle  -fmodules-embed-all-files for us.
-   interp.getCI()->getFrontendOpts().ModulesEmbedAllFiles = true;
-   interp.getCI()->getSourceManager().setAllFilesAreTransient(true);
+   CI->getFrontendOpts().ModulesEmbedAllFiles = true;
+   CI->getSourceManager().setAllFilesAreTransient(true);
+
+   clang::Preprocessor &PP = CI->getPreprocessor();
+   clang::HeaderSearch &headerSearch = PP.getHeaderSearchInfo();
+   clang::ModuleMap &moduleMap = headerSearch.getModuleMap();
+   auto &diags = interp.getDiagnostics();
+
+   // Manually enable the module build remarks. We don't enable them via the
+   // normal clang command line arg because otherwise we would get remarks for
+   // building STL/libc when starting the interpreter in rootcling_stage1.
+   // We can't prevent these diags in any other way because we can only attach
+   // our own diag client now after the interpreter has already started.
+   diags.setSeverity(clang::diag::remark_module_build, clang::diag::Severity::Remark, clang::SourceLocation());
+
+   // Attach our own diag client that listens to the module_build remarks from
+   // clang to check that we don't build dictionary C++ modules implicitly.
+   auto recordingClient = new CheckModuleBuildClient(diags.getClient(), diags.ownsClient(), moduleMap);
+   diags.setClient(recordingClient, true);
 
    if (ROOT::TMetaUtils::GetErrorIgnoreLevel() == ROOT::TMetaUtils::kInfo) {
       ROOT::TMetaUtils::Info(0, "\n");
@@ -4537,7 +4651,6 @@ int RootClingMain(int argc,
 
    // Ignore these #pragmas to suppress "unknown pragma" warnings.
    // See LinkdefReader.cxx.
-   clang::Preprocessor& PP = interp.getCI()->getPreprocessor();
    PP.AddPragmaHandler(new IgnoringPragmaHandler("link"));
    PP.AddPragmaHandler(new IgnoringPragmaHandler("extra_include"));
    PP.AddPragmaHandler(new IgnoringPragmaHandler("read"));
@@ -4650,7 +4763,6 @@ int RootClingMain(int argc,
    ROOT::TMetaUtils::RConstructorTypes constructorTypes;
 
    // Select using DictSelection
-   clang::CompilerInstance *CI = interp.getCI();
    const unsigned int selRulesInitialSize = selectionRules.Size();
    if (dictSelection && !onepcm)
       DictSelectionReader dictSelReader(interp, selectionRules, CI->getASTContext(), normCtxt);