[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Other than the giant header in the test we're still discussing, this basically 
LGTM.




Comment at: clang/test/Modules/Inputs/System/usr/include/stdint.h:2
 typedef int my_awesome_nonstandard_integer_type;
+
+/* C99 7.18.1.1 Exact-width integer types.

iana wrote:
> iana wrote:
> > benlangmuir wrote:
> > > Why do we need all this code now (I assume this is copied from the real 
> > > header)?
> > It's to support the tests I added to Modules/compiler_builtins.m. 
> > stdatomic.h and inttypes.h rely on stdint.h contents from the C library, 
> > and on complex.h and inttypes.h and math.h being present. I could just test 
> > the modules with `-ffreestanding` I think, would that be better?
> (and yes I just copied it from the builtin header)
I'm not sure what the best approach is here, but this seems heavy for a test.  
Do we expect to need to keep this up to date? If so maybe ffreestanding for the 
specific test cases this affects would be a better tradeoff.  Maybe someone 
else has a suggestion here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/test/Modules/Inputs/System/usr/include/stdint.h:2
 typedef int my_awesome_nonstandard_integer_type;
+
+/* C99 7.18.1.1 Exact-width integer types.

Why do we need all this code now (I assume this is copied from the real header)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159483: [Modules] Add a flag to control builtin headers being in system modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

I suggest we add a comment explaining the weird has_feature checks being added 
here (even if they're less weird than what they're replacing).  Maybe just a 
comment in stdarg.h and/or stddef.h and then the __std(arg|def) headers could 
just add a one-liner reference  "see comment in std(arg|def).h".




Comment at: clang/include/clang/Basic/Features.def:233
 FEATURE(modules, LangOpts.Modules)
+FEATURE(builtin_headers_in_system_modules, 
LangOpts.BuiltinHeadersInSystemModules)
 FEATURE(safe_stack, LangOpts.Sanitize.has(SanitizerKind::SafeStack))

iana wrote:
> benlangmuir wrote:
> > This appears to be in a list of `// C++ TSes`.  Near the bottom there is 
> > 'Miscellaneous language extensions' which might be a better fit.
> I thought it went with `FEATURE(modules, LangOpts.Modules)`, but I can put it 
> down in 'Miscellaneous language extensions' if that's better.
I think misc is probably better.



Comment at: clang/include/clang/Driver/Options.td:2944
   [NoXarchOption], [ClangOption, CLOption]>>;
+def fbuiltin_headers_in_system_modules : Flag <["-"], 
"fbuiltin-headers-in-system-modules">,
+  Group,

iana wrote:
> I don't love this flag name, but I couldn't come up with anything more 
> succinct. It actually does two things.
> 
>   # Turns on `ModuleMap::isBuiltinHeader` behavior, that is the builtin 
> headers get added into the system modules.
>   # Causes the module map parser to ignore all of the new builtin modules 
> added in D159064. This is needed before the modules are added to assist with 
> Apple internal staging with the Swift compiler.
> 
I think the name is good enough for a -cc1 option, unless someone else has a 
better suggestion. I suggest adding a HelpText - you could basically copy the 
description you added to LangOptions.def



Comment at: clang/include/clang/Driver/Options.td:2945-2946
+def fbuiltin_headers_in_system_modules : Flag <["-"], 
"fbuiltin-headers-in-system-modules">,
+  Group,
+  Flags<[NoXarchOption]>,
+  Visibility<[CC1Option]>,

iana wrote:
> I don't fully understand what these two do, but I think they apply here?
I agree they apply. `f_Group` affects a `ClaimAllArgs` call (via inheritance 
from `CompileOnly_Group`) but mostly puts it under a group in documentation.  
`NoXarchOption` means you can't modify this option per-arch in a multi-arch 
compilation.  That seems like what you want.



Comment at: clang/lib/Lex/ModuleMap.cpp:1540
+/// or added to Map's Modules/ModuleScopeIDs).
+bool IgnoreModules = false;
+

iana wrote:
> This an everything under it is gross, but the only other thing I could think 
> of was to duplicate the module map and decided to load a special one for 
> BuiltinHeadersInSystemModules. That seemed weirder.
Would it be possible to get the right semantics using a `requires` decl in the 
modules plus a new module feature?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159483: [Modules] Add a flag to control builtin headers being in system modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

>> How does this work today? Wouldn't the include guard prevent this?
>
> Today they don't define their include guard when building with modules.

Thanks - I can see some headers behave that way, or in other cases there are 
other sorts of has_feature(modules) checks that I can see you're modifying, but 
what about all the *va_* headers that currently just seem to have normal header 
guards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159483: [Modules] Add a flag to control builtin headers being in system modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

> but need to be repeatedly included when they're used in system modules

How does this work today? Wouldn't the include guard prevent this?




Comment at: clang/include/clang/Basic/Features.def:233
 FEATURE(modules, LangOpts.Modules)
+FEATURE(builtin_headers_in_system_modules, 
LangOpts.BuiltinHeadersInSystemModules)
 FEATURE(safe_stack, LangOpts.Sanitize.has(SanitizerKind::SafeStack))

This appears to be in a list of `// C++ TSes`.  Near the bottom there is 
'Miscellaneous language extensions' which might be a better fit.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2851
+  if (!SDKInfo)
+return false;
+  

I would have expected the default to be true; do old SDKs not have SDKInfo or 
something?



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2856
+  case Darwin::MacOS:
+return SDKVersion >= VersionTuple(100U);
+  case Darwin::IPhoneOS:

Are these really supposed to be 100 or is this a placeholder?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Headers/module.modulemap:156
 
-module _Builtin_stddef_max_align_t [system] [extern_c] {
-  header "__stddef_max_align_t.h"

Is the assumption here that directly `@import _Builtin_stddef_max_align_t` is 
unsupported so we don't need to provide a shim to keep this working?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159064

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159016: [clang] Fix assertion failure using -MJ with -fsyntax-only

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/test/Driver/compilation_database_fsyntax_only.c:1
+// RUN: mkdir -p %t.workdir && cd %t.workdir
+// RUN: %clang -fsyntax-only %s -MJ - 2>&1 | FileCheck %s

MaskRay wrote:
> The test can be added into `compilation_database.c`.
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159016

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159016: [clang] Fix assertion failure using -MJ with -fsyntax-only

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaca23d8ac30d: [clang] Fix assertion failure using -MJ with 
-fsyntax-only (authored by benlangmuir).

Changed prior to commit:
  https://reviews.llvm.org/D159016?vs=554046=554765#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159016

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/compilation_database.c


Index: clang/test/Driver/compilation_database.c
===
--- clang/test/Driver/compilation_database.c
+++ clang/test/Driver/compilation_database.c
@@ -1,11 +1,21 @@
 // RUN: mkdir -p %t.workdir && cd %t.workdir
 // RUN: %clang -no-canonical-prefixes -fintegrated-as -MD -MP 
--sysroot=somewhere -c -x c %s -xc++ %s -Wall -MJ - 2>&1 | FileCheck %s
 // RUN: not %clang -no-canonical-prefixes -c -x c %s -MJ %s/non-existant 2>&1 
| FileCheck --check-prefix=ERROR %s
+// RUN: %clang -fsyntax-only %s -MJ - 2>&1 | FileCheck %s 
-check-prefix=SYNTAX_ONLY
 
 // CHECK: { "directory": "{{[^"]*}}workdir",  "file": 
"[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": 
"compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc", 
"[[SRC]]", "-o", "compilation_database.o", "-no-canonical-prefixes", 
"-fintegrated-as", "--sysroot=somewhere", "-c", "-Wall",{{.*}} 
"--target={{[^"]+}}"]},
 // CHECK: { "directory": "{{.*}}",  "file": 
"[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": 
"compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc++", 
"[[SRC]]", "-o", "compilation_database.o", "-no-canonical-prefixes", 
"-fintegrated-as", "--sysroot=somewhere", "-c", "-Wall",{{.*}} 
"--target={{[^"]+}}"]},
 // ERROR: error: compilation database '{{.*}}/non-existant' could not be 
opened:
 
+// SYNTAX_ONLY:  {
+// SYNTAX_ONLY-SAME: "directory": "{{[^"]*}}workdir",
+// SYNTAX_ONLY-SAME: "file": "{{[^"]*}}compilation_database.c"
+// SYNTAX_ONLY-NOT:  "output"
+// SYNTAX_ONLY-SAME: "arguments": [
+// SYNTAX_ONLY-NOT:"-o"
+// SYNTAX_ONLY-SAME: ]
+// SYNTAX_ONLY-SAME: }
+
 int main(void) {
   return 0;
 }
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2433,7 +2433,8 @@
 CWD = ".";
   CDB << "{ \"directory\": \"" << escape(*CWD) << "\"";
   CDB << ", \"file\": \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
+  if (Output.isFilename())
+CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
   CDB << ", \"arguments\": [\"" << escape(D.ClangExecutable) << "\"";
   SmallString<128> Buf;
   Buf = "-x";
@@ -2445,7 +2446,8 @@
 CDB << ", \"" << escape(Buf) << "\"";
   }
   CDB << ", \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"-o\", \"" << escape(Output.getFilename()) << "\"";
+  if (Output.isFilename())
+CDB << ", \"-o\", \"" << escape(Output.getFilename()) << "\"";
   for (auto : Args) {
 auto  = A->getOption();
 // Skip language selection, which is positional.


Index: clang/test/Driver/compilation_database.c
===
--- clang/test/Driver/compilation_database.c
+++ clang/test/Driver/compilation_database.c
@@ -1,11 +1,21 @@
 // RUN: mkdir -p %t.workdir && cd %t.workdir
 // RUN: %clang -no-canonical-prefixes -fintegrated-as -MD -MP --sysroot=somewhere -c -x c %s -xc++ %s -Wall -MJ - 2>&1 | FileCheck %s
 // RUN: not %clang -no-canonical-prefixes -c -x c %s -MJ %s/non-existant 2>&1 | FileCheck --check-prefix=ERROR %s
+// RUN: %clang -fsyntax-only %s -MJ - 2>&1 | FileCheck %s -check-prefix=SYNTAX_ONLY
 
 // CHECK: { "directory": "{{[^"]*}}workdir",  "file": "[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": "compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc", "[[SRC]]", "-o", "compilation_database.o", "-no-canonical-prefixes", "-fintegrated-as", "--sysroot=somewhere", "-c", "-Wall",{{.*}} "--target={{[^"]+}}"]},
 // CHECK: { "directory": "{{.*}}",  "file": "[[SRC:[^"]+[/|\\]compilation_database.c]]", "output": "compilation_database.o", "arguments": ["{{[^"]*}}clang{{[^"]*}}", "-xc++", "[[SRC]]", "-o", "compilation_database.o", "-no-canonical-prefixes", "-fintegrated-as", "--sysroot=somewhere", "-c", "-Wall",{{.*}} "--target={{[^"]+}}"]},
 // ERROR: error: compilation database '{{.*}}/non-existant' could not be opened:
 
+// SYNTAX_ONLY:  {
+// SYNTAX_ONLY-SAME: "directory": "{{[^"]*}}workdir",
+// SYNTAX_ONLY-SAME: "file": "{{[^"]*}}compilation_database.c"
+// SYNTAX_ONLY-NOT:  "output"
+// SYNTAX_ONLY-SAME: "arguments": [
+// SYNTAX_ONLY-NOT:"-o"
+// SYNTAX_ONLY-SAME: ]
+// SYNTAX_ONLY-SAME: }
+
 int main(void) {
   return 0;
 }
Index: clang/lib/Driver/ToolChains/Clang.cpp

[PATCH] D159016: [clang] Fix assertion failure using -MJ with -fsyntax-only

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added a reviewer: MaskRay.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If there is no output filename we should not assert when writing output for -MJ.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159016

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/compilation_database_fsyntax_only.c


Index: clang/test/Driver/compilation_database_fsyntax_only.c
===
--- /dev/null
+++ clang/test/Driver/compilation_database_fsyntax_only.c
@@ -0,0 +1,16 @@
+// RUN: mkdir -p %t.workdir && cd %t.workdir
+// RUN: %clang -fsyntax-only %s -MJ - 2>&1 | FileCheck %s
+
+// CHECK:  {
+// CHECK-SAME: "directory": "{{[^"]*}}workdir",
+// CHECK-SAME: "file": "{{[^"]*}}compilation_database_fsyntax_only.c"
+// CHECK-NOT:  "output"
+// CHECK-SAME: "arguments": [
+// CHECK-NOT:"-o"
+// CHECK-SAME: ]
+// CHECK-SAME: }
+
+
+int main(void) {
+  return 0;
+}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2426,7 +2426,8 @@
 CWD = ".";
   CDB << "{ \"directory\": \"" << escape(*CWD) << "\"";
   CDB << ", \"file\": \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
+  if (Output.isFilename())
+CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
   CDB << ", \"arguments\": [\"" << escape(D.ClangExecutable) << "\"";
   SmallString<128> Buf;
   Buf = "-x";
@@ -2438,7 +2439,8 @@
 CDB << ", \"" << escape(Buf) << "\"";
   }
   CDB << ", \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"-o\", \"" << escape(Output.getFilename()) << "\"";
+  if (Output.isFilename())
+CDB << ", \"-o\", \"" << escape(Output.getFilename()) << "\"";
   for (auto : Args) {
 auto  = A->getOption();
 // Skip language selection, which is positional.


Index: clang/test/Driver/compilation_database_fsyntax_only.c
===
--- /dev/null
+++ clang/test/Driver/compilation_database_fsyntax_only.c
@@ -0,0 +1,16 @@
+// RUN: mkdir -p %t.workdir && cd %t.workdir
+// RUN: %clang -fsyntax-only %s -MJ - 2>&1 | FileCheck %s
+
+// CHECK:  {
+// CHECK-SAME: "directory": "{{[^"]*}}workdir",
+// CHECK-SAME: "file": "{{[^"]*}}compilation_database_fsyntax_only.c"
+// CHECK-NOT:  "output"
+// CHECK-SAME: "arguments": [
+// CHECK-NOT:"-o"
+// CHECK-SAME: ]
+// CHECK-SAME: }
+
+
+int main(void) {
+  return 0;
+}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2426,7 +2426,8 @@
 CWD = ".";
   CDB << "{ \"directory\": \"" << escape(*CWD) << "\"";
   CDB << ", \"file\": \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
+  if (Output.isFilename())
+CDB << ", \"output\": \"" << escape(Output.getFilename()) << "\"";
   CDB << ", \"arguments\": [\"" << escape(D.ClangExecutable) << "\"";
   SmallString<128> Buf;
   Buf = "-x";
@@ -2438,7 +2439,8 @@
 CDB << ", \"" << escape(Buf) << "\"";
   }
   CDB << ", \"" << escape(Input.getFilename()) << "\"";
-  CDB << ", \"-o\", \"" << escape(Output.getFilename()) << "\"";
+  if (Output.isFilename())
+CDB << ", \"-o\", \"" << escape(Output.getFilename()) << "\"";
   for (auto : Args) {
 auto  = A->getOption();
 // Skip language selection, which is positional.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158469: [clang][deps] Compute command lines and file deps on-demand

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

> I tried that approach, but found it way too easy to keep `ModuleDeps` around, 
> which keep scanning instances alive, and use tons of memory.

It seems like the problem (too easy to keep around MD) is the same either way, 
it's just instead of wasting memory it's leaving dangling pointers that could 
cause UAF if they're actually used.  Where were you seeing MD held too long? I 
wonder if there's another way to fix that.

> Hmm, maybe we could avoid holding on to the whole CompilerInstance.

This seems promising!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158469

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158469: [clang][deps] Compute command lines and file deps on-demand

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

I find this a bit hard to understand as far as object lifetime is concerned: 
you're storing the `ScanInstance` in `TranslationUnitDeps`, but that's a layer 
above the actual consumer interface, which means every consumer needs to 
understand how this lifetime is managed or it will have dangling references by 
default.  You also seem to have ScanInstance stored twice -- once explicitly 
and once in the graph.

What do you think of an alternate design where `ModuleDeps` has 
`shared_ptr MDC` and `ModuleDepCollector` has 
`shared_ptr`?  That way we don't need to explicitly expose 
the scan instance to the consumer, it comes with the `ModuleDeps`, which then 
keeps all the necessary memory alive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158469

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158572: [clang][modules] Use relative offsets for input files

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/include/clang/Serialization/ModuleFile.h:249
+  /// Absolute offset of the start of the input-files block.
+  uint64_t InputFilesOffsetBase;
+

jansvoboda11 wrote:
> benlangmuir wrote:
> > Doesn't `InputFilesCursor` already know where the input files block starts?
> I think it does. We should be able to remove this and always call 
> `InputFilesCursor::GetCurrentBitNo()` at the start. I implemented it this way 
> because it's consistent with the existing pattern: 
> `SLocEntryCursor`/`SourceManagerBlockStartOffset`, 
> `MacroCursor`/`MacroOffsetsBase`, `DeclsCursor`/`DeclsBlockStartOffset`.
> 
> LMK if you feel strongly about it and I can look into getting rid of the 
> extra offset base members.
Since it's consistent with other uses I'm fine with keeping it as-is for this 
patch. Would be nice to clean them all up in a follow up, but not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158572

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158573: [clang][modules] Move `UNHASHED_CONTROL_BLOCK` up in the AST file

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Serialization/ASTWriter.cpp:1169
+  writeSignature(Sig, Out);
+  std::copy_n(Out.begin(), Out.size(), Buffer.begin() + Offset);
+};

jansvoboda11 wrote:
> I don't feel great about removing `const` from `Buffer` and writing into it 
> directly, circumventing `Stream`. This currently works fine, because the 
> `Stream` in `ASTWriter` is never backed by a file (and therefore never 
> flushed). But if that ever changes, this code is problematic. Do you think 
> this is worth spending more time on?
> 
> `Stream` already has the `BackpatchWord()` function, which makes sure the 
> underlying file is updated as well in case we're backpatching already-flushed 
> data.
Interesting; what was the reason for not using BackpatchWord from the start? 
IIUC our signatures should be a multiple of 4 bytes already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158573

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158573: [clang][modules] Move `UNHASHED_CONTROL_BLOCK` up in the AST file

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:2685
+  for (unsigned I = 0; I < ASTFileSignature::size; ++I)
+Sig[I] = endian::readNext(Blob);
+  return Sig;

uint8_t has no endianness and has alignment 1 anyway; you can just load it 
directly or use the same implementation as ASTFileSignature::create



Comment at: clang/lib/Serialization/ASTWriter.cpp:1143
 
-ASTFileSignature ASTWriter::writeUnhashedControlBlock(Preprocessor ,
-  ASTContext ) {
+static constexpr ASTFileSignature DummySignature{
+{1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}};

I wonder if we should hoist these dummy values to the ASTFileSignature type and 
then assert that they are never read from an ASTReader



Comment at: clang/lib/Serialization/ASTWriter.cpp:1152
+  using namespace llvm::support;
+  endian::Writer LE(Out, little);
+  for (uint8_t Byte : Sig)

We don't need an endian writer when it's bytes



Comment at: clang/lib/Serialization/ASTWriter.cpp:1161
+  if (WritingModule &&
+  PP->getHeaderSearchInfo().getHeaderSearchOpts().ModulesHashContent) {
+

Nit: I think this would be clearer with an early-exit now that it's in a 
separate function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158573

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158572: [clang][modules] Use relative offsets for input files

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/include/clang/Serialization/ModuleFile.h:249
+  /// Absolute offset of the start of the input-files block.
+  uint64_t InputFilesOffsetBase;
+

Doesn't `InputFilesCursor` already know where the input files block starts?



Comment at: clang/lib/Serialization/ASTReader.cpp:5334
   BitstreamCursor InputFilesCursor;
+  uint64_t InputFilesOffsetBase;
 

We should initialize this to something - either 0 or maybe ~0 so it will be 
invalid?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158572

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158136: [clang][modules] Avoid storing command-line macro definitions into implicitly built PCM files

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/Modules/module_file_info.m:44
 // CHECK:   Uses compiler/target-specific predefines [-undef]: Yes
 // CHECK:   Uses detailed preprocessing record (for indexing): No
 // CHECK: Input file: {{.*}}module.map

`CHECK-NOT: Predefined macros` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158021: [clang][modules] Mark builtin header 'inttypes.h' for modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 550474.
benlangmuir added a reviewer: vsapsai.
benlangmuir added a comment.

Add missing test updates: tests using the `Inputs/System/usr/include` should be 
using `-internal-isystem` to get the correct search path order with respect to 
the resource dir. The tests that were previously using `-isystem` were only 
working before because the other headers wrap their `#include_next` in 
`__has_include_next`, which was causing them to silently be missing these 
headers. With inttypes.h the include_next is unguarded, which revealed the 
issue.  Note: even if we someday add the has_include_next guard to inttypes.h 
the test change is still correct.


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

https://reviews.llvm.org/D158021

Files:
  clang/lib/Lex/ModuleMap.cpp
  clang/test/Modules/Inputs/System/usr/include/module.map
  clang/test/Modules/Werror-Wsystem-headers.m
  clang/test/Modules/crash-vfs-include-pch.m
  clang/test/Modules/cstd.m
  clang/test/Modules/pch-used.m

Index: clang/test/Modules/pch-used.m
===
--- clang/test/Modules/pch-used.m
+++ clang/test/Modules/pch-used.m
@@ -1,8 +1,8 @@
 // UNSUPPORTED: target={{.*}}-zos{{.*}}, target={{.*}}-aix{{.*}}
 // RUN: rm -rf %t
 // RUN: mkdir %t
-// RUN: %clang_cc1 -x objective-c-header -emit-pch %S/Inputs/pch-used.h -o %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 -isystem %S/Inputs/System/usr/include
-// RUN: %clang_cc1 %s -include-pch %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 -isystem %S/Inputs/System/usr/include -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -x objective-c-header -emit-pch %S/Inputs/pch-used.h -o %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 -internal-isystem %S/Inputs/System/usr/include
+// RUN: %clang_cc1 %s -include-pch %t/pch-used.h.pch -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 -internal-isystem %S/Inputs/System/usr/include -emit-llvm -o - | FileCheck %s
 
 void f(void) { SPXTrace(); }
 void g(void) { double x = DBL_MAX; }
Index: clang/test/Modules/cstd.m
===
--- clang/test/Modules/cstd.m
+++ clang/test/Modules/cstd.m
@@ -28,3 +28,11 @@
 #  error "bool was not defined!"
 #endif
 
+// Supplied by compiler, which forwards to the "/usr/include" version.
+@import cstd.inttypes;
+#ifndef __CLANG_INTTYPES_H
+#error "__CLANG_INTTYPES_H was not defined!"
+#endif
+#ifndef MY_PRIi32
+#error "MY_PRIi32 was not defined!"
+#endif
Index: clang/test/Modules/crash-vfs-include-pch.m
===
--- clang/test/Modules/crash-vfs-include-pch.m
+++ clang/test/Modules/crash-vfs-include-pch.m
@@ -6,12 +6,12 @@
 // RUN: %clang_cc1 -x objective-c-header -emit-pch %S/Inputs/pch-used.h \
 // RUN: -o %t/out/pch-used.h.pch -fmodules -fimplicit-module-maps \
 // RUN: -fmodules-cache-path=%t/cache -O0 \
-// RUN: -isystem %S/Inputs/System/usr/include
+// RUN: -internal-isystem %S/Inputs/System/usr/include
 
 // RUN: env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
 // RUN: not %clang %s -E -include-pch %t/out/pch-used.h.pch -fmodules -nostdlibinc \
 // RUN: -fimplicit-module-maps -fmodules-cache-path=%t/cache -O0 \
-// RUN: -Xclang -fno-validate-pch -isystem %S/Inputs/System/usr/include \
+// RUN: -Xclang -fno-validate-pch -Xclang -internal-isystem -Xclang %S/Inputs/System/usr/include \
 // RUN: -o %t/output.E 2>&1 | FileCheck %s
 
 // RUN: FileCheck --check-prefix=CHECKSH %s -input-file %t/crash-vfs-*.sh
Index: clang/test/Modules/Werror-Wsystem-headers.m
===
--- clang/test/Modules/Werror-Wsystem-headers.m
+++ clang/test/Modules/Werror-Wsystem-headers.m
@@ -4,17 +4,17 @@
 
 // Initial module build
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN: -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify
+// RUN: -internal-isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify
 // RUN: cp %t/cstd.pcm %t-saved/cstd.pcm
 
 // Even with -Werror don't rebuild a system module
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN: -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify -Werror
+// RUN: -internal-isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify -Werror
 // RUN: diff %t/cstd.pcm %t-saved/cstd.pcm
 
 // Unless -Wsystem-headers is on
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash \
-// RUN: -isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify \
+// RUN: -internal-isystem %S/Inputs/System/usr/include -fsyntax-only %s -verify \
 // RUN: 

[PATCH] D158021: [clang][modules] Mark builtin header 'inttypes.h' for modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: iana, jansvoboda11.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Like the other enumerated builtin headers, inttypes.h can be declared by 
another system module.

rdar://113924039


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158021

Files:
  clang/lib/Lex/ModuleMap.cpp
  clang/test/Modules/Inputs/System/usr/include/module.map
  clang/test/Modules/cstd.m


Index: clang/test/Modules/cstd.m
===
--- clang/test/Modules/cstd.m
+++ clang/test/Modules/cstd.m
@@ -28,3 +28,11 @@
 #  error "bool was not defined!"
 #endif
 
+// Supplied by compiler, which forwards to the "/usr/include" version.
+@import cstd.inttypes;
+#ifndef __CLANG_INTTYPES_H
+#error "__CLANG_INTTYPES_H was not defined!"
+#endif
+#ifndef MY_PRIi32
+#error "MY_PRIi32 was not defined!"
+#endif
Index: clang/test/Modules/Inputs/System/usr/include/module.map
===
--- clang/test/Modules/Inputs/System/usr/include/module.map
+++ clang/test/Modules/Inputs/System/usr/include/module.map
@@ -4,6 +4,11 @@
 header "float.h"
   }
 
+  // In both directories (compiler support version wins, forwards)
+  module inttypes {
+header "inttypes.h"
+  }
+
   // Only in system headers directory
   module stdio {
 header "stdio.h"
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -379,6 +379,7 @@
 bool ModuleMap::isBuiltinHeader(StringRef FileName) {
   return llvm::StringSwitch(FileName)
.Case("float.h", true)
+   .Case("inttypes.h", true)
.Case("iso646.h", true)
.Case("limits.h", true)
.Case("stdalign.h", true)


Index: clang/test/Modules/cstd.m
===
--- clang/test/Modules/cstd.m
+++ clang/test/Modules/cstd.m
@@ -28,3 +28,11 @@
 #  error "bool was not defined!"
 #endif
 
+// Supplied by compiler, which forwards to the "/usr/include" version.
+@import cstd.inttypes;
+#ifndef __CLANG_INTTYPES_H
+#error "__CLANG_INTTYPES_H was not defined!"
+#endif
+#ifndef MY_PRIi32
+#error "MY_PRIi32 was not defined!"
+#endif
Index: clang/test/Modules/Inputs/System/usr/include/module.map
===
--- clang/test/Modules/Inputs/System/usr/include/module.map
+++ clang/test/Modules/Inputs/System/usr/include/module.map
@@ -4,6 +4,11 @@
 header "float.h"
   }
 
+  // In both directories (compiler support version wins, forwards)
+  module inttypes {
+header "inttypes.h"
+  }
+
   // Only in system headers directory
   module stdio {
 header "stdio.h"
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -379,6 +379,7 @@
 bool ModuleMap::isBuiltinHeader(StringRef FileName) {
   return llvm::StringSwitch(FileName)
.Case("float.h", true)
+   .Case("inttypes.h", true)
.Case("iso646.h", true)
.Case("limits.h", true)
.Case("stdalign.h", true)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=

[Ben Langmuir via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc5cbba3196d: [clang][modules] Add 
-Wsystem-headers-in-module= (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156948

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Wsystem-headers-in-module.c
  clang/test/Modules/Wsystem-headers-in-module.c

Index: clang/test/Modules/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/Modules/Wsystem-headers-in-module.c
@@ -0,0 +1,32 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN:   -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN:   2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
Index: clang/test/ClangScanDeps/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Wsystem-headers-in-module.c
@@ -0,0 +1,56 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+  "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -12,6 +12,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/BLAKE3.h"
 #include "llvm/Support/StringSaver.h"
 #include 
@@ -172,6 +173,13 @@
 CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
   }
 
+  // Apply -Wsystem-headers-in-module for the current module.
+  if (llvm::is_contained(CI.getDiagnosticOpts().SystemHeaderWarningsModules,
+ Deps.ID.ModuleName))
+CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+  // Remove the now unused option(s).
+  CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
   Optimize(CI);
 
   return 

[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked an inline comment as done.
benlangmuir added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticOptions.h:128
+  /// whether -Wsystem-headers is enabled on a per-module basis.
+  std::vector SystemHeaderWarningsModules;
+

iana wrote:
> benlangmuir wrote:
> > jansvoboda11 wrote:
> > > Out of interest, is there an existing use-case for having multiple 
> > > modules here, or is this just future-proofing?
> > I'm not using it currently, but I think it makes sense to handle any 
> > number. You could have multiple such modules you're testing together.
> I'd imagine you'd typically have Framework and Framework_Private in there
Ah, good point. I am in fact doing that already and just forgot.


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

https://reviews.llvm.org/D156948

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157066: [clang][modules][deps] Create more efficient API for visitation of `ModuleFile` inputs

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Serialization/ModuleFile.h:72
+  bool TopLevel;
+  bool ModuleMap;
 };

jansvoboda11 wrote:
> benlangmuir wrote:
> > Is there something using this new split? It seems like every user is using 
> > `&&` for these
> You're right. I recall needing this on (now probably abandoned) patch, so I 
> thought I might as well add the extra bit now, sine I'm changing the format 
> anyways. I'm fine with removing this.
It's fine, I was just making sure I didn't miss something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157066: [clang][modules][deps] Create more efficient API for visitation of `ModuleFile` inputs

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a subscriber: vsapsai.
benlangmuir added a comment.

CC @vsapsai since he was also making name vs. name-as-requested change in 
modules




Comment at: clang/include/clang/Serialization/ModuleFile.h:72
+  bool TopLevel;
+  bool ModuleMap;
 };

Is there something using this new split? It seems like every user is using `&&` 
for these



Comment at: clang/lib/Serialization/ASTReader.cpp:2411
   bool Transient = FI.Transient;
-  StringRef Filename = FI.Filename;
+  StringRef Filename = FI.FilenameAsRequested;
   uint64_t StoredContentHash = FI.ContentHash;

It's not clear to me why this one changed



Comment at: clang/lib/Serialization/ASTReader.cpp:9323
 
+void ASTReader::visitInputFileInfos(
+serialization::ModuleFile , bool IncludeSystem,

Can we rewrite `visitInputFiles` on top of this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157029: [llvm] Construct option spelling at compile-time

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: llvm/include/llvm/Option/Option.h:103
 
+  StringLiteral getSpelling() const {
+assert(Info && "Must have a valid info!");

This could use a doc comment to differentiate it from other string 
representations.

How does this compare with `Arg::getSpelling`? With `Arg`, IIUC the "spelling" 
is how it was actually written rather than a canonical form.  That might be 
confusing if this one is canonical; so we should at least clearly document it 
or maybe put "canonical" in the API name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157029

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked an inline comment as done.
benlangmuir added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticOptions.h:128
+  /// whether -Wsystem-headers is enabled on a per-module basis.
+  std::vector SystemHeaderWarningsModules;
+

jansvoboda11 wrote:
> Out of interest, is there an existing use-case for having multiple modules 
> here, or is this just future-proofing?
I'm not using it currently, but I think it makes sense to handle any number. 
You could have multiple such modules you're testing together.



Comment at: clang/lib/Frontend/CompilerInstance.cpp:1238-1239
 
+  for (StringRef Name : DiagOpts.SystemHeaderWarningsModules)
+if (Name == ModuleName)
+  Instance.getDiagnostics().setSuppressSystemWarnings(false);

jansvoboda11 wrote:
> I assume `llvm::is_contained()` wouldn't be okay with the `std::string` and 
> `StringRef` mismatch?
Nope, I just didn't think of it here for some reason. Updated.


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

https://reviews.llvm.org/D156948

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 547266.
benlangmuir added a comment.

Use `llvm::is_contained`


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

https://reviews.llvm.org/D156948

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Wsystem-headers-in-module.c
  clang/test/Modules/Wsystem-headers-in-module.c

Index: clang/test/Modules/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/Modules/Wsystem-headers-in-module.c
@@ -0,0 +1,32 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN:   -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN:   2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
Index: clang/test/ClangScanDeps/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Wsystem-headers-in-module.c
@@ -0,0 +1,56 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+  "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -12,6 +12,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/BLAKE3.h"
 #include "llvm/Support/StringSaver.h"
 #include 
@@ -172,6 +173,13 @@
 CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
   }
 
+  // Apply -Wsystem-headers-in-module for the current module.
+  if (llvm::is_contained(CI.getDiagnosticOpts().SystemHeaderWarningsModules,
+ Deps.ID.ModuleName))
+CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+  // Remove the now unused option(s).
+  CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
   Optimize(CI);
 
   return CI;
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ 

[PATCH] D157046: [clang] Abstract away string allocation in command line generation

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4323
+GenerateArg(Consumer, OPT_darwin_target_variant_sdk_version_EQ,
+Opts.DarwinTargetVariantSDKVersion.getAsString());
 }

Maybe not worth micro optimizing, but I noticed these two are allocating 
strings unnecessarily if we had an overload for things that can print to a 
raw_ostream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157050: [clang] NFC: Avoid double allocation when generating command line

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Dupe of https://reviews.llvm.org/D157048 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157050

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157035: [clang][cli] Accept option spelling as `Twine`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

This code asserts that the Twine is a single stringref, but it's also relying 
on the fact it's null-terminated; can we check for that as well, or at least 
document it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157035

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, iana.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Add a way to enable -Wsystem-headers only for a specific module. This is useful 
for validating a module that would otherwise not see system header diagnostics 
without being flooded by diagnostics for unrelated headers/modules. It's 
relatively common for a module to be marked [system] but still wish to validate 
itself explicitly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156948

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Wsystem-headers-in-module.c
  clang/test/Modules/Wsystem-headers-in-module.c

Index: clang/test/Modules/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/Modules/Wsystem-headers-in-module.c
@@ -0,0 +1,32 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN:   -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN:   2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
Index: clang/test/ClangScanDeps/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Wsystem-headers-in-module.c
@@ -0,0 +1,56 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+  "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -172,6 +172,13 @@
 CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
   }
 
+  // Apply -Wsystem-headers-in-module for the current module.
+  for (StringRef Name : CI.getDiagnosticOpts().SystemHeaderWarningsModules)
+if (Name == Deps.ID.ModuleName)
+  CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+  // Remove the now unused option(s).
+  CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
   Optimize(CI);
 
   return CI;
Index: 

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

MrTrillian wrote:
> benlangmuir wrote:
> > MrTrillian wrote:
> > > benlangmuir wrote:
> > > > MrTrillian wrote:
> > > > > benlangmuir wrote:
> > > > > > Removing .. can change where the path points in the presence of 
> > > > > > symlinks; is this needed?
> > > > > > Removing .. can change where the path points in the presence of 
> > > > > > symlinks; is this needed?
> > > > > 
> > > > > @benlangmuir That's true and not ideal, but `makeAbsolute` will not 
> > > > > resolve `/./` or `/../` in paths, so it's not a canonicalization and 
> > > > > some tests were failing because of that. One alternative would be to 
> > > > > use `makeAbsolute` + `remove_dots` on Windows (where removing dot 
> > > > > dots is semantically correct) and `getRealPath` on Unix, like I do in 
> > > > > lit. Suggestions?
> > > > Wouldn't removing .. have the same issue with symlinks on Windows? I 
> > > > know symlinks are less common there, but it's not clear to me why it 
> > > > would be correct.  I guess you could also check if the paths resolve to 
> > > > the same file after removing ..
> > > > 
> > > > 
> > > @benlangmuir Windows simplifies `\..\` in paths before starting to walk 
> > > the filesystem. 
> > > https://superuser.com/questions/1502360/different-behavior-of-double-dots-and-symlinks-in-windows-and-unix
> > > 
> > > If I detected that the path didn't resolve to the same file after 
> > > removing `..`, what would I do?
> > > 
> > > I think the current logic will only end up using the `else` branch on 
> > > Windows. At least I can't think of a scenario where the root would change 
> > > from using realpath on unix.
> > Thanks, I wasn't aware of that subtlety! I suggest we add a comment about 
> > that here and also mention (or assert) that this is only reachable on 
> > Windows.
> > Thanks, I wasn't aware of that subtlety! I suggest we add a comment about 
> > that here and also mention (or assert) that this is only reachable on 
> > Windows.
> 
> @benlangmuir Accounted for in lastest revision. The root-preserving 
> canonicalization logic is now Windows-only.
Thanks, looks good.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156563: [clang][deps] Remove `ModuleDeps::ImportedByMainFile`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

This flag was always weird to me, thanks for cleaning it up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156492: [clang][deps] Make the C++ API more type-safe

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:175
   std::vector PrebuiltModuleDeps;
-  llvm::MapVector>
+  llvm::MapVector>
   ClangModuleDeps;

You can drop the third template argument; it now matches the default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156492

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

LGTM other than my suggestion to add a comment. @tahonermann are all your 
comments addressed at this point?




Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

MrTrillian wrote:
> benlangmuir wrote:
> > MrTrillian wrote:
> > > benlangmuir wrote:
> > > > Removing .. can change where the path points in the presence of 
> > > > symlinks; is this needed?
> > > > Removing .. can change where the path points in the presence of 
> > > > symlinks; is this needed?
> > > 
> > > @benlangmuir That's true and not ideal, but `makeAbsolute` will not 
> > > resolve `/./` or `/../` in paths, so it's not a canonicalization and some 
> > > tests were failing because of that. One alternative would be to use 
> > > `makeAbsolute` + `remove_dots` on Windows (where removing dot dots is 
> > > semantically correct) and `getRealPath` on Unix, like I do in lit. 
> > > Suggestions?
> > Wouldn't removing .. have the same issue with symlinks on Windows? I know 
> > symlinks are less common there, but it's not clear to me why it would be 
> > correct.  I guess you could also check if the paths resolve to the same 
> > file after removing ..
> > 
> > 
> @benlangmuir Windows simplifies `\..\` in paths before starting to walk the 
> filesystem. 
> https://superuser.com/questions/1502360/different-behavior-of-double-dots-and-symlinks-in-windows-and-unix
> 
> If I detected that the path didn't resolve to the same file after removing 
> `..`, what would I do?
> 
> I think the current logic will only end up using the `else` branch on 
> Windows. At least I can't think of a scenario where the root would change 
> from using realpath on unix.
Thanks, I wasn't aware of that subtlety! I suggest we add a comment about that 
here and also mention (or assert) that this is only reachable on Windows.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

MrTrillian wrote:
> benlangmuir wrote:
> > Removing .. can change where the path points in the presence of symlinks; 
> > is this needed?
> > Removing .. can change where the path points in the presence of symlinks; 
> > is this needed?
> 
> @benlangmuir That's true and not ideal, but `makeAbsolute` will not resolve 
> `/./` or `/../` in paths, so it's not a canonicalization and some tests were 
> failing because of that. One alternative would be to use `makeAbsolute` + 
> `remove_dots` on Windows (where removing dot dots is semantically correct) 
> and `getRealPath` on Unix, like I do in lit. Suggestions?
Wouldn't removing .. have the same issue with symlinks on Windows? I know 
symlinks are less common there, but it's not clear to me why it would be 
correct.  I guess you could also check if the paths resolve to the same file 
after removing ..





Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:190
 
-StringRef FileName = File->tryGetRealPathName().empty()
- ? File->getName()
- : File->tryGetRealPathName();
+StringRef FileName = SM.getFileManager().getCanonicalName(File);
 

MrTrillian wrote:
> benlangmuir wrote:
> > Why is this change needed?
> > Why is this change needed?
> 
> @benlangmuir We don't want raw `getRealPath`s on Windows because of 
> substitute drives and MAX_PATH issues. That is the idea behind this diff. If 
> I leave the `tryGetRealPathName` here, I need to change the 
> `relative_include.m` test as in this previous diff: 
> https://reviews.llvm.org/D154130?id=539683 , which is undesirable.
I wonder if we should just remove `tryGetRealPathName`; it's not actually the 
real path in many cases.  Anyway, not for this patch, your change here seems 
fine.



Comment at: llvm/utils/lit/lit/LitConfig.py:192
 f = f.f_back.f_back
-file = os.path.abspath(inspect.getsourcefile(f))
+file = lit.util.abs_path_preserve_drive(inspect.getsourcefile(f))
 line = inspect.getlineno(f)

Why are you changing abspath (here and elsewhere)? I understand why you are 
changing realpath -> lit.util.abs_path_preserve_drive, but what's the issue 
with bare abspath?



Comment at: llvm/utils/lit/lit/TestRunner.py:1282
+# a leading slash.
+substitutions.append(("%:" + letter, colonNormalizePath(path)))
 

This change drops the `+ ".tmp"`  that was previously added to 
`%t:regex_replacement` and `%:t`.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:655
+  SmallString<4096> AbsPathBuf = Name;
+  SmallString<4096> RealPathBuf;
+  if (!FS->makeAbsolute(AbsPathBuf)) {

8k is a lot of stack space. The only reason this was 4k in the first place is 
it was originally using `char[PATH_MAX]` and unix `realpath` directly.  I'd 
suggest just dropping to 128 per path.



Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

Removing .. can change where the path points in the presence of symlinks; is 
this needed?



Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:190
 
-StringRef FileName = File->tryGetRealPathName().empty()
- ? File->getName()
- : File->tryGetRealPathName();
+StringRef FileName = SM.getFileManager().getCanonicalName(File);
 

Why is this change needed?


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

> I took a look at the code and it looks to me like it would be safe to change 
> ModuleMap::canonicalizeModuleMapPath() to use a drive preserving 
> canonicalization

This sounds reasonable to me (the person who added canonicalizeModuleMapPath), 
though I am not at all a Windows path expert.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155131: [clang][modules] Deserialize included files lazily

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Now that it's not eagerly deserialized, should `Preprocessor::alreadyIncluded` 
call `HeaderInfo.getFileInfo(File)` to ensure the information is up to date?  
Similarly, we expose the list of files in `Preprocessor::getIncludedFiles` -- 
is it okay if this list is incomplete?




Comment at: clang/lib/Serialization/ASTReader.cpp:1947
+if (const FileEntry *FE = getFile(key))
+  Reader.getPreprocessor().getIncludedFiles().insert(FE);
+

`Reader.getPreprocessor().markIncluded`?



Comment at: clang/lib/Serialization/ASTWriter.cpp:2545
-raw_svector_ostream Out(Buffer);
-writeIncludedFiles(Out, PP);
-RecordData::value_type Record[] = {PP_INCLUDED_FILES};

Can we remove `ASTWriter::writeIncludedFiles` now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155131

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154905: [clang] Implement `PointerLikeTraits` for `{File,Directory}EntryRef`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154905

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154905: [clang] Implement `PointerLikeTraits` for `{File,Directory}EntryRef`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/include/clang/Basic/DirectoryEntry.h:211
+
+  static constexpr int NumLowBitsAvailable = 3;
+};

I suggest not hard-coding it if you can get away with it; maybe 
`PointerLikeTypeTraits::NumLowBitsAvailable`? I think 3 could be wrong on a 32-bit platform for 
example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154905

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135849: [llvm] Return canonical virtual path from `RedirectingFileSystem`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D135849#4469347 , @jansvoboda11 
wrote:

> It'd be really nice to have DirectoryEntry::getDir() API, so that we can walk 
> up the directory hierarchy while preserving the virtual/real distinction 
> between directories in the overlay/on disk, never accidentally "bubbling up" 
> into the overlay again. What's your take on that?

Can you say more about how you would do this and preserve the real/virtual 
distinction? Currently FileManager is just filling in the directory based on 
the parent path with a lookup to the VFS, so isn't it the same issue? Or did 
you mean we would keep more info?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135849: [llvm] Return canonical virtual path from `RedirectingFileSystem`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

> You mean diagnosing whenever the spelling in the VFS definition differs from 
> its realpath?

Right, this would be ideal, but may be too expensive in practice.

> How could we make that work with symlinks in place?

Ah right, we are intentionally allowing symlinks in the VFS path (e.g. 
Foo.framework/Headers) because we don't correctly handle symlink resolution in 
the VFS itself.

You can get the canonical spelling of a symlink in a couple of ways: you can 
readdir the parent directory and find all case-insensitive matches.  If there 
is only 1, that's the spelling, if there are more than 1  then it's a 
case-sensitive filesystem and the original path must be correct.  Another way 
that is OS-specific is on Darwin you can `open(..., O_SYMLINK)` the path 
component to open the symlink itself and then `fcntl(..., F_GETPATH, ...)` to 
get its realpath.  Both of these approaches require handling each component of 
the path individually, though each step is cacheable.  Not sure if there are 
any better ways.

As I said, not sure we can afford to diagnose this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154257: [clang] Fix leak in LoadFromCommandLineWorkingDirectory unit test

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.

Gotta love `return AST.release()` being passed directly back into 
`std::unique_ptr(...)`.  Thanks for fixing!




Comment at: clang/tools/libclang/CIndex.cpp:3965
   unsaved_files);
-  std::unique_ptr Unit(ASTUnit::LoadFromCommandLine(
+  auto Unit = ASTUnit::LoadFromCommandLine(
   Args->data(), Args->data() + Args->size(),

Nit: the style guidance for `auto` in llvm is "type is already obvious from the 
context".  Since I don't think that's obvious here (in particular, this code 
would have compiled with the raw pointer return type), and the type name will 
not be verbose, I think we should keep `std::unique_ptr`.  Same for 
the other calls below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154257

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135849: [llvm] Return canonical virtual path from `RedirectingFileSystem`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

> Just realized this most likely won't work if the case-insensitive VFS is 
> specified with wrong spelling too, e.g. Fw.framework.

Is this about the spelling in the VFS definition itself, or about the path 
being looked up? If it's the VFS definition maybe we can say that's a bug for 
whoever wrote the VFS? Ideally we could diagnose but I'm not sure we want to 
pay for a lot of calls to realpath.




Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:876
+/// Chain of parent directory entries for \c E.
+std::vector Parents;
+

SmallVector?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154016: [clang][modules] Avoid serializing all diag mappings in non-deterministic order

[Ben Langmuir via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
benlangmuir marked an inline comment as done.
Closed by commit rG1ede7b47493f: [clang][modules] Avoid serializing all diag 
mappings in non-deterministic order (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154016

Files:
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/lib/Basic/Diagnostic.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/diag-mappings.c

Index: clang/test/Modules/diag-mappings.c
===
--- /dev/null
+++ clang/test/Modules/diag-mappings.c
@@ -0,0 +1,94 @@
+// Test that diagnostic mappings are emitted only when needed and in order of
+// diagnostic ID rather than non-deterministically. This test passes 3
+// -W options and expects exactly 3 mappings to be emitted in the pcm. The -W
+// options are chosen to be far apart in ID (see DiagnosticIDs.h) so we can
+// check they are ordered. We also intentionally trigger several other warnings
+// inside the module and ensure they do not show up in the pcm as mappings.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: mv %t/cache/A.pcm %t/A1.pcm
+
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/A1.pcm | FileCheck %s
+
+// CHECK:  2000
+// CHECK-SAME: op7=[[FLOAT_EQ:[2-9][0-9][0-9][0-9]]] op8=
+
+// == Pragmas:
+// Each pragma creates a mapping table; and each copies the previous table. The
+// initial mappings are copied as well, but are not serialized since they have
+// isPragma=false.
+
+// == ignored "-Wfloat-equal"
+// CHECK-SAME: op{{[0-9]+}}=1
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == ignored "-Wstack-protector"
+// CHECK-SAME: op{{[0-9]+}}=2
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == warning "-Wempty-translation-unit"
+// CHECK-SAME: op{{[0-9]+}}=3
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[EMPTY_TU]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == warning "-Wstack-protector"
+// CHECK-SAME: op{{[0-9]+}}=3
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[EMPTY_TU]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: diff %t/cache/A.pcm %t/A1.pcm
+
+//--- module.modulemap
+module A { header "a.h" }
+
+//--- a.h
+// Lex warning
+#warning "w"
+
+static inline void f() {
+// Parse warning
+  ;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wfloat-equal"
+#pragma clang diagnostic ignored "-Wstack-protector"
+
+static inline void g() {
+// Sema warning
+  int x;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic warning "-Wempty-translation-unit"
+#pragma clang diagnostic warning "-Wstack-protector"
+
+#pragma clang diagnostic pop
+#pragma clang diagnostic pop
+
+//--- main.m
+#import "a.h"
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2997,20 +2997,41 @@
 assert(Flags == EncodeDiagStateFlags(State) &&
"diag state flags vary in single AST file");
 
+// If we ever serialize non-pragma mappings outside the initial state, the
+// code below will need to consider more than getDefaultMapping.
+assert(!IncludeNonPragmaStates ||
+   State == Diag.DiagStatesByLoc.FirstDiagState);
+
 unsigned  = DiagStateIDMap[State];
 Record.push_back(DiagStateID);
 
 if (DiagStateID == 0) {
   DiagStateID = ++CurrID;
+  SmallVector> Mappings;
 
   // Add a placeholder for the number of mappings.
   auto SizeIdx = Record.size();
   Record.emplace_back();
   for (const auto  : *State) {
-if (I.second.isPragma() || IncludeNonPragmaStates) {
-  Record.push_back(I.first);
-  Record.push_back(I.second.serialize());
-}
+// Maybe skip non-pragmas.
+if (!I.second.isPragma() && !IncludeNonPragmaStates)
+  continue;
+// Skip default mappings. We have a mapping for every diagnostic ever
+// emitted, regardless of 

[PATCH] D154016: [clang][modules] Avoid serializing all diag mappings in non-deterministic order

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked an inline comment as done.
benlangmuir added inline comments.



Comment at: clang/lib/Serialization/ASTWriter.cpp:3016
   for (const auto  : *State) {
-if (I.second.isPragma() || IncludeNonPragmaStates) {
-  Record.push_back(I.first);
-  Record.push_back(I.second.serialize());
-}
+// Maybe skip non-pragmas.
+if (!I.second.isPragma() && !IncludeNonPragmaStates)

steven_wu wrote:
> Is pragma in this context refer to `#pragma diagnostics push/pop`? Do we have 
> test to cover those to be deterministic?
> Is pragma in this context refer to `#pragma diagnostics push/pop`?

Yeah, this is diagnostic pragmas.

> Do we have test to cover those to be deterministic?

I extended the current test to include some pragmas as well.  It's harder to 
check them exhaustively because the encoded format gets more complex, but I 
checked the specific mappings are in order and the `diff` part of the test 
should help catch any other non-determinism there could be.


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

https://reviews.llvm.org/D154016

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154016: [clang][modules] Avoid serializing all diag mappings in non-deterministic order

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 535882.
benlangmuir added a comment.

Extended test to include diagnostic pragmas


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

https://reviews.llvm.org/D154016

Files:
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/lib/Basic/Diagnostic.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/diag-mappings.c

Index: clang/test/Modules/diag-mappings.c
===
--- /dev/null
+++ clang/test/Modules/diag-mappings.c
@@ -0,0 +1,94 @@
+// Test that diagnostic mappings are emitted only when needed and in order of
+// diagnostic ID rather than non-deterministically. This test passes 3
+// -W options and expects exactly 3 mappings to be emitted in the pcm. The -W
+// options are chosen to be far apart in ID (see DiagnosticIDs.h) so we can
+// check they are ordered. We also intentionally trigger several other warnings
+// inside the module and ensure they do not show up in the pcm as mappings.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: mv %t/cache/A.pcm %t/A1.pcm
+
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/A1.pcm | FileCheck %s
+
+// CHECK:  2000
+// CHECK-SAME: op7=[[FLOAT_EQ:[2-9][0-9][0-9][0-9]]] op8=
+
+// == Pragmas:
+// Each pragma creates a mapping table; and each copies the previous table. The
+// initial mappings are copied as well, but are not serialized since they have
+// isPragma=false.
+
+// == ignored "-Wfloat-equal"
+// CHECK-SAME: op{{[0-9]+}}=1
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == ignored "-Wstack-protector"
+// CHECK-SAME: op{{[0-9]+}}=2
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == warning "-Wempty-translation-unit"
+// CHECK-SAME: op{{[0-9]+}}=3
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[EMPTY_TU]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == warning "-Wstack-protector"
+// CHECK-SAME: op{{[0-9]+}}=3
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[EMPTY_TU]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: diff %t/cache/A.pcm %t/A1.pcm
+
+//--- module.modulemap
+module A { header "a.h" }
+
+//--- a.h
+// Lex warning
+#warning "w"
+
+static inline void f() {
+// Parse warning
+  ;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wfloat-equal"
+#pragma clang diagnostic ignored "-Wstack-protector"
+
+static inline void g() {
+// Sema warning
+  int x;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic warning "-Wempty-translation-unit"
+#pragma clang diagnostic warning "-Wstack-protector"
+
+#pragma clang diagnostic pop
+#pragma clang diagnostic pop
+
+//--- main.m
+#import "a.h"
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2997,20 +2997,41 @@
 assert(Flags == EncodeDiagStateFlags(State) &&
"diag state flags vary in single AST file");
 
+// If we ever serialize non-pragma mappings outside the initial state, the
+// code below will need to consider more than getDefaultMapping.
+assert(!IncludeNonPragmaStates ||
+   State == Diag.DiagStatesByLoc.FirstDiagState);
+
 unsigned  = DiagStateIDMap[State];
 Record.push_back(DiagStateID);
 
 if (DiagStateID == 0) {
   DiagStateID = ++CurrID;
+  SmallVector> Mappings;
 
   // Add a placeholder for the number of mappings.
   auto SizeIdx = Record.size();
   Record.emplace_back();
   for (const auto  : *State) {
-if (I.second.isPragma() || IncludeNonPragmaStates) {
-  Record.push_back(I.first);
-  Record.push_back(I.second.serialize());
-}
+// Maybe skip non-pragmas.
+if (!I.second.isPragma() && !IncludeNonPragmaStates)
+  continue;
+// Skip default mappings. We have a mapping for every diagnostic ever
+// emitted, regardless of whether it was customized.
+if (!I.second.isPragma() &&
+I.second == DiagnosticIDs::getDefaultMapping(I.first))
+  continue;
+Mappings.push_back(I);
+  }
+
+  // Sort by diag::kind for 

[PATCH] D154016: [clang][modules] Avoid serializing all diag mappings in non-deterministic order

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: steven_wu, jansvoboda11, akyrtzi.
Herald added a subscriber: mgrang.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When writing a pcm, we serialize diagnostic mappings in order to
accurately reproduce the diagnostic environment inside any headers from
that module. However, the diagnostic state mapping table contains
entries for every diagnostic ID ever accessed, while we only want to
serialize the ones that are actually modified from their default value.
Futher, we need to serialize them in a deterministic order.

rdar://111477511


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154016

Files:
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/lib/Basic/Diagnostic.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/diag-mappings.c

Index: clang/test/Modules/diag-mappings.c
===
--- /dev/null
+++ clang/test/Modules/diag-mappings.c
@@ -0,0 +1,52 @@
+// Test that diagnostic mappings are emitted only when needed and in order of
+// diagnostic ID rather than non-deterministically. This test passes 3
+// -W options and expects exactly 3 mappings to be emitted in the pcm. The -W
+// options are chosen to be far apart in ID (see DiagnosticIDs.h) so we can
+// check they are ordered. We also intentionally trigger several other warnings
+// inside the module and ensure they do not show up in the pcm as mappings.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: mv %t/cache/A.pcm %t/A1.pcm
+
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/A1.pcm | FileCheck %s
+
+// CHECK:  2000
+// CHECK-SAME: op7={{[2-9][0-9][0-9][0-9]}} op8=
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: diff %t/cache/A.pcm %t/A1.pcm
+
+//--- module.modulemap
+module A { header "a.h" }
+
+//--- a.h
+// Lex warning
+#warning "w"
+
+static inline void f() {
+// Parse warning
+  ;
+// Sema warning
+  int x;
+}
+
+//--- main.m
+#import "a.h"
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2997,20 +2997,41 @@
 assert(Flags == EncodeDiagStateFlags(State) &&
"diag state flags vary in single AST file");
 
+// If we ever serialize non-pragma mappings outside the initial state, the
+// code below will need to consider more than getDefaultMapping.
+assert(!IncludeNonPragmaStates ||
+   State == Diag.DiagStatesByLoc.FirstDiagState);
+
 unsigned  = DiagStateIDMap[State];
 Record.push_back(DiagStateID);
 
 if (DiagStateID == 0) {
   DiagStateID = ++CurrID;
+  SmallVector> Mappings;
 
   // Add a placeholder for the number of mappings.
   auto SizeIdx = Record.size();
   Record.emplace_back();
   for (const auto  : *State) {
-if (I.second.isPragma() || IncludeNonPragmaStates) {
-  Record.push_back(I.first);
-  Record.push_back(I.second.serialize());
-}
+// Maybe skip non-pragmas.
+if (!I.second.isPragma() && !IncludeNonPragmaStates)
+  continue;
+// Skip default mappings. We have a mapping for every diagnostic ever
+// emitted, regardless of whether it was customized.
+if (!I.second.isPragma() &&
+I.second == DiagnosticIDs::getDefaultMapping(I.first))
+  continue;
+Mappings.push_back(I);
+  }
+
+  // Sort by diag::kind for deterministic output.
+  llvm::sort(Mappings, [](const auto , const auto ) {
+return LHS.first < RHS.first;
+  });
+
+  for (const auto  : Mappings) {
+Record.push_back(I.first);
+Record.push_back(I.second.serialize());
   }
   // Update the placeholder.
   Record[SizeIdx] = (Record.size() - SizeIdx) / 2;
Index: clang/lib/Basic/DiagnosticIDs.cpp
===
--- clang/lib/Basic/DiagnosticIDs.cpp
+++ clang/lib/Basic/DiagnosticIDs.cpp
@@ -256,7 +256,7 @@
   return Found;
 }
 
-static DiagnosticMapping GetDefaultDiagMapping(unsigned DiagID) {
+DiagnosticMapping DiagnosticIDs::getDefaultMapping(unsigned DiagID) {
   DiagnosticMapping Info = DiagnosticMapping::Make(
   diag::Severity::Fatal, /*IsUser=*/false, 

[PATCH] D153670: [clang/HeaderSearch] Make sure `loadSubdirectoryModuleMaps` doesn't cause loading of regular files

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/unittests/Tooling/DependencyScannerTest.cpp:274
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+  ReadFiles.push_back(Path.str());

Should we add `status` override as well? I think we don't want to stat it 
either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153670

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151855: [clang] Use `{File,Directory}EntryRef` in modular header search (part 2/2)

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

> I don't think it's worth blocking this patch on it, though. What do you think?

Agreed, I don't think this is making it worse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151855: [clang] Use `{File,Directory}EntryRef` in modular header search (part 2/2)

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a subscriber: JDevlieghere.
benlangmuir added a comment.

> I think it should be fine to allow dropping the 
> A.framework/Frameworks/B.framework directory from the reproducer VFS

I think technically this is wrong, since if you're missing the symlink, then A 
might not build -- e.g. it could be doing a relative include that needs the 
symlink.  But am I understanding correctly that the reproducer was already 
broken in this case? If so I'm fine with this.

The right thing to do would be to capture both the framework and the symlink. 
I'm not sure how practical that is with the current architecture.  
@JDevlieghere any thoughts? Longer term, our CAS work could ultimately end up 
solving this in a better way by fully capturing all the inputs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151855

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151277: [clang][modules] Mark fewer identifiers as out-of-date

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

LGTM, but I would prefer at least one more person who understands the 
identifier handling code review this. It's been years since I thought about 
this code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151938: [clang][index] NFCI: Make `CXFile` a `FileEntryRef`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/test/Modules/crash-vfs-umbrella-frameworks.m:13
 // RUN: env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
-// RUN: not %clang -nostdinc -fsyntax-only %s \
+// RUN: not --crash %clang -nostdinc -fsyntax-only %s \
 // RUN: -F %/t/i/Frameworks -fmodules \

This looks suspicious; what's going on here?



Comment at: clang/tools/libclang/CXFile.h:1
+//===--===//
+//

Missing filename and C++ language tag from header comment.



Comment at: clang/tools/libclang/CXLoadedDiagnostic.cpp:279
   else {
-LoadedLoc.file = const_cast(TopDiags->Files[FileID]);
+LoadedLoc.file = cxfile::makeCXFile(TopDiags->Files.find(FileID)->second);
 if (!LoadedLoc.file)

Does this preserve behaviour in call cases? operator[] will construct nullptr 
if the key is missing, but the new code will crash dereferencing an invalid 
iterator.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151938

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151931: [clang] Remove `DirectoryEntry::getName()`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Given this was a commonly used function before and there's a decent chance it's 
used out of tree somewhere, should we wait until the next llvm release branch 
has been cut before landing this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151931

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151852: [clang] NFCI: Use `FileEntryRef` in `ModuleMapCallbacks`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

Nice simplification!




Comment at: clang/include/clang/Lex/ModuleMap.h:72
   /// \param Header The umbrella header to collect.
-  virtual void moduleMapAddUmbrellaHeader(FileManager *FileMgr,
-  const FileEntry *Header) {}
+  virtual void moduleMapAddUmbrellaHeader(FileEntryRef Header) {}
 };

Doc comment needs update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151852

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151854: [clang] Use `FileEntryRef` in modular header search (part 1/2)

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.






Comment at: clang/include/clang/Lex/HeaderSearch.h:763
   /// find this file due to requirements from \p RequestingModule.
-  bool findUsableModuleForHeader(const FileEntry *File,
+  bool findUsableModuleForHeader(OptionalFileEntryRef File,
  const DirectoryEntry *Root,

This should probably be non-Optional. I can't find any calls to this API that 
can pass null, they all pass something that is already being dereferenced.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151854

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151277: [clang][modules] Mark fewer identifiers as out-of-date

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:4413
+// first/next use via ASTReader::updateOutOfDateIdentifier().
+II = ().get(Key);
+  }

Why did this change from `getOwn` to `get`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151584: [clang][modules] NFCI: Use `DirectoryEntryRef` for umbrella directory

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Basic/Module.h:160
+  llvm::PointerUnion
   Umbrella;

Would it make sense to implement `PointerLikeTypeTraits` for 
FileEntryRef/DirectoryEntryRef so you don't need to use the MapEntry explicitly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151584

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151586: [clang][modules] NFCI: Extract optionality out of `Module::{Header,DirectoryName}`

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Basic/Module.cpp:486
 
-  if (Header H = getWrittenUmbrellaHeader()) {
+  if (auto H = getWrittenUmbrellaHeader()) {
 OS.indent(Indent + 2);

These are on the edge, but I think I would lean towards spelling the typename 
here since it's not obvious from the name of the API what exactly it would be


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151581: [clang][modules] NFCI: Distinguish written/effective umbrella directories

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Basic/Module.h:655
+  /// Retrieve the explicitly written umbrella directory for this module.
+  DirectoryName getWrittenUmbrellaDir() const {
+if (const auto *ME = Umbrella.dyn_cast())

Nit: I would prefer we use the same `...AsWritten` naming scheme for these 
methods that we use for the `UmbrellaAsWritten` field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

> @benlangmuir, do you need any assistance from my side with it?

@ivanmurashko thanks for your patience. I discussed this with some colleagues 
and we're in favour of making this fix.  LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150479: [clang][modules][deps] Allow skipping submodule definitions

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/test/ClangScanDeps/modules-private-framework-submodule.c:43
 // CHECK-NEXT:   "context-hash": "{{.*}}",
-// CHECK-NEXT:   "module-name": "FW2_Private"
+// CHECK-NEXT:   "module-name": "FW2"
 // CHECK-NEXT: }

jansvoboda11 wrote:
> benlangmuir wrote:
> > Does this lose any test coverage for the FW2_Private case?
> This still tests the same general code path, just a more specific case of it. 
> I can keep this test as is and create new one specifically to test the 
> "explicit submodule" case if that makes more sense to you.
Yeah, my preference would be to add a case instead of modifying this one, 
unless there is somewhere else we're already testing the same thing. The exact 
behaviour of FW2.Private vs FW2_Private vs FW2Private is all subtlely different


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150479

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150479: [clang][modules][deps] Allow skipping submodule definitions

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/test/ClangScanDeps/modules-private-framework-submodule.c:43
 // CHECK-NEXT:   "context-hash": "{{.*}}",
-// CHECK-NEXT:   "module-name": "FW2_Private"
+// CHECK-NEXT:   "module-name": "FW2"
 // CHECK-NEXT: }

Does this lose any test coverage for the FW2_Private case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150479

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150478: [clang][modules][deps] Parse "FW_Private" module map even after loading "FW" PCM

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

LGTM. I ran into a similar issue in a downstream branch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150478

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150318: [clang][deps] NFC: Pass around the whole scanning service

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h:69
+  /// The preprocessing mode used for scanning.
+  ScanningMode Mode;
+  /// The output format.

jansvoboda11 wrote:
> benlangmuir wrote:
> > Why drop `const`?
> I don't think it adds much, since the members are private and only ever 
> accessed in functions already marked `const`. I'm fine with keeping the 
> `const` here if you think there's value in it.
It's fine, just wanted to check I didn't misunderstand this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150318

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150320: [clang][deps] Avoid relocatable modules checks

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

With my naming comment addressed, LGTM




Comment at: clang/include/clang/Lex/PreprocessorOptions.h:73
+  /// Perform extra checks for relocatable modules when loading PCM files.
+  bool RelocatableModules = true;
+

Suggestion: `CheckRelocatableModules`.  The current name sounds like a compiler 
feature rather than controlling diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150320

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150319: [clang][deps] Always use -fmodules-validate-once-per-build-session

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp:26
+  ? BuildSessionTimestamp
+  : std::chrono::system_clock::now().time_since_epoch().count()) {
   // Initialize targets for object file support.

I'm not sure this is guaranteed to line up with the filesystem timestamps. I 
wonder if you should be writing a file and reading back that stamp instead.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:188
+// FIXME: Consider diagnosing when this overwrites existing timestamp.
+ScanInstance.getHeaderSearchOpts().BuildSessionTimestamp =
+Service.getBuildSessionTimestamp();

Why would it diagnose? This is going to be common.  Maybe we should use the 
minimum value between the service and the existing timestamp to get the widest 
possible session if the build system is already providing one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150319

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150318: [clang][deps] NFC: Pass around the whole scanning service

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h:69
+  /// The preprocessing mode used for scanning.
+  ScanningMode Mode;
+  /// The output format.

Why drop `const`?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:19
 #include "clang/Serialization/ASTReader.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "llvm/ADT/DenseMap.h"

Forward declaration for the header file should be enough


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150318

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150151: [clang] Prevent creation of new submodules in ASTWriter

[Ben Langmuir via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5984ea216d2a: [clang] Prevent creation of new submodules in 
ASTWriter (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150151

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -185,7 +185,8 @@
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
   continue;
 
-for (const auto  : HS.findAllModulesForHeader(File)) {
+for (const auto  :
+ HS.findAllModulesForHeader(File, /*AllowCreation=*/false)) {
   if (!KH.getModule())
 continue;
   ModulesToProcess.push_back(KH.getModule());
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -683,12 +683,12 @@
 }
 
 ArrayRef
-ModuleMap::findAllModulesForHeader(const FileEntry *File) {
+ModuleMap::findAllModulesForHeader(const FileEntry *File, bool AllowCreation) {
   HeadersMap::iterator Known = findKnownHeader(File);
   if (Known != Headers.end())
 return Known->second;
 
-  if (findOrCreateModuleForHeaderInUmbrellaDir(File))
+  if (AllowCreation && findOrCreateModuleForHeaderInUmbrellaDir(File))
 return Headers.find(File)->second;
 
   return std::nullopt;
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1565,13 +1565,14 @@
 }
 
 ArrayRef
-HeaderSearch::findAllModulesForHeader(const FileEntry *File) const {
+HeaderSearch::findAllModulesForHeader(const FileEntry *File,
+  bool AllowCreation) const {
   if (ExternalSource) {
 // Make sure the external source has handled header info about this file,
 // which includes whether the file is part of a module.
 (void)getExistingFileInfo(File);
   }
-  return ModMap.findAllModulesForHeader(File);
+  return ModMap.findAllModulesForHeader(File, AllowCreation);
 }
 
 static bool suggestModule(HeaderSearch , const FileEntry *File,
Index: clang/include/clang/Lex/ModuleMap.h
===
--- clang/include/clang/Lex/ModuleMap.h
+++ clang/include/clang/Lex/ModuleMap.h
@@ -448,9 +448,13 @@
   /// and does not consult the external source. (Those checks are the
   /// responsibility of \ref HeaderSearch.)
   ///
+  /// \param AllowCreation Whether to allow inference of a new submodule, or to
+  ///only return existing known modules.
+  ///
   /// Typically, \ref findModuleForHeader should be used instead, as it picks
   /// the preferred module for the header.
-  ArrayRef findAllModulesForHeader(const FileEntry *File);
+  ArrayRef findAllModulesForHeader(const FileEntry *File,
+bool AllowCreation = true);
 
   /// Like \ref findAllModulesForHeader, but do not attempt to infer module
   /// ownership from umbrella headers if we've not already done so.
Index: clang/include/clang/Lex/HeaderSearch.h
===
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -665,9 +665,13 @@
 
   /// Retrieve all the modules corresponding to the given file.
   ///
+  /// \param AllowCreation Whether to allow inference of a new submodule, or to
+  ///only return existing known modules.
+  ///
   /// \ref findModuleForHeader should typically be used instead of this.
   ArrayRef
-  findAllModulesForHeader(const FileEntry *File) const;
+  findAllModulesForHeader(const FileEntry *File,
+  bool AllowCreation = true) const;
 
   /// Read the contents of the given module map file.
   ///


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -185,7 +185,8 @@
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
   continue;
 
-for (const auto  : HS.findAllModulesForHeader(File)) {
+for (const auto  :
+ HS.findAllModulesForHeader(File, /*AllowCreation=*/false)) {
   if (!KH.getModule())
 continue;
   ModulesToProcess.push_back(KH.getModule());
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ 

[PATCH] D149884: [clang][deps] Teach dep directive scanner about _Pragma

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGee8ed0b3099e: [clang][deps] Teach dep directive scanner 
about _Pragma (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149884

Files:
  clang/include/clang/Lex/Pragma.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/ClangScanDeps/_Pragma-once.c
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -503,6 +503,92 @@
   EXPECT_STREQ("#pragma clang module import\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, UnderscorePragma) {
+  SmallVector Out;
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_)", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma)", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma()", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma())", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma(")", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma("A"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("push_macro(\"MACRO\")"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("push_macro(\"MACRO\")"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("pop_macro(\"MACRO\")"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("pop_macro(\"MACRO\")"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("include_alias(\"A\", \"B\")"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("include_alias(\"A\", \"B\")"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("include_alias(, )"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("include_alias(, )"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives(R"(_Pragma("clang"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives(R"(_Pragma("clang module"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma("clang module impor"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma("clang module import"))", Out));
+  EXPECT_STREQ(R"(_Pragma("clang module import"))"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma("clang \
+  module \
+  import"))",
+  Out));
+  EXPECT_STREQ(R"(_Pragma("clang \
+  module \
+  import"))"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma(L"clang module import"))", Out));
+  EXPECT_STREQ(R"(_Pragma(L"clang module import"))"
+   "\n",
+   Out.data());
+
+  // FIXME: u"" strings depend on using C11 language mode
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma(u"clang module import"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  // FIXME: R"()" strings depend on using C++ 11 language mode
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma(R"abc(clang module import)abc"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Include) {
   SmallVector Out;
 
@@ -757,20 +843,26 @@
 #pragma once
 // another comment
 #include 
+_Pragma("once")
 )";
   ASSERT_FALSE(
   minimizeSourceToDependencyDirectives(Source, Out, Tokens, Directives));
-  EXPECT_STREQ("#pragma once\n#include \n", Out.data());
-  ASSERT_EQ(Directives.size(), 3u);
+  EXPECT_STREQ("#pragma once\n#include \n_Pragma(\"once\")\n",
+   Out.data());
+  ASSERT_EQ(Directives.size(), 4u);
   EXPECT_EQ(Directives[0].Kind, dependency_directives_scan::pp_pragma_once);
+  EXPECT_EQ(Directives[2].Kind, dependency_directives_scan::pp_pragma_once);
 
   Source = R"(// comment
 #pragma once extra tokens
 // another comment
 #include 
+_Pragma("once") extra tokens
 )";
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
-  EXPECT_STREQ("#pragma once extra tokens\n#include \n", Out.data());
+  EXPECT_STREQ("#pragma once extra tokens\n#include "
+   

[PATCH] D150151: [clang] Prevent creation of new submodules in ASTWriter

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

No test, because I haven't come up with a test case that wouldn't be 
invalidated by https://reviews.llvm.org/D103930. Let me know if you have any 
ideas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150151

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150151: [clang] Prevent creation of new submodules in ASTWriter

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added a reviewer: jansvoboda11.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Avoid inferring new submodules for headers in ASTWriter's collection of 
affecting modulemap files, since we don't want to pick up dependencies that 
didn't actually exist during parsing.

rdar://108681805


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150151

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -185,7 +185,8 @@
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
   continue;
 
-for (const auto  : HS.findAllModulesForHeader(File)) {
+for (const auto  :
+ HS.findAllModulesForHeader(File, /*AllowCreation=*/false)) {
   if (!KH.getModule())
 continue;
   ModulesToProcess.push_back(KH.getModule());
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -683,12 +683,12 @@
 }
 
 ArrayRef
-ModuleMap::findAllModulesForHeader(const FileEntry *File) {
+ModuleMap::findAllModulesForHeader(const FileEntry *File, bool AllowCreation) {
   HeadersMap::iterator Known = findKnownHeader(File);
   if (Known != Headers.end())
 return Known->second;
 
-  if (findOrCreateModuleForHeaderInUmbrellaDir(File))
+  if (AllowCreation && findOrCreateModuleForHeaderInUmbrellaDir(File))
 return Headers.find(File)->second;
 
   return std::nullopt;
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1565,13 +1565,14 @@
 }
 
 ArrayRef
-HeaderSearch::findAllModulesForHeader(const FileEntry *File) const {
+HeaderSearch::findAllModulesForHeader(const FileEntry *File,
+  bool AllowCreation) const {
   if (ExternalSource) {
 // Make sure the external source has handled header info about this file,
 // which includes whether the file is part of a module.
 (void)getExistingFileInfo(File);
   }
-  return ModMap.findAllModulesForHeader(File);
+  return ModMap.findAllModulesForHeader(File, AllowCreation);
 }
 
 static bool suggestModule(HeaderSearch , const FileEntry *File,
Index: clang/include/clang/Lex/ModuleMap.h
===
--- clang/include/clang/Lex/ModuleMap.h
+++ clang/include/clang/Lex/ModuleMap.h
@@ -448,9 +448,13 @@
   /// and does not consult the external source. (Those checks are the
   /// responsibility of \ref HeaderSearch.)
   ///
+  /// \param AllowCreation Whether to allow inference of a new submodule, or to
+  ///only return existing known modules.
+  ///
   /// Typically, \ref findModuleForHeader should be used instead, as it picks
   /// the preferred module for the header.
-  ArrayRef findAllModulesForHeader(const FileEntry *File);
+  ArrayRef findAllModulesForHeader(const FileEntry *File,
+bool AllowCreation = true);
 
   /// Like \ref findAllModulesForHeader, but do not attempt to infer module
   /// ownership from umbrella headers if we've not already done so.
Index: clang/include/clang/Lex/HeaderSearch.h
===
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -665,9 +665,13 @@
 
   /// Retrieve all the modules corresponding to the given file.
   ///
+  /// \param AllowCreation Whether to allow inference of a new submodule, or to
+  ///only return existing known modules.
+  ///
   /// \ref findModuleForHeader should typically be used instead of this.
   ArrayRef
-  findAllModulesForHeader(const FileEntry *File) const;
+  findAllModulesForHeader(const FileEntry *File,
+  bool AllowCreation = true) const;
 
   /// Read the contents of the given module map file.
   ///


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -185,7 +185,8 @@
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
   continue;
 
-for (const auto  : HS.findAllModulesForHeader(File)) {
+for (const auto  :
+ HS.findAllModulesForHeader(File, /*AllowCreation=*/false)) {
   if (!KH.getModule())
 continue;
   ModulesToProcess.push_back(KH.getModule());
Index: 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

We have several new build failures with this change that I'm looking through. 
So far, a common one is an error of the form

  /source/module.modulemap: error: redefinition of module
  /build/Foo.framework/Modules/module.modulemap: note: previously defined here

ie. we're now finding the same module in two places where we didn't before. 
It's possible we could avoid this for framework modules if we ignore framework 
modules that are not actually in framework directories, but we have seen this 
with non-frameworks modules as well.

Another class of errors is where the module we find does not compile in the 
current context, or it doesn't provide what the includer wanted when built as a 
module. Basically we silently depended on it being a textual include.

Still looking at issues and not sure whether these are blockers or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D103930#4310061 , @ivanmurashko 
wrote:

> Friendly ping
>
> @arphaman, @jansvoboda11, I have made the patch buildable on all platforms 
> and have all tests passed. There was also a small fix (temp path for modules 
> artefact) at the test that could fix its run on some platforms. Could you 
> look at it? Does it have any issues on your side?

I was actually looking at this issue myself today, and @jansvoboda11 mentioned 
this patch. I'll kick off some testing on our side and see what it turns up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149884: [clang][deps] Teach dep directive scanner about _Pragma

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: akyrtzi, Bigcheese, jansvoboda11.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

While we cannot handle `_Pragma` used inside macros, we an handle this at the 
top level, and it some projects use the `_Pragma("once")` spelling like that, 
which was causing spurious failures in the scanner.

Limitations

- Cannot handle #define ONCE _Pragma("once"), same issue as using @import in a 
macro -- ideally we should diagnose this in obvious cases
- Our LangOpts are currently fixed, so we are not handling u"" strings or R"()" 
strings that require C11/C++11.

rdar://108629982


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149884

Files:
  clang/include/clang/Lex/Pragma.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/ClangScanDeps/_Pragma-once.c
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -503,6 +503,92 @@
   EXPECT_STREQ("#pragma clang module import\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, UnderscorePragma) {
+  SmallVector Out;
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_)", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma)", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma()", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma())", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma(")", Out));
+  EXPECT_STREQ("\n", Out.data());
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(R"(_Pragma("A"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("push_macro(\"MACRO\")"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("push_macro(\"MACRO\")"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("pop_macro(\"MACRO\")"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("pop_macro(\"MACRO\")"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("include_alias(\"A\", \"B\")"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("include_alias(\"A\", \"B\")"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"x(_Pragma("include_alias(, )"))x", Out));
+  EXPECT_STREQ(R"x(_Pragma("include_alias(, )"))x"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives(R"(_Pragma("clang"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives(R"(_Pragma("clang module"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma("clang module impor"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma("clang module import"))", Out));
+  EXPECT_STREQ(R"(_Pragma("clang module import"))"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma("clang \
+  module \
+  import"))",
+  Out));
+  EXPECT_STREQ(R"(_Pragma("clang \
+  module \
+  import"))"
+   "\n",
+   Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma(L"clang module import"))", Out));
+  EXPECT_STREQ(R"(_Pragma(L"clang module import"))"
+   "\n",
+   Out.data());
+
+  // FIXME: u"" strings depend on using C11 language mode
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma(u"clang module import"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+
+  // FIXME: R"()" strings depend on using C++ 11 language mode
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+  R"(_Pragma(R"abc(clang module import)abc"))", Out));
+  EXPECT_STREQ("\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Include) {
   SmallVector Out;
 
@@ -757,20 +843,26 @@
 #pragma once
 // another comment
 #include 
+_Pragma("once")
 )";
   ASSERT_FALSE(
   minimizeSourceToDependencyDirectives(Source, Out, Tokens, Directives));
-  EXPECT_STREQ("#pragma once\n#include \n", Out.data());
-  ASSERT_EQ(Directives.size(), 3u);
+  EXPECT_STREQ("#pragma once\n#include \n_Pragma(\"once\")\n",
+   Out.data());
+  ASSERT_EQ(Directives.size(), 4u);
   

[PATCH] D149777: [clang][deps] Teach dep directive scanner about #pragma clang system_header

[Ben Langmuir via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7b492d1be0ea: [clang][deps] Teach dep directive scanner 
about #pragma clang system_header (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149777

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -90,7 +90,8 @@
"#pragma pop_macro(A)\n"
"#pragma include_alias(, )\n"
"export module m;\n"
-   "import m;\n",
+   "import m;\n"
+   "#pragma clang system_header\n",
Out, Tokens, Directives));
   EXPECT_EQ(pp_define, Directives[0].Kind);
   EXPECT_EQ(pp_undef, Directives[1].Kind);
@@ -113,7 +114,8 @@
   EXPECT_EQ(pp_pragma_include_alias, Directives[18].Kind);
   EXPECT_EQ(cxx_export_module_decl, Directives[19].Kind);
   EXPECT_EQ(cxx_import_decl, Directives[20].Kind);
-  EXPECT_EQ(pp_eof, Directives[21].Kind);
+  EXPECT_EQ(pp_pragma_system_header, Directives[21].Kind);
+  EXPECT_EQ(pp_eof, Directives[22].Kind);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, EmptyHash) {
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4480,6 +4480,7 @@
 case pp_pragma_push_macro:
 case pp_pragma_pop_macro:
 case pp_pragma_include_alias:
+case pp_pragma_system_header:
 case pp_include_next:
 case decl_at_import:
 case cxx_module_decl:
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -652,9 +652,22 @@
 return false;
   }
 
-  // #pragma clang.
-  if (!isNextIdentifierOrSkipLine("module", First, End))
+  FoundId = tryLexIdentifierOrSkipLine(First, End);
+  if (!FoundId)
 return false;
+  Id = *FoundId;
+
+  // #pragma clang system_header
+  if (Id == "system_header") {
+lexPPDirectiveBody(First, End);
+pushDirective(pp_pragma_system_header);
+return false;
+  }
+
+  if (Id != "module") {
+skipLine(First, End);
+return false;
+  }
 
   // #pragma clang module.
   if (!isNextIdentifierOrSkipLine("import", First, End))
Index: clang/include/clang/Lex/DependencyDirectivesScanner.h
===
--- clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -68,6 +68,7 @@
   pp_pragma_push_macro,
   pp_pragma_pop_macro,
   pp_pragma_include_alias,
+  pp_pragma_system_header,
   pp_include_next,
   pp_if,
   pp_ifdef,


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -90,7 +90,8 @@
"#pragma pop_macro(A)\n"
"#pragma include_alias(, )\n"
"export module m;\n"
-   "import m;\n",
+   "import m;\n"
+   "#pragma clang system_header\n",
Out, Tokens, Directives));
   EXPECT_EQ(pp_define, Directives[0].Kind);
   EXPECT_EQ(pp_undef, Directives[1].Kind);
@@ -113,7 +114,8 @@
   EXPECT_EQ(pp_pragma_include_alias, Directives[18].Kind);
   EXPECT_EQ(cxx_export_module_decl, Directives[19].Kind);
   EXPECT_EQ(cxx_import_decl, Directives[20].Kind);
-  EXPECT_EQ(pp_eof, Directives[21].Kind);
+  EXPECT_EQ(pp_pragma_system_header, Directives[21].Kind);
+  EXPECT_EQ(pp_eof, Directives[22].Kind);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, EmptyHash) {
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4480,6 +4480,7 @@
 case pp_pragma_push_macro:
 case pp_pragma_pop_macro:
 case pp_pragma_include_alias:
+case pp_pragma_system_header:
 case 

[PATCH] D149777: [clang][deps] Teach dep directive scanner about #pragma clang system_header

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added a reviewer: akyrtzi.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This ensures we get the correct FileCharacteristic during scanning. In a
yet-to-be-upstreamed branch this fixes observable failures, but it's
also good to handle this on principle: the FileCharacteristic is a
property of the file that is observable in the scanner, so there is
nothing preventing us from depending on it.

rdar://108627403


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149777

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -90,7 +90,8 @@
"#pragma pop_macro(A)\n"
"#pragma include_alias(, )\n"
"export module m;\n"
-   "import m;\n",
+   "import m;\n"
+   "#pragma clang system_header\n",
Out, Tokens, Directives));
   EXPECT_EQ(pp_define, Directives[0].Kind);
   EXPECT_EQ(pp_undef, Directives[1].Kind);
@@ -113,7 +114,8 @@
   EXPECT_EQ(pp_pragma_include_alias, Directives[18].Kind);
   EXPECT_EQ(cxx_export_module_decl, Directives[19].Kind);
   EXPECT_EQ(cxx_import_decl, Directives[20].Kind);
-  EXPECT_EQ(pp_eof, Directives[21].Kind);
+  EXPECT_EQ(pp_pragma_system_header, Directives[21].Kind);
+  EXPECT_EQ(pp_eof, Directives[22].Kind);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, EmptyHash) {
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4480,6 +4480,7 @@
 case pp_pragma_push_macro:
 case pp_pragma_pop_macro:
 case pp_pragma_include_alias:
+case pp_pragma_system_header:
 case pp_include_next:
 case decl_at_import:
 case cxx_module_decl:
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -652,9 +652,22 @@
 return false;
   }
 
-  // #pragma clang.
-  if (!isNextIdentifierOrSkipLine("module", First, End))
+  FoundId = tryLexIdentifierOrSkipLine(First, End);
+  if (!FoundId)
 return false;
+  Id = *FoundId;
+
+  // #pragma clang system_header
+  if (Id == "system_header") {
+lexPPDirectiveBody(First, End);
+pushDirective(pp_pragma_system_header);
+return false;
+  }
+
+  if (Id != "module") {
+skipLine(First, End);
+return false;
+  }
 
   // #pragma clang module.
   if (!isNextIdentifierOrSkipLine("import", First, End))
Index: clang/include/clang/Lex/DependencyDirectivesScanner.h
===
--- clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -68,6 +68,7 @@
   pp_pragma_push_macro,
   pp_pragma_pop_macro,
   pp_pragma_include_alias,
+  pp_pragma_system_header,
   pp_include_next,
   pp_if,
   pp_ifdef,


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -90,7 +90,8 @@
"#pragma pop_macro(A)\n"
"#pragma include_alias(, )\n"
"export module m;\n"
-   "import m;\n",
+   "import m;\n"
+   "#pragma clang system_header\n",
Out, Tokens, Directives));
   EXPECT_EQ(pp_define, Directives[0].Kind);
   EXPECT_EQ(pp_undef, Directives[1].Kind);
@@ -113,7 +114,8 @@
   EXPECT_EQ(pp_pragma_include_alias, Directives[18].Kind);
   EXPECT_EQ(cxx_export_module_decl, Directives[19].Kind);
   EXPECT_EQ(cxx_import_decl, Directives[20].Kind);
-  EXPECT_EQ(pp_eof, Directives[21].Kind);
+  EXPECT_EQ(pp_pragma_system_header, Directives[21].Kind);
+  EXPECT_EQ(pp_eof, Directives[22].Kind);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, EmptyHash) {
Index: clang/lib/Lex/Lexer.cpp
===

[PATCH] D149693: [clang][deps] Make clang-scan-deps write modules in raw format

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fe8d69ddf88: [clang][deps] Make clang-scan-deps write 
modules in raw format (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149693

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang/include/clang/CodeGen/ObjectFilePCHContainerOperations.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Serialization/PCHContainerOperations.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/module-format.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -552,7 +552,7 @@
 
   // Make sure to use the raw module format.
   CInvok->getHeaderSearchOpts().ModuleFormat = std::string(
-  CXXIdx->getPCHContainerOperations()->getRawReader().getFormat());
+  CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front());
 
   auto Unit = ASTUnit::create(CInvok, Diags, CaptureDiagnostics,
   /*UserFilesAreVolatile=*/true);
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3964,7 +3964,7 @@
   TUKind, CacheCodeCompletionResults, IncludeBriefCommentsInCodeCompletion,
   /*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
   /*UserFilesAreVolatile=*/true, ForSerialization, RetainExcludedCB,
-  CXXIdx->getPCHContainerOperations()->getRawReader().getFormat(),
+  CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front(),
   ));
 
   // Early failures in LoadFromCommandLine may return with ErrUnit unset.
Index: clang/test/ClangScanDeps/module-format.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/module-format.c
@@ -0,0 +1,64 @@
+// Check that the scanner produces raw ast files, even when builds produce the
+// obj format, and that the scanner can read obj format from PCH and modules
+// imported by PCH.
+
+// Unsupported on AIX because we don't support the requisite "__clangast"
+// section in XCOFF yet.
+// UNSUPPORTED: target={{.*}}-aix{{.*}}
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: cp %S/Inputs/modules-pch/* %t
+
+// Scan dependencies of the PCH:
+//
+// RUN: rm -f %t/cdb_pch.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_pch.json > %t/cdb_pch.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json -format experimental-full \
+// RUN:   -module-files-dir %t/build > %t/result_pch.json
+
+// Explicitly build the PCH:
+//
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModCommon1 > %t/mod_common_1.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModCommon2 > %t/mod_common_2.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModPCH > %t/mod_pch.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --tu-index=0 > %t/pch.rsp
+//
+// RUN: %clang @%t/mod_common_1.cc1.rsp
+// RUN: %clang @%t/mod_common_2.cc1.rsp
+// RUN: %clang @%t/mod_pch.cc1.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan dependencies of the TU:
+//
+// RUN: rm -f %t/cdb_tu.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_tu.json > %t/cdb_tu.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_tu.json -format experimental-full \
+// RUN:   -module-files-dir %t/build > %t/result_tu.json
+
+// Explicitly build the TU:
+//
+// RUN: %deps-to-rsp %t/result_tu.json --module-name=ModTU > %t/mod_tu.cc1.rsp
+// RUN: %deps-to-rsp %t/result_tu.json --tu-index=0 > %t/tu.rsp
+//
+// RUN: %clang @%t/mod_tu.cc1.rsp
+// RUN: %clang @%t/tu.rsp
+
+// Check the module format for scanner modules:
+//
+// RUN: find %t/cache -name "*.pcm" -exec %clang_cc1 -module-file-info "{}" ";" | FileCheck %s -check-prefix=SCAN
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+
+// Check the module format for built modules:
+//
+// RUN: find %t/build -name "*.pcm" -exec %clang_cc1 -module-file-info "{}" ";" | FileCheck %s -check-prefix=BUILD
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+
+// FIXME: check pch format as well; -module-file-info does not work with a PCH
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ 

[PATCH] D149693: [clang][deps] Make clang-scan-deps write modules in raw format

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 518891.
Herald added a subscriber: kadircet.
Herald added a project: clang-tools-extra.

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

https://reviews.llvm.org/D149693

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang/include/clang/CodeGen/ObjectFilePCHContainerOperations.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Serialization/PCHContainerOperations.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/module-format.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -552,7 +552,7 @@
 
   // Make sure to use the raw module format.
   CInvok->getHeaderSearchOpts().ModuleFormat = std::string(
-  CXXIdx->getPCHContainerOperations()->getRawReader().getFormat());
+  CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front());
 
   auto Unit = ASTUnit::create(CInvok, Diags, CaptureDiagnostics,
   /*UserFilesAreVolatile=*/true);
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3964,7 +3964,7 @@
   TUKind, CacheCodeCompletionResults, IncludeBriefCommentsInCodeCompletion,
   /*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
   /*UserFilesAreVolatile=*/true, ForSerialization, RetainExcludedCB,
-  CXXIdx->getPCHContainerOperations()->getRawReader().getFormat(),
+  CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front(),
   ));
 
   // Early failures in LoadFromCommandLine may return with ErrUnit unset.
Index: clang/test/ClangScanDeps/module-format.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/module-format.c
@@ -0,0 +1,64 @@
+// Check that the scanner produces raw ast files, even when builds produce the
+// obj format, and that the scanner can read obj format from PCH and modules
+// imported by PCH.
+
+// Unsupported on AIX because we don't support the requisite "__clangast"
+// section in XCOFF yet.
+// UNSUPPORTED: target={{.*}}-aix{{.*}}
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: cp %S/Inputs/modules-pch/* %t
+
+// Scan dependencies of the PCH:
+//
+// RUN: rm -f %t/cdb_pch.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_pch.json > %t/cdb_pch.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json -format experimental-full \
+// RUN:   -module-files-dir %t/build > %t/result_pch.json
+
+// Explicitly build the PCH:
+//
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModCommon1 > %t/mod_common_1.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModCommon2 > %t/mod_common_2.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModPCH > %t/mod_pch.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --tu-index=0 > %t/pch.rsp
+//
+// RUN: %clang @%t/mod_common_1.cc1.rsp
+// RUN: %clang @%t/mod_common_2.cc1.rsp
+// RUN: %clang @%t/mod_pch.cc1.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan dependencies of the TU:
+//
+// RUN: rm -f %t/cdb_tu.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_tu.json > %t/cdb_tu.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_tu.json -format experimental-full \
+// RUN:   -module-files-dir %t/build > %t/result_tu.json
+
+// Explicitly build the TU:
+//
+// RUN: %deps-to-rsp %t/result_tu.json --module-name=ModTU > %t/mod_tu.cc1.rsp
+// RUN: %deps-to-rsp %t/result_tu.json --tu-index=0 > %t/tu.rsp
+//
+// RUN: %clang @%t/mod_tu.cc1.rsp
+// RUN: %clang @%t/tu.rsp
+
+// Check the module format for scanner modules:
+//
+// RUN: find %t/cache -name "*.pcm" -exec %clang_cc1 -module-file-info "{}" ";" | FileCheck %s -check-prefix=SCAN
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+
+// Check the module format for built modules:
+//
+// RUN: find %t/build -name "*.pcm" -exec %clang_cc1 -module-file-info "{}" ";" | FileCheck %s -check-prefix=BUILD
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+
+// FIXME: check pch format as well; -module-file-info does not work with a PCH
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -181,6 +181,7 @@
 ScanInstance.getFrontendOpts().GenerateGlobalModuleIndex 

[PATCH] D149693: [clang][deps] Make clang-scan-deps write modules in raw format

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, akyrtzi, Bigcheese.
Herald added a subscriber: arphaman.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We have no use for debug info for the scanner modules, and writing raw ast 
files speeds up scanning ~15% in some cases. Note that the compile commands 
produced by the scanner will still build the obj format (if requested), and the 
scanner can *read* obj format pcms, e.g. from a PCH.

rdar://108807592


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149693

Files:
  clang/include/clang/CodeGen/ObjectFilePCHContainerOperations.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Serialization/PCHContainerOperations.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/module-format.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -552,7 +552,7 @@
 
   // Make sure to use the raw module format.
   CInvok->getHeaderSearchOpts().ModuleFormat = std::string(
-  CXXIdx->getPCHContainerOperations()->getRawReader().getFormat());
+  CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front());
 
   auto Unit = ASTUnit::create(CInvok, Diags, CaptureDiagnostics,
   /*UserFilesAreVolatile=*/true);
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3964,7 +3964,7 @@
   TUKind, CacheCodeCompletionResults, IncludeBriefCommentsInCodeCompletion,
   /*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
   /*UserFilesAreVolatile=*/true, ForSerialization, RetainExcludedCB,
-  CXXIdx->getPCHContainerOperations()->getRawReader().getFormat(),
+  CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front(),
   ));
 
   // Early failures in LoadFromCommandLine may return with ErrUnit unset.
Index: clang/test/ClangScanDeps/module-format.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/module-format.c
@@ -0,0 +1,64 @@
+// Check that the scanner produces raw ast files, even when builds produce the
+// obj format, and that the scanner can read obj format from PCH and modules
+// imported by PCH.
+
+// Unsupported on AIX because we don't support the requisite "__clangast"
+// section in XCOFF yet.
+// UNSUPPORTED: target={{.*}}-aix{{.*}}
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: cp %S/Inputs/modules-pch/* %t
+
+// Scan dependencies of the PCH:
+//
+// RUN: rm -f %t/cdb_pch.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_pch.json > %t/cdb_pch.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json -format experimental-full \
+// RUN:   -module-files-dir %t/build > %t/result_pch.json
+
+// Explicitly build the PCH:
+//
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModCommon1 > %t/mod_common_1.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModCommon2 > %t/mod_common_2.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --module-name=ModPCH > %t/mod_pch.cc1.rsp
+// RUN: %deps-to-rsp %t/result_pch.json --tu-index=0 > %t/pch.rsp
+//
+// RUN: %clang @%t/mod_common_1.cc1.rsp
+// RUN: %clang @%t/mod_common_2.cc1.rsp
+// RUN: %clang @%t/mod_pch.cc1.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan dependencies of the TU:
+//
+// RUN: rm -f %t/cdb_tu.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_tu.json > %t/cdb_tu.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_tu.json -format experimental-full \
+// RUN:   -module-files-dir %t/build > %t/result_tu.json
+
+// Explicitly build the TU:
+//
+// RUN: %deps-to-rsp %t/result_tu.json --module-name=ModTU > %t/mod_tu.cc1.rsp
+// RUN: %deps-to-rsp %t/result_tu.json --tu-index=0 > %t/tu.rsp
+//
+// RUN: %clang @%t/mod_tu.cc1.rsp
+// RUN: %clang @%t/tu.rsp
+
+// Check the module format for scanner modules:
+//
+// RUN: find %t/cache -name "*.pcm" -exec %clang_cc1 -module-file-info "{}" ";" | FileCheck %s -check-prefix=SCAN
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+// SCAN: Module format: raw
+
+// Check the module format for built modules:
+//
+// RUN: find %t/build -name "*.pcm" -exec %clang_cc1 -module-file-info "{}" ";" | FileCheck %s -check-prefix=BUILD
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+// BUILD: Module format: obj
+
+// FIXME: check pch format as well; 

[PATCH] D148176: [clang][modules] Avoid re-exporting PCH imports on every later module import

[Ben Langmuir via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe06a91c5996b: [clang][modules] Avoid re-exporting PCH 
imports on every later module import (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148176

Files:
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Serialization/ASTReader.cpp
  clang/test/ClangScanDeps/modules-pch-imports.c
  clang/test/Modules/submodule-visibility-pch.c

Index: clang/test/Modules/submodule-visibility-pch.c
===
--- /dev/null
+++ clang/test/Modules/submodule-visibility-pch.c
@@ -0,0 +1,56 @@
+// Verify that the use of a PCH does not accidentally make modules from the PCH
+// visible to submodules when using -fmodules-local-submodule-visibility
+// and -fmodule-name to trigger a textual include.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// First check that it works with a header
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -fmodule-name=Mod \
+// RUN:   %t/tu.c -include %t/prefix.h -I %t -verify
+
+// Now with a PCH
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -x c-header %t/prefix.h -emit-pch -o %t/prefix.pch -I %t
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -fmodule-name=Mod \
+// RUN:   %t/tu.c -include-pch %t/prefix.pch -I %t -verify
+
+//--- module.modulemap
+module ModViaPCH { header "ModViaPCH.h" }
+module ModViaInclude { header "ModViaInclude.h" }
+module Mod { header "Mod.h" }
+module SomeOtherMod { header "SomeOtherMod.h" }
+
+//--- ModViaPCH.h
+#define ModViaPCH 1
+
+//--- ModViaInclude.h
+#define ModViaInclude 2
+
+//--- SomeOtherMod.h
+// empty
+
+//--- Mod.h
+#include "SomeOtherMod.h"
+#ifdef ModViaPCH
+#error "Visibility violation ModViaPCH"
+#endif
+#ifdef ModViaInclude
+#error "Visibility violation ModViaInclude"
+#endif
+
+//--- prefix.h
+#include "ModViaPCH.h"
+
+//--- tu.c
+#include "ModViaInclude.h"
+#include "Mod.h"
+// expected-no-diagnostics
Index: clang/test/ClangScanDeps/modules-pch-imports.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-pch-imports.c
@@ -0,0 +1,108 @@
+// Check that a module from -fmodule-name= does not accidentally pick up extra
+// dependencies that come from a PCH.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: sed "s|DIR|%/t|g" %t/cdb_pch.json.template > %t/cdb_pch.json
+
+// Scan PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json \
+// RUN:   -format experimental-full -mode preprocess-dependency-directives \
+// RUN:   > %t/deps_pch.json
+
+// Build PCH
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name A > %t/A.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name B > %t/B.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
+// RUN: %clang @%t/A.rsp
+// RUN: %clang @%t/B.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan TU with PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -mode preprocess-dependency-directives \
+// RUN:   > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// Verify that the only modular import in C is E and not the unrelated modules
+// A or B that come from the PCH.
+
+// CHECK:  {
+// CHECK-NEXT:  "modules": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK:"module-name": "E"
+// CHECK:  }
+// CHECK-NEXT:   ]
+// CHECK:"clang-modulemap-file": "[[PREFIX]]/module.modulemap"
+// CHECK:"command-line": [
+// CHECK-NOT:  "-fmodule-file=
+// CHECK:  "-fmodule-file={{(E=)?}}[[PREFIX]]/{{.*}}/E-{{.*}}.pcm"
+// CHECK-NOT:  "-fmodule-file=
+// CHECK:]
+// CHECK:"name": "C"
+// CHECK:  }
+
+
+//--- cdb_pch.json.template
+[{
+  "file": "DIR/prefix.h",
+  "directory": "DIR",
+  "command": "clang -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodule-name=C -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- module.modulemap
+module A { header "A.h" export * }
+module B { header "B.h" export * }
+module C { header 

[PATCH] D148176: [clang][modules] Avoid re-exporting PCH imports on every later module import

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D148176#4283942 , @jansvoboda11 
wrote:

> Can you explain why we need to keep separate `PendingImportedModulesSema` 
> now? In what situation will it end up aggregating more than one 
> `PendingImportedModules`?

I don't know if this happens in practice, but I haven't been able to prove to 
myself that it cannot. I'm not sure there is much simplification we can get 
here regardless, because we still need some kind of state tracking here for the 
case when Sema is not provided until later.  If we are sure it can never add 
more `PendingImportedModules` later, then a flag telling us whether we have 
handled these modules in PP (but not yet Sema) would be sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148176

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148176: [clang][modules] Avoid re-exporting PCH imports on every later module import

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148176

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148176: [clang][modules] Avoid re-exporting PCH imports on every later module import

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: akyrtzi, jansvoboda11, Bigcheese.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We only want to make PCH imports visible once for the the TU, not repeatedly 
after every subsequent import. This causes some incorrect behaviour with 
submodule visibility, and causes us to get extra module dependencies in the 
scanner. So far I have only seen obviously incorrect behaviour when building 
with -fmodule-name to cause a submodule to be textually included when using the 
PCH, though the old behaviour seems wrong regardless.

rdar://107449644


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148176

Files:
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Serialization/ASTReader.cpp
  clang/test/ClangScanDeps/modules-pch-imports.c
  clang/test/Modules/submodule-visibility-pch.c

Index: clang/test/Modules/submodule-visibility-pch.c
===
--- /dev/null
+++ clang/test/Modules/submodule-visibility-pch.c
@@ -0,0 +1,56 @@
+// Verify that the use of a PCH does not accidentally make modules from the PCH
+// visible to submodules when using -fmodules-local-submodule-visibility
+// and -fmodule-name to trigger a textual include.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// First check that it works with a header
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -fmodule-name=Mod \
+// RUN:   %t/tu.c -include %t/prefix.h -I %t -verify
+
+// Now with a PCH
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -x c-header %t/prefix.h -emit-pch -o %t/prefix.pch -I %t
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN:   -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN:   -fmodule-name=Mod \
+// RUN:   %t/tu.c -include-pch %t/prefix.pch -I %t -verify
+
+//--- module.modulemap
+module ModViaPCH { header "ModViaPCH.h" }
+module ModViaInclude { header "ModViaInclude.h" }
+module Mod { header "Mod.h" }
+module SomeOtherMod { header "SomeOtherMod.h" }
+
+//--- ModViaPCH.h
+#define ModViaPCH 1
+
+//--- ModViaInclude.h
+#define ModViaInclude 2
+
+//--- SomeOtherMod.h
+// empty
+
+//--- Mod.h
+#include "SomeOtherMod.h"
+#ifdef ModViaPCH
+#error "Visibility violation ModViaPCH"
+#endif
+#ifdef ModViaInclude
+#error "Visibility violation ModViaInclude"
+#endif
+
+//--- prefix.h
+#include "ModViaPCH.h"
+
+//--- tu.c
+#include "ModViaInclude.h"
+#include "Mod.h"
+// expected-no-diagnostics
Index: clang/test/ClangScanDeps/modules-pch-imports.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-pch-imports.c
@@ -0,0 +1,108 @@
+// Check that a module from -fmodule-name= does not accidentally pick up extra
+// dependencies that come from a PCH.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: sed "s|DIR|%/t|g" %t/cdb_pch.json.template > %t/cdb_pch.json
+
+// Scan PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json \
+// RUN:   -format experimental-full -mode preprocess-dependency-directives \
+// RUN:   > %t/deps_pch.json
+
+// Build PCH
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name A > %t/A.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name B > %t/B.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
+// RUN: %clang @%t/A.rsp
+// RUN: %clang @%t/B.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan TU with PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -mode preprocess-dependency-directives \
+// RUN:   > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// Verify that the only modular import in C is E and not the unrelated modules
+// A or B that come from the PCH.
+
+// CHECK:  {
+// CHECK-NEXT:  "modules": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK:"module-name": "E"
+// CHECK:  }
+// CHECK-NEXT:   ]
+// CHECK:"clang-modulemap-file": "[[PREFIX]]/module.modulemap"
+// CHECK:"command-line": [
+// CHECK-NOT:  "-fmodule-file=
+// CHECK:  "-fmodule-file={{(E=)?}}[[PREFIX]]/{{.*}}/E-{{.*}}.pcm"
+// CHECK-NOT:  "-fmodule-file=
+// CHECK:]
+// CHECK:"name": "C"
+// CHECK:  }
+
+
+//--- cdb_pch.json.template
+[{
+  "file": "DIR/prefix.h",
+  "directory": "DIR",
+  "command": "clang -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"

[PATCH] D147477: [clang][modules] Handle explicit modules when checking for .Private -> _Private

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8ec36e6956cb: [clang][modules] Handle explicit modules when 
checking for .Private - _Private (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147477

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Modules/implicit-private-with-submodule-explicit.m


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,31 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+
+// Check lazily-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only 
%t/tu.m
+
+// Check eagerly-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=%t/A.pcm -fmodule-file=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 
'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, 
true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, 
IsInclusionDirective);
   if (Sub) {
 MapPrivateSubModToTopLevel = true;


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,31 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+
+// Check lazily-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+// Check eagerly-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=%t/A.pcm -fmodule-file=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  

[PATCH] D147477: [clang][modules] Handle explicit modules when checking for .Private -> _Private

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 510629.
benlangmuir added a comment.

Upload correct diff this time


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

https://reviews.llvm.org/D147477

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Modules/implicit-private-with-submodule-explicit.m


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,31 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+
+// Check lazily-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only 
%t/tu.m
+
+// Check eagerly-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=%t/A.pcm -fmodule-file=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 
'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, 
true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, 
IsInclusionDirective);
   if (Sub) {
 MapPrivateSubModToTopLevel = true;


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,31 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+
+// Check lazily-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+// Check eagerly-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=%t/A.pcm -fmodule-file=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 

[PATCH] D147477: [clang][modules] Handle explicit modules when checking for .Private -> _Private

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 510628.
benlangmuir added a comment.

Also test eagerly-loaded pcm


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

https://reviews.llvm.org/D147477

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Modules/implicit-private-with-submodule-explicit.m


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,31 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+
+// Check lazily-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only 
%t/tu.m
+
+// Check eagerly-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=A=%t/A.pcm -fmodule-file=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 
'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, 
true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, 
IsInclusionDirective);
   if (Sub) {
 MapPrivateSubModToTopLevel = true;


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,31 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+
+// Check lazily-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+// Check eagerly-loaded module
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=A=%t/A.pcm -fmodule-file=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != 

[PATCH] D147477: [clang][modules] Handle explicit modules when checking for .Private -> _Private

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 510623.

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

https://reviews.llvm.org/D147477

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Modules/implicit-private-with-submodule-explicit.m


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,26 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only 
%t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 
'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, 
true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, 
IsInclusionDirective);
   if (Sub) {
 MapPrivateSubModToTopLevel = true;


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,26 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, IsInclusionDirective);
   if (Sub) {
 MapPrivateSubModToTopLevel = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147477: [clang][modules] Handle explicit modules when checking for .Private -> _Private

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: akyrtzi, jansvoboda11.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

While we eventually want to remove the mapping from .Private to _Private 
modules, until we do, ensure that it behaves the same for explicit modules.

rdar://107449872


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147477

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Modules/implicit-private-with-submodule-explicit.m


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,26 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module 
-fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules 
-fno-implicit-module-maps -fmodule-file=A=%t/A.pcm 
-fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 
'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, 
true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, 
IsInclusionDirective);
   if (Sub) {
 MapPrivateSubModToTopLevel = true;


Index: clang/test/Modules/implicit-private-with-submodule-explicit.m
===
--- /dev/null
+++ clang/test/Modules/implicit-private-with-submodule-explicit.m
@@ -0,0 +1,26 @@
+// Checks that the use of .Private to refer to _Private modules works with an
+// explicit module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A %t/module.modulemap -o %t/A.pcm
+// RUN: %clang_cc1 -x objective-c -fmodules -fno-implicit-modules -emit-module -fmodule-name=A_Private %t/module.modulemap -o %t/A_Private.pcm
+// RUN: %clang_cc1 -x objective-c -verify -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-file=A=%t/A.pcm -fmodule-file=A_Private=%t/A_Private.pcm -fsyntax-only %t/tu.m
+
+//--- module.modulemap
+module A { header "a.h" }
+module A_Private { header "priv.h" }
+
+//--- a.h
+
+//--- priv.h
+void priv(void);
+
+//--- tu.m
+@import A.Private; // expected-warning{{no submodule named 'Private' in module 'A'; using top level 'A_Private'}}
+// expected-note@*:* {{defined here}}
+
+void tu(void) {
+  priv();
+}
\ No newline at end of file
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -2026,8 +2026,12 @@
   PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID());
   PrivPath.push_back(std::make_pair(, Path[0].second));
 
+  std::string FileName;
+  // If there is a modulemap module or prebuilt module, load it.
   if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, ImportLoc, true,
- !IsInclusionDirective))
+ !IsInclusionDirective) ||
+  selectModuleSource(nullptr, PrivateModule, FileName, BuiltModules,
+ PP->getHeaderSearchInfo()) != MS_ModuleNotFound)
 Sub = loadModule(ImportLoc, PrivPath, Visibility, IsInclusionDirective);
   if (Sub) {
 

[PATCH] D147282: [clang][deps] Remove -coverage-data-file and -coverage-notes-file from modules

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG758bca648385: [clang][deps] Remove -coverage-data-file and 
-coverage-notes-file from modules (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147282

Files:
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
  clang/test/ClangScanDeps/removed-args.c


Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -23,6 +23,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir="
 // CHECK-NOT:  "-fcoverage-compilation-dir="
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
@@ -46,6 +48,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir=
 // CHECK-NOT:  "-fcoverage-compilation-dir=
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
Index: clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
===
--- clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
+++ clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
@@ -1,7 +1,7 @@
 [
   {
 "directory": "DIR",
-"command": "clang -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps 
-fmodules-validate-once-per-build-session 
-fbuild-session-file=DIR/build-session -fmodules-prune-interval=123 
-fmodules-prune-after=123 -fmodules-cache-path=DIR/cache -include DIR/header.h 
-grecord-command-line -fdebug-compilation-dir=DIR/debug 
-fcoverage-compilation-dir=DIR/coverage -o DIR/tu.o -serialize-diagnostics 
DIR/tu.diag -MT tu -MD -MF DIR/tu.d",
+"command": "clang -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps 
-fmodules-validate-once-per-build-session 
-fbuild-session-file=DIR/build-session -fmodules-prune-interval=123 
-fmodules-prune-after=123 -fmodules-cache-path=DIR/cache -include DIR/header.h 
-grecord-command-line -fdebug-compilation-dir=DIR/debug 
-fcoverage-compilation-dir=DIR/coverage -ftest-coverage -o DIR/tu.o 
-serialize-diagnostics DIR/tu.diag -MT tu -MD -MF DIR/tu.d",
 "file": "DIR/tu.c"
   }
 ]
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -100,6 +100,8 @@
   if (!CI.getLangOpts()->ModulesCodegen) {
 CI.getCodeGenOpts().DebugCompilationDir.clear();
 CI.getCodeGenOpts().CoverageCompilationDir.clear();
+CI.getCodeGenOpts().CoverageDataFile.clear();
+CI.getCodeGenOpts().CoverageNotesFile.clear();
   }
 
   // Map output paths that affect behaviour to "-" so their existence is in the


Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -23,6 +23,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir="
 // CHECK-NOT:  "-fcoverage-compilation-dir="
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
@@ -46,6 +48,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir=
 // CHECK-NOT:  "-fcoverage-compilation-dir=
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
Index: clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
===
--- clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
+++ clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
@@ -1,7 +1,7 @@
 [
   {
 "directory": "DIR",
-"command": "clang -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps -fmodules-validate-once-per-build-session -fbuild-session-file=DIR/build-session -fmodules-prune-interval=123 -fmodules-prune-after=123 -fmodules-cache-path=DIR/cache -include DIR/header.h -grecord-command-line -fdebug-compilation-dir=DIR/debug -fcoverage-compilation-dir=DIR/coverage -o DIR/tu.o 

[PATCH] D147282: [clang][deps] Remove -coverage-data-file and -coverage-notes-file from modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: akyrtzi, jansvoboda11.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When not performing codegen, we can strip the coverage-data-file and 
coverage-notes-file options to improve canonicalization.

rdar://107443796


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147282

Files:
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
  clang/test/ClangScanDeps/removed-args.c


Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -23,6 +23,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir="
 // CHECK-NOT:  "-fcoverage-compilation-dir="
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
@@ -46,6 +48,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir=
 // CHECK-NOT:  "-fcoverage-compilation-dir=
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
Index: clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
===
--- clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
+++ clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
@@ -1,7 +1,7 @@
 [
   {
 "directory": "DIR",
-"command": "clang -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps 
-fmodules-validate-once-per-build-session 
-fbuild-session-file=DIR/build-session -fmodules-prune-interval=123 
-fmodules-prune-after=123 -fmodules-cache-path=DIR/cache -include DIR/header.h 
-grecord-command-line -fdebug-compilation-dir=DIR/debug 
-fcoverage-compilation-dir=DIR/coverage -o DIR/tu.o -serialize-diagnostics 
DIR/tu.diag -MT tu -MD -MF DIR/tu.d",
+"command": "clang -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps 
-fmodules-validate-once-per-build-session 
-fbuild-session-file=DIR/build-session -fmodules-prune-interval=123 
-fmodules-prune-after=123 -fmodules-cache-path=DIR/cache -include DIR/header.h 
-grecord-command-line -fdebug-compilation-dir=DIR/debug 
-fcoverage-compilation-dir=DIR/coverage -ftest-coverage -o DIR/tu.o 
-serialize-diagnostics DIR/tu.diag -MT tu -MD -MF DIR/tu.d",
 "file": "DIR/tu.c"
   }
 ]
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -100,6 +100,8 @@
   if (!CI.getLangOpts()->ModulesCodegen) {
 CI.getCodeGenOpts().DebugCompilationDir.clear();
 CI.getCodeGenOpts().CoverageCompilationDir.clear();
+CI.getCodeGenOpts().CoverageDataFile.clear();
+CI.getCodeGenOpts().CoverageNotesFile.clear();
   }
 
   // Map output paths that affect behaviour to "-" so their existence is in the


Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -23,6 +23,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir="
 // CHECK-NOT:  "-fcoverage-compilation-dir="
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
@@ -46,6 +48,8 @@
 // CHECK-NEXT: "-cc1"
 // CHECK-NOT:  "-fdebug-compilation-dir=
 // CHECK-NOT:  "-fcoverage-compilation-dir=
+// CHECK-NOT:  "-coverage-notes-file
+// CHECK-NOT:  "-coverage-data-file
 // CHECK-NOT:  "-dwarf-debug-flags"
 // CHECK-NOT:  "-main-file-name"
 // CHECK-NOT:  "-include"
Index: clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
===
--- clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
+++ clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
@@ -1,7 +1,7 @@
 [
   {
 "directory": "DIR",
-"command": "clang -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps -fmodules-validate-once-per-build-session -fbuild-session-file=DIR/build-session -fmodules-prune-interval=123 -fmodules-prune-after=123 -fmodules-cache-path=DIR/cache -include DIR/header.h 

[PATCH] D145838: [clang][deps] Handle response files in dep scanner

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfcab930cd32e: [clang][deps] Handle response files in dep 
scanner (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145838

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/response-file.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -391,55 +391,17 @@
   const char *ProgName =
   ToolContext.NeedsPrependArg ? ToolContext.PrependArg : ToolContext.Path;
 
-  // Parse response files using the GNU syntax, unless we're in CL mode. There
-  // are two ways to put clang in CL compatibility mode: ProgName is either
-  // clang-cl or cl, or --driver-mode=cl is on the command line. The normal
-  // command line parsing can't happen until after response file parsing, so we
-  // have to manually search for a --driver-mode=cl argument the hard way.
-  // Finally, our -cc1 tools don't care which tokenization mode we use because
-  // response files written by clang will tokenize the same way in either mode.
   bool ClangCLMode =
   IsClangCL(getDriverMode(ProgName, llvm::ArrayRef(Args).slice(1)));
-  enum { Default, POSIX, Windows } RSPQuoting = Default;
-  for (const char *F : Args) {
-if (strcmp(F, "--rsp-quoting=posix") == 0)
-  RSPQuoting = POSIX;
-else if (strcmp(F, "--rsp-quoting=windows") == 0)
-  RSPQuoting = Windows;
-  }
 
-  // Determines whether we want nullptr markers in Args to indicate response
-  // files end-of-lines. We only use this for the /LINK driver argument with
-  // clang-cl.exe on Windows.
-  bool MarkEOLs = ClangCLMode;
-
-  llvm::cl::TokenizerCallback Tokenizer;
-  if (RSPQuoting == Windows || (RSPQuoting == Default && ClangCLMode))
-Tokenizer = ::cl::TokenizeWindowsCommandLine;
-  else
-Tokenizer = ::cl::TokenizeGNUCommandLine;
-
-  if (MarkEOLs && Args.size() > 1 && StringRef(Args[1]).startswith("-cc1"))
-MarkEOLs = false;
-  llvm::cl::ExpansionContext ECtx(A, Tokenizer);
-  ECtx.setMarkEOLs(MarkEOLs);
-  if (llvm::Error Err = ECtx.expandResponseFiles(Args)) {
+  if (llvm::Error Err = expandResponseFiles(Args, ClangCLMode, A)) {
 llvm::errs() << toString(std::move(Err)) << '\n';
 return 1;
   }
 
-  // Handle -cc1 integrated tools, even if -cc1 was expanded from a response
-  // file.
-  auto FirstArg = llvm::find_if(llvm::drop_begin(Args),
-[](const char *A) { return A != nullptr; });
-  if (FirstArg != Args.end() && StringRef(*FirstArg).startswith("-cc1")) {
-// If -cc1 came from a response file, remove the EOL sentinels.
-if (MarkEOLs) {
-  auto newEnd = std::remove(Args.begin(), Args.end(), nullptr);
-  Args.resize(newEnd - Args.begin());
-}
+  // Handle -cc1 integrated tools.
+  if (Args.size() >= 2 && StringRef(Args[1]).startswith("-cc1"))
 return ExecuteCC1Tool(Args, ToolContext);
-  }
 
   // Handle options that need handling before the real command line parsing in
   // Driver::BuildCompilation()
Index: clang/test/ClangScanDeps/response-file.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/response-file.c
@@ -0,0 +1,41 @@
+// Check that the scanner can handle a response file input.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -format experimental-full -compilation-database %t/cdb.json > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  "command-line": [
+// CHECK:"-fsyntax-only"
+// CHECK:"-x"
+// CHECK-NEXT:   "c"
+// CHECK:"tu.c"
+// CHECK:"-I"
+// CHECK-NEXT:   "include"
+// CHECK:  ],
+// CHECK:  "file-deps": [
+// CHECK-NEXT:   "[[PREFIX]]/tu.c"
+// CHECK-NEXT:   "[[PREFIX]]/include/header.h"
+// CHECK-NEXT: ]
+
+//--- cdb.json.template
+[{
+  "file": "DIR/t.c",
+  "directory": "DIR",
+  "command": "clang @DIR/args.txt"
+}]
+
+//--- args.txt
+@args_nested.txt
+-fsyntax-only tu.c
+
+//--- args_nested.txt
+-I include
+
+//--- include/header.h
+
+//--- tu.c
+#include "header.h"
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 

[PATCH] D145838: [clang][deps] Handle response files in dep scanner

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, Bigcheese.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Extract the code the driver uses to expand response files and reuse it in the 
dependency scanner.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145838

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/response-file.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -391,55 +391,17 @@
   const char *ProgName =
   ToolContext.NeedsPrependArg ? ToolContext.PrependArg : ToolContext.Path;
 
-  // Parse response files using the GNU syntax, unless we're in CL mode. There
-  // are two ways to put clang in CL compatibility mode: ProgName is either
-  // clang-cl or cl, or --driver-mode=cl is on the command line. The normal
-  // command line parsing can't happen until after response file parsing, so we
-  // have to manually search for a --driver-mode=cl argument the hard way.
-  // Finally, our -cc1 tools don't care which tokenization mode we use because
-  // response files written by clang will tokenize the same way in either mode.
   bool ClangCLMode =
   IsClangCL(getDriverMode(ProgName, llvm::ArrayRef(Args).slice(1)));
-  enum { Default, POSIX, Windows } RSPQuoting = Default;
-  for (const char *F : Args) {
-if (strcmp(F, "--rsp-quoting=posix") == 0)
-  RSPQuoting = POSIX;
-else if (strcmp(F, "--rsp-quoting=windows") == 0)
-  RSPQuoting = Windows;
-  }
 
-  // Determines whether we want nullptr markers in Args to indicate response
-  // files end-of-lines. We only use this for the /LINK driver argument with
-  // clang-cl.exe on Windows.
-  bool MarkEOLs = ClangCLMode;
-
-  llvm::cl::TokenizerCallback Tokenizer;
-  if (RSPQuoting == Windows || (RSPQuoting == Default && ClangCLMode))
-Tokenizer = ::cl::TokenizeWindowsCommandLine;
-  else
-Tokenizer = ::cl::TokenizeGNUCommandLine;
-
-  if (MarkEOLs && Args.size() > 1 && StringRef(Args[1]).startswith("-cc1"))
-MarkEOLs = false;
-  llvm::cl::ExpansionContext ECtx(A, Tokenizer);
-  ECtx.setMarkEOLs(MarkEOLs);
-  if (llvm::Error Err = ECtx.expandResponseFiles(Args)) {
+  if (llvm::Error Err = expandResponseFiles(Args, ClangCLMode, A)) {
 llvm::errs() << toString(std::move(Err)) << '\n';
 return 1;
   }
 
-  // Handle -cc1 integrated tools, even if -cc1 was expanded from a response
-  // file.
-  auto FirstArg = llvm::find_if(llvm::drop_begin(Args),
-[](const char *A) { return A != nullptr; });
-  if (FirstArg != Args.end() && StringRef(*FirstArg).startswith("-cc1")) {
-// If -cc1 came from a response file, remove the EOL sentinels.
-if (MarkEOLs) {
-  auto newEnd = std::remove(Args.begin(), Args.end(), nullptr);
-  Args.resize(newEnd - Args.begin());
-}
+  // Handle -cc1 integrated tools.
+  if (Args.size() >= 2 && StringRef(Args[1]).startswith("-cc1"))
 return ExecuteCC1Tool(Args, ToolContext);
-  }
 
   // Handle options that need handling before the real command line parsing in
   // Driver::BuildCompilation()
Index: clang/test/ClangScanDeps/response-file.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/response-file.c
@@ -0,0 +1,41 @@
+// Check that the scanner can handle a response file input.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -format experimental-full -compilation-database %t/cdb.json > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  "command-line": [
+// CHECK:"-fsyntax-only"
+// CHECK:"-x"
+// CHECK-NEXT:   "c"
+// CHECK:"tu.c"
+// CHECK:"-I"
+// CHECK-NEXT:   "include"
+// CHECK:  ],
+// CHECK:  "file-deps": [
+// CHECK-NEXT:   "[[PREFIX]]/tu.c"
+// CHECK-NEXT:   "[[PREFIX]]/include/header.h"
+// CHECK-NEXT: ]
+
+//--- cdb.json.template
+[{
+  "file": "DIR/t.c",
+  "directory": "DIR",
+  "command": "clang @DIR/args.txt"
+}]
+
+//--- args.txt
+@args_nested.txt
+-fsyntax-only tu.c
+
+//--- args_nested.txt
+-I include
+
+//--- include/header.h
+
+//--- tu.c
+#include "header.h"
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -7,6 +7,7 @@
 

[PATCH] D144058: [clang][deps] Split lookupModuleOutput out of DependencyConsumer NFC

[Ben Langmuir via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG296ba5bbd387: [clang][deps] Split lookupModuleOutput out of 
DependencyConsumer NFC (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144058

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -56,16 +56,16 @@
 void ModuleDepCollector::addOutputPaths(CompilerInvocation ,
 ModuleDeps ) {
   CI.getFrontendOpts().OutputFile =
-  Consumer.lookupModuleOutput(Deps.ID, ModuleOutputKind::ModuleFile);
+  Controller.lookupModuleOutput(Deps.ID, ModuleOutputKind::ModuleFile);
   if (!CI.getDiagnosticOpts().DiagnosticSerializationFile.empty())
 CI.getDiagnosticOpts().DiagnosticSerializationFile =
-Consumer.lookupModuleOutput(
+Controller.lookupModuleOutput(
 Deps.ID, ModuleOutputKind::DiagnosticSerializationFile);
   if (!CI.getDependencyOutputOpts().OutputFile.empty()) {
-CI.getDependencyOutputOpts().OutputFile =
-Consumer.lookupModuleOutput(Deps.ID, ModuleOutputKind::DependencyFile);
+CI.getDependencyOutputOpts().OutputFile = Controller.lookupModuleOutput(
+Deps.ID, ModuleOutputKind::DependencyFile);
 CI.getDependencyOutputOpts().Targets =
-splitString(Consumer.lookupModuleOutput(
+splitString(Controller.lookupModuleOutput(
 Deps.ID, ModuleOutputKind::DependencyTargets),
 '\0');
 if (!CI.getDependencyOutputOpts().OutputFile.empty() &&
@@ -205,7 +205,7 @@
 CompilerInvocation , ArrayRef ClangModuleDeps) const {
   for (const ModuleID  : ClangModuleDeps) {
 std::string PCMPath =
-Consumer.lookupModuleOutput(MID, ModuleOutputKind::ModuleFile);
+Controller.lookupModuleOutput(MID, ModuleOutputKind::ModuleFile);
 if (EagerLoadModules)
   CI.getFrontendOpts().ModuleFiles.push_back(std::move(PCMPath));
 else
@@ -577,11 +577,11 @@
 ModuleDepCollector::ModuleDepCollector(
 std::unique_ptr Opts,
 CompilerInstance , DependencyConsumer ,
-CompilerInvocation OriginalCI, bool OptimizeArgs, bool EagerLoadModules,
-bool IsStdModuleP1689Format)
-: ScanInstance(ScanInstance), Consumer(C), Opts(std::move(Opts)),
-  OriginalInvocation(std::move(OriginalCI)), OptimizeArgs(OptimizeArgs),
-  EagerLoadModules(EagerLoadModules),
+DependencyActionController , CompilerInvocation OriginalCI,
+bool OptimizeArgs, bool EagerLoadModules, bool IsStdModuleP1689Format)
+: ScanInstance(ScanInstance), Consumer(C), Controller(Controller),
+  Opts(std::move(Opts)), OriginalInvocation(std::move(OriginalCI)),
+  OptimizeArgs(OptimizeArgs), EagerLoadModules(EagerLoadModules),
   IsStdModuleP1689Format(IsStdModuleP1689Format) {}
 
 void ModuleDepCollector::attachToPreprocessor(Preprocessor ) {
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -146,13 +146,14 @@
 public:
   DependencyScanningAction(
   StringRef WorkingDirectory, DependencyConsumer ,
+  DependencyActionController ,
   llvm::IntrusiveRefCntPtr DepFS,
   ScanningOutputFormat Format, bool OptimizeArgs, bool EagerLoadModules,
   bool DisableFree, std::optional ModuleName = std::nullopt)
   : WorkingDirectory(WorkingDirectory), Consumer(Consumer),
-DepFS(std::move(DepFS)), Format(Format), OptimizeArgs(OptimizeArgs),
-EagerLoadModules(EagerLoadModules), DisableFree(DisableFree),
-ModuleName(ModuleName) {}
+Controller(Controller), DepFS(std::move(DepFS)), Format(Format),
+OptimizeArgs(OptimizeArgs), EagerLoadModules(EagerLoadModules),
+DisableFree(DisableFree), ModuleName(ModuleName) {}
 
   bool runInvocation(std::shared_ptr Invocation,
  FileManager *FileMgr,
@@ -250,8 +251,8 @@
 case ScanningOutputFormat::P1689:
 case ScanningOutputFormat::Full:
   MDC = std::make_shared(
-  std::move(Opts), ScanInstance, Consumer, 

[PATCH] D144058: [clang][deps] Split lookupModuleOutput out of DependencyConsumer NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 504196.

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

https://reviews.llvm.org/D144058

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -56,16 +56,16 @@
 void ModuleDepCollector::addOutputPaths(CompilerInvocation ,
 ModuleDeps ) {
   CI.getFrontendOpts().OutputFile =
-  Consumer.lookupModuleOutput(Deps.ID, ModuleOutputKind::ModuleFile);
+  Controller.lookupModuleOutput(Deps.ID, ModuleOutputKind::ModuleFile);
   if (!CI.getDiagnosticOpts().DiagnosticSerializationFile.empty())
 CI.getDiagnosticOpts().DiagnosticSerializationFile =
-Consumer.lookupModuleOutput(
+Controller.lookupModuleOutput(
 Deps.ID, ModuleOutputKind::DiagnosticSerializationFile);
   if (!CI.getDependencyOutputOpts().OutputFile.empty()) {
-CI.getDependencyOutputOpts().OutputFile =
-Consumer.lookupModuleOutput(Deps.ID, ModuleOutputKind::DependencyFile);
+CI.getDependencyOutputOpts().OutputFile = Controller.lookupModuleOutput(
+Deps.ID, ModuleOutputKind::DependencyFile);
 CI.getDependencyOutputOpts().Targets =
-splitString(Consumer.lookupModuleOutput(
+splitString(Controller.lookupModuleOutput(
 Deps.ID, ModuleOutputKind::DependencyTargets),
 '\0');
 if (!CI.getDependencyOutputOpts().OutputFile.empty() &&
@@ -205,7 +205,7 @@
 CompilerInvocation , ArrayRef ClangModuleDeps) const {
   for (const ModuleID  : ClangModuleDeps) {
 std::string PCMPath =
-Consumer.lookupModuleOutput(MID, ModuleOutputKind::ModuleFile);
+Controller.lookupModuleOutput(MID, ModuleOutputKind::ModuleFile);
 if (EagerLoadModules)
   CI.getFrontendOpts().ModuleFiles.push_back(std::move(PCMPath));
 else
@@ -577,11 +577,11 @@
 ModuleDepCollector::ModuleDepCollector(
 std::unique_ptr Opts,
 CompilerInstance , DependencyConsumer ,
-CompilerInvocation OriginalCI, bool OptimizeArgs, bool EagerLoadModules,
-bool IsStdModuleP1689Format)
-: ScanInstance(ScanInstance), Consumer(C), Opts(std::move(Opts)),
-  OriginalInvocation(std::move(OriginalCI)), OptimizeArgs(OptimizeArgs),
-  EagerLoadModules(EagerLoadModules),
+DependencyActionController , CompilerInvocation OriginalCI,
+bool OptimizeArgs, bool EagerLoadModules, bool IsStdModuleP1689Format)
+: ScanInstance(ScanInstance), Consumer(C), Controller(Controller),
+  Opts(std::move(Opts)), OriginalInvocation(std::move(OriginalCI)),
+  OptimizeArgs(OptimizeArgs), EagerLoadModules(EagerLoadModules),
   IsStdModuleP1689Format(IsStdModuleP1689Format) {}
 
 void ModuleDepCollector::attachToPreprocessor(Preprocessor ) {
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -146,13 +146,14 @@
 public:
   DependencyScanningAction(
   StringRef WorkingDirectory, DependencyConsumer ,
+  DependencyActionController ,
   llvm::IntrusiveRefCntPtr DepFS,
   ScanningOutputFormat Format, bool OptimizeArgs, bool EagerLoadModules,
   bool DisableFree, std::optional ModuleName = std::nullopt)
   : WorkingDirectory(WorkingDirectory), Consumer(Consumer),
-DepFS(std::move(DepFS)), Format(Format), OptimizeArgs(OptimizeArgs),
-EagerLoadModules(EagerLoadModules), DisableFree(DisableFree),
-ModuleName(ModuleName) {}
+Controller(Controller), DepFS(std::move(DepFS)), Format(Format),
+OptimizeArgs(OptimizeArgs), EagerLoadModules(EagerLoadModules),
+DisableFree(DisableFree), ModuleName(ModuleName) {}
 
   bool runInvocation(std::shared_ptr Invocation,
  FileManager *FileMgr,
@@ -250,8 +251,8 @@
 case ScanningOutputFormat::P1689:
 case ScanningOutputFormat::Full:
   MDC = std::make_shared(
-  std::move(Opts), ScanInstance, Consumer, OriginalInvocation,
-  OptimizeArgs, EagerLoadModules,
+  std::move(Opts), ScanInstance, Consumer, Controller,
+  OriginalInvocation, OptimizeArgs, EagerLoadModules,
   Format == ScanningOutputFormat::P1689);
   

[PATCH] D145101: [clang][deps] NFC: Simplify worker loop

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

> I guess it was originally done this way because we have a number of 
> DependencyScanning{Tool,Worker} instances

Oh of course, this makes sense.  Yeah we could maybe find a different way to do 
it if we care, but this is a sufficiently good reason to keep doing what we're 
doing and shouldn't hold you up.

Thanks for explaining; LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145101: [clang][deps] NFC: Simplify worker loop

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:802
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I) {
-Pool.async([I, , , , , , , ,
-, ]() {
+Pool.async([&, I]() {
   llvm::StringSet<> AlreadySeenModules;

Why are we second-guessing the `ThreadPool` at all? I would think we should do

```
for (unsigned Index = 0; E = Inputs.size(); Index + E; ++Index) {
  Pool.async([Index, &]{ ... });
}
```

Then the thread pool is responsible for dispatching the tasks when it has 
available resources instead of us manually looping inside the threads.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145098: [clang][deps] Preserve input ordering in the full output

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:291
 
-std::unique_lock ul(Lock);
-Inputs.push_back(std::move(ID));
+Inputs[InputIndex] = std::move(ID);
   }

Since the input index is coming from "outside": does this `operator[]` assert 
if the index is out of range? If not, we should do so here.  I would also 
suggest asserting the value isn't already populated (e.g. 
`assert(ID.FileName.empty())`).



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:788
+  if (Format == ScanningOutputFormat::Full && ModuleName.empty())
+FD.resize(Inputs.size());
+

Since you should never resize `FullDeps` after you start, how about removing 
`resize` and instead:
* Make `FullDeps` constructor take the size
* Change `FullDeps FD;` to `std::optional`
* Change this line to `FullDeps.emplace(Inputs.size())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145098

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144495: [NFC][clang] Refine tests by adding `:` to checks

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

LGTM. The vfsroot-include.c is a -cc1 invocation, so it could probably be 
switched to use `-verify` instead of FileCheck'ing the output, unless there's 
something I'm missing.  But that doesn't need to be for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144495

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144058: [clang][deps] Split lookupModuleOutput out of DependencyConsumer NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:64-66
+/// Dependency scanner callbacks that are used during scanning to influence the
+/// behaviour of the scan - for example, to customize the scanned invocations.
+class DependencyActions {

jansvoboda11 wrote:
> What do the downstream callbacks do? The class name sounds a bit generic for 
> something you can call `lookupModuleOutput()` on, but maybe that's the right 
> name with more context. It's also very similar to the existing 
> `DependencyScanningAction`.
They are callbacks that modify the ScanInstance itself, or one of the 
CompilerInvocations being constructed (either for the TU or for module 
dependencies).  In practice, we use this to add caching support - modifying 
invocations to use inputs from a CAS. I chose "actions" based on the fact they 
are acting on the scanner/invocations, rather than simply consuming the 
information.  While lookupModuleOutput does not directly modify the invcation, 
it does indirectly via the ModuleDepCollector incorporating its results into 
the invocation.  If you're interested, here are our downstream callbacks that I 
would move into this interface: 
https://github.com/apple/llvm-project/blob/next/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h#L49

RE: DependencyScanningAction: I agree it's not ideal that these names are 
"close".  On the other hand, they're not too likely to be confused in practice 
since they have very different roles -- the DependencyScanningAction is not 
exposed to clients, and it's a tooling action not something you use to control 
the scan.  We also have other "actions" like the FrontendAction we create, 
which again have a different role.

Happy to consider suggestions for a better name!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144058

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144058: [clang][deps] Split lookupModuleOutput out of DependencyConsumer NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, akyrtzi, steven_wu.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The idea is to split the callbacks that are used to consume dependency
information (DependencyConsumer) from callbacks that modify the scan
behaviour itself in any way (DependencyActions). Currently this is just
lookupModuleOutput, but we have additional callbacks related to CAS
support that we intend to upstream in the future.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144058

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -56,16 +56,16 @@
 void ModuleDepCollector::addOutputPaths(CompilerInvocation ,
 ModuleDeps ) {
   CI.getFrontendOpts().OutputFile =
-  Consumer.lookupModuleOutput(Deps.ID, ModuleOutputKind::ModuleFile);
+  Actions.lookupModuleOutput(Deps.ID, ModuleOutputKind::ModuleFile);
   if (!CI.getDiagnosticOpts().DiagnosticSerializationFile.empty())
 CI.getDiagnosticOpts().DiagnosticSerializationFile =
-Consumer.lookupModuleOutput(
+Actions.lookupModuleOutput(
 Deps.ID, ModuleOutputKind::DiagnosticSerializationFile);
   if (!CI.getDependencyOutputOpts().OutputFile.empty()) {
 CI.getDependencyOutputOpts().OutputFile =
-Consumer.lookupModuleOutput(Deps.ID, ModuleOutputKind::DependencyFile);
+Actions.lookupModuleOutput(Deps.ID, ModuleOutputKind::DependencyFile);
 CI.getDependencyOutputOpts().Targets =
-splitString(Consumer.lookupModuleOutput(
+splitString(Actions.lookupModuleOutput(
 Deps.ID, ModuleOutputKind::DependencyTargets),
 '\0');
 if (!CI.getDependencyOutputOpts().OutputFile.empty() &&
@@ -205,7 +205,7 @@
 CompilerInvocation , ArrayRef ClangModuleDeps) const {
   for (const ModuleID  : ClangModuleDeps) {
 std::string PCMPath =
-Consumer.lookupModuleOutput(MID, ModuleOutputKind::ModuleFile);
+Actions.lookupModuleOutput(MID, ModuleOutputKind::ModuleFile);
 if (EagerLoadModules)
   CI.getFrontendOpts().ModuleFiles.push_back(std::move(PCMPath));
 else
@@ -577,11 +577,11 @@
 ModuleDepCollector::ModuleDepCollector(
 std::unique_ptr Opts,
 CompilerInstance , DependencyConsumer ,
-CompilerInvocation OriginalCI, bool OptimizeArgs, bool EagerLoadModules,
-bool IsStdModuleP1689Format)
-: ScanInstance(ScanInstance), Consumer(C), Opts(std::move(Opts)),
-  OriginalInvocation(std::move(OriginalCI)), OptimizeArgs(OptimizeArgs),
-  EagerLoadModules(EagerLoadModules),
+DependencyActions , CompilerInvocation OriginalCI,
+bool OptimizeArgs, bool EagerLoadModules, bool IsStdModuleP1689Format)
+: ScanInstance(ScanInstance), Consumer(C), Actions(Actions),
+  Opts(std::move(Opts)), OriginalInvocation(std::move(OriginalCI)),
+  OptimizeArgs(OptimizeArgs), EagerLoadModules(EagerLoadModules),
   IsStdModuleP1689Format(IsStdModuleP1689Format) {}
 
 void ModuleDepCollector::attachToPreprocessor(Preprocessor ) {
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -146,13 +146,14 @@
 public:
   DependencyScanningAction(
   StringRef WorkingDirectory, DependencyConsumer ,
+  DependencyActions ,
   llvm::IntrusiveRefCntPtr DepFS,
   ScanningOutputFormat Format, bool OptimizeArgs, bool EagerLoadModules,
   bool DisableFree, std::optional ModuleName = std::nullopt)
   : WorkingDirectory(WorkingDirectory), Consumer(Consumer),
-DepFS(std::move(DepFS)), Format(Format), OptimizeArgs(OptimizeArgs),
-EagerLoadModules(EagerLoadModules), DisableFree(DisableFree),
-ModuleName(ModuleName) {}
+Actions(Actions), DepFS(std::move(DepFS)), Format(Format),
+OptimizeArgs(OptimizeArgs), EagerLoadModules(EagerLoadModules),
+DisableFree(DisableFree), ModuleName(ModuleName) {}
 
   bool runInvocation(std::shared_ptr Invocation,
  FileManager *FileMgr,
@@ -250,7 +251,7 @@
 case