[clang] Set ShowInSystemHeader for module-build and module-import remarks

[kastiglione]

Summary

Without this change, the use of -Rmodule-build and -Rmodule-import only produces diagnostics for modules built or imported by non-system code.

For example, if a project source file requires the Foundation module to be built, then -Rmodule-build will show a single diagnostic for Foundation, but not in turn for any of Foundation's (direct or indirect) dependencies. This is because the locations of those transitive module builds are initiated from system headers, which are ignored by default. When wanting to observe module building/importing, the system modules can represent a significant amount of module diagnostics, and I think should be shown by default when -Rmodule-build and -Rmodule-import are specified.

I noticed some other remarks use ShowInSystemHeader.

[jansvoboda11]

Nice. It would be nice to have tests for this, though.

Also, do you think this should be configurable on the command line? I'm thinking about users that are trying to debug their own modular code, but don't really care what happens in the SDK.

[kastiglione]

Also, do you think this should be configurable on the command line?

Do you have a flag in mind? I have worked around the current behavior with -Wno-system-header, but there are two problems with that: first, it's not obvious that it's needed (I had to debug clang to figure out why diagnostics were missing, most people wouldn't do that), and second it can result in many many warnings, which – if you are only interested in module remarks, represents a bunch of noise.

I'm thinking about users that are trying to debug their own modular code, but don't really care what happens in the SDK.

My first question is how many of those users are there? It seems to me that the module remarks are more for tooling developers than end users. Speaking as someone working on tools, I think I would always want all module-build remarks, since – correct me if I'm wrong – they're relevant only to performance, and for performance I want to see all the builds.

For module-import remarks, I could see it being more interesting to developers, but I don't know how many developers know about and use -Rmodule-import. If we want to proactively support those users, maybe the solution is two flags, say -Rmodule-import and -Rmodule-import-all (I can't think of a better spelling).

For relevant comparisons, the only thing I can think about is the -H flag, which shows all header includes, it does not exclude system headers. Speaking of which, an equivalent for modules might be helpful.

[jansvoboda11]

In D139653#3982272, @kastiglione wrote:

Also, do you think this should be configurable on the command line?

Do you have a flag in mind?

We could have a separate -Ruser-module-{build,import} (and maybe -Rsystem-module-{build,import}) besides the -Rmodule-{build,import} (that would now include both).

I'm thinking about users that are trying to debug their own modular code, but don't really care what happens in the SDK.

My first question is how many of those users are there? It seems to me that the module remarks are more for tooling developers than end users.

I'm not sure TBH. But I think Clang modules are used by people that are not familiar with the inner workings of the SDK.

Speaking as someone working on tools, I think I would always want all module-build remarks, since – correct me if I'm wrong – they're relevant only to performance, and for performance I want to see all the builds.

FWIW, these remarks have been useful for me when checking correctness of implicit builds as well. As for performance, let's pretend I'm trying to optimize LLVM's modular build. I can imagine myself starting investigating our own modules (e.g. seeing how many times clangFrontend gets built), and only dropping down into system headers once that's optimized. Getting bombarded by all the modules that are built for the SDK right away would be too noisy.

For module-import remarks, I could see it being more interesting to developers, but I don't know how many developers know about and use -Rmodule-import. If we want to proactively support those users, maybe the solution is two flags, say -Rmodule-import and -Rmodule-import-all (I can't think of a better spelling).

For relevant comparisons, the only thing I can think about is the -H flag, which shows all header includes, it does not exclude system headers. Speaking of which, an equivalent for modules might be helpful.

I'm fine with landing current functionality provided it's tested (and people like @vsapsai and @Bigcheese don't have objections).

[kastiglione]

We could have a separate -Ruser-module-{build,import} (and maybe -Rsystem-module-{build,import}) besides the -Rmodule-{build,import} (that would now include both).

that sounds good to me.

I'll add tests if there are no objections.

[vsapsai]

From my experience, ShowInSystemHeader for -Rmodule-build is overall useful. I haven't heard from people who want no remarks from system headers and the rest is just hypothetical guesses that can go both ways (some people might like one option while some people might like another option).

Don't have strong opinion about -Rmodule-import as don't use it extensively. But don't have any reasons to avoid consistency in this case, so ShowInSystemHeader for this flag makes sense.

A test case would be great, especially I think that at some point I've seen remarks from system headers (though maybe it was with -Wsystem-header).

[kastiglione]

Add a test

[kastiglione]

minor cleanup of test

[aaron.ballman]

CC @ChuanqiXu -- this relates to our discussions about diagnostics in modules. Are you okay with the direction this change is going?

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 def warn_module_config_macro_undef : Warning<
   "%select{definition|#undef}0 of configuration macro '%1' has no effect on "
   "the import of '%2'; pass '%select{-D%1=...|-U%1}0' on the command line "
   "to configure the module">,
   InGroup<ConfigMacros>;
 def note_module_def_undef_here : Note<
   "macro was %select{defined|#undef'd}0 here">;
 def remark_module_build : Remark<"building module '%0' as '%1'">,
+  ShowInSystemHeader,
   InGroup<ModuleBuild>;
 def remark_module_build_done : Remark<"finished building module '%0'">,
+  ShowInSystemHeader,
   InGroup<ModuleBuild>;
 def remark_module_lock : Remark<"locking '%0' to build module '%1'">,
   InGroup<ModuleLock>;
 def err_modules_embed_file_not_found :
   Error<"file '%0' specified by '-fmodules-embed-file=' not found">,
   DefaultFatal;
 def err_module_header_file_not_found :
   Error<"module header file '%0' not found">, DefaultFatal;

clang/include/clang/Basic/DiagnosticSerializationKinds.td

 def err_module_file_missing_top_level_submodule : Error<
   "module file '%0' is missing its top-level submodule">, DefaultFatal;
 def note_module_file_conflict : Note<
   "this is generally caused by modules with the same name found in multiple "
   "paths">;
 
 def remark_module_import : Remark<
   "importing module '%0'%select{| into '%3'}2 from '%1'">,
+  ShowInSystemHeader,
   InGroup<ModuleImport>;
 
 def err_imported_module_not_found : Error<
     "module '%0' in AST file '%1' %select{(imported by AST file '%2') |}4"
     "is not defined in any loaded module map file; "
     "maybe you need to load '%3'?">, DefaultFatal;
 def note_imported_by_pch_module_not_found : Note<
     "consider adding '%0' to the header search path">;

clang/test/Modules/system-Rmodule-build.m

+// RUN: rm -rf %t
+// RUN: mkdir -p %t/sys
+// RUN: echo '#include <B.h>' > %t/sys/A.h
+// RUN: echo '' > %t/sys/B.h
+// RUN: echo 'module A { header "A.h" }' > %t/sys/module.modulemap
+// RUN: echo 'module B { header "B.h" }' >> %t/sys/module.modulemap
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fsyntax-only %s \
+// RUN:            -isystem %t/sys -Rmodule-build 2>&1 | FileCheck %s
+
+@import A;
+
+// CHECK: building module 'A' as
+// CHECK: building module 'B' as
+// CHECK: finished building module 'B'
+// CHECK: finished building module 'A'