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

Wed May 5 07:14:56 PDT 2021

dexonsmith added inline comments.

================
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
----------------
dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > 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?)
> > > 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 see, that's a good point.
> > 
> > > 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. 
> > 
> > Using tests from `clang/test/Modules` sounds nice. What are your concerns regarding stripping `-D` options?
> > 
> > > 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?
> > 
> > The `Tooling/DependencyScanning` library already prepends the `"-cc1"` argument so that build tools using the API don't have to do that on their own.
> > 
> > Using tests from clang/test/Modules sounds nice. What are your concerns regarding stripping -D options?
> 
> Oh, it’s a bit mundane, but that’ll affect the identifier info, which will in turn change the AST block and its hash. I think?
> 
> For a truly explicit module, removing “unused” `-D` options from the identifier table might not be correct, since for semantics you might want importers to pick up those definitions (I think? Are they considered exported?). For implicitly-discovered modules, we know all transitive importers have a superset of `-D`s on their command-lines so it’s safe. 
> 
> Maybe what we can do at the time is turn on a flag to avoid writing unused `-D`s to the serialized identifier table so the hashes will match? Anyway, I’m sure there is a solution. 
> > 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?
> The Tooling/DependencyScanning library already prepends the "-cc1" argument so that build tools using the API don't have to do that on their own.

I suggest adding a `.cc1` extension to the response files to make that clear to anyone reading the test. Maybe `t.inferred.cc1.rsp`.

The third clang invocation doesn't have the response file as the first argument, though, so I imagine that one doesn't contain a `-cc1`? (Or if it did, I'm not sure how it would work... I'm guessing it only adds arguments that happen to match between `-cc1` and driver?)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100934