[PATCH] D100934: [clang][modules] Build inferred modules

Tue May 4 13:38:27 PDT 2021

dexonsmith added a comment.

In D100934#2735955 <https://reviews.llvm.org/D100934#2735955>, @jansvoboda11 wrote:

> In D100934#2733857 <https://reviews.llvm.org/D100934#2733857>, @dexonsmith wrote:
>
>> Given that there are four different things being done in this commit, it sounds naively like it should be four separate commits. If the logic is too intertwined to land each of the four pieces separately (certainly possible), can you quickly explain why, to motivate landing them together? (Or maybe there's an NFC refactoring that could be separated as a prep, to make the functional changes more isolated / easy to reason about?)
>
> I think that landing everything together provides more context for people going through git log in the future.
>
> If you really think splitting this up would be better, how about this?
>
> 1. Fix `AsWritten` for umbrella headers/directories.
> 2. Add tests to `clang/test/Modules`.

It sounds like these two pieces could be done together (since the refactor for `AsWritten` seems like it deserves a test), as a commit that adds support for building inferred modules explicitly.

> 3. Add tests to `clang/test/ClangScanDeps`, make the necessary changes to `Tooling/DependencyScanning`, add response file Python script.

Followed by this, for changing clang-scan-deps to support scanning for inferred modules.

WDYT?

================
Comment at: clang/test/ClangScanDeps/modules-inferred-explicit-build.m:10-12
+// RUN: %S/module-deps-to-rsp.py %t.db --module-name=Inferred > %t.inferred.rsp
+// RUN: %S/module-deps-to-rsp.py %t.db --module-name=System > %t.system.rsp
+// RUN: %S/module-deps-to-rsp.py %t.db --tu-index=0 > %t.tu.rsp
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > I took me a while to figure out that `.rsp` and `-to-rsp` means "response file". Can we just use `response` (or even `response-file`?) in both cases, or is `.rsp` a well-known extension I'm just not aware of?
> > 
> > I'm also not sure we want to use this in a test (see my next comment). If it's a useful utility regardless perhaps it could have a home in clang/utils.
> The Clang driver already has an option `--rsp-quoting={posix,windows}`, so I think the shortcut is somewhat known. I can rename the script to `module-deps-to-response-file`, if you think that's clearer. It could be useful in general, co moving this  to `clang/utils` sounds fine.
It's probably fine as `rsp` then, just a name I personally didn't know.

================
Comment at: clang/test/ClangScanDeps/modules-inferred-explicit-build.m:13-17
+// RUN: %clang @%t.inferred.rsp -pedantic -Werror
+// RUN: %clang @%t.system.rsp -pedantic -Werror
+// RUN: %clang -x objective-c -fsyntax-only %t.dir/modules_cdb_input.cpp \
+// RUN:   -F%S/Inputs/frameworks -fmodules -fimplicit-module-maps \
+// RUN:   -pedantic -Werror @%t.tu.rsp
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > I feel like the clang invocations that build/use modules should be in `clang/test/Modules`. Two independent things:
> > - Can clang build inferred modules explicitly? (tested in clang/test/Modules)
> > - Can clang-scan-deps generate deps for inferred modules? (tested in clang/test/ClangScanDeps)
> I agree that we should test explicit build of inferred modules in `clang/test/Modules` (without `clang-scan-deps`). I'll look into it.
> 
> I'm not sure I'd be happy with only checking the dependencies produced by `clang-scan-deps` here. Testing that the generated command-line actually works is as important IMO, and it will be even more so when we start optimizing the command line (stripping out unused header search paths, definitions etc.).
I'm not sure precisely what you mean by "works". I'm not sure just checking if it still compiles tells us much. What's important is that the modified options have the same semantics, and I think a subtle change that still compiles is more likely than preventing compilation entirely.

I don't think it would scale to check for semantic problems here -- that needs a body of testcases that covers all the things modules support.

One option would be to use the testcases (or a selection of them) in clang/test/Modules, by adding an extra RUN line that builds clang-scan-deps-discovered modules both with and without command-line stripping. For most changes, we can arrange the AST block such that skipped options won't affect it, and we could compare the hash of just that block. If and when we start stripping ignored `-D` options we'll need something smarter, but we can solve that problem later. (Ideally, this would just be a mode in clang, `clang -Xclang -fmodules-depscan`, which does an initial depscan and builds the modules in order. This might actually be an improvement on the existing implicit modules.)

@Bigcheese, maybe you can weigh in? If you both think `clang -cc1` should be tested here, I'm open to it. (In that case, though, this should not be invoking `%clang`, but `%clang_cc1`, I think... or does the response file create a driver command-line?)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100934/new/

https://reviews.llvm.org/D100934