jansvoboda11 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: > 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?) > I suggest adding a `.cc1` extension to the response files to make that clear > to anyone reading the test. Maybe `t.inferred.cc1.rsp`. Fair enough. > 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?) That's correct, it only contains additional arguments. The input to `clang-scan-deps` is a compilation database, so the assumption there is that it will contain driver command lines. Therefore, we're generating **driver** arguments. @Bigcheese, tests in your original PR invoke -cc1, what was the reasoning behind that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100934/new/ https://reviews.llvm.org/D100934 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
