[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: yamaguchi, v.g.vassilev, ruiu, teemperor.
Herald added a subscriber: cfe-commits.

We have a regex that needs to match a tab character in the command output, but 
on macOS `sed` doesn't support '\t', causing it to split on the 't' character 
instead. Some options:

- use `perl -p -e 's/\t.*//'`, which does handle '\t'
- put a literal tab character in the string passed to sed
- use `sed -e $'s/\t.*//'`, which will cause bash to expand the '\t' before 
passing it to sed


Repository:
  rC Clang

https://reviews.llvm.org/D47273

Files:
  utils/bash-autocomplete.sh


Index: utils/bash-autocomplete.sh
===
--- utils/bash-autocomplete.sh
+++ utils/bash-autocomplete.sh
@@ -38,7 +38,7 @@
 
   # expand ~ to $HOME
   eval local path=${COMP_WORDS[0]}
-  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e 's/\t.*//' )
+  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | perl -p -e 's/\t.*//' )
   # If clang is old that it does not support --autocomplete,
   # fall back to the filename completion.
   if [[ "$?" != 0 ]]; then


Index: utils/bash-autocomplete.sh
===
--- utils/bash-autocomplete.sh
+++ utils/bash-autocomplete.sh
@@ -38,7 +38,7 @@
 
   # expand ~ to $HOME
   eval local path=${COMP_WORDS[0]}
-  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e 's/\t.*//' )
+  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | perl -p -e 's/\t.*//' )
   # If clang is old that it does not support --autocomplete,
   # fall back to the filename completion.
   if [[ "$?" != 0 ]]; then
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In https://reviews.llvm.org/D47273#1109934, @ruiu wrote:

> I wonder if you could replace \t with \0x09. At least it works on my machine 
> which has GNU sed.


Unfortunately that doesn't work either on mac :-\


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In https://reviews.llvm.org/D47273#1109985, @ruiu wrote:

>   sed -e "$(echo -e 's/\t.*//')"


Yep, that works!  Is there a reason you prefer that over the more succinct 
`$'s/\t.*//'`?


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 148278.
benlangmuir edited the summary of this revision.
benlangmuir added a comment.

Thanks for looking up  the supported bash version!  Updated diff.


https://reviews.llvm.org/D47273

Files:
  utils/bash-autocomplete.sh


Index: utils/bash-autocomplete.sh
===
--- utils/bash-autocomplete.sh
+++ utils/bash-autocomplete.sh
@@ -38,7 +38,8 @@
 
   # expand ~ to $HOME
   eval local path=${COMP_WORDS[0]}
-  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e 's/\t.*//' )
+  # Use $'\t' so that bash expands the \t for older versions of sed.
+  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e $'s/\t.*//' )
   # If clang is old that it does not support --autocomplete,
   # fall back to the filename completion.
   if [[ "$?" != 0 ]]; then


Index: utils/bash-autocomplete.sh
===
--- utils/bash-autocomplete.sh
+++ utils/bash-autocomplete.sh
@@ -38,7 +38,8 @@
 
   # expand ~ to $HOME
   eval local path=${COMP_WORDS[0]}
-  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e 's/\t.*//' )
+  # Use $'\t' so that bash expands the \t for older versions of sed.
+  flags=$( "$path" --autocomplete="$arg" 2>/dev/null | sed -e $'s/\t.*//' )
   # If clang is old that it does not support --autocomplete,
   # fall back to the filename completion.
   if [[ "$?" != 0 ]]; then
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir closed this revision.
benlangmuir added a comment.

r333202


https://reviews.llvm.org/D47273



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

[PATCH] D34256: [PR33394] Avoid lexing editor placeholders when running the preprocessor only

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

I agree with not detecting these during PP-only, but there's nothing wrong with 
`<#>`.  It's either not a placeholder, or it's part of a placeholder like 
`<#>#>`, which is a placeholder containing the text ">".  Similarly, `<##` 
could be the start of an empty placeholder `<##>`.


Repository:
  rL LLVM

https://reviews.llvm.org/D34256



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

[PATCH] D34256: [PR33394] Avoid lexing editor placeholders when running the preprocessor only

[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, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D34256



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

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

It would be nice if the doc comment for the single file parse mode flag was 
updated to include this new functionality.




Comment at: lib/Lex/PPDirectives.cpp:2709
+CurPPLexer->pushConditionalLevel(IfToken.getLocation(), /*wasskip*/true,
+   /*foundnonskip*/false, /*foundelse*/false);
+  } else if (ConditionalTrue) {

Nitpick: this is misaligned.  Same with many other calls to 
pushConditionalLevel in this patch.



Comment at: lib/Lex/PPDirectives.cpp:2774
+// the directive blocks.
+CurPPLexer->pushConditionalLevel(CI.IfLoc, /*wasskip*/false,
+   /*foundnonskip*/true, /*foundelse*/true);

Why is wasSkipping false here?


https://reviews.llvm.org/D34263



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

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: lib/Lex/PPDirectives.cpp:2774
+// the directive blocks.
+CurPPLexer->pushConditionalLevel(CI.IfLoc, /*wasskip*/false,
+   /*foundnonskip*/true, /*foundelse*/true);

akyrtzi wrote:
> benlangmuir wrote:
> > Why is wasSkipping false here?
> For `#if` it sets `wasskip` to `true` to indicate later for 
> `HandleElseDirective` that the block should not be skipped.
> But here (inside `HandleElseDirective`) setting `wasskip` doesn't really 
> affect anything, the `HandleEndifDirective` doesn't check it. I chose to set 
> it to `false` as indication that the `#else` block does get parsed.
Thanks for explaining. That makes sense to me.


https://reviews.llvm.org/D34263



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

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Thanks, this looks good to me.  I'd appreciate if @klimek could take a quick 
look though.




Comment at: include/clang/Lex/PreprocessorOptions.h:102
+  /// in preprocessor directive conditions it causes all blocks to be parsed so
+  /// that the client can get the max amount of info from the parser.
   bool SingleFileParseMode = false;

nitpick: should use the full words "maximum" and probably also "information" 
here


https://reviews.llvm.org/D34263



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

[PATCH] D41526: [libclang] Avoid builtin trap for __debug parser_crash pragma

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Did you consider just using a different pragma that triggers this behaviour 
instead of avoiding the crash?  I don't really have a strong preference but I'd 
be interested to hear what you think the pros/cons are.




Comment at: lib/Parse/ParseDecl.cpp:5657
+  else
+PP.getPreprocessorOpts().SeenParserCrashPragma = true;
+}

This should return immediately (and keep exiting until ~ParseAST) or it won't 
really behave like a trap.


Repository:
  rC Clang

https://reviews.llvm.org/D41526



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

[PATCH] D44652: [vfs] Don't bail out after a missing -ivfsoverlay file

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: bruno, vsapsai.
Herald added a subscriber: cfe-commits.

This make -ivfsoverlay behave more like other fatal errors (e.g. missing
-include file) by skipping the missing file instead of bailing out of
the whole compilation. This makes it possible for libclang to still
provide some functionallity as well as to correctly produce the fatal
error diagnostic (previously we lost the diagnostic in libclang since
there was no TU to tie it to).

  

rdar://33385423


Repository:
  rC Clang

https://reviews.llvm.org/D44652

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  test/Index/missing_vfs.c
  test/VFS/Inputs/MissingVFS/a.h
  test/VFS/Inputs/MissingVFS/module.modulemap
  test/VFS/Inputs/MissingVFS/vfsoverlay.yaml
  test/VFS/module_missing_vfs.m

Index: test/VFS/module_missing_vfs.m
===
--- /dev/null
+++ test/VFS/module_missing_vfs.m
@@ -0,0 +1,16 @@
+// REQUIRES: shell
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: echo "void funcA(void);" >> %t/a.h
+
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp -I %S/Inputs/MissingVFS %s -fsyntax-only -ivfsoverlay %t/vfs.yaml 2>&1 | FileCheck %s -check-prefix=ERROR
+// ERROR: virtual filesystem overlay file '{{.*}}' not found
+// RUN: find %t/mcp -name "A-*.pcm" | count 1
+
+// RUN: sed -e "s:INPUT_DIR:%S/Inputs:g" -e "s:OUT_DIR:%t:g" %S/Inputs/MissingVFS/vfsoverlay.yaml > %t/vfs.yaml
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp -I %S/Inputs/MissingVFS %s -fsyntax-only -ivfsoverlay %t/vfs.yaml
+// RUN: find %t/mcp -name "A-*.pcm" | count 1
+
+@import A;
+void test(void) {
+  funcA();
+}
Index: test/VFS/Inputs/MissingVFS/vfsoverlay.yaml
===
--- /dev/null
+++ test/VFS/Inputs/MissingVFS/vfsoverlay.yaml
@@ -0,0 +1,13 @@
+{
+  'version': 0,
+  'ignore-non-existent-contents': false,
+  'roots': [
+{ 'name': 'INPUT_DIR', 'type': 'directory',
+  'contents': [
+{ 'name': 'a.h', 'type': 'file',
+  'external-contents': 'OUT_DIR/a.h'
+}
+  ]
+}
+  ]
+}
Index: test/VFS/Inputs/MissingVFS/module.modulemap
===
--- /dev/null
+++ test/VFS/Inputs/MissingVFS/module.modulemap
@@ -0,0 +1,3 @@
+module A {
+  header "a.h"
+}
Index: test/VFS/Inputs/MissingVFS/a.h
===
--- /dev/null
+++ test/VFS/Inputs/MissingVFS/a.h
@@ -0,0 +1 @@
+// void funcA(void);
Index: test/Index/missing_vfs.c
===
--- /dev/null
+++ test/Index/missing_vfs.c
@@ -0,0 +1,6 @@
+// RUN: c-index-test -test-load-source local %s -ivfsoverlay %t/does-not-exist.yaml &> %t.out
+// RUN: FileCheck -check-prefix=STDERR %s < %t.out
+// STDERR: fatal error: virtual filesystem overlay file '{{.*}}' not found
+// RUN: FileCheck %s < %t.out
+// CHECK: missing_vfs.c:[[@LINE+1]]:6: FunctionDecl=foo:[[@LINE+1]]:6
+void foo(void);
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -303,8 +303,6 @@
 
   VFS =
   createVFSFromCompilerInvocation(Clang->getInvocation(), Diagnostics, VFS);
-  if (!VFS)
-return BuildPreambleError::CouldntCreateVFSOverlay;
 
   // Create a file manager object to provide access to and cache the filesystem.
   Clang->setFileManager(new FileManager(Clang->getFileSystemOpts(), VFS));
@@ -756,8 +754,6 @@
 return "Could not create temporary file for PCH";
   case BuildPreambleError::CouldntCreateTargetInfo:
 return "CreateTargetInfo() return null";
-  case BuildPreambleError::CouldntCreateVFSOverlay:
-return "Could not create VFS Overlay";
   case BuildPreambleError::BeginSourceFileFailed:
 return "BeginSourceFile() return an error";
   case BuildPreambleError::CouldntEmitPCH:
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -3072,16 +3072,16 @@
 BaseFS->getBufferForFile(File);
 if (!Buffer) {
   Diags.Report(diag::err_missing_vfs_overlay_file) << File;
-  return IntrusiveRefCntPtr();
+  continue;
 }
 
 IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
 std::move(Buffer.get()), /*DiagHandler*/ nullptr, File);
-if (!FS.get()) {
+if (FS) {
+  Overlay->pushOverlay(FS);
+} else {
   Diags.Report(diag::err_invalid_vfs_overlay) << File;
-  return IntrusiveRefCntPtr();
 }
-Overlay->pushOverlay(FS);
   }
   return Overlay;
 }
Index:

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

@sammccall RE using USRs: we want to integrate clangd into a larger source 
tooling system that is already using USRs extensively to identify symbols. The 
most obvious case is that we have an index outside of clangd that uses USRs 
from clang-the-compiler, so exposing USRs in clangd lets us integrate 
information from both places.  The most critical piece of this is being able to 
ask "what symbol is at this location" and correlate that with index queries. 
Beyond this one case, my experience is that any time a language service API is 
providing information about a symbol or list of symbols it may be useful to add 
the USR (e.g. hover, definition or workspace symbols).

You mentioned a concern that this would make us diverge from LSP - could you 
expand on that?  I think for any existing requests the USR is purely additive.  
If the concern is about affecting the return values of existing queries, would 
it help to put this behind a configuration option?

There is also an extension that might be interesting to look at here: 
https://github.com/sourcegraph/language-server-protocol/blob/master/extension-symbol-descriptor.md
  In this case the symbol descriptor is opaque, but in practice our use case 
would require knowing how to dig out a USR string.

For the specific use case of getting cursor info, as @jkorous mentioned we 
might want to move this out of "definition".  Our experience with sourcekitd 
for Swift is that a cursor info request is a useful place to put an assortment 
of information that may grow over time - e.g. name, type name, USR, type USR, 
symbol kind, local declaration/definition location, etc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status

[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, although I made a small suggestion for clarity.  FYI `InPCH` was used by 
PTH, which was removed a couple of months ago.




Comment at: clang/lib/Basic/FileManager.cpp:305
+UFE = &UniqueRealFiles[Status.getUniqueID()];
+Status = llvm::vfs::Status(
+  Status.getName(), Status.getUniqueID(),

Why copy Status back into Status instead of mutating the relevant fields?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58924



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

[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:305
+UFE = &UniqueRealFiles[Status.getUniqueID()];
+Status = llvm::vfs::Status(
+  Status.getName(), Status.getUniqueID(),

harlanhaskins wrote:
> benlangmuir wrote:
> > Why copy Status back into Status instead of mutating the relevant fields?
> The fields don't have setters exposed, and I couldn't decide if adding the 
> setters vs. re-constructing a Status was better here. Would it be worth 
> adding setters to the properties in `Status`?
Nah, just go with the code you already wrote.  Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58924



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

[PATCH] D60735: [FileSystemStatCache] Return std::error_code from stat cache methods

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

LGTM, but I'd appreciate someone who has worked on this more recently taking a 
look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60735



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

[PATCH] D57965: Clean up ObjCPropertyDecl printing

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added subscribers: akyrtzi, benlangmuir.
benlangmuir added a comment.

@arphaman @akyrtzi could you take a look at this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57965



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

[PATCH] D57965: Clean up ObjCPropertyDecl printing

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

> @property(attr, attr2) instead of @property ( attr,attr2 ).

The style I see most often is `@property (attr, attr2)`, which is in between 
those two.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57965



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In https://reviews.llvm.org/D34512#856301, @xazax.hun wrote:

> In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote:
>
> > In either case, the important scenario I think we should support is 
> > choosing at a call site to a C function the most likely definition of the 
> > called function, based on number and type of parameters, from multiple 
> > possible definitions in other translation units. If the API is rich enough 
> > to support this then I think that is a good indication it will be useful 
> > for other scenarios as well.
>
>
> Note that the lookup is already based on USR which is similar to mangled 
> names in a sense that it contains information about the function parameters. 
> So the only way to get multiple candidates from the lookup is having multiple 
> function definitions with the same signature.


I just want to clarify that C function USRs do not contain type information, 
although C++ USRs do.


https://reviews.llvm.org/D34512



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

[PATCH] D48685: [PCH+Modules] Load -fmodule-map-file content before including PCHs

[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


https://reviews.llvm.org/D48685



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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked 2 inline comments as done.
benlangmuir added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:295
+if (MDC)
+  MDC->applyDiscoveredDependencies(CI);
+LastCC1Arguments = CI.getCC1CommandLine();

jansvoboda11 wrote:
> I'm not a fan of storing `MDC` as a member, but I don't see any clearly 
> better alternatives. One option would be to pass `CompilerInvocation` 
> out-parameter to `ModuleDepCollector`'s constructor and relying on the 
> collector to apply discovered dependencies, but that's not super obvious.
We are applying changes to multiple CompilerInvocations (the scanned one, but 
also any later ones that are downstream of it), so it's less obvious to me how 
to do that here.  We would need to pass in all of the invocations, but 
currently only one of them is in-memory at a time.  I'm open to other ideas 
here, but I haven't come up with any great ideas.  Maybe there's a refactoring 
of MDC that splits the scanning state from the state necessary to apply changes 
to the CI so we would only expose the minimum information needed, but I expect 
that would be an invasive change so prefer not to attempt it in this patch.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[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 rGf80a0ea76072: [clang][deps] Split translation units into 
individual -cc1 or other commands (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-implicit-dot-private.m
  clang/test/ClangScanDeps/modules-incomplete-umbrella.c
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -182,6 +182,11 @@
 llvm::cl::desc("The names of dependency targets for the dependency file"),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt DeprecatedDriverCommand(
+"deprecated-driver-command", llvm::cl::Optional,
+llvm::cl::desc("use a single driver command to build the tu (deprecated)"),
+llvm::cl::cat(DependencyScannerCategory));
+
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
@@ -256,7 +261,7 @@
 public:
   void mergeDeps(StringRef Input, FullDependenciesResult FDR,
  size_t InputIndex) {
-const FullDependencies &FD = FDR.FullDeps;
+FullDependencies &FD = FDR.FullDeps;
 
 InputDeps ID;
 ID.FileName = std::string(Input);
@@ -274,7 +279,8 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = FD.CommandLine;
+ID.DriverCommandLine = std::move(FD.DriverCommandLine);
+ID.Commands = std::move(FD.Commands);
 Inputs.push_back(std::move(ID));
   }
 
@@ -311,14 +317,33 @@
 
 Array TUs;
 for (auto &&I : Inputs) {
-  Object O{
-  {"input-file", I.FileName},
-  {"clang-context-hash", I.ContextHash},
-  {"file-deps", I.FileDeps},
-  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
-  {"command-line", I.CommandLine},
-  };
-  TUs.push_back(std::move(O));
+  Array Commands;
+  if (I.DriverCommandLine.empty()) {
+for (const auto &Cmd : I.Commands) {
+  Object O{
+  {"input-file", I.FileName},
+  {"clang-context-hash", I.ContextHash},
+  {"file-deps", I.FileDeps},
+  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
+  {"executable", Cmd.Executable},
+  {"command-line", Cmd.Ar

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/include/clang/Lex/DependencyDirectivesScanner.h:131
+/// \p dependency_directives_scan::tokens_present_before_eof, otherwise this
+/// directive will be ignored.
 ///

Why would you want to print without this? It seems important for correctness of 
the output.  I would have expected we would always print it.



Comment at: clang/lib/Lex/DependencyDirectivesScanner.cpp:868
+PrintMarkerForTokensBeforeEOF)
+  OS << "";
 Optional PrevTokenKind;

How about "TokBeforeEOF"?  I think "BEOF" is too cryptic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133357

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

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/include/clang/Lex/DependencyDirectivesScanner.h:131
+/// \p dependency_directives_scan::tokens_present_before_eof, otherwise this
+/// directive will be ignored.
 ///

akyrtzi wrote:
> benlangmuir wrote:
> > Why would you want to print without this? It seems important for 
> > correctness of the output.  I would have expected we would always print it.
> The `-print-dependency-directives-minimized-source` clang option is using 
> this function, and if you print the sources with "" at the end then 
> the source text will not be parsable.
> 
> But the tests that pass `-print-dependency-directives-minimized-source` don't 
> try to parse the code, so I think I can switch the default to be `true` for 
> printing the "tokens-before-eof marker" and if a caller has a need to ignore 
> it it can pass `false` for the parameter.
If someone uses `-print-dependency-directives-minimized-source` and creates a 
minimized file for each header, it will "parse", but it won't behave correctly 
for multiple includes, right?  My preference is we don't allow printing this 
without the TokBEOF.  If we care about making it parse, we should add a real 
token of some kind -- maybe there is a no-op `#pragma` or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133357

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

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/include/clang/Lex/DependencyDirectivesScanner.h:131
+/// \p dependency_directives_scan::tokens_present_before_eof, otherwise this
+/// directive will be ignored.
 ///

benlangmuir wrote:
> akyrtzi wrote:
> > benlangmuir wrote:
> > > Why would you want to print without this? It seems important for 
> > > correctness of the output.  I would have expected we would always print 
> > > it.
> > The `-print-dependency-directives-minimized-source` clang option is using 
> > this function, and if you print the sources with "" at the end 
> > then the source text will not be parsable.
> > 
> > But the tests that pass `-print-dependency-directives-minimized-source` 
> > don't try to parse the code, so I think I can switch the default to be 
> > `true` for printing the "tokens-before-eof marker" and if a caller has a 
> > need to ignore it it can pass `false` for the parameter.
> If someone uses `-print-dependency-directives-minimized-source` and creates a 
> minimized file for each header, it will "parse", but it won't behave 
> correctly for multiple includes, right?  My preference is we don't allow 
> printing this without the TokBEOF.  If we care about making it parse, we 
> should add a real token of some kind -- maybe there is a no-op `#pragma` or 
> something?
Oops, missed half my comment. Meant to also add:

> But the tests that pass -print-dependency-directives-minimized-source don't 
> try to parse the code, so I think I can switch the default to be true for 
> printing the "tokens-before-eof marker" and if a caller has a need to ignore 
> it it can pass false for the parameter.

SGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133357

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

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Ben Langmuir via Phabricator via cfe-commits]

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

You forgot to remove the ` \param PrintMarkerForTokensBeforeEOF ...` from the 
doc comment.  Otherwise LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133357

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

[PATCH] D133611: [clang] sort additional module maps when serializing

[Ben Langmuir via Phabricator via cfe-commits]

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

Good catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133611

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

[PATCH] D133617: [Clang][ScanDeps] Change multiple-commands.c test to use -fmodules-cache-path on implicit builds

[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 for catching this!




Comment at: clang/test/ClangScanDeps/multiple-commands.c:11
 // RUN: split-file %s %t
-// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: sed -e "s|DIR|%/t|g" -e "s|CACHE_PATH|%t.modcache|g" %t/cdb.json.in > 
%t/cdb.json
 

Nit: we could just put it inside DIR/modcache and not need a second 
substitution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133617

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

[PATCH] D133622: [clang][test] Disallow using the default module cache path in lit tests

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: bruno, jansvoboda11, arphaman.
Herald added a subscriber: delcypher.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Make the default module cache path invalid when running lit tests so
that tests are forced to provide a cache path. This avoids accidentally
escaping to the system default location, and would have caught the
failure recently found in ClangScanDeps/multiple-commands.c.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133622

Files:
  clang/test/Driver/modules-cache-path.m
  clang/test/Modules/driver.c
  llvm/utils/lit/lit/llvm/config.py


Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -495,6 +495,10 @@
 
 self.clear_environment(possibly_dangerous_env_vars)
 
+# Make the default module cache path invalid so that tests are forced 
to
+# provide a cache path if they use implicit modules.
+self.with_environment('CLANG_MODULE_CACHE_PATH', '/dev/null')
+
 # Tweak the PATH to include the tools dir and the scripts dir.
 # Put Clang first to avoid LLVM from overriding out-of-tree clang
 # builds.
Index: clang/test/Modules/driver.c
===
--- clang/test/Modules/driver.c
+++ clang/test/Modules/driver.c
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck 
-check-prefix CHECK-NO_MODULE_CACHE %s
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -fimplicit-module-maps 
%s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
 // RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=blarg %s 
-### 2>&1 | FileCheck -check-prefix CHECK-WITH_MODULE_CACHE %s
 
 // CHECK-NO_MODULE_CACHE: {{clang.*"-fmodules-cache-path=.*ModuleCache"}}
Index: clang/test/Driver/modules-cache-path.m
===
--- clang/test/Driver/modules-cache-path.m
+++ clang/test/Driver/modules-cache-path.m
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s 
-check-prefix=CHECK-DEFAULT
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -### %s 2>&1 | 
FileCheck %s -check-prefix=CHECK-DEFAULT
 // CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
 
 // RUN: env CLANG_MODULE_CACHE_PATH=/dev/null \


Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -495,6 +495,10 @@
 
 self.clear_environment(possibly_dangerous_env_vars)
 
+# Make the default module cache path invalid so that tests are forced to
+# provide a cache path if they use implicit modules.
+self.with_environment('CLANG_MODULE_CACHE_PATH', '/dev/null')
+
 # Tweak the PATH to include the tools dir and the scripts dir.
 # Put Clang first to avoid LLVM from overriding out-of-tree clang
 # builds.
Index: clang/test/Modules/driver.c
===
--- clang/test/Modules/driver.c
+++ clang/test/Modules/driver.c
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
 // RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=blarg %s -### 2>&1 | FileCheck -check-prefix CHECK-WITH_MODULE_CACHE %s
 
 // CHECK-NO_MODULE_CACHE: {{clang.*"-fmodules-cache-path=.*ModuleCache"}}
Index: clang/test/Driver/modules-cache-path.m
===
--- clang/test/Driver/modules-cache-path.m
+++ clang/test/Driver/modules-cache-path.m
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
 // CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
 
 // RUN: env CLANG_MODULE_CACHE_PATH=/dev/null \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133617: [Clang][ScanDeps] Change multiple-commands.c test to use -fmodules-cache-path on implicit builds

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Attempting to not do this again: https://reviews.llvm.org/D133622


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133617

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

[PATCH] D133622: [clang][test] Disallow using the default module cache path in lit tests

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd96f526196ac: [clang][test] Disallow using the default 
module cache path in lit tests (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133622

Files:
  clang/test/Driver/modules-cache-path.m
  clang/test/Modules/driver.c
  llvm/utils/lit/lit/llvm/config.py


Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -495,6 +495,10 @@
 
 self.clear_environment(possibly_dangerous_env_vars)
 
+# Make the default module cache path invalid so that tests are forced 
to
+# provide a cache path if they use implicit modules.
+self.with_environment('CLANG_MODULE_CACHE_PATH', '/dev/null')
+
 # Tweak the PATH to include the tools dir and the scripts dir.
 # Put Clang first to avoid LLVM from overriding out-of-tree clang
 # builds.
Index: clang/test/Modules/driver.c
===
--- clang/test/Modules/driver.c
+++ clang/test/Modules/driver.c
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck 
-check-prefix CHECK-NO_MODULE_CACHE %s
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -fimplicit-module-maps 
%s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
 // RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=blarg %s 
-### 2>&1 | FileCheck -check-prefix CHECK-WITH_MODULE_CACHE %s
 
 // CHECK-NO_MODULE_CACHE: {{clang.*"-fmodules-cache-path=.*ModuleCache"}}
Index: clang/test/Driver/modules-cache-path.m
===
--- clang/test/Driver/modules-cache-path.m
+++ clang/test/Driver/modules-cache-path.m
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s 
-check-prefix=CHECK-DEFAULT
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -### %s 2>&1 | 
FileCheck %s -check-prefix=CHECK-DEFAULT
 // CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
 
 // RUN: env CLANG_MODULE_CACHE_PATH=/dev/null \


Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -495,6 +495,10 @@
 
 self.clear_environment(possibly_dangerous_env_vars)
 
+# Make the default module cache path invalid so that tests are forced to
+# provide a cache path if they use implicit modules.
+self.with_environment('CLANG_MODULE_CACHE_PATH', '/dev/null')
+
 # Tweak the PATH to include the tools dir and the scripts dir.
 # Put Clang first to avoid LLVM from overriding out-of-tree clang
 # builds.
Index: clang/test/Modules/driver.c
===
--- clang/test/Modules/driver.c
+++ clang/test/Modules/driver.c
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -fimplicit-module-maps %s -### 2>&1 | FileCheck -check-prefix CHECK-NO_MODULE_CACHE %s
 // RUN: %clang -fmodules -fimplicit-module-maps -fmodules-cache-path=blarg %s -### 2>&1 | FileCheck -check-prefix CHECK-WITH_MODULE_CACHE %s
 
 // CHECK-NO_MODULE_CACHE: {{clang.*"-fmodules-cache-path=.*ModuleCache"}}
Index: clang/test/Driver/modules-cache-path.m
===
--- clang/test/Driver/modules-cache-path.m
+++ clang/test/Driver/modules-cache-path.m
@@ -1,4 +1,4 @@
-// RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
+// RUN: env -u CLANG_MODULE_CACHE_PATH %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
 // CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
 
 // RUN: env CLANG_MODULE_CACHE_PATH=/dev/null \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133622: [clang][test] Disallow using the default module cache path in lit tests

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Reverted due to failure on a bot 
https://lab.llvm.org/buildbot/#/builders/214/builds/3252

I'm not sure how to deal with missing `env -u`.

- We could do `env CLANG_MODULE_CACHE_PATH=` and change the compiler's 
interpretation of empty string for this variable. I'm not sure if the current 
behaviour (there will be no module cache in the cc1 at all) is intentional or 
useful.  Hesitant to change this behaviour.
- We could try to enumerate all the environments that don't support `env -u` 
and disable these two tests on  those platforms.  So far it was just one AIX 
bot, but I wouldn't be surprised if other less commonly used unixes have the 
same issue.
- We could make the command inscrutable, like `not env -u X true || env -u ... 
real command ...` so that if `env -u X true` fails (presumably due to not 
supporting `-u` option) we won't run the rest of the RUN line.

If someone has a suggestion for a simple fix, I can try again.  But otherwise I 
doubt it's worth putting much time into this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133622

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

[PATCH] D133622: [clang][test] Disallow using the default module cache path in lit tests

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D133622#3788218 , @bruno wrote:

>> I'm not sure how to deal with missing `env -u`.
>>
>> - We could do `env CLANG_MODULE_CACHE_PATH=` and change the compiler's 
>> interpretation of empty string for this variable. I'm not sure if the 
>> current behaviour (there will be no module cache in the cc1 at all) is 
>> intentional or useful.  Hesitant to change this behaviour.
>
> How about using `self.with_environment('CLANG_MODULE_CACHE_PATH', '')` so we 
> don't need to worry about using `env -u` to unset? My understand is that (1) 
> `getDefaultModuleCachePath` is the only place using 
> `CLANG_MODULE_CACHE_PATH`, and (2) `std::getenv` return nullptr if it's 
> empty, which will fallback to system provided path instead.

Where are you thinking we would call `self.with_environment` in this case? We 
explicitly do not want the system-provided path in most tests.  I think we 
would need to set it to `None`, since

> (2) `std::getenv` return nullptr if it's empty, which will fallback to system 
> provided path instead.

getenv returns empty string, not nullptr.

> Not sure it helps much but according to `option-X.test`, `system-aix` support 
> `unset`.

Heh, I'm worried we'll just hit an issue with a different platform (Windows?).  
If we can't find a better fix I guess I can at least attempt it and see what 
breaks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133622

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

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

This broke some compiler-rt tests on Darwin:
https://green.lab.llvm.org/green/job/clang-stage1-RA/29920/

  Test Result (3 failures / +3)
  Builtins-x86_64-darwin.Builtins-x86_64-darwin.extendhfsf2_test.c
  Builtins-x86_64-darwin.Builtins-x86_64-darwin.truncdfhf2_test.c
  Builtins-x86_64-darwin.Builtins-x86_64-darwin.truncsfhf2_test.c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128571

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

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Reverted in eab2a06f0fde 
 due to 
the Darwin test failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128571

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

[PATCH] D128772: [Lex] Make sure to notify `MultipleIncludeOpt` for "read tokens" during fast dependency directive lexing

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Lex/Lexer.cpp:4251
   DepDirectives.front().Tokens[NextDepDirectiveTokenIndex++];
+  if (NextDepDirectiveTokenIndex > 1 || DDTok.Kind != tok::hash) {
+// Read something other than a preprocessor directive hash.

Why do we need the >1 check? I'm not familiar with the details of MIO here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128772

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

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:1016
+return *Name;
   return StringRef();
 }

Just a suggestion: `value_or` can be a nice way to express simple cases like 
this:

```
getFilename(getFileID(SpellingLoc)).value_or(StringRef());
```



Comment at: clang/lib/Lex/PPLexerChange.cpp:136
PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;

Why does `LexedFileChanged` have `PrevFID` set, but `FileChanged` does not (it 
has a default argument of `FileID()`?  I would have expected that when you call 
both `FileChanged` and `LexedFileChanged` for the same event this would match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128947

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

[PATCH] D129220: [clang] Cleanup ASTContext before output files in crash recovery for modules

[Ben Langmuir via Phabricator via cfe-commits]

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

When we recover from a crash in a module compilation thread, we need to ensure 
any output streams owned by the `ASTConsumer` (e.g. in 
`RawPCHContainerGenerator`) are deleted before we call `clearOutputFiles()`.  
This has the same theoretical issues with proxy streams that Duncan discusses 
in the commit 2d133867833fe8eb 
.  In 
practice, this was observed as a use-after-free crash on a downstream branch 
that uses such a proxy stream in this code path.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129220

Files:
  clang/lib/Frontend/CompilerInstance.cpp


Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1235,8 +1235,7 @@
 
   // Execute the action to actually build the module in-place. Use a separate
   // thread so that we get a stack large enough.
-  llvm::CrashRecoveryContext CRC;
-  CRC.RunSafelyOnThread(
+  bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
   [&]() {
 GenerateModuleFromModuleMapAction Action;
 Instance.ExecuteAction(Action);
@@ -1249,6 +1248,13 @@
 diag::remark_module_build_done)
 << ModuleName;
 
+  // Clear the ASTConsumer if it hasn't been already, in case it owns streams
+  // that must be closed before clearing output files.
+  if (Crashed) {
+Instance.setSema(nullptr);
+Instance.setASTConsumer(nullptr);
+  }
+
   // Delete any remaining temporary files related to Instance, in case the
   // module generation thread crashed.
   Instance.clearOutputFiles(/*EraseFiles=*/true);


Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1235,8 +1235,7 @@
 
   // Execute the action to actually build the module in-place. Use a separate
   // thread so that we get a stack large enough.
-  llvm::CrashRecoveryContext CRC;
-  CRC.RunSafelyOnThread(
+  bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
   [&]() {
 GenerateModuleFromModuleMapAction Action;
 Instance.ExecuteAction(Action);
@@ -1249,6 +1248,13 @@
 diag::remark_module_build_done)
 << ModuleName;
 
+  // Clear the ASTConsumer if it hasn't been already, in case it owns streams
+  // that must be closed before clearing output files.
+  if (Crashed) {
+Instance.setSema(nullptr);
+Instance.setASTConsumer(nullptr);
+  }
+
   // Delete any remaining temporary files related to Instance, in case the
   // module generation thread crashed.
   Instance.clearOutputFiles(/*EraseFiles=*/true);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129220: [clang] Cleanup ASTContext before output files in crash recovery for modules

[Ben Langmuir via Phabricator via cfe-commits]

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

Updated per out-of-band feedback from @steven_wu :

- Added an assert to `clearOutputFiles` that the `ASTConsumer` is removed. 
Normally this would be done in `FrontendAction::EndSourceFile`.  This should 
help avoid regressions and also means it is covered by existing tests.
- Updated `FailureCleanup` in `FrontendAction::BeginSourceFile` to reset the 
`ASTConsumer`. This is needed due to the assertion, but I don't think it 
changes much in practice since there would also be no output files at this 
stage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129220

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp


Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -581,6 +581,7 @@
   auto FailureCleanup = llvm::make_scope_exit([&]() {
 if (HasBegunSourceFile)
   CI.getDiagnosticClient().EndSourceFile();
+CI.setASTConsumer(nullptr);
 CI.clearOutputFiles(/*EraseFiles=*/true);
 CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
 setCurrentInput(FrontendInputFile());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -757,6 +757,8 @@
 // Output Files
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
+  // The ASTConsumer can own streams that write to the output files.
+  assert(!hasASTConsumer() && "ASTConsumer should be reset");
   // Ignore errors that occur when trying to discard the temp file.
   for (OutputFile &OF : OutputFiles) {
 if (EraseFiles) {
@@ -1235,8 +1237,7 @@
 
   // Execute the action to actually build the module in-place. Use a separate
   // thread so that we get a stack large enough.
-  llvm::CrashRecoveryContext CRC;
-  CRC.RunSafelyOnThread(
+  bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
   [&]() {
 GenerateModuleFromModuleMapAction Action;
 Instance.ExecuteAction(Action);
@@ -1249,9 +1250,15 @@
 diag::remark_module_build_done)
 << ModuleName;
 
-  // Delete any remaining temporary files related to Instance, in case the
-  // module generation thread crashed.
-  Instance.clearOutputFiles(/*EraseFiles=*/true);
+  if (Crashed) {
+// Clear the ASTConsumer if it hasn't been already, in case it owns streams
+// that must be closed before clearing output files.
+Instance.setSema(nullptr);
+Instance.setASTConsumer(nullptr);
+
+// Delete any remaining temporary files related to Instance.
+Instance.clearOutputFiles(/*EraseFiles=*/true);
+  }
 
   // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
   // occurred.


Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -581,6 +581,7 @@
   auto FailureCleanup = llvm::make_scope_exit([&]() {
 if (HasBegunSourceFile)
   CI.getDiagnosticClient().EndSourceFile();
+CI.setASTConsumer(nullptr);
 CI.clearOutputFiles(/*EraseFiles=*/true);
 CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
 setCurrentInput(FrontendInputFile());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -757,6 +757,8 @@
 // Output Files
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
+  // The ASTConsumer can own streams that write to the output files.
+  assert(!hasASTConsumer() && "ASTConsumer should be reset");
   // Ignore errors that occur when trying to discard the temp file.
   for (OutputFile &OF : OutputFiles) {
 if (EraseFiles) {
@@ -1235,8 +1237,7 @@
 
   // Execute the action to actually build the module in-place. Use a separate
   // thread so that we get a stack large enough.
-  llvm::CrashRecoveryContext CRC;
-  CRC.RunSafelyOnThread(
+  bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
   [&]() {
 GenerateModuleFromModuleMapAction Action;
 Instance.ExecuteAction(Action);
@@ -1249,9 +1250,15 @@
 diag::remark_module_build_done)
 << ModuleName;
 
-  // Delete any remaining temporary files related to Instance, in case the
-  // module generation thread crashed.
-  Instance.clearOutputFiles(/*EraseFiles=*/true);
+  if (Crashed) {
+// Clear the ASTConsumer if it hasn't been already, in case it owns streams
+// that must be closed before clearing output files.
+

[PATCH] D129220: [clang] Cleanup ASTContext before output files in crash recovery for modules

[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 rG67a84ec8105e: [clang] Cleanup ASTContext before output files 
in crash recovery for modules (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129220

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp


Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -581,6 +581,7 @@
   auto FailureCleanup = llvm::make_scope_exit([&]() {
 if (HasBegunSourceFile)
   CI.getDiagnosticClient().EndSourceFile();
+CI.setASTConsumer(nullptr);
 CI.clearOutputFiles(/*EraseFiles=*/true);
 CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
 setCurrentInput(FrontendInputFile());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -757,6 +757,8 @@
 // Output Files
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
+  // The ASTConsumer can own streams that write to the output files.
+  assert(!hasASTConsumer() && "ASTConsumer should be reset");
   // Ignore errors that occur when trying to discard the temp file.
   for (OutputFile &OF : OutputFiles) {
 if (EraseFiles) {
@@ -1235,8 +1237,7 @@
 
   // Execute the action to actually build the module in-place. Use a separate
   // thread so that we get a stack large enough.
-  llvm::CrashRecoveryContext CRC;
-  CRC.RunSafelyOnThread(
+  bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
   [&]() {
 GenerateModuleFromModuleMapAction Action;
 Instance.ExecuteAction(Action);
@@ -1249,9 +1250,15 @@
 diag::remark_module_build_done)
 << ModuleName;
 
-  // Delete any remaining temporary files related to Instance, in case the
-  // module generation thread crashed.
-  Instance.clearOutputFiles(/*EraseFiles=*/true);
+  if (Crashed) {
+// Clear the ASTConsumer if it hasn't been already, in case it owns streams
+// that must be closed before clearing output files.
+Instance.setSema(nullptr);
+Instance.setASTConsumer(nullptr);
+
+// Delete any remaining temporary files related to Instance.
+Instance.clearOutputFiles(/*EraseFiles=*/true);
+  }
 
   // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
   // occurred.


Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -581,6 +581,7 @@
   auto FailureCleanup = llvm::make_scope_exit([&]() {
 if (HasBegunSourceFile)
   CI.getDiagnosticClient().EndSourceFile();
+CI.setASTConsumer(nullptr);
 CI.clearOutputFiles(/*EraseFiles=*/true);
 CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
 setCurrentInput(FrontendInputFile());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -757,6 +757,8 @@
 // Output Files
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
+  // The ASTConsumer can own streams that write to the output files.
+  assert(!hasASTConsumer() && "ASTConsumer should be reset");
   // Ignore errors that occur when trying to discard the temp file.
   for (OutputFile &OF : OutputFiles) {
 if (EraseFiles) {
@@ -1235,8 +1237,7 @@
 
   // Execute the action to actually build the module in-place. Use a separate
   // thread so that we get a stack large enough.
-  llvm::CrashRecoveryContext CRC;
-  CRC.RunSafelyOnThread(
+  bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
   [&]() {
 GenerateModuleFromModuleMapAction Action;
 Instance.ExecuteAction(Action);
@@ -1249,9 +1250,15 @@
 diag::remark_module_build_done)
 << ModuleName;
 
-  // Delete any remaining temporary files related to Instance, in case the
-  // module generation thread crashed.
-  Instance.clearOutputFiles(/*EraseFiles=*/true);
+  if (Crashed) {
+// Clear the ASTConsumer if it hasn't been already, in case it owns streams
+// that must be closed before clearing output files.
+Instance.setSema(nullptr);
+Instance.setASTConsumer(nullptr);
+
+// Delete any remaining temporary files related to Instance.
+Instance.clearOutputFiles(/*EraseFiles=*/true);
+  }
 
   // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
   // occurred.
_

[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[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 a project: clang.
Herald added a subscriber: cfe-commits.

When building modules, override secondary outputs (dependency file, dependency 
targets, serialized diagnostic file) in addition to the pcm file path. This 
avoids inheriting per-TU command-line options that cause non-determinism in the 
results (non-deterministic command-line for the module build, non-determinism 
in which TU's .diag and .d files will contain the module outputs). In 
clang-scan-deps we infer whether to generate dependency or serialized 
diagnostic files based on an original command-line. In a real build system this 
should be modeled explicitly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129389

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
  clang/test/ClangScanDeps/generate-modules-path-args.c
  clang/test/ClangScanDeps/preserved-args.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -288,11 +288,16 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = GenerateModulesPathArgs
- ? FD.getCommandLine(
-   [&](ModuleID MID) { return lookupPCMPath(MID); })
- : FD.getCommandLineWithoutModulePaths();
-
+if (Inputs.size() == 0)
+  inferOutputOptions(FD.OriginalCommandLine);
+
+ID.CommandLine =
+GenerateModulesPathArgs
+? FD.getCommandLine(
+  [&](ModuleID MID) -> const ModuleOutputOptions & {
+return lookupModuleOutputs(MID);
+  })
+: FD.getCommandLineWithoutModulePaths();
 Inputs.push_back(std::move(ID));
   }
 
@@ -325,7 +330,9 @@
   {"command-line",
GenerateModulesPathArgs
? MD.getCanonicalCommandLine(
- [&](ModuleID MID) { return lookupPCMPath(MID); })
+ [&](ModuleID MID) -> const ModuleOutputOptions & {
+   return lookupModuleOutputs(MID);
+ })
: MD.getCanonicalCommandLineWithoutModulePaths()},
   };
   OutModules.push_back(std::move(O));
@@ -352,11 +359,17 @@
   }
 
 private:
-  StringRef lookupPCMPath(ModuleID MID) {
-auto PCMPath = PCMPaths.insert({MID, ""});
-if (PCMPath.second)
-  PCMPath.first->second = constructPCMPath(MID);
-return PCMPath.first->second;
+  const ModuleOutputOptions &lookupModuleOutputs(const ModuleID &MID) {
+auto MO = ModuleOutputs.insert({MID, {}});
+if (MO.second) {
+  ModuleOutputOptions &Opts = MO.first->second;
+  Opts.ModuleFile = constructPCMPath(MID);
+  if (DependencyOutputFile)
+Opts.DependencyFile = Opts.ModuleFile + ".d";
+  if (SerializeDiags)
+Opts.DiagnosticSerializationFile = Opts.ModuleFile + ".diag";
+}
+return MO.first->second;
   }
 
   /// Construct a path for the explicitly built PCM.
@@ -375,6 +388,19 @@
 return std::string(ExplicitPCMPath);
   }
 
+  /// Infer whether modules should write serialized diagnostic, .d, etc.
+  ///
+  /// A build system should model this directly, but here we infer it from an
+  /// original TU command.
+  void inferOutputOptions(ArrayRef Args) {
+for (StringRef Arg : Args) {
+  if (Arg == "-serialize-diagnostics")
+SerializeDiags = true;
+  else if (Arg == "-M" || Arg == "-MM" || Arg == "-MMD" || Arg == "-MD")
+DependencyOutputFile = true;
+}
+  }
+
   struct IndexedModuleID {
 ModuleID ID;
 mutable size_t InputIndex;
@@ -404,8 +430,11 @@
   std::mutex Lock;
   std::unordered_map
   Modules;
-  std::unordered_map PCMPaths;
+  std::unordered_map
+  ModuleOutputs;
   std::vector Inputs;
+  bool SerializeDiags = false;
+  bool DependencyOutputFile = false;
 };
 
 static bool handleFullDependencyToolResult(
Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -29,6 +29,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
+// CHECK-NOT:  "-dependency-file"
+// CHECK-NOT: 

[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[Ben Langmuir via Phabricator via cfe-commits]

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

Updates:

- Made lookup of module outputs fallible. Not currently used by 
clang-scan-deps, but since the expectation is for a build system to provide 
these settings account for possibility of errors.
- Attempt to fix windows path issue in test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129389

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
  clang/test/ClangScanDeps/generate-modules-path-args.c
  clang/test/ClangScanDeps/preserved-args.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -288,11 +288,16 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = GenerateModulesPathArgs
- ? FD.getCommandLine(
-   [&](ModuleID MID) { return lookupPCMPath(MID); })
- : FD.getCommandLineWithoutModulePaths();
-
+if (Inputs.size() == 0)
+  inferOutputOptions(FD.OriginalCommandLine);
+
+ID.CommandLine =
+GenerateModulesPathArgs
+? llvm::cantFail(FD.getCommandLine(
+  [&](ModuleID MID) -> const ModuleOutputOptions & {
+return lookupModuleOutputs(MID);
+  }))
+: FD.getCommandLineWithoutModulePaths();
 Inputs.push_back(std::move(ID));
   }
 
@@ -324,8 +329,10 @@
   {"clang-modulemap-file", MD.ClangModuleMapFile},
   {"command-line",
GenerateModulesPathArgs
-   ? MD.getCanonicalCommandLine(
- [&](ModuleID MID) { return lookupPCMPath(MID); })
+   ? llvm::cantFail(MD.getCanonicalCommandLine(
+ [&](ModuleID MID) -> const ModuleOutputOptions & {
+   return lookupModuleOutputs(MID);
+ }))
: MD.getCanonicalCommandLineWithoutModulePaths()},
   };
   OutModules.push_back(std::move(O));
@@ -352,11 +359,17 @@
   }
 
 private:
-  StringRef lookupPCMPath(ModuleID MID) {
-auto PCMPath = PCMPaths.insert({MID, ""});
-if (PCMPath.second)
-  PCMPath.first->second = constructPCMPath(MID);
-return PCMPath.first->second;
+  const ModuleOutputOptions &lookupModuleOutputs(const ModuleID &MID) {
+auto MO = ModuleOutputs.insert({MID, {}});
+if (MO.second) {
+  ModuleOutputOptions &Opts = MO.first->second;
+  Opts.ModuleFile = constructPCMPath(MID);
+  if (DependencyOutputFile)
+Opts.DependencyFile = Opts.ModuleFile + ".d";
+  if (SerializeDiags)
+Opts.DiagnosticSerializationFile = Opts.ModuleFile + ".diag";
+}
+return MO.first->second;
   }
 
   /// Construct a path for the explicitly built PCM.
@@ -375,6 +388,19 @@
 return std::string(ExplicitPCMPath);
   }
 
+  /// Infer whether modules should write serialized diagnostic, .d, etc.
+  ///
+  /// A build system should model this directly, but here we infer it from an
+  /// original TU command.
+  void inferOutputOptions(ArrayRef Args) {
+for (StringRef Arg : Args) {
+  if (Arg == "-serialize-diagnostics")
+SerializeDiags = true;
+  else if (Arg == "-M" || Arg == "-MM" || Arg == "-MMD" || Arg == "-MD")
+DependencyOutputFile = true;
+}
+  }
+
   struct IndexedModuleID {
 ModuleID ID;
 mutable size_t InputIndex;
@@ -404,8 +430,11 @@
   std::mutex Lock;
   std::unordered_map
   Modules;
-  std::unordered_map PCMPaths;
+  std::unordered_map
+  ModuleOutputs;
   std::vector Inputs;
+  bool SerializeDiags = false;
+  bool DependencyOutputFile = false;
 };
 
 static bool handleFullDependencyToolResult(
Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -29,6 +29,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
+// CHECK-NOT:  "-dependency-file"
+// CHECK-NOT:  "-MT"
+// CHECK-NOT:  "-serialize-diagnostic-file"
 // CHECK:],
 // CHECK-NEXT:   "context-hash": "[[HASH_MOD_HEADER:.*]]",
 // CHECK-NEXT:   "file-deps": [
@@ -50,6 +53,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT: 

[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added inline comments.



Comment at: clang/include/clang/Lex/HeaderSearch.h:758
+ bool IsSystemHeaderDir,
+ StringRef FileName = "");
 

This parameter could use a comment, even if it just points you to read the 
FIXME in the implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123104

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

[PATCH] D129884: [clang][deps] Include canonical invocation in ContextHash

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D129884#3677435 , @jansvoboda11 
wrote:

> Would it make sense for this to replace the existing strict context hash 
> implementation?

It's not clear to me whether this would be a good tradeoff or not: the explicit 
build canonicalizes its invocation, but the implicit build does not do so to 
the same extent. Some of that could be improved with some effort, but for 
example the optimizations we use for search path pruning in the explicit build 
cannot be done up-front in the implicit build, since we don't yet know which 
paths are relevant.  I think we should consider this separately.

One necessary difference is that after scanning dependencies we want to hash 
the module dependencies (name + context hash), but we cannot do that in the 
implicit build since the dependencies are not discovered yet. The rest of the 
invocation hashing implementation could be shared though, if we decided it is 
the right approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129884

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

[PATCH] D129884: [clang][deps] Include canonical invocation in ContextHash

[Ben Langmuir via Phabricator via cfe-commits]

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

- Added comments to tests
- Added a missing test that I accidentally deleted
- Rebased on latest main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129884

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-module-map-path.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c

Index: clang/test/ClangScanDeps/modules-context-hash-warnings.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-context-hash-warnings.c
@@ -0,0 +1,77 @@
+// Differences in -W warning options should result in different canonical module
+// build commands.
+
+// 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 -j 1 \
+// RUN:   -format experimental-full > %t/deps.json
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK:  {
+// CHECK:"command-line": [
+// CHECK:  "-Wall"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH1:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NOT:  "-Wall"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH2:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH1]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK:  "-Wall"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu1.c"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH2]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK-NOT:  "-Wall"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu2.c"
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -Wall -fsyntax-only DIR/tu1.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+"file": "DIR/tu1.c"
+  },
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/tu2.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+"file": "DIR/tu2.c"
+  },
+]
+
+//--- module.modulemap
+module Mod { header "Mod.h" }
+
+//--- Mod.h
+
+//--- tu1.c
+#include "Mod.h"
+
+//--- tu2.c
+#include "Mod.h"
Index: clang/test/ClangScanDeps/modules-context-hash-outputs.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-context-hash-outputs.c
@@ -0,0 +1,77 @@
+// If secondary output files such as .d are enabled, ensure it affects the
+// module context hash since it may impact the resulting module build commands.
+
+// 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 -j 1 -generate-modules-path-args \
+// RUN:   -format experimental-full > %t/deps.json
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK:  {
+// CHECK:"command-line": [
+// CHECK:  "-dependency-file"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH1:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NOT:  "-dependency-file"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH2:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH1]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK:  "-MF"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu1.c"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Is the functionality provided by `ORIGINAL_PCH_DIR` still useful for making it 
possible to move a PCH and its referenced headers?  It's not completely clear 
to me when this feature is used in practice.  It would be nice to remove it or 
change the default behaviour if possible, rather than require a new option, but 
I'm open to this approach if we think we can't change the default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130710

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

[PATCH] D129884: [clang][deps] Include canonical invocation in ContextHash

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG028717014002: [clang][deps] Include canonical invocation in 
ContextHash (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129884

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-module-map-path.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c

Index: clang/test/ClangScanDeps/modules-context-hash-warnings.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-context-hash-warnings.c
@@ -0,0 +1,77 @@
+// Differences in -W warning options should result in different canonical module
+// build commands.
+
+// 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 -j 1 \
+// RUN:   -format experimental-full > %t/deps.json
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK:  {
+// CHECK:"command-line": [
+// CHECK:  "-Wall"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH1:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NOT:  "-Wall"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH2:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH1]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK:  "-Wall"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu1.c"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH2]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK-NOT:  "-Wall"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu2.c"
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -Wall -fsyntax-only DIR/tu1.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+"file": "DIR/tu1.c"
+  },
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/tu2.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+"file": "DIR/tu2.c"
+  },
+]
+
+//--- module.modulemap
+module Mod { header "Mod.h" }
+
+//--- Mod.h
+
+//--- tu1.c
+#include "Mod.h"
+
+//--- tu2.c
+#include "Mod.h"
Index: clang/test/ClangScanDeps/modules-context-hash-outputs.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-context-hash-outputs.c
@@ -0,0 +1,77 @@
+// If secondary output files such as .d are enabled, ensure it affects the
+// module context hash since it may impact the resulting module build commands.
+
+// 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 -j 1 -generate-modules-path-args \
+// RUN:   -format experimental-full > %t/deps.json
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK:  {
+// CHECK:"command-line": [
+// CHECK:  "-dependency-file"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH1:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NOT:  "-dependency-file"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH2:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH1]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK:  "-MF"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu1.c"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:   

[PATCH] D130934: [clang] Update code that assumes FileEntry::getName is absolute NFC

[Ben Langmuir via Phabricator via cfe-commits]

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

It's an accident that we started return asbolute paths from FileEntry::getName 
for all relative paths. Prepare for getName to get (closer to) return the 
requested path. Note: conceptually it might make sense for the dependency 
scanner to allow relative paths and have the DependencyConsumer decide if it 
wants to make them absolute, but we currently document that it's absolute and I 
didn't want to change behaviour here.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130934

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  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
@@ -240,8 +240,7 @@
   // We do not want #line markers to affect dependency generation!
   if (Optional Filename =
   SM.getNonBuiltinFilenameForID(SM.getFileID(SM.getExpansionLoc(Loc
-MDC.FileDeps.push_back(
-std::string(llvm::sys::path::remove_leading_dotslash(*Filename)));
+MDC.addFileDep(llvm::sys::path::remove_leading_dotslash(*Filename));
 }
 
 void ModuleDepCollectorPP::InclusionDirective(
@@ -252,7 +251,7 @@
   if (!File && !Imported) {
 // This is a non-modular include that HeaderSearch failed to find. Add it
 // here as `FileChanged` will never see it.
-MDC.FileDeps.push_back(std::string(FileName));
+MDC.addFileDep(FileName);
   }
   handleImport(Imported);
 }
@@ -282,8 +281,7 @@
  ->getName());
 
   if (!MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude.empty())
-MDC.FileDeps.push_back(
-MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude);
+MDC.addFileDep(MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude);
 
   for (const Module *M : DirectModularDeps) {
 // A top-level module might not be actually imported as a module when
@@ -346,10 +344,10 @@
 // handle it like normal. With explicitly built modules we don't need
 // to play VFS tricks, so replace it with the correct module map.
 if (IF.getFile()->getName().endswith("__inferred_module.map")) {
-  MD.FileDeps.insert(ModuleMap->getName());
+  MDC.addFileDep(MD, ModuleMap->getName());
   return;
 }
-MD.FileDeps.insert(IF.getFile()->getName());
+MDC.addFileDep(MD, IF.getFile()->getName());
   });
 
   // We usually don't need to list the module map files of our dependencies when
@@ -494,3 +492,24 @@
  PrebuiltModuleFileIt->second == M->getASTFile()->getName());
   return true;
 }
+
+static StringRef makeAbsolute(CompilerInstance &CI, StringRef Path,
+  SmallVectorImpl &Storage) {
+  if (llvm::sys::path::is_absolute(Path))
+return Path;
+  Storage.assign(Path.begin(), Path.end());
+  CI.getFileManager().makeAbsolutePath(Storage);
+  return StringRef(Storage.data(), Storage.size());
+}
+
+void ModuleDepCollector::addFileDep(StringRef Path) {
+  llvm::SmallString<256> Storage;
+  Path = makeAbsolute(ScanInstance, Path, Storage);
+  FileDeps.push_back(std::string(Path));
+}
+
+void ModuleDepCollector::addFileDep(ModuleDeps &MD, StringRef Path) {
+  llvm::SmallString<256> Storage;
+  Path = makeAbsolute(ScanInstance, Path, Storage);
+  MD.FileDeps.insert(Path);
+}
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -28,8 +28,9 @@
 class DependencyConsumerForwarder : public DependencyFileGenerator {
 public:
   DependencyConsumerForwarder(std::unique_ptr Opts,
-  DependencyConsumer &C)
-  : DependencyFileGenerator(*Opts), Opts(std::move(Opts)), C(C) {}
+  StringRef WorkingDirectory, DependencyConsumer &C)
+  : DependencyFileGenerator(*Opts), WorkingDirectory(WorkingDirectory),
+Opts(std::move(Opts)), C(C) {}
 
   void finishedMainFile(DiagnosticsEngine &Diags) override {
 C.handleDependencyOutputOpts(*Opts);
@@ -37,11 +38,13 @@
 for (const auto &File : getDependencies()) {
   CanonPath = File;
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
+  llvm::sys::fs::make_absolute(WorkingDirectory, CanonPath);
  

[PATCH] D130935: [clang] Only modify FileEntryRef names that are externally remapped

[Ben Langmuir via Phabricator via cfe-commits]

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

As progress towards having FileEntryRef contain the requested name of the file, 
this commit narrows the "remap" hack to only apply to paths that were remapped 
to an external contents path by a VFS. That was always the original intent of 
this code, and the fact it was making relative paths absolute was an unintended 
side effect.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130935

Files:
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileManagerTest.cpp


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -44,16 +44,16 @@
   }
 }
 
-if (!StatPath)
-  StatPath = Path;
-
 auto fileType = IsFile ?
   llvm::sys::fs::file_type::regular_file :
   llvm::sys::fs::file_type::directory_file;
-llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+llvm::vfs::Status Status(StatPath ? StatPath : Path,
+ llvm::sys::fs::UniqueID(1, INode),
  /*MTime*/{}, /*User*/0, /*Group*/0,
  /*Size*/0, fileType,
  llvm::sys::fs::perms::all_all);
+if (StatPath)
+  Status.ExposesExternalVFSPath = true;
 StatCalls[Path] = Status;
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -274,8 +274,8 @@
   if (!UFE)
 UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (Status.getName() == Filename || !Status.ExposesExternalVFSPath) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
 // Name mismatch. We need a redirect. First grab the actual entry we want
@@ -292,19 +292,7 @@
 // filesystems behave and confuses parts of clang expect to see the
 // name-as-accessed on the \a FileEntryRef.
 //
-// Further, it isn't *just* external names, but will also give back 
absolute
-// paths when a relative path was requested - the check is comparing the
-// name from the status, which is passed an absolute path resolved from the
-// current working directory. `clang-apply-replacements` appears to depend
-// on this behaviour, though it's adjusting the working directory, which is
-// definitely not supported. Once that's fixed this hack should be able to
-// be narrowed to only when there's an externally mapped name given back.
-//
 // A potential plan to remove this is as follows -
-//   - Add API to determine if the name has been rewritten by the VFS.
-//   - Fix `clang-apply-replacements` to pass down the absolute path rather
-// than changing the CWD. Narrow this hack down to just externally
-// mapped paths.
 //   - Expose the requested filename. One possibility would be to allow
 // redirection-FileEntryRefs to be returned, rather than returning
 // the pointed-at-FileEntryRef, and customizing `getName()` to look


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -44,16 +44,16 @@
   }
 }
 
-if (!StatPath)
-  StatPath = Path;
-
 auto fileType = IsFile ?
   llvm::sys::fs::file_type::regular_file :
   llvm::sys::fs::file_type::directory_file;
-llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+llvm::vfs::Status Status(StatPath ? StatPath : Path,
+ llvm::sys::fs::UniqueID(1, INode),
  /*MTime*/{}, /*User*/0, /*Group*/0,
  /*Size*/0, fileType,
  llvm::sys::fs::perms::all_all);
+if (StatPath)
+  Status.ExposesExternalVFSPath = true;
 StatCalls[Path] = Status;
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -274,8 +274,8 @@
   if (!UFE)
 UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (Status.getName() == Filename || !Status.ExposesExternalVFSPath) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
 // Name mis

[PATCH] D130935: [clang] Only modify FileEntryRef names that are externally remapped

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:277-278
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (Status.getName() == Filename || !Status.ExposesExternalVFSPath) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);

bnbarham wrote:
> Is it possible for an `ExposesExternalVFSPath` to have the same filename as 
> the one that was requested? Just in the case of mapping `A -> A` or something 
> equally pointless? Could check for that in the VFS, but probably doesn't 
> matter in the long run.
I think in practice it won't happen, but in theory the VFS can do whatever it 
wants, so I didn't want to introduce any possibility of circularity here.  I 
will flip these checks so the boolean comes before the string compare though, 
since that will be cheaper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130935

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

[PATCH] D130935: [clang] Only modify FileEntryRef names that are externally remapped

[Ben Langmuir via Phabricator via cfe-commits]

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

Flip the order of comparisons to avoid unnecessary string comparisons.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130935

Files:
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileManagerTest.cpp


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -44,16 +44,16 @@
   }
 }
 
-if (!StatPath)
-  StatPath = Path;
-
 auto fileType = IsFile ?
   llvm::sys::fs::file_type::regular_file :
   llvm::sys::fs::file_type::directory_file;
-llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+llvm::vfs::Status Status(StatPath ? StatPath : Path,
+ llvm::sys::fs::UniqueID(1, INode),
  /*MTime*/{}, /*User*/0, /*Group*/0,
  /*Size*/0, fileType,
  llvm::sys::fs::perms::all_all);
+if (StatPath)
+  Status.ExposesExternalVFSPath = true;
 StatCalls[Path] = Status;
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -274,8 +274,8 @@
   if (!UFE)
 UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
 // Name mismatch. We need a redirect. First grab the actual entry we want
@@ -292,19 +292,7 @@
 // filesystems behave and confuses parts of clang expect to see the
 // name-as-accessed on the \a FileEntryRef.
 //
-// Further, it isn't *just* external names, but will also give back 
absolute
-// paths when a relative path was requested - the check is comparing the
-// name from the status, which is passed an absolute path resolved from the
-// current working directory. `clang-apply-replacements` appears to depend
-// on this behaviour, though it's adjusting the working directory, which is
-// definitely not supported. Once that's fixed this hack should be able to
-// be narrowed to only when there's an externally mapped name given back.
-//
 // A potential plan to remove this is as follows -
-//   - Add API to determine if the name has been rewritten by the VFS.
-//   - Fix `clang-apply-replacements` to pass down the absolute path rather
-// than changing the CWD. Narrow this hack down to just externally
-// mapped paths.
 //   - Expose the requested filename. One possibility would be to allow
 // redirection-FileEntryRefs to be returned, rather than returning
 // the pointed-at-FileEntryRef, and customizing `getName()` to look


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -44,16 +44,16 @@
   }
 }
 
-if (!StatPath)
-  StatPath = Path;
-
 auto fileType = IsFile ?
   llvm::sys::fs::file_type::regular_file :
   llvm::sys::fs::file_type::directory_file;
-llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+llvm::vfs::Status Status(StatPath ? StatPath : Path,
+ llvm::sys::fs::UniqueID(1, INode),
  /*MTime*/{}, /*User*/0, /*Group*/0,
  /*Size*/0, fileType,
  llvm::sys::fs::perms::all_all);
+if (StatPath)
+  Status.ExposesExternalVFSPath = true;
 StatCalls[Path] = Status;
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -274,8 +274,8 @@
   if (!UFE)
 UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
 // Name mismatch. We need a redirect. First grab the actual entry we want
@@ -292,19 +292,7 @@
 // filesystems behave and confuses parts of clang expect to see the
 // name-as-accessed on the \a FileEntryRef.
 //
-// Further, it isn't *just* external names, but will also give back absolute
-// paths when a relative path was requested - the c

[PATCH] D130934: [clang] Update code that assumes FileEntry::getName is absolute NFC

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb4c6dc2e6637: [clang] Update code that assumes 
FileEntry::getName is absolute NFC (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130934

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  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
@@ -240,8 +240,7 @@
   // We do not want #line markers to affect dependency generation!
   if (Optional Filename =
   SM.getNonBuiltinFilenameForID(SM.getFileID(SM.getExpansionLoc(Loc
-MDC.FileDeps.push_back(
-std::string(llvm::sys::path::remove_leading_dotslash(*Filename)));
+MDC.addFileDep(llvm::sys::path::remove_leading_dotslash(*Filename));
 }
 
 void ModuleDepCollectorPP::InclusionDirective(
@@ -252,7 +251,7 @@
   if (!File && !Imported) {
 // This is a non-modular include that HeaderSearch failed to find. Add it
 // here as `FileChanged` will never see it.
-MDC.FileDeps.push_back(std::string(FileName));
+MDC.addFileDep(FileName);
   }
   handleImport(Imported);
 }
@@ -282,8 +281,7 @@
  ->getName());
 
   if (!MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude.empty())
-MDC.FileDeps.push_back(
-MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude);
+MDC.addFileDep(MDC.ScanInstance.getPreprocessorOpts().ImplicitPCHInclude);
 
   for (const Module *M : DirectModularDeps) {
 // A top-level module might not be actually imported as a module when
@@ -346,10 +344,10 @@
 // handle it like normal. With explicitly built modules we don't need
 // to play VFS tricks, so replace it with the correct module map.
 if (IF.getFile()->getName().endswith("__inferred_module.map")) {
-  MD.FileDeps.insert(ModuleMap->getName());
+  MDC.addFileDep(MD, ModuleMap->getName());
   return;
 }
-MD.FileDeps.insert(IF.getFile()->getName());
+MDC.addFileDep(MD, IF.getFile()->getName());
   });
 
   // We usually don't need to list the module map files of our dependencies when
@@ -494,3 +492,24 @@
  PrebuiltModuleFileIt->second == M->getASTFile()->getName());
   return true;
 }
+
+static StringRef makeAbsolute(CompilerInstance &CI, StringRef Path,
+  SmallVectorImpl &Storage) {
+  if (llvm::sys::path::is_absolute(Path))
+return Path;
+  Storage.assign(Path.begin(), Path.end());
+  CI.getFileManager().makeAbsolutePath(Storage);
+  return StringRef(Storage.data(), Storage.size());
+}
+
+void ModuleDepCollector::addFileDep(StringRef Path) {
+  llvm::SmallString<256> Storage;
+  Path = makeAbsolute(ScanInstance, Path, Storage);
+  FileDeps.push_back(std::string(Path));
+}
+
+void ModuleDepCollector::addFileDep(ModuleDeps &MD, StringRef Path) {
+  llvm::SmallString<256> Storage;
+  Path = makeAbsolute(ScanInstance, Path, Storage);
+  MD.FileDeps.insert(Path);
+}
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -28,8 +28,9 @@
 class DependencyConsumerForwarder : public DependencyFileGenerator {
 public:
   DependencyConsumerForwarder(std::unique_ptr Opts,
-  DependencyConsumer &C)
-  : DependencyFileGenerator(*Opts), Opts(std::move(Opts)), C(C) {}
+  StringRef WorkingDirectory, DependencyConsumer &C)
+  : DependencyFileGenerator(*Opts), WorkingDirectory(WorkingDirectory),
+Opts(std::move(Opts)), C(C) {}
 
   void finishedMainFile(DiagnosticsEngine &Diags) override {
 C.handleDependencyOutputOpts(*Opts);
@@ -37,11 +38,13 @@
 for (const auto &File : getDependencies()) {
   CanonPath = File;
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
+  llvm::sys::fs::make_absolute(WorkingDirectory, CanonPath);
   C.handleFileDependency(CanonPath);
 }
   }
 
 private:
+  StringRef WorkingDirectory;
   std::unique_ptr Opts;
   DependencyConsumer &C;
 };
@@ -221,8 +224,8 @@
 switch (Format) {
 case ScanningOutputFormat::Make:
   ScanInstance.addDependencyCollector(
-  std::make_shared(std::move(Opts),
-Consumer));
+  std::make_sh

[PATCH] D130935: [clang] Only modify FileEntryRef names that are externally remapped

[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 rG98cf745a032e: [clang] Only modify FileEntryRef names that 
are externally remapped (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130935

Files:
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileManagerTest.cpp


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -44,16 +44,16 @@
   }
 }
 
-if (!StatPath)
-  StatPath = Path;
-
 auto fileType = IsFile ?
   llvm::sys::fs::file_type::regular_file :
   llvm::sys::fs::file_type::directory_file;
-llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+llvm::vfs::Status Status(StatPath ? StatPath : Path,
+ llvm::sys::fs::UniqueID(1, INode),
  /*MTime*/{}, /*User*/0, /*Group*/0,
  /*Size*/0, fileType,
  llvm::sys::fs::perms::all_all);
+if (StatPath)
+  Status.ExposesExternalVFSPath = true;
 StatCalls[Path] = Status;
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -274,8 +274,8 @@
   if (!UFE)
 UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
 // Name mismatch. We need a redirect. First grab the actual entry we want
@@ -292,19 +292,7 @@
 // filesystems behave and confuses parts of clang expect to see the
 // name-as-accessed on the \a FileEntryRef.
 //
-// Further, it isn't *just* external names, but will also give back 
absolute
-// paths when a relative path was requested - the check is comparing the
-// name from the status, which is passed an absolute path resolved from the
-// current working directory. `clang-apply-replacements` appears to depend
-// on this behaviour, though it's adjusting the working directory, which is
-// definitely not supported. Once that's fixed this hack should be able to
-// be narrowed to only when there's an externally mapped name given back.
-//
 // A potential plan to remove this is as follows -
-//   - Add API to determine if the name has been rewritten by the VFS.
-//   - Fix `clang-apply-replacements` to pass down the absolute path rather
-// than changing the CWD. Narrow this hack down to just externally
-// mapped paths.
 //   - Expose the requested filename. One possibility would be to allow
 // redirection-FileEntryRefs to be returned, rather than returning
 // the pointed-at-FileEntryRef, and customizing `getName()` to look


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -44,16 +44,16 @@
   }
 }
 
-if (!StatPath)
-  StatPath = Path;
-
 auto fileType = IsFile ?
   llvm::sys::fs::file_type::regular_file :
   llvm::sys::fs::file_type::directory_file;
-llvm::vfs::Status Status(StatPath, llvm::sys::fs::UniqueID(1, INode),
+llvm::vfs::Status Status(StatPath ? StatPath : Path,
+ llvm::sys::fs::UniqueID(1, INode),
  /*MTime*/{}, /*User*/0, /*Group*/0,
  /*Size*/0, fileType,
  llvm::sys::fs::perms::all_all);
+if (StatPath)
+  Status.ExposesExternalVFSPath = true;
 StatCalls[Path] = Status;
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -274,8 +274,8 @@
   if (!UFE)
 UFE = new (FilesAlloc.Allocate()) FileEntry();
 
-  if (Status.getName() == Filename) {
-// The name matches. Set the FileEntry.
+  if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
+// Use the requested name. Set the FileEntry.
 NamedFileEnt->second = FileEntryRef::MapValue(*UFE, DirInfo);
   } else {
 // Name mismatch. We need a redirect. First grab the actual entry we want
@@ -292,19 +292,7 @@
 // filesystems behave and confuses parts of clang expect to see the
 // name-as-accessed on the \a FileEntryRef.
 //
-// Further, it isn't *just

[PATCH] D131004: [clang] Add FileEntryRef::getNameAsRequested()

[Ben Langmuir via Phabricator via cfe-commits]

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

As progress towards having FileManager::getFileRef() return the path 
as-requested by default, return a FileEntryRef that can use 
getNameAsRequested() to retrieve this path, with the ultimate goal that this 
should be the behaviour of getName() and clients should explicitly request the 
"external" name if they need to (see comment in FileManager::getFileRef). For 
now, getName() continues to return the external path by looking through the 
redirects.

For now, the new function is only used in unit tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131004

Files:
  clang/include/clang/Basic/FileEntry.h
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileEntryTest.cpp
  clang/unittests/Basic/FileManagerTest.cpp

Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -356,9 +356,13 @@
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
+  EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
+  EXPECT_EQ("dir/f1-redirect.cpp", F1Redirect->getNameAsRequested());
+
   // Compare against FileEntry*.
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -374,7 +378,7 @@
 
   // Compare using isSameRef.
   EXPECT_TRUE(F1->isSameRef(*F1Again));
-  EXPECT_TRUE(F1->isSameRef(*F1Redirect));
+  EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
 }
Index: clang/unittests/Basic/FileEntryTest.cpp
===
--- clang/unittests/Basic/FileEntryTest.cpp
+++ clang/unittests/Basic/FileEntryTest.cpp
@@ -50,6 +50,14 @@
 const_cast(Base.getFileEntry()), DR)})
  .first);
   }
+  FileEntryRef addFileRedirect(StringRef Name, FileEntryRef Base) {
+return FileEntryRef(
+*Files
+ .insert({Name, FileEntryRef::MapValue(
+const_cast(
+Base.getMapEntry()))})
+ .first);
+  }
 };
 
 namespace {
@@ -58,13 +66,23 @@
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ("1", R1.getName());
   EXPECT_EQ("2", R2.getName());
   EXPECT_EQ("1-also", R1Also.getName());
+  EXPECT_EQ("1", R1Redirect.getName());
+  EXPECT_EQ("1", R1Redirect2.getName());
+
+  EXPECT_EQ("1", R1.getNameAsRequested());
+  EXPECT_EQ("1-redirect", R1Redirect.getNameAsRequested());
+  EXPECT_EQ("1-redirect2", R1Redirect2.getNameAsRequested());
 
   EXPECT_NE(&R1.getFileEntry(), &R2.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), &R1Also.getFileEntry());
+  EXPECT_EQ(&R1.getFileEntry(), &R1Redirect.getFileEntry());
+  EXPECT_EQ(&R1Redirect.getFileEntry(), &R1Redirect2.getFileEntry());
 
   const FileEntry *CE1 = R1;
   EXPECT_EQ(CE1, &R1.getFileEntry());
@@ -93,6 +111,8 @@
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ(R1, &R1.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), R1);
@@ -100,6 +120,8 @@
   EXPECT_NE(R1, &R2.getFileEntry());
   EXPECT_NE(&R2.getFileEntry(), R1);
   EXPECT_NE(R1, R2);
+  EXPECT_EQ(R1, R1Redirect);
+  EXPECT_EQ(R1, R1Redirect2);
 
   OptionalFileEntryRefDegradesToFileEntryPtr M1 = R1;
 
@@ -114,11 +136,16 @@
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1)));
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1.getMapEntry(;
   EXPECT_FALSE(R1.isSameRef(R2));
   EXPECT_FALSE(R1.isSameRef(R1Also));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect2));
+  EXPECT_FALSE(R1Redirect.isSameRef(R1Redirect2));
 }
 
 TEST(FileEntryTest, DenseMapInfo) {
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/F

[PATCH] D131017: Fix use-after-free in clang-apply-replacements

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: bnbarham, fmayer.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Accidentally introduced a dangling StringRef in b4c6dc2e6637 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131017

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -24,6 +24,7 @@
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -140,7 +141,7 @@
 static llvm::DenseMap>
 groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
   const clang::SourceManager &SM) {
-  std::set Warned;
+  llvm::StringSet<> Warned;
   llvm::DenseMap>
   GroupedReplacements;
 


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -24,6 +24,7 @@
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -140,7 +141,7 @@
 static llvm::DenseMap>
 groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
   const clang::SourceManager &SM) {
-  std::set Warned;
+  llvm::StringSet<> Warned;
   llvm::DenseMap>
   GroupedReplacements;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130934: [clang] Update code that assumes FileEntry::getName is absolute NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

@fmayer thanks, fix is: https://reviews.llvm.org/D131017


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130934

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

[PATCH] D131017: Fix use-after-free in clang-apply-replacements

[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 rG54110b8aa010: Fix use-after-free in clang-apply-replacements 
(authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131017

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -24,6 +24,7 @@
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -140,7 +141,7 @@
 static llvm::DenseMap>
 groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
   const clang::SourceManager &SM) {
-  std::set Warned;
+  llvm::StringSet<> Warned;
   llvm::DenseMap>
   GroupedReplacements;
 


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -24,6 +24,7 @@
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -140,7 +141,7 @@
 static llvm::DenseMap>
 groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
   const clang::SourceManager &SM) {
-  std::set Warned;
+  llvm::StringSet<> Warned;
   llvm::DenseMap>
   GroupedReplacements;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131076: [clang][modules] Don't depend on sharing FileManager during module build

[Ben Langmuir via Phabricator via cfe-commits]

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

Sharing the FileManager between the importer and the module build should only 
be an optimization. Add a cc1 option -fno-modules-share-filemanager to allow us 
to test this. Fix the path to modulemap files, which previously depended on the 
shared FileManager when using path mapped to an external file in a VFS.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131076

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/ClangScanDeps/modulemap-via-vfs.m
  clang/test/Modules/submodule-in-private-mmap-vfs.m
  clang/test/VFS/module-import.m

Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -1,6 +1,7 @@
-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
 @import not_real;
 
@@ -18,9 +19,12 @@
 // Override the module map (vfsoverlay2 on top)
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
 
 // vfsoverlay2 not present
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
 
 // vfsoverlay2 on the bottom
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
Index: clang/test/Modules/submodule-in-private-mmap-vfs.m
===
--- /dev/null
+++ clang/test/Modules/submodule-in-private-mmap-vfs.m
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+// RUN: %clang_cc1 -fmodules -fno-modules-share-filemanager -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t -I/tests -ivfsoverlay %t/vfs.json -fsyntax-only %t/tu.m -verify
+
+//--- Dir1/module.modulemap
+
+//--- Dir2/module.private.modulemap
+module Foo_Private {}
+
+//--- vfs.json.in
+{
+  'version': 0,
+  'use-external-names': true,
+  'roots': [
+{
+  'name': '/tests',
+  'type': 'directory',
+  'contents': [
+{
+  'name': 'module.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir1/module.modulemap'
+},
+{
+  'name': 'module.private.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir2/module.private.modulemap'
+}
+  ]
+}
+  ]
+}
+
+//--- tu.m
+@import Foo_Private;
+// expected-no-diagnostics
\ No newline at end of file
Index: clang/test/ClangScanDeps/modulemap-via-vfs.m
===
--- clang/test/ClangScanDeps/modulemap-via-vfs.m
+++ clang/test/ClangScanDeps/modulemap-via-vfs.m
@@ -9,6 +9,7 @@
 
 // CHECK-NOT: build/module.modulemap
 // CHECK: A/module.modulemap
+// CHECK-NOT: build/module.modulemap
 
 //--- build/compile-commands.json.in
 
@@ -17,6 +18,11 @@
   "directory": "DIR",
   "command": "clang DIR/main.m -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
   "file": "DIR/main.m"
+},
+{
+  "directory": "DIR",
+  "command": "clang DIR/main.m -Imodules/A -fmodules -Xclang -fno-modules-share-filemana

[PATCH] D131004: [clang] Add FileEntryRef::getNameAsRequested()

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6a79e2ff1989: [clang] Add FileEntryRef::getNameAsRequested() 
(authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131004

Files:
  clang/include/clang/Basic/FileEntry.h
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileEntryTest.cpp
  clang/unittests/Basic/FileManagerTest.cpp

Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -356,9 +356,13 @@
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
+  EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
+  EXPECT_EQ("dir/f1-redirect.cpp", F1Redirect->getNameAsRequested());
+
   // Compare against FileEntry*.
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -374,7 +378,7 @@
 
   // Compare using isSameRef.
   EXPECT_TRUE(F1->isSameRef(*F1Again));
-  EXPECT_TRUE(F1->isSameRef(*F1Redirect));
+  EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
 }
Index: clang/unittests/Basic/FileEntryTest.cpp
===
--- clang/unittests/Basic/FileEntryTest.cpp
+++ clang/unittests/Basic/FileEntryTest.cpp
@@ -50,6 +50,14 @@
 const_cast(Base.getFileEntry()), DR)})
  .first);
   }
+  FileEntryRef addFileRedirect(StringRef Name, FileEntryRef Base) {
+return FileEntryRef(
+*Files
+ .insert({Name, FileEntryRef::MapValue(
+const_cast(
+Base.getMapEntry()))})
+ .first);
+  }
 };
 
 namespace {
@@ -58,13 +66,23 @@
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ("1", R1.getName());
   EXPECT_EQ("2", R2.getName());
   EXPECT_EQ("1-also", R1Also.getName());
+  EXPECT_EQ("1", R1Redirect.getName());
+  EXPECT_EQ("1", R1Redirect2.getName());
+
+  EXPECT_EQ("1", R1.getNameAsRequested());
+  EXPECT_EQ("1-redirect", R1Redirect.getNameAsRequested());
+  EXPECT_EQ("1-redirect2", R1Redirect2.getNameAsRequested());
 
   EXPECT_NE(&R1.getFileEntry(), &R2.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), &R1Also.getFileEntry());
+  EXPECT_EQ(&R1.getFileEntry(), &R1Redirect.getFileEntry());
+  EXPECT_EQ(&R1Redirect.getFileEntry(), &R1Redirect2.getFileEntry());
 
   const FileEntry *CE1 = R1;
   EXPECT_EQ(CE1, &R1.getFileEntry());
@@ -93,6 +111,8 @@
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ(R1, &R1.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), R1);
@@ -100,6 +120,8 @@
   EXPECT_NE(R1, &R2.getFileEntry());
   EXPECT_NE(&R2.getFileEntry(), R1);
   EXPECT_NE(R1, R2);
+  EXPECT_EQ(R1, R1Redirect);
+  EXPECT_EQ(R1, R1Redirect2);
 
   OptionalFileEntryRefDegradesToFileEntryPtr M1 = R1;
 
@@ -114,11 +136,16 @@
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1)));
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1.getMapEntry(;
   EXPECT_FALSE(R1.isSameRef(R2));
   EXPECT_FALSE(R1.isSameRef(R1Also));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect2));
+  EXPECT_FALSE(R1Redirect.isSameRef(R1Redirect2));
 }
 
 TEST(FileEntryTest, DenseMapInfo) {
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -293,12 +293,8 @@
 // name-as-accessed on the \a FileEntryRef.
 //
 // A potential plan to remove this is as follows -
-//   - Expose the requested filename. One possibility would be to allow
-// redirection-FileEntryRefs to be returned, rather than returning
-// the pointed-at-FileEntryRef, and customizing `getName()` to look
-// through the indirection.
 //   - Update callers such as

[PATCH] D131076: [clang][modules] Don't depend on sharing FileManager during module build

[Ben Langmuir via Phabricator via cfe-commits]

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

- Updates for review feedback
- Attempt to fix one of the Windows path issues - I'm just guessing based on 
what other VFS tests are doing.  I don't know what the other windows failure is 
about, and probably need to debug it.


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

https://reviews.llvm.org/D131076

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/ClangScanDeps/modulemap-via-vfs.m
  clang/test/Modules/submodule-in-private-mmap-vfs.m
  clang/test/VFS/module-import.m

Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -1,6 +1,7 @@
-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
 @import not_real;
 
@@ -18,9 +19,12 @@
 // Override the module map (vfsoverlay2 on top)
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
 
 // vfsoverlay2 not present
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
 
 // vfsoverlay2 on the bottom
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
Index: clang/test/Modules/submodule-in-private-mmap-vfs.m
===
--- /dev/null
+++ clang/test/Modules/submodule-in-private-mmap-vfs.m
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+// RUN: %clang_cc1 -fmodules -fno-modules-share-filemanager -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t -I%t/Virtual -ivfsoverlay %t/vfs.json -fsyntax-only %t/tu.m -verify
+
+//--- Dir1/module.modulemap
+
+//--- Dir2/module.private.modulemap
+module Foo_Private {}
+
+//--- vfs.json.in
+{
+  'version': 0,
+  'use-external-names': true,
+  'roots': [
+{
+  'name': 'DIR/Virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'name': 'module.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir1/module.modulemap'
+},
+{
+  'name': 'module.private.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir2/module.private.modulemap'
+}
+  ]
+}
+  ]
+}
+
+//--- tu.m
+@import Foo_Private;
+// expected-no-diagnostics
Index: clang/test/ClangScanDeps/modulemap-via-vfs.m
===
--- clang/test/ClangScanDeps/modulemap-via-vfs.m
+++ clang/test/ClangScanDeps/modulemap-via-vfs.m
@@ -9,6 +9,7 @@
 
 // CHECK-NOT: build/module.modulemap
 // CHECK: A/module.modulemap
+// CHECK-NOT: build/module.modulemap
 
 //--- build/compile-commands.json.in
 
@@ -17,6 +18,11 @@
   "directory": "DIR",
   "command": "clang DIR/main.m -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
   "file": "DIR/main.m"
+},
+{
+  "directory": "DIR",
+  "command": "clang DIR/main.m -Imodules/A -fmodules -Xclang -fno-modules-share-filemanager -fmodules-cache-path=DIR/module-cache-unshared -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
+  "file": "DIR/main.m"
 }
 ]
 
@@ -35,6 +41,7 @@
 {
   "version": 0,
   "case-sensitive": "false

[PATCH] D131076: [clang][modules] Don't depend on sharing FileManager during module build

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 450313.
benlangmuir added a comment.
Herald added a subscriber: ormris.

Attempt to fix test failure seen on Windows. It revealed two bugs

- Avoid reusing the FileManager at the top-level in clang-scan-deps to make the 
test behave as expected
- Make the FileManager return the cached redirecting entry, to match how it 
behaves when it's a new file, and test this case explicitly.

I think it was mostly luck of exactly which file lookups happen that we didn't 
hit this on other platforms.


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

https://reviews.llvm.org/D131076

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/ClangScanDeps/modulemap-via-vfs.m
  clang/test/Modules/submodule-in-private-mmap-vfs.m
  clang/test/VFS/module-import.m
  clang/unittests/Basic/FileManagerTest.cpp

Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -340,6 +340,7 @@
   auto F1Again = manager.getFileRef("dir/f1.cpp");
   auto F1Also = manager.getFileRef("dir/f1-also.cpp");
   auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+  auto F1RedirectAgain = manager.getFileRef("dir/f1-redirect.cpp");
   auto F2 = manager.getFileRef("dir/f2.cpp");
 
   // Check Expected for error.
@@ -347,6 +348,7 @@
   ASSERT_FALSE(!F1Also);
   ASSERT_FALSE(!F1Again);
   ASSERT_FALSE(!F1Redirect);
+  ASSERT_FALSE(!F1RedirectAgain);
   ASSERT_FALSE(!F2);
 
   // Check names.
@@ -354,6 +356,7 @@
   EXPECT_EQ("dir/f1.cpp", F1Again->getName());
   EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+  EXPECT_EQ("dir/f1.cpp", F1RedirectAgain->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
   EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
@@ -363,6 +366,7 @@
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
   EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1RedirectAgain->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -371,6 +375,7 @@
   EXPECT_EQ(*F1, *F1Again);
   EXPECT_EQ(*F1, *F1Redirect);
   EXPECT_EQ(*F1Also, *F1Redirect);
+  EXPECT_EQ(*F1, *F1RedirectAgain);
   EXPECT_NE(*F2, *F1);
   EXPECT_NE(*F2, *F1Also);
   EXPECT_NE(*F2, *F1Again);
@@ -381,6 +386,7 @@
   EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
+  EXPECT_TRUE(F1Redirect->isSameRef(*F1RedirectAgain));
 }
 
 // getFile() Should return the same entry as getVirtualFile if the file actually
Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -1,6 +1,7 @@
-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
 @import not_real;
 
@@ -18,9 +19,12 @@
 // Override the module map (vfsoverlay2 on top)
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
 
 // vfsoverlay2 not present
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
 
 // vfsoverlay2 on the bottom
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -

[PATCH] D131273: [clang] Fix redirection behaviour for cached FileEntryRef

[Ben Langmuir via Phabricator via cfe-commits]

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

In 6a79e2ff1989b 
 we 
changed Filemanager::getEntryRef() to return the
redirecting FileEntryRef instead of looking through the redirection.
This commit fixes the case when looking up a cached file path to also
return the redirecting FileEntryRef. This mainly affects the behaviour
of calling getNameAsRequested() on the resulting entry ref.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131273

Files:
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileManagerTest.cpp


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -340,6 +340,7 @@
   auto F1Again = manager.getFileRef("dir/f1.cpp");
   auto F1Also = manager.getFileRef("dir/f1-also.cpp");
   auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+  auto F1RedirectAgain = manager.getFileRef("dir/f1-redirect.cpp");
   auto F2 = manager.getFileRef("dir/f2.cpp");
 
   // Check Expected for error.
@@ -347,6 +348,7 @@
   ASSERT_FALSE(!F1Also);
   ASSERT_FALSE(!F1Again);
   ASSERT_FALSE(!F1Redirect);
+  ASSERT_FALSE(!F1RedirectAgain);
   ASSERT_FALSE(!F2);
 
   // Check names.
@@ -354,6 +356,7 @@
   EXPECT_EQ("dir/f1.cpp", F1Again->getName());
   EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+  EXPECT_EQ("dir/f1.cpp", F1RedirectAgain->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
   EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
@@ -363,6 +366,7 @@
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
   EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1RedirectAgain->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -371,6 +375,7 @@
   EXPECT_EQ(*F1, *F1Again);
   EXPECT_EQ(*F1, *F1Redirect);
   EXPECT_EQ(*F1Also, *F1Redirect);
+  EXPECT_EQ(*F1, *F1RedirectAgain);
   EXPECT_NE(*F2, *F1);
   EXPECT_NE(*F2, *F1Also);
   EXPECT_NE(*F2, *F1Again);
@@ -381,6 +386,7 @@
   EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
+  EXPECT_TRUE(F1Redirect->isSameRef(*F1RedirectAgain));
 }
 
 // getFile() Should return the same entry as getVirtualFile if the file 
actually
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,13 +212,7 @@
 if (!SeenFileInsertResult.first->second)
   return llvm::errorCodeToError(
   SeenFileInsertResult.first->second.getError());
-// Construct and return and FileEntryRef, unless it's a redirect to another
-// filename.
-FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
-if (LLVM_LIKELY(Value.V.is()))
-  return FileEntryRef(*SeenFileInsertResult.first);
-return FileEntryRef(*reinterpret_cast(
-Value.V.get()));
+return FileEntryRef(*SeenFileInsertResult.first);
   }
 
   // We've not seen this before. Fill it in.


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -340,6 +340,7 @@
   auto F1Again = manager.getFileRef("dir/f1.cpp");
   auto F1Also = manager.getFileRef("dir/f1-also.cpp");
   auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+  auto F1RedirectAgain = manager.getFileRef("dir/f1-redirect.cpp");
   auto F2 = manager.getFileRef("dir/f2.cpp");
 
   // Check Expected for error.
@@ -347,6 +348,7 @@
   ASSERT_FALSE(!F1Also);
   ASSERT_FALSE(!F1Again);
   ASSERT_FALSE(!F1Redirect);
+  ASSERT_FALSE(!F1RedirectAgain);
   ASSERT_FALSE(!F2);
 
   // Check names.
@@ -354,6 +356,7 @@
   EXPECT_EQ("dir/f1.cpp", F1Again->getName());
   EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+  EXPECT_EQ("dir/f1.cpp", F1RedirectAgain->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
   EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
@@ -363,6 +366,7 @@
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
   EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1RedirectAgain->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -371,6 +375,7 @@
   EXPECT_EQ(*F1, *F1Again);
   EXPECT_EQ(*F1, *F1Redirect);
   EXPECT_EQ(*F1Also, *F1Redirect);
+  EXPECT_EQ(*F1, *F1Redirect

[PATCH] D131273: [clang] Fix redirection behaviour for cached FileEntryRef

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd038bb196c51: [clang] Fix redirection behaviour for cached 
FileEntryRef (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131273

Files:
  clang/lib/Basic/FileManager.cpp
  clang/unittests/Basic/FileManagerTest.cpp


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -340,6 +340,7 @@
   auto F1Again = manager.getFileRef("dir/f1.cpp");
   auto F1Also = manager.getFileRef("dir/f1-also.cpp");
   auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+  auto F1RedirectAgain = manager.getFileRef("dir/f1-redirect.cpp");
   auto F2 = manager.getFileRef("dir/f2.cpp");
 
   // Check Expected for error.
@@ -347,6 +348,7 @@
   ASSERT_FALSE(!F1Also);
   ASSERT_FALSE(!F1Again);
   ASSERT_FALSE(!F1Redirect);
+  ASSERT_FALSE(!F1RedirectAgain);
   ASSERT_FALSE(!F2);
 
   // Check names.
@@ -354,6 +356,7 @@
   EXPECT_EQ("dir/f1.cpp", F1Again->getName());
   EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+  EXPECT_EQ("dir/f1.cpp", F1RedirectAgain->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
   EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
@@ -363,6 +366,7 @@
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
   EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1RedirectAgain->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -371,6 +375,7 @@
   EXPECT_EQ(*F1, *F1Again);
   EXPECT_EQ(*F1, *F1Redirect);
   EXPECT_EQ(*F1Also, *F1Redirect);
+  EXPECT_EQ(*F1, *F1RedirectAgain);
   EXPECT_NE(*F2, *F1);
   EXPECT_NE(*F2, *F1Also);
   EXPECT_NE(*F2, *F1Again);
@@ -381,6 +386,7 @@
   EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
+  EXPECT_TRUE(F1Redirect->isSameRef(*F1RedirectAgain));
 }
 
 // getFile() Should return the same entry as getVirtualFile if the file 
actually
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,13 +212,7 @@
 if (!SeenFileInsertResult.first->second)
   return llvm::errorCodeToError(
   SeenFileInsertResult.first->second.getError());
-// Construct and return and FileEntryRef, unless it's a redirect to another
-// filename.
-FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
-if (LLVM_LIKELY(Value.V.is()))
-  return FileEntryRef(*SeenFileInsertResult.first);
-return FileEntryRef(*reinterpret_cast(
-Value.V.get()));
+return FileEntryRef(*SeenFileInsertResult.first);
   }
 
   // We've not seen this before. Fill it in.


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -340,6 +340,7 @@
   auto F1Again = manager.getFileRef("dir/f1.cpp");
   auto F1Also = manager.getFileRef("dir/f1-also.cpp");
   auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+  auto F1RedirectAgain = manager.getFileRef("dir/f1-redirect.cpp");
   auto F2 = manager.getFileRef("dir/f2.cpp");
 
   // Check Expected for error.
@@ -347,6 +348,7 @@
   ASSERT_FALSE(!F1Also);
   ASSERT_FALSE(!F1Again);
   ASSERT_FALSE(!F1Redirect);
+  ASSERT_FALSE(!F1RedirectAgain);
   ASSERT_FALSE(!F2);
 
   // Check names.
@@ -354,6 +356,7 @@
   EXPECT_EQ("dir/f1.cpp", F1Again->getName());
   EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+  EXPECT_EQ("dir/f1.cpp", F1RedirectAgain->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
   EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
@@ -363,6 +366,7 @@
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
   EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1RedirectAgain->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
@@ -371,6 +375,7 @@
   EXPECT_EQ(*F1, *F1Again);
   EXPECT_EQ(*F1, *F1Redirect);
   EXPECT_EQ(*F1Also, *F1Redirect);
+  EXPECT_EQ(*F1, *F1RedirectAgain);
   EXPECT_NE(*F2, *F1);
   EXPECT_NE(*F2, *F1Also);
   EXPECT_NE(*F2, *F1Again);
@@ -381,6 +386,7 @@
   EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
+  EXPECT_TRUE(F1Redirect->isSameRef(*F1RedirectAgain));
 }
 
 // getFile() Should return the same entry as getVirtualFile if the file actually

[PATCH] D131076: [clang][modules] Don't depend on sharing FileManager during module build

[Ben Langmuir via Phabricator via cfe-commits]

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

Split out the FileManager change into https://reviews.llvm.org/D131273


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

https://reviews.llvm.org/D131076

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/ClangScanDeps/modulemap-via-vfs.m
  clang/test/Modules/submodule-in-private-mmap-vfs.m
  clang/test/VFS/module-import.m

Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -1,6 +1,7 @@
-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
 @import not_real;
 
@@ -18,9 +19,12 @@
 // Override the module map (vfsoverlay2 on top)
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
 
 // vfsoverlay2 not present
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
 
 // vfsoverlay2 on the bottom
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
Index: clang/test/Modules/submodule-in-private-mmap-vfs.m
===
--- /dev/null
+++ clang/test/Modules/submodule-in-private-mmap-vfs.m
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+// RUN: %clang_cc1 -fmodules -fno-modules-share-filemanager -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t -I%t/Virtual -ivfsoverlay %t/vfs.json -fsyntax-only %t/tu.m -verify
+
+//--- Dir1/module.modulemap
+
+//--- Dir2/module.private.modulemap
+module Foo_Private {}
+
+//--- vfs.json.in
+{
+  'version': 0,
+  'use-external-names': true,
+  'roots': [
+{
+  'name': 'DIR/Virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'name': 'module.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir1/module.modulemap'
+},
+{
+  'name': 'module.private.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir2/module.private.modulemap'
+}
+  ]
+}
+  ]
+}
+
+//--- tu.m
+@import Foo_Private;
+// expected-no-diagnostics
Index: clang/test/ClangScanDeps/modulemap-via-vfs.m
===
--- clang/test/ClangScanDeps/modulemap-via-vfs.m
+++ clang/test/ClangScanDeps/modulemap-via-vfs.m
@@ -2,13 +2,15 @@
 // RUN: split-file %s %t.dir
 // RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/compile-commands.json.in > %t.dir/build/compile-commands.json
 // RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/vfs.yaml.in > %t.dir/build/vfs.yaml
-// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json -j 1 -format experimental-full \
+// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json \
+// RUN:   -reuse-filemanager=0 -j 1 -format experimental-full \
 // RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > %t.db
 // RUN: %deps-to-rsp %t.db --module-name=A > %t.A.cc1.rsp
 // RUN: cat %t.A.cc1.rsp | sed 's:\?:/:g' | FileCheck %s
 
 // CHECK-NOT: build/module.modulemap
 // CHECK: A/module.modulemap
+// CHECK-NOT: build/module.modulemap
 
 //--- build/compile-commands.json.in
 
@@ -17,6 +19,11 @@
   "directo

[PATCH] D131076: [clang][modules] Don't depend on sharing FileManager during module build

[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 rGfb89cc0ddbd9: [clang][modules] Don't depend on sharing 
FileManager during module build (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131076

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/ClangScanDeps/modulemap-via-vfs.m
  clang/test/Modules/submodule-in-private-mmap-vfs.m
  clang/test/VFS/module-import.m

Index: clang/test/VFS/module-import.m
===
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -1,6 +1,7 @@
-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
 @import not_real;
 
@@ -18,9 +19,12 @@
 // Override the module map (vfsoverlay2 on top)
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
 
 // vfsoverlay2 not present
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
 
 // vfsoverlay2 on the bottom
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
Index: clang/test/Modules/submodule-in-private-mmap-vfs.m
===
--- /dev/null
+++ clang/test/Modules/submodule-in-private-mmap-vfs.m
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+// RUN: %clang_cc1 -fmodules -fno-modules-share-filemanager -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t -I%t/Virtual -ivfsoverlay %t/vfs.json -fsyntax-only %t/tu.m -verify
+
+//--- Dir1/module.modulemap
+
+//--- Dir2/module.private.modulemap
+module Foo_Private {}
+
+//--- vfs.json.in
+{
+  'version': 0,
+  'use-external-names': true,
+  'roots': [
+{
+  'name': 'DIR/Virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'name': 'module.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir1/module.modulemap'
+},
+{
+  'name': 'module.private.modulemap',
+  'type': 'file',
+  'external-contents': 'DIR/Dir2/module.private.modulemap'
+}
+  ]
+}
+  ]
+}
+
+//--- tu.m
+@import Foo_Private;
+// expected-no-diagnostics
Index: clang/test/ClangScanDeps/modulemap-via-vfs.m
===
--- clang/test/ClangScanDeps/modulemap-via-vfs.m
+++ clang/test/ClangScanDeps/modulemap-via-vfs.m
@@ -2,13 +2,15 @@
 // RUN: split-file %s %t.dir
 // RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/compile-commands.json.in > %t.dir/build/compile-commands.json
 // RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/vfs.yaml.in > %t.dir/build/vfs.yaml
-// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json -j 1 -format experimental-full \
+// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json \
+// RUN:   -reuse-filemanager=0 -j 1 -format experimental-full \
 // RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > %t.db
 // RUN: %deps-to-rsp %t.db --module-name=A > %t.A.cc1.rsp
 // RUN: cat %t.A.cc1.rsp | sed 's:\?:/:g' | FileCheck %s
 
 // CHECK-NOT: build/module

[PATCH] D131412: [clang][deps] Stop sharing FileManager across module builds in 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 a project: clang.
Herald added a subscriber: cfe-commits.

Sharing the FileManager across implicit module builds currently leaks
paths looked up in an importer into the built module itself. This can
cause non-deterministic results across scans. It is especially bad for
modules since the path can be saved into the pcm file itself, leading to
stateful behaviour if the cache is shared.

This should not impact the number of real filesystem accesses in the
scanner, since it is already caching in the
DependencyScanningWorkerFilesystem.

Note: this change does not affect whether or not the FileManager is
shared across TUs in the scanner, which is a separate issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131412

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/modules-file-path-isolation.c


Index: clang/test/ClangScanDeps/modules-file-path-isolation.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-file-path-isolation.c
@@ -0,0 +1,44 @@
+// Ensure that the spelling of a path seen outside a module (e.g. header via
+// symlink) does not leak into the compilation of that module unnecessarily.
+// Note: the spelling of the modulemap path still depends on the includer, 
since
+// that is the only source of information about it.
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: ln -s A.h %t/Z.h
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 -format 
experimental-full \
+// RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > 
%t/output
+// RUN: FileCheck %s < %t/output
+
+// CHECK:  "modules": [
+// CHECK-NEXT:   {
+// CHECK:  "file-deps": [
+// CHECK-NEXT:   "{{.*}}A.h",
+// CHECK-NEXT:   "{{.*}}module.modulemap"
+// CHECK-NEXT: ],
+// CHECK-NEXT: "name": "A"
+// CHECK-NEXT:   }
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -fmodules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fimplicit-module-maps",
+  "file": "DIR/tu.c"
+}]
+
+//--- module.modulemap
+module A { header "A.h" }
+module B { header "B.h" }
+module C { header "C.h" }
+
+//--- A.h
+
+//--- B.h
+#include "Z.h"
+
+//--- tu.c
+#include "B.h"
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -170,6 +170,7 @@
 
 ScanInstance.getFrontendOpts().GenerateGlobalModuleIndex = false;
 ScanInstance.getFrontendOpts().UseGlobalModuleIndex = false;
+ScanInstance.getFrontendOpts().ModulesShareFileManager = false;
 
 FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
 ScanInstance.setFileManager(FileMgr);


Index: clang/test/ClangScanDeps/modules-file-path-isolation.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-file-path-isolation.c
@@ -0,0 +1,44 @@
+// Ensure that the spelling of a path seen outside a module (e.g. header via
+// symlink) does not leak into the compilation of that module unnecessarily.
+// Note: the spelling of the modulemap path still depends on the includer, since
+// that is the only source of information about it.
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: ln -s A.h %t/Z.h
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 -format experimental-full \
+// RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > %t/output
+// RUN: FileCheck %s < %t/output
+
+// CHECK:  "modules": [
+// CHECK-NEXT:   {
+// CHECK:  "file-deps": [
+// CHECK-NEXT:   "{{.*}}A.h",
+// CHECK-NEXT:   "{{.*}}module.modulemap"
+// CHECK-NEXT: ],
+// CHECK-NEXT: "name": "A"
+// CHECK-NEXT:   }
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/tu.c"
+}]
+
+//--- module.modulemap
+module A { header "A.h" }
+module B { header "B.h" }
+module C { header "C.h" }
+
+//--- A.h
+
+//--- B.h
+#include "Z.h"
+
+//--- tu.c
+#include "B.h"
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.

[PATCH] D131420: [clang][deps] Always generate module paths

[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, thanks for cleaning it up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131420

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

[PATCH] D131412: [clang][deps] Stop sharing FileManager across module builds in scanner

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG33af4b22f8ea: [clang][deps] Stop sharing FileManager across 
module builds in scanner (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131412

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/modules-file-path-isolation.c


Index: clang/test/ClangScanDeps/modules-file-path-isolation.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-file-path-isolation.c
@@ -0,0 +1,44 @@
+// Ensure that the spelling of a path seen outside a module (e.g. header via
+// symlink) does not leak into the compilation of that module unnecessarily.
+// Note: the spelling of the modulemap path still depends on the includer, 
since
+// that is the only source of information about it.
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: ln -s A.h %t/Z.h
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 -format 
experimental-full \
+// RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > 
%t/output
+// RUN: FileCheck %s < %t/output
+
+// CHECK:  "modules": [
+// CHECK-NEXT:   {
+// CHECK:  "file-deps": [
+// CHECK-NEXT:   "{{.*}}A.h",
+// CHECK-NEXT:   "{{.*}}module.modulemap"
+// CHECK-NEXT: ],
+// CHECK-NEXT: "name": "A"
+// CHECK-NEXT:   }
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -fmodules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fimplicit-module-maps",
+  "file": "DIR/tu.c"
+}]
+
+//--- module.modulemap
+module A { header "A.h" }
+module B { header "B.h" }
+module C { header "C.h" }
+
+//--- A.h
+
+//--- B.h
+#include "Z.h"
+
+//--- tu.c
+#include "B.h"
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -170,6 +170,7 @@
 
 ScanInstance.getFrontendOpts().GenerateGlobalModuleIndex = false;
 ScanInstance.getFrontendOpts().UseGlobalModuleIndex = false;
+ScanInstance.getFrontendOpts().ModulesShareFileManager = false;
 
 FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
 ScanInstance.setFileManager(FileMgr);


Index: clang/test/ClangScanDeps/modules-file-path-isolation.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-file-path-isolation.c
@@ -0,0 +1,44 @@
+// Ensure that the spelling of a path seen outside a module (e.g. header via
+// symlink) does not leak into the compilation of that module unnecessarily.
+// Note: the spelling of the modulemap path still depends on the includer, since
+// that is the only source of information about it.
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: ln -s A.h %t/Z.h
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 -format experimental-full \
+// RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > %t/output
+// RUN: FileCheck %s < %t/output
+
+// CHECK:  "modules": [
+// CHECK-NEXT:   {
+// CHECK:  "file-deps": [
+// CHECK-NEXT:   "{{.*}}A.h",
+// CHECK-NEXT:   "{{.*}}module.modulemap"
+// CHECK-NEXT: ],
+// CHECK-NEXT: "name": "A"
+// CHECK-NEXT:   }
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/tu.c"
+}]
+
+//--- module.modulemap
+module A { header "A.h" }
+module B { header "B.h" }
+module C { header "C.h" }
+
+//--- A.h
+
+//--- B.h
+#include "Z.h"
+
+//--- tu.c
+#include "B.h"
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -170,6 +170,7 @@
 
 ScanInstance.getFrontendOpts().GenerateGlobalModuleIndex = false;
 ScanInstance.getFrontendOpts().UseGlobalModuleIndex = false;
+ScanInstance.getFrontendOpts().ModulesShareFileManager = false;
 
 FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
 ScanInstance.setFileManager(FileMgr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/unittests/Basic/FileEntryTest.cpp:47-52
   FileEntryRef addFile(StringRef Name) {
-FEs.push_back(std::make_unique());
+const FileEntry *FE =
+FM.getVirtualFile(std::to_string(UniqueNameCounter++), 0, 0);
 return FileEntryRef(
-*Files.insert({Name, FileEntryRef::MapValue(*FEs.back().get(), DR)})
- .first);
+*Files.try_emplace(Name, FileEntryRef::MapValue(*FE, DR)).first);
   }

dexonsmith wrote:
> I don't love this from a layering perspective. FileEntryTest is testing the 
> low-level pieces that are used inside FileManager... using FileManager to 
> generate the FileEntries is awkward.
> 
> Maybe it'd be okay if `FileManager::getVirtualFile()` were trivial, but it's 
> not.
> 
Could we add `friend class FileEntryTest` to `FileEntry` to get access to the 
private constructor and go back to the original approach?  I see some other 
places in llvm that use that pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123197

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

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/unittests/Basic/FileEntryTest.cpp:47-52
   FileEntryRef addFile(StringRef Name) {
-FEs.push_back(std::make_unique());
+const FileEntry *FE =
+FM.getVirtualFile(std::to_string(UniqueNameCounter++), 0, 0);
 return FileEntryRef(
-*Files.insert({Name, FileEntryRef::MapValue(*FEs.back().get(), DR)})
- .first);
+*Files.try_emplace(Name, FileEntryRef::MapValue(*FE, DR)).first);
   }

benlangmuir wrote:
> dexonsmith wrote:
> > I don't love this from a layering perspective. FileEntryTest is testing the 
> > low-level pieces that are used inside FileManager... using FileManager to 
> > generate the FileEntries is awkward.
> > 
> > Maybe it'd be okay if `FileManager::getVirtualFile()` were trivial, but 
> > it's not.
> > 
> Could we add `friend class FileEntryTest` to `FileEntry` to get access to the 
> private constructor and go back to the original approach?  I see some other 
> places in llvm that use that pattern.
(just saw Sam made the same suggestion)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123197

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

[PATCH] D124288: [Index] Remove reference to `UnresolvedUsingIfExists`

[Ben Langmuir via Phabricator via cfe-commits]

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

Minor suggestion for the test case, but otherwise LGTM.




Comment at: clang/test/Index/using_if_exists.cpp:9
+// CHECK: [[@LINE-1]]:11 | using/C++ | foo | c:@UD@foo |  | Decl | 
rel: 0
+// CHECK-NOT: [[@LINE-2]]:11 | 

Could we drop the `[[@LINE-2]]:11 |` part of the CHECK-NOT to make it a bit 
more robust?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124288

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

[PATCH] D124635: Frontend: Delete output streams before closing CompilerInstance outputs

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

LGTM, although I made a suggestion about the spelling.

> This fixes theoretical bugs related to proxy streams, which may have
> cleanups to run in their destructor. E.g., buffer_ostream supports
> pwrite() by holding all content in a buffer until destruction time.

The general point about streams with destructors is well taken, but it seems a 
bit broken that `buffer_ostream` cannot `flush`.  I guess the layering it just 
weird in that we have buffering in the base class, but `buffer_ostream` wants 
to do something different.  Could we afford to make `flush` virtual?  Not 
something needed for this patch.




Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:796
   SGSerializer.serialize(*OS);
-  OS->flush();
+  OS = nullptr;
 }

Nit: `OS.reset(nullptr);` makes it more obvious this is a `unique_ptr` and not 
a raw pointer. Same for the other cases as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124635

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

[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[Ben Langmuir via Phabricator via cfe-commits]

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



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:55
+  getCommandLine(llvm::function_ref<
+ Expected(const ModuleID &)>
+ LookupModuleOutputs) const;

jansvoboda11 wrote:
> I'm curious whether you have encountered situations where being able to 
> return an error from `LookupModuleOutputs` is useful.
I ended up backing this change out: it was motivated by a downstream libclang 
API change that I have now re-evaluated based on your other feedback to use a 
per-output callback instead of a single callback.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:297
+? llvm::cantFail(FD.getCommandLine(
+  [&](ModuleID MID) -> const ModuleOutputOptions & {
+return lookupModuleOutputs(MID);

jansvoboda11 wrote:
> Is my understanding correct that this lambda of type `const 
> ModuleOutputOptions &(ModuleID)` gets implicitly converted to 
> `llvm::function_ref(const ModuleID &)>`?
> 
> If so, I think it would be clearer to take the `ModuleID` by const ref here 
> also and wrap the return type with `Expected`, to match the... expected 
> `function_ref` type. WDYT?
This was unintentional, I just missed these couple of places when I changed the 
API from `ModuleID` to `const ModuleID &`.  Will fix, thanks!



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:397
+for (StringRef Arg : Args) {
+  if (Arg == "-serialize-diagnostics")
+SerializeDiags = true;

jansvoboda11 wrote:
> I think we should be avoiding ad-hoc command line parsing, it can be 
> incorrect in many ways. Could we move this check somewhere where we have a 
> properly constructed `CompilerInvocation`? I think we could do something like 
> this in `ModuleDeps::getCanonicalCommandLine`:
> 
> ```
> if (!CI.getDiagnosticOpts().DiagnosticSerializationFile.empty())
>   CI.getDiagnosticOpts().DiagnosticSerializationFile
> = LookupModuleOutput(ID, 
> ModuleOutputKind::DiagnosticSerializationFile);
> ```
Yeah, we can do that. I originally avoided this due to it "leaking" whether the 
TU used these outputs into the module command-lines (since the set of callbacks 
would differ), but I suspect in practice that doesn't matter since you're 
unlikely to mix compilations that have and don't have serialized diagnostics. 
To be 100% sound, it will require adding the existence of the outputs to the 
module context hash (not the actual path, just whether there was a diag and/or 
d file at all). I will do the context hash change later if you're okay with it 
- there's nowhere to feed the extra info into `getModuleHash` right now, but I 
was already planning to change the hashing which will make it easier to do.  If 
you think it's critical we could add a parameter to `getModuleHash` temporarily 
to handle it.

I liked your idea to make the callback per-output as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129389

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

[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[Ben Langmuir via Phabricator via cfe-commits]

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

Updates per review

- Switched to a per-output callback
- Removed preserved-args.c test
- Removed error handling that I no longer have a real use for
- Only request .d and .diag paths if they were enabled in the original TU


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

https://reviews.llvm.org/D129389

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
  clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
  clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
  clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
  clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
  clang/test/ClangScanDeps/generate-modules-path-args.c
  clang/test/ClangScanDeps/preserved-args.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -288,11 +288,12 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = GenerateModulesPathArgs
- ? FD.getCommandLine(
-   [&](ModuleID MID) { return lookupPCMPath(MID); })
- : FD.getCommandLineWithoutModulePaths();
-
+ID.CommandLine =
+GenerateModulesPathArgs
+? FD.getCommandLine([&](const ModuleID &MID, ModuleOutputKind MOK) {
+return lookupModuleOutput(MID, MOK);
+  })
+: FD.getCommandLineWithoutModulePaths();
 Inputs.push_back(std::move(ID));
   }
 
@@ -325,7 +326,9 @@
   {"command-line",
GenerateModulesPathArgs
? MD.getCanonicalCommandLine(
- [&](ModuleID MID) { return lookupPCMPath(MID); })
+ [&](const ModuleID &MID, ModuleOutputKind MOK) {
+   return lookupModuleOutput(MID, MOK);
+ })
: MD.getCanonicalCommandLineWithoutModulePaths()},
   };
   OutModules.push_back(std::move(O));
@@ -352,11 +355,22 @@
   }
 
 private:
-  StringRef lookupPCMPath(ModuleID MID) {
+  std::string lookupModuleOutput(const ModuleID &MID, ModuleOutputKind MOK) {
+// Cache the PCM path, since it will be queried repeatedly for each module.
+// The other outputs are only queried once during getCanonicalCommandLine.
 auto PCMPath = PCMPaths.insert({MID, ""});
 if (PCMPath.second)
   PCMPath.first->second = constructPCMPath(MID);
-return PCMPath.first->second;
+switch (MOK) {
+case ModuleOutputKind::ModuleFile:
+  return PCMPath.first->second;
+case ModuleOutputKind::DependencyFile:
+  return PCMPath.first->second + ".d";
+case ModuleOutputKind::DependencyTargets:
+  return ""; // Will get the default target name.
+case ModuleOutputKind::DiagnosticSerializationFile:
+  return PCMPath.first->second + ".diag";
+}
   }
 
   /// Construct a path for the explicitly built PCM.
Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -29,6 +29,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
+// CHECK-NOT:  "-dependency-file"
+// CHECK-NOT:  "-MT"
+// CHECK-NOT:  "-serialize-diagnostic-file"
 // CHECK:],
 // CHECK-NEXT:   "context-hash": "[[HASH_MOD_HEADER:.*]]",
 // CHECK-NEXT:   "file-deps": [
@@ -50,6 +53,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
+// CHECK-NOT:  "-dependency-file"
+// CHECK-NOT:  "-MT"
+// CHECK-NOT:  "-serialize-diagnostic-file"
 // CHECK:],
 // CHECK-NEXT:   "context-hash": "[[HASH_MOD_TU:.*]]",
 // CHECK-NEXT:   "file-deps": [
Index: clang/test/ClangScanDeps/preserved-args.c
===
--- clang/test/ClangScanDeps/preserved-args.c
+++ /dev/null
@@ -1,24 +0,0 @@
-// RUN: rm -rf %t && mkdir %t
-// RUN: cp -r %S/Inputs/preserved-args/* %t
-// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
-
-// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
-// RUN: cat %t/result.json | sed 's:\?:/

[PATCH] D129504: [libclang][ObjC] Inherit availability attribute from containing decls or interface decls

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/tools/libclang/CIndex.cpp:8271
+else if (auto *IMD = dyn_cast(D))
+  CD = IMD->getCategoryDecl();
+else if (auto *ID = dyn_cast(DC))

So this goes Impl -> Class, and CategoryImpl -> Category.  Should it also do 
Category -> Class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129504

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

[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6626f6fec3d3: [clang][deps] Override dependency and 
serialized diag files for modules (authored by benlangmuir).

Changed prior to commit:
  https://reviews.llvm.org/D129389?vs=443755&id=443955#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129389

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
  clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
  clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
  clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
  clang/test/ClangScanDeps/Inputs/removed-args/cdb.json.template
  clang/test/ClangScanDeps/generate-modules-path-args.c
  clang/test/ClangScanDeps/preserved-args.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -288,11 +288,12 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = GenerateModulesPathArgs
- ? FD.getCommandLine(
-   [&](ModuleID MID) { return lookupPCMPath(MID); })
- : FD.getCommandLineWithoutModulePaths();
-
+ID.CommandLine =
+GenerateModulesPathArgs
+? FD.getCommandLine([&](const ModuleID &MID, ModuleOutputKind MOK) {
+return lookupModuleOutput(MID, MOK);
+  })
+: FD.getCommandLineWithoutModulePaths();
 Inputs.push_back(std::move(ID));
   }
 
@@ -325,7 +326,9 @@
   {"command-line",
GenerateModulesPathArgs
? MD.getCanonicalCommandLine(
- [&](ModuleID MID) { return lookupPCMPath(MID); })
+ [&](const ModuleID &MID, ModuleOutputKind MOK) {
+   return lookupModuleOutput(MID, MOK);
+ })
: MD.getCanonicalCommandLineWithoutModulePaths()},
   };
   OutModules.push_back(std::move(O));
@@ -352,11 +355,22 @@
   }
 
 private:
-  StringRef lookupPCMPath(ModuleID MID) {
+  std::string lookupModuleOutput(const ModuleID &MID, ModuleOutputKind MOK) {
+// Cache the PCM path, since it will be queried repeatedly for each module.
+// The other outputs are only queried once during getCanonicalCommandLine.
 auto PCMPath = PCMPaths.insert({MID, ""});
 if (PCMPath.second)
   PCMPath.first->second = constructPCMPath(MID);
-return PCMPath.first->second;
+switch (MOK) {
+case ModuleOutputKind::ModuleFile:
+  return PCMPath.first->second;
+case ModuleOutputKind::DependencyFile:
+  return PCMPath.first->second + ".d";
+case ModuleOutputKind::DependencyTargets:
+  return ""; // Will get the default target name.
+case ModuleOutputKind::DiagnosticSerializationFile:
+  return PCMPath.first->second + ".diag";
+}
   }
 
   /// Construct a path for the explicitly built PCM.
Index: clang/test/ClangScanDeps/removed-args.c
===
--- clang/test/ClangScanDeps/removed-args.c
+++ clang/test/ClangScanDeps/removed-args.c
@@ -29,6 +29,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
+// CHECK-NOT:  "-dependency-file"
+// CHECK-NOT:  "-MT"
+// CHECK-NOT:  "-serialize-diagnostic-file"
 // CHECK:],
 // CHECK-NEXT:   "context-hash": "[[HASH_MOD_HEADER:.*]]",
 // CHECK-NEXT:   "file-deps": [
@@ -50,6 +53,9 @@
 // CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
+// CHECK-NOT:  "-dependency-file"
+// CHECK-NOT:  "-MT"
+// CHECK-NOT:  "-serialize-diagnostic-file"
 // CHECK:],
 // CHECK-NEXT:   "context-hash": "[[HASH_MOD_TU:.*]]",
 // CHECK-NEXT:   "file-deps": [
Index: clang/test/ClangScanDeps/preserved-args.c
===
--- clang/test/ClangScanDeps/preserved-args.c
+++ /dev/null
@@ -1,24 +0,0 @@
-// RUN: rm -rf %t && mkdir %t
-// RUN: cp -r %S/Inputs/preserved-args/* %t
-// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
-
-// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
-// RUN: cat %t/res

[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:341-342
+  !MDC.OriginalInvocation.getDependencyOutputOpts().OutputFile.empty();
+  // FIXME: HadSerializedDiagnostics and HadDependencyFile should be included 
in
+  // the context hash since it can affect the command-line.
   MD.ID.ContextHash = MD.BuildInvocation.getModuleHash();

jansvoboda11 wrote:
> This is a bit unfortunate but I don't see a better alternative.
> 
> Ideally, we would compute the hash with the `.d` and `.dia` paths already 
> filled in. That would ensure the command line we end up reporting to the 
> build system really **does** have the context hash associated with the 
> module. (We'd need to include every field set in `getCanonicalCommandLine()` 
> too.) But for the path lookup, we already need some kind of (currently 
> partial) context hash.
The other things added in getCanonicalCommandLine are currently:
* module map path - I'm planning to include this in my changes to the context 
hash since it is significant exactly how the path is spelled. This is already 
part of the implicit module build's notion of identity.
* pcm paths - these are sound as long as we always get the same paths returned 
in the callback during a build (across separate builds it would be fine to 
change them as long as you're going to rebuild anything whose path changed, and 
anything downstream of that).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129389

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

[PATCH] D129607: [clang][deps] Fix handling of -MT in module command-line

[Ben Langmuir via Phabricator via cfe-commits]

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

Follow-up to 6626f6fec3d3 
, this 
fixes the handling of `-MT`

- If no targets are provided, we need to invent one since `cc1` expects the 
driver to have handled it. The default is to use `-o`, quoting as necessary for 
a make target.
- Fix the splitting for empty string, which was incorrectly treated as `{""}` 
instead of `{}`.
- Add a way to test this behaviour in clang-scan-deps.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129607

Files:
  clang/include/clang/Basic/MakeSupport.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/MakeSupport.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/generate-modules-path-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -196,6 +196,12 @@
 llvm::cl::desc("the module of which the dependencies are to be computed"),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::list ModuleDepTargets(
+"dependency-target",
+llvm::cl::desc("With '-generate-modules-path-args', the names of "
+   "dependency targets for the dependency file"),
+llvm::cl::cat(DependencyScannerCategory));
+
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
@@ -367,7 +373,8 @@
 case ModuleOutputKind::DependencyFile:
   return PCMPath.first->second + ".d";
 case ModuleOutputKind::DependencyTargets:
-  return ""; // Will get the default target name.
+  // Null-separate the list of targets.
+  return join(ModuleDepTargets, StringRef("\0", 1));
 case ModuleOutputKind::DiagnosticSerializationFile:
   return PCMPath.first->second + ".diag";
 }
Index: clang/test/ClangScanDeps/generate-modules-path-args.c
===
--- clang/test/ClangScanDeps/generate-modules-path-args.c
+++ clang/test/ClangScanDeps/generate-modules-path-args.c
@@ -5,6 +5,12 @@
 // RUN: clang-scan-deps -compilation-database %t/cdb.json \
 // RUN:   -format experimental-full -generate-modules-path-args > %t/deps.json
 // RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -generate-modules-path-args -dependency-target foo > %t/deps_mt1.json
+// RUN: cat %t/deps_mt1.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s -check-prefix=DEPS_MT1
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -generate-modules-path-args -dependency-target foo -dependency-target bar > %t/deps_mt2.json
+// RUN: cat %t/deps_mt2.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s -check-prefix=DEPS_MT2
 // RUN: clang-scan-deps -compilation-database %t/cdb_without.json \
 // RUN:   -format experimental-full -generate-modules-path-args > %t/deps_without.json
 // RUN: cat %t/deps_without.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t -check-prefix=WITHOUT %s
@@ -16,10 +22,20 @@
 // CHECK-NEXT: "-cc1"
 // CHECK:  "-serialize-diagnostic-file"
 // CHECK-NEXT: "[[PREFIX]]{{.*}}Mod{{.*}}.diag"
+// CHECK:  "-MT"
+// CHECK-NEXT: "[[PREFIX]]{{.*}}Mod{{.*}}.pcm"
 // CHECK:  "-dependency-file"
 // CHECK-NEXT: "[[PREFIX]]{{.*}}Mod{{.*}}.d"
 // CHECK:],
 
+// DEPS_MT1:  "-MT"
+// DEPS_MT1-NEXT: "foo"
+
+// DEPS_MT2:  "-MT"
+// DEPS_MT2-NEXT: "foo"
+// DEPS_MT2-NEXT: "-MT"
+// DEPS_MT2-NEXT: "bar"
+
 // WITHOUT:  {
 // WITHOUT-NEXT:   "modules": [
 // WITHOUT-NEXT: {
@@ -27,6 +43,7 @@
 // WITHOUT-NEXT: "-cc1"
 // WITHOUT-NOT:  "-serialize-diagnostic-file"
 // WITHOUT-NOT:  "-dependency-file"
+// WITHOUT-NOT:  "-MT"
 // WITHOUT:],
 
 //--- cdb.json.template
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
 
+#include "clang/Basic/MakeSupport.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWor

[PATCH] D129607: [clang][deps] Fix handling of -MT in module command-line

[Ben Langmuir via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3ce78cbd2392: [clang][deps] Fix handling of -MT in module 
command-line (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129607

Files:
  clang/include/clang/Basic/MakeSupport.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/MakeSupport.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/generate-modules-path-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -196,6 +196,12 @@
 llvm::cl::desc("the module of which the dependencies are to be computed"),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::list ModuleDepTargets(
+"dependency-target",
+llvm::cl::desc("With '-generate-modules-path-args', the names of "
+   "dependency targets for the dependency file"),
+llvm::cl::cat(DependencyScannerCategory));
+
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
@@ -367,7 +373,8 @@
 case ModuleOutputKind::DependencyFile:
   return PCMPath.first->second + ".d";
 case ModuleOutputKind::DependencyTargets:
-  return ""; // Will get the default target name.
+  // Null-separate the list of targets.
+  return join(ModuleDepTargets, StringRef("\0", 1));
 case ModuleOutputKind::DiagnosticSerializationFile:
   return PCMPath.first->second + ".diag";
 }
Index: clang/test/ClangScanDeps/generate-modules-path-args.c
===
--- clang/test/ClangScanDeps/generate-modules-path-args.c
+++ clang/test/ClangScanDeps/generate-modules-path-args.c
@@ -5,6 +5,12 @@
 // RUN: clang-scan-deps -compilation-database %t/cdb.json \
 // RUN:   -format experimental-full -generate-modules-path-args > %t/deps.json
 // RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -generate-modules-path-args -dependency-target foo > %t/deps_mt1.json
+// RUN: cat %t/deps_mt1.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s -check-prefix=DEPS_MT1
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN:   -format experimental-full -generate-modules-path-args -dependency-target foo -dependency-target bar > %t/deps_mt2.json
+// RUN: cat %t/deps_mt2.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s -check-prefix=DEPS_MT2
 // RUN: clang-scan-deps -compilation-database %t/cdb_without.json \
 // RUN:   -format experimental-full -generate-modules-path-args > %t/deps_without.json
 // RUN: cat %t/deps_without.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t -check-prefix=WITHOUT %s
@@ -16,10 +22,20 @@
 // CHECK-NEXT: "-cc1"
 // CHECK:  "-serialize-diagnostic-file"
 // CHECK-NEXT: "[[PREFIX]]{{.*}}Mod{{.*}}.diag"
+// CHECK:  "-MT"
+// CHECK-NEXT: "[[PREFIX]]{{.*}}Mod{{.*}}.pcm"
 // CHECK:  "-dependency-file"
 // CHECK-NEXT: "[[PREFIX]]{{.*}}Mod{{.*}}.d"
 // CHECK:],
 
+// DEPS_MT1:  "-MT"
+// DEPS_MT1-NEXT: "foo"
+
+// DEPS_MT2:  "-MT"
+// DEPS_MT2-NEXT: "foo"
+// DEPS_MT2-NEXT: "-MT"
+// DEPS_MT2-NEXT: "bar"
+
 // WITHOUT:  {
 // WITHOUT-NEXT:   "modules": [
 // WITHOUT-NEXT: {
@@ -27,6 +43,7 @@
 // WITHOUT-NEXT: "-cc1"
 // WITHOUT-NOT:  "-serialize-diagnostic-file"
 // WITHOUT-NOT:  "-dependency-file"
+// WITHOUT-NOT:  "-MT"
 // WITHOUT:],
 
 //--- cdb.json.template
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
 
+#include "clang/Basic/MakeSupport.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
@@ -112,7 +113,7 @@
 
 static std::vector splitString(std::string S, char Separator) {
   SmallVector Segments;
-  StringRef(S).split(Segments, Separator);
+  StringRef(S).split(Segments, Separator, /*MaxSplit=*/-1, /*KeepEmpty=*/false);
   std::vector Result;
   Result.reserve(Segments.size());
   for (StringRef Segment : Segments)
@@ -135,10 +136,17 @@
 CI.getDiagnosticOpts().DiagnosticSerializationFile =
 LookupModuleOutput(ID, ModuleOutputKin

[PATCH] D129884: [clang][deps] Include canonical invocation in ContextHash

[Ben Langmuir via Phabricator via cfe-commits]

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

The "strict context hash" is insufficient to identify module
dependencies during scanning, leading to different module build commands
being produced for a single module, and non-deterministically choosing
between them. This commit switches to hashing the canonicalized
`CompilerInvocation` of the module. By hashing the invocation we are
converting these from correctness issues to performance issues, and we
can then incrementally improve our ability to canonicalize
command-lines.

This change can cause a regression in the number of modules needed. Of
the 4 projects I tested, 3 had no regression, but 1, which was
clang+llvm itself, had a 66% regression in number of modules (4%
regression in total invocations). This is almost entirely due to
differences between -W options across targets.  Of this, 25% of the
additional modules are system modules, which we could avoid if we
canonicalized -W options when -Wsystem-headers is not present --
unfortunately this is non-trivial due to some warnings being enabled in
system headers by default. The rest of the additional modules are mostly
real differences in potential warnings, reflecting incorrect behaviour
in the current scanner.

There were also a couple of differences due to `-DFOO`
`-fmodule-ignore-macro=FOO`, which I fixed here.

Since the output paths for the module depend on its context hash, we
hash the invocation before filling in outputs, and rely on the build
system to always return the same output paths for a given module.

Note: since the scanner itself uses an implicit modules build, there can
still be non-determinism, but it will now present as different
module+hashes rather than different command-lines for the same
module+hash.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129884

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-module-map-path.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c

Index: clang/test/ClangScanDeps/modules-context-hash-warnings.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-context-hash-warnings.c
@@ -0,0 +1,74 @@
+// 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 -j 1 \
+// RUN:   -format experimental-full > %t/deps.json
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK:  {
+// CHECK:"command-line": [
+// CHECK:  "-Wall"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH1:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NOT:  "-Wall"
+// CHECK:]
+// CHECK:"context-hash": "[[HASH2:.*]]"
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH1]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK:  "-Wall"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu1.c"
+// CHECK-NEXT: }
+// CHECK-NEXT: {
+// CHECK:"clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "[[HASH2]]"
+// CHECK-NEXT:"module-name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   "command-line": [
+// CHECK-NOT:  "-Wall"
+// CHECK:]
+// CHECK:"input-file": "{{.*}}tu2.c"
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -Wall -fsyntax-only DIR/tu1.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+"file": "DIR/tu1.c"
+  },
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/tu2.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+"file": "DIR/tu2.c"
+  },
+]
+
+//--- module.modulemap
+module Mod { header "Mod.h" }
+
+//--- Mod.h
+
+//--- tu1.c
+#include "Mod.h"
+
+//--- tu2.c
+#include "Mod.h"
Index: clang/test/ClangScanDeps/modules-context-hash-module-map-path.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-context-hash-module-map-

[PATCH] D129504: [libclang][ObjC] Inherit availability attribute from containing decls or interface decls

[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, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129504

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

[PATCH] D121733: Clean pathnames in FileManager.

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:218
+  llvm::sys::path::remove_dots(CleanFilename, /*remove_dot_dot=*/false);
+  Filename = CleanFilename;
+

ppluzhnikov wrote:
> kadircet wrote:
> > this is actually breaking the [contract of 
> > FileEntryRef](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileEntry.h#L58-L59),
> >  is this the point?
> > 
> > ```
> > /// A reference to a \c FileEntry that includes the name of the file as it 
> > was
> > /// accessed by the FileManager's client.
> > ```
> > 
> > I don't see mention of this contract being changed explicitly anywhere, if 
> > so can you mention it in the commit message and also update the 
> > documentation? (the same applies to DirectoryEntryRef changes as well)
> > 
> > I am not able to take a detailed look at this at the moment, but this 
> > doesn't feel like a safe change for all clients. Because people might be 
> > relying on this contract without explicitly testing the behaviour for "./" 
> > in the filenames. So tests passing (especially when you're just updating 
> > them to not have `./`) might not be implying it's safe.
> I chased this comment down to commit 4dc5573acc0d2e7c59d8bac2543eb25cb4b32984.
> 
> The commit message says:
> 
>This commit introduces a parallel API to FileManager's getFile: 
> getFileEntryRef, which returns
> a reference to the FileEntry, and the name that was used to access the 
> file. In the case of
> a VFS with 'use-external-names', the FileEntyRef contains the external 
> name of the file,
> not the filename that was used to access it.
> 
> So it appears that the comment itself is not quite correct.
> 
> It's also a bit ambiguous (I think) -- "name used to access the file"
> could be interpreted as the name which clang itself used to access
> the file, and not necessarily the name client used.
> 
> > people might be relying on this contract without explicitly testing the 
> > behaviour for "./" in the filenames.
> 
> That's a possibility.
> 
> I am not sure how to evaluate the benefit of this patch (avoiding noise 
> everywhere) vs. possibly breaking clients.
While the comment is not currently accurate due to use-external-names, [[ 
https://github.com/llvm/llvm-project/blob/47be07074a73bd469b16af440923e3cf3b6b3f10/clang/lib/Basic/FileManager.cpp#L319
 | the goal is to eliminate that behaviour ]] and have a separate API for 
getting the external names.  It's maybe still fine to eliminate `./` here, I 
don't have a strong opinion.

> It's also a bit ambiguous (I think) -- "name used to access the file"
could be interpreted as the name which clang itself used to access
the file, and not necessarily the name client used.

It means the name passed to getFileRef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121733

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

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir removed a reviewer: benlangmuir.
benlangmuir added a comment.

Removing myself as reviewer since I no longer work in this area.  Someone who 
is more involved with llvm Support/ADT should review this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92480

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

[PATCH] D104348: [index] Fix performance regression with indexing macros

[Ben Langmuir via Phabricator via cfe-commits]

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

When using FileIndexRecord with macros, symbol references can be seen out of 
source order, which was causing a regression to insert the symbols into a 
vector. Instead, we now lazily sort the vector. The impact is small on most 
code, but in very large files with many macro references (M) near the beginning 
of the file followed by many decl references (D) it was O(M*D). A particularly 
bad protobuf-generated header was observed with a 100% regression in practice.

  

rdar://78628133


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104348

Files:
  clang/lib/Index/FileIndexRecord.cpp
  clang/lib/Index/FileIndexRecord.h


Index: clang/lib/Index/FileIndexRecord.h
===
--- clang/lib/Index/FileIndexRecord.h
+++ clang/lib/Index/FileIndexRecord.h
@@ -27,14 +27,13 @@
 private:
   FileID FID;
   bool IsSystem;
-  std::vector Decls;
+  mutable bool IsSorted = false;
+  mutable std::vector Decls;
 
 public:
   FileIndexRecord(FileID FID, bool IsSystem) : FID(FID), IsSystem(IsSystem) {}
 
-  ArrayRef getDeclOccurrencesSortedByOffset() const {
-return Decls;
-  }
+  ArrayRef getDeclOccurrencesSortedByOffset() const;
 
   FileID getFileID() const { return FID; }
   bool isSystem() const { return IsSystem; }
Index: clang/lib/Index/FileIndexRecord.cpp
===
--- clang/lib/Index/FileIndexRecord.cpp
+++ clang/lib/Index/FileIndexRecord.cpp
@@ -17,23 +17,16 @@
 using namespace clang;
 using namespace clang::index;
 
-static void addOccurrence(std::vector &Decls,
-  DeclOccurrence Info) {
-  auto IsNextOccurence = [&]() -> bool {
-if (Decls.empty())
-  return true;
-auto &Last = Decls.back();
-return Last.Offset < Info.Offset;
-  };
-
-  if (IsNextOccurence()) {
-Decls.push_back(std::move(Info));
-return;
+ArrayRef
+FileIndexRecord::getDeclOccurrencesSortedByOffset() const {
+  if (!IsSorted) {
+llvm::stable_sort(Decls,
+  [](const DeclOccurrence &A, const DeclOccurrence &B) {
+return A.Offset < B.Offset;
+  });
+IsSorted = true;
   }
-
-  // We keep Decls in order as we need to access them in this order in all 
cases.
-  auto It = llvm::upper_bound(Decls, Info);
-  Decls.insert(It, std::move(Info));
+  return Decls;
 }
 
 void FileIndexRecord::addDeclOccurence(SymbolRoleSet Roles, unsigned Offset,
@@ -41,13 +34,15 @@
ArrayRef Relations) {
   assert(D->isCanonicalDecl() &&
  "Occurrences should be associated with their canonical decl");
-  addOccurrence(Decls, DeclOccurrence(Roles, Offset, D, Relations));
+  IsSorted = false;
+  Decls.emplace_back(Roles, Offset, D, Relations);
 }
 
 void FileIndexRecord::addMacroOccurence(SymbolRoleSet Roles, unsigned Offset,
 const IdentifierInfo *Name,
 const MacroInfo *MI) {
-  addOccurrence(Decls, DeclOccurrence(Roles, Offset, Name, MI));
+  IsSorted = false;
+  Decls.emplace_back(Roles, Offset, Name, MI);
 }
 
 void FileIndexRecord::removeHeaderGuardMacros() {


Index: clang/lib/Index/FileIndexRecord.h
===
--- clang/lib/Index/FileIndexRecord.h
+++ clang/lib/Index/FileIndexRecord.h
@@ -27,14 +27,13 @@
 private:
   FileID FID;
   bool IsSystem;
-  std::vector Decls;
+  mutable bool IsSorted = false;
+  mutable std::vector Decls;
 
 public:
   FileIndexRecord(FileID FID, bool IsSystem) : FID(FID), IsSystem(IsSystem) {}
 
-  ArrayRef getDeclOccurrencesSortedByOffset() const {
-return Decls;
-  }
+  ArrayRef getDeclOccurrencesSortedByOffset() const;
 
   FileID getFileID() const { return FID; }
   bool isSystem() const { return IsSystem; }
Index: clang/lib/Index/FileIndexRecord.cpp
===
--- clang/lib/Index/FileIndexRecord.cpp
+++ clang/lib/Index/FileIndexRecord.cpp
@@ -17,23 +17,16 @@
 using namespace clang;
 using namespace clang::index;
 
-static void addOccurrence(std::vector &Decls,
-  DeclOccurrence Info) {
-  auto IsNextOccurence = [&]() -> bool {
-if (Decls.empty())
-  return true;
-auto &Last = Decls.back();
-return Last.Offset < Info.Offset;
-  };
-
-  if (IsNextOccurence()) {
-Decls.push_back(std::move(Info));
-return;
+ArrayRef
+FileIndexRecord::getDeclOccurrencesSortedByOffset() const {
+  if (!IsSorted) {
+llvm::stable_sort(Decls,
+  [](const DeclOccurrence &A, const DeclOccurrence &B) {
+return A.Offset < B.Offset;
+  });
+IsSorted = true;
   }
-
-  

[PATCH] D104348: [index] Fix performance regression with indexing macros

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir closed this revision.
benlangmuir added a comment.

Pushed rG773ad55a393f 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104348

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

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

(catching up after I was away last week) I agree with @dexonsmith's analysis 
above. One historical note:

> As a heuristic on top of (4), "last-ref" (before this patch) probably gets 
> the answer the user expects more often than "first-ref" (this patch) does, 
> which I assume is why it was originally implemented.

It was originally implemented this way because it works out more often for 
module-related files, and vfs overlays; debug info and preprocessor output of 
this kind was not considered. This is part of a workaround for lack of proper 
FileEntryRef support across the compiler that we should unwind over time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135220

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

[PATCH] D135634: [clang][modules] Serialize VFS overlay paths into PCMs

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a subscriber: bnbarham.
benlangmuir added a comment.

I think we should deduplicate the vfs overlays if the same ivfsoverlay is 
specified in both the pcm and the command-line.

@bnbarham any concern about overlay vs chaining behaviour here? I remember you 
looking at that a while ago.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135634

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

[PATCH] D136717: [clang] Move getenv call for SOURCE_DATE_EPOCH out of frontend 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 rG211f5af38af1: [clang] Move getenv call for SOURCE_DATE_EPOCH 
out of frontend NFC (authored by benlangmuir).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D136717?vs=470635&id=470904#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136717

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/SOURCE_DATE_EPOCH.c
  clang/test/Preprocessor/SOURCE_DATE_EPOCH.c

Index: clang/test/Preprocessor/SOURCE_DATE_EPOCH.c
===
--- clang/test/Preprocessor/SOURCE_DATE_EPOCH.c
+++ clang/test/Preprocessor/SOURCE_DATE_EPOCH.c
@@ -1,10 +1,10 @@
-// RUN: env SOURCE_DATE_EPOCH=0 %clang_cc1 -E %s | FileCheck %s --check-prefix=19700101
+// RUN: %clang_cc1 -source-date-epoch 0 -E %s | FileCheck %s --check-prefix=19700101
 
 // 19700101:  const char date[] = "Jan  1 1970";
 // 19700101-NEXT: const char time[] = "00:00:00";
 // 19700101-NEXT: const char timestamp[] = "Thu Jan  1 00:00:00 1970";
 
-// RUN: env SOURCE_DATE_EPOCH=2147483647 %clang_cc1 -E -Wdate-time %s 2>&1 | FileCheck %s --check-prefix=Y2038
+// RUN: %clang_cc1 -source-date-epoch 2147483647 -E -Wdate-time %s 2>&1 | FileCheck %s --check-prefix=Y2038
 
 // Y2038:  warning: expansion of date or time macro is not reproducible [-Wdate-time]
 // Y2038:  const char date[] = "Jan 19 2038";
@@ -12,18 +12,18 @@
 // Y2038-NEXT: const char timestamp[] = "Tue Jan 19 03:14:07 2038";
 
 /// Test a large timestamp if the system uses 64-bit time_t and known to support large timestamps.
-// RUN: %if !system-windows && clang-target-64-bits %{ env SOURCE_DATE_EPOCH=253402300799 %clang_cc1 -E -Wdate-time %s 2>&1 | FileCheck %s --check-prefix=1231 %}
+// RUN: %if !system-windows && clang-target-64-bits %{ %clang_cc1 -source-date-epoch 253402300799 -E -Wdate-time %s 2>&1 | FileCheck %s --check-prefix=1231 %}
 
 // 1231:  warning: expansion of date or time macro is not reproducible [-Wdate-time]
 // 1231:  const char date[] = "Dec 31 ";
 // 1231-NEXT: const char time[] = "23:59:59";
 // 1231-NEXT: const char timestamp[] = "Fri Dec 31 23:59:59 ";
 
-// RUN: env SOURCE_DATE_EPOCH=253402300800 not %clang_cc1 -E %s 2>&1 | FileCheck %s --check-prefix=TOOBIG
+// RUN: not %clang_cc1 -source-date-epoch 253402300800 -E %s 2>&1 | FileCheck %s --check-prefix=TOOBIG
 
 // TOOBIG: error: environment variable 'SOURCE_DATE_EPOCH' ('253402300800') must be a non-negative decimal integer <= {{(2147483647|253402300799)}}
 
-// RUN: env SOURCE_DATE_EPOCH=0x0 not %clang_cc1 -E %s 2>&1 | FileCheck %s --check-prefix=NOTDECIMAL
+// RUN: not %clang_cc1 -source-date-epoch 0x0 -E %s 2>&1 | FileCheck %s --check-prefix=NOTDECIMAL
 
 // NOTDECIMAL: error: environment variable 'SOURCE_DATE_EPOCH' ('0x0') must be a non-negative decimal integer <= {{(2147483647|253402300799)}}
 
Index: clang/test/Driver/SOURCE_DATE_EPOCH.c
===
--- /dev/null
+++ clang/test/Driver/SOURCE_DATE_EPOCH.c
@@ -0,0 +1,5 @@
+// RUN: %clang -E %s -### 2>&1 | FileCheck %s -check-prefix=NO_EPOCH
+// NO_EPOCH-NOT: "-source-date-epoch"
+
+// RUN: env SOURCE_DATE_EPOCH=123 %clang -E %s -### 2>&1 | FileCheck %s
+// CHECK: "-source-date-epoch" "123"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -4227,6 +4227,9 @@
   for (const auto &RF : Opts.RemappedFiles)
 GenerateArg(Args, OPT_remap_file, RF.first + ";" + RF.second, SA);
 
+  if (Opts.SourceDateEpoch)
+GenerateArg(Args, OPT_source_date_epoch, Twine(*Opts.SourceDateEpoch), SA);
+
   // Don't handle LexEditorPlaceholders. It is implied by the action that is
   // generated elsewhere.
 }
@@ -4309,14 +4312,15 @@
 Opts.addRemappedFile(Split.first, Split.second);
   }
 
-  if (const char *Epoch = std::getenv("SOURCE_DATE_EPOCH")) {
+  if (const Arg *A = Args.getLastArg(OPT_source_date_epoch)) {
+StringRef Epoch = A->getValue();
 // SOURCE_DATE_EPOCH, if specified, must be a non-negative decimal integer.
 // On time64 systems, pick 253402300799 (the UNIX timestamp of
 // -12-31T23:59:59Z) as the upper bound.
 const uint64_t MaxTimestamp =
 std::min(std::numeric_limits::max(), 253402300799);
 uint64_t V;
-if (StringRef(Epoch).getAsInteger(10, V) || V > MaxTimestamp) {
+if (Epoch.getAsInteger(10, V) || V > MaxTimestamp) {
   Diags.Report(diag::err_fe_invalid_source_date_epoch)
   << Epoch << MaxTimestamp;

[PATCH] D136717: [clang] Move getenv call for SOURCE_DATE_EPOCH out of frontend NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4324
+if (Epoch.getAsInteger(10, V) || V > MaxTimestamp) {
   Diags.Report(diag::err_fe_invalid_source_date_epoch)
   << Epoch << MaxTimestamp;

benlangmuir wrote:
> MaskRay wrote:
> > Is it worth making `err_fe_invalid_source_date_epoch` a driver diagnostic? 
> > I think driver validation is more common.
> I wasn't sure if we could just move the validation to the driver or if we 
> would end up duplicating it -- how bad is it to have a value that's too 
> large? If it could cause UB or something I wouldn't want to remove the check 
> from the frontend.
I kept the validation in the frontend for now but happy to iterate on this if 
you'd like.



Comment at: clang/test/Driver/SOURCE_DATE_EPOCH.c:2
+// RUN: %clang -E %s -### 2>&1 | FileCheck %s -check-prefix=NO_EPOCH
+// NO_EPOCH-NOT: source-date-epoch
+

MaskRay wrote:
> In case someone put the build directory under a directory with the string 
> `-source-date-epoch`, even if it is highly unlikely.
Fair point, done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136717

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

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added a reviewer: steven_wu.
Herald added a subscriber: hiraditya.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, MaskRay.
Herald added projects: clang, LLVM.

Avoid calling getenv in the MC layer and let the clang driver do it so that it 
is reflected in the command-line as an -mllvm option.

rdar://101558354


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136888

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/AS_SECURE_LOG_FILE.s
  llvm/include/llvm/MC/MCContext.h
  llvm/lib/MC/MCContext.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/test/MC/AsmParser/secure_log_unique.s

Index: llvm/test/MC/AsmParser/secure_log_unique.s
===
--- llvm/test/MC/AsmParser/secure_log_unique.s
+++ llvm/test/MC/AsmParser/secure_log_unique.s
@@ -1,6 +1,6 @@
 // RUN: rm -f %t
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file-name=%t -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file-name=%t -triple x86_64-apple-darwin %s
 // RUN: FileCheck --input-file=%t %s
 .secure_log_unique "foobar"
 
Index: llvm/lib/MC/MCParser/DarwinAsmParser.cpp
===
--- llvm/lib/MC/MCParser/DarwinAsmParser.cpp
+++ llvm/lib/MC/MCParser/DarwinAsmParser.cpp
@@ -767,8 +767,8 @@
 return Error(IDLoc, ".secure_log_unique specified multiple times");
 
   // Get the secure log path.
-  const char *SecureLogFile = getContext().getSecureLogFile();
-  if (!SecureLogFile)
+  StringRef SecureLogFile = getContext().getSecureLogFile();
+  if (SecureLogFile.empty())
 return Error(IDLoc, ".secure_log_unique used but AS_SECURE_LOG_FILE "
  "environment variable unset.");
 
@@ -776,7 +776,7 @@
   raw_fd_ostream *OS = getContext().getSecureLog();
   if (!OS) {
 std::error_code EC;
-auto NewOS = std::make_unique(StringRef(SecureLogFile), EC,
+auto NewOS = std::make_unique(SecureLogFile, EC,
   sys::fs::OF_Append |
   sys::fs::OF_TextWithCRLF);
 if (EC)
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -59,11 +59,10 @@
 
 using namespace llvm;
 
-static cl::opt
+static cl::opt
 AsSecureLogFileName("as-secure-log-file-name",
 cl::desc("As secure log file name (initialized from "
- "AS_SECURE_LOG_FILE env variable)"),
-cl::init(getenv("AS_SECURE_LOG_FILE")), cl::Hidden);
+ "AS_SECURE_LOG_FILE env variable)"), cl::Hidden);
 
 static void defaultDiagHandler(const SMDiagnostic &SMD, bool, const SourceMgr &,
std::vector &) {
Index: llvm/include/llvm/MC/MCContext.h
===
--- llvm/include/llvm/MC/MCContext.h
+++ llvm/include/llvm/MC/MCContext.h
@@ -178,7 +178,7 @@
   /// The file name of the log file from the environment variable
   /// AS_SECURE_LOG_FILE.  Which must be set before the .secure_log_unique
   /// directive is used or it is an error.
-  char *SecureLogFile;
+  std::string SecureLogFile;
   /// The stream that gets written to for the .secure_log_unique directive.
   std::unique_ptr SecureLog;
   /// Boolean toggled when .secure_log_unique / .secure_log_reset is seen to
@@ -828,7 +828,7 @@
 
   /// @}
 
-  char *getSecureLogFile() { return SecureLogFile; }
+  StringRef getSecureLogFile() { return SecureLogFile; }
   raw_fd_ostream *getSecureLog() { return SecureLog.get(); }
 
   void setSecureLog(std::unique_ptr Value) {
Index: clang/test/Driver/AS_SECURE_LOG_FILE.s
===
--- /dev/null
+++ clang/test/Driver/AS_SECURE_LOG_FILE.s
@@ -0,0 +1,3 @@
+// RUN: env AS_SECURE_LOG_FILE=%t %clang -target x86_64-apple-darwin -c %s -o %t.o -### 2>&1 | FileCheck %s -DLOG_FILE=%t
+// CHECK: "-cc1as"
+// CHECK-SAME: "-mllvm" "-as-secure-log-file-name=[[LOG_FILE]]"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2737,6 +2737,12 @@
   if (C.getDriver().embedBitcodeEnabled() ||
   C.getDriver().embedBitcodeMarkerOnly())
 Args.AddLastArg(CmdArgs, options::OPT_fembed_bitcode_EQ);
+
+  if (const char *AsSecureLogFile = getenv("AS_SECURE_LOG_FILE")) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString(Twine("-as-secure-log-file-name=") +
+ AsSecureLogFile));
+  }
 }
 
 static void 

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: llvm/lib/MC/MCContext.cpp:62
 
-static cl::opt
+static cl::opt
 AsSecureLogFileName("as-secure-log-file-name",

Interestingly, `opt` doesn't have a parser; this code only worked with 
the default value before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136888

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

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: llvm/lib/MC/MCContext.cpp:62
 
-static cl::opt
+static cl::opt
 AsSecureLogFileName("as-secure-log-file-name",

steven_wu wrote:
> benlangmuir wrote:
> > Interestingly, `opt` doesn't have a parser; this code only worked 
> > with the default value before.
> ha, maybe we should just deprecate the directive since this hasn't been 
> working for more than 6 years!
It works if you use the environment variable because that feeds directly into 
the default value for the cl::opt. It's only if you actually tried to pass 
`-mllvm -as-secure-log-file-name ...` it would fail to parse the option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136888

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

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 471351.
benlangmuir added a comment.
Herald added a subscriber: ormris.

- Updated to use cc1 option in clang
- Moved the llvm-mc option to a central location so it can be used when 
initialized MCTargetOptions
- Added tests to cover cc1 and cc1as since each one passes in the string to MC 
in a slightly different way (CodeGenOpts vs. AssemblerInvocation)


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

https://reviews.llvm.org/D136888

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/as-secure-log-file.c
  clang/test/Driver/AS_SECURE_LOG_FILE.s
  clang/test/Misc/cc1as-as-secure-log-file.s
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/include/llvm/MC/MCTargetOptionsCommandFlags.h
  llvm/lib/MC/MCContext.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
  llvm/test/MC/AsmParser/secure_log_unique.s

Index: llvm/test/MC/AsmParser/secure_log_unique.s
===
--- llvm/test/MC/AsmParser/secure_log_unique.s
+++ llvm/test/MC/AsmParser/secure_log_unique.s
@@ -1,6 +1,6 @@
 // RUN: rm -f %t
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file %t -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file %t -triple x86_64-apple-darwin %s
 // RUN: FileCheck --input-file=%t %s
 .secure_log_unique "foobar"
 
Index: llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
===
--- llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
+++ llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
@@ -45,6 +45,7 @@
 MCOPT(bool, NoDeprecatedWarn)
 MCOPT(bool, NoTypeCheck)
 MCOPT(std::string, ABIName)
+MCOPT(std::string, AsSecureLogFile)
 
 llvm::mc::RegisterMCTargetOptionsFlags::RegisterMCTargetOptionsFlags() {
 #define MCBINDOPT(NAME)\
@@ -114,6 +115,10 @@
   cl::init(""));
   MCBINDOPT(ABIName);
 
+  static cl::opt AsSecureLogFile(
+  "as-secure-log-file", cl::desc("As secure log file name"), cl::Hidden);
+  MCBINDOPT(AsSecureLogFile);
+
 #undef MCBINDOPT
 }
 
@@ -130,6 +135,7 @@
   Options.MCNoDeprecatedWarn = getNoDeprecatedWarn();
   Options.MCNoTypeCheck = getNoTypeCheck();
   Options.EmitDwarfUnwind = getEmitDwarfUnwind();
+  Options.AsSecureLogFile = getAsSecureLogFile();
 
   return Options;
 }
Index: llvm/lib/MC/MCParser/DarwinAsmParser.cpp
===
--- llvm/lib/MC/MCParser/DarwinAsmParser.cpp
+++ llvm/lib/MC/MCParser/DarwinAsmParser.cpp
@@ -767,8 +767,8 @@
 return Error(IDLoc, ".secure_log_unique specified multiple times");
 
   // Get the secure log path.
-  const char *SecureLogFile = getContext().getSecureLogFile();
-  if (!SecureLogFile)
+  StringRef SecureLogFile = getContext().getSecureLogFile();
+  if (SecureLogFile.empty())
 return Error(IDLoc, ".secure_log_unique used but AS_SECURE_LOG_FILE "
  "environment variable unset.");
 
@@ -776,9 +776,8 @@
   raw_fd_ostream *OS = getContext().getSecureLog();
   if (!OS) {
 std::error_code EC;
-auto NewOS = std::make_unique(StringRef(SecureLogFile), EC,
-  sys::fs::OF_Append |
-  sys::fs::OF_TextWithCRLF);
+auto NewOS = std::make_unique(
+SecureLogFile, EC, sys::fs::OF_Append | sys::fs::OF_TextWithCRLF);
 if (EC)
return Error(IDLoc, Twine("can't open secure log file: ") +
SecureLogFile + " (" + EC.message() + ")");
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -59,12 +59,6 @@
 
 using namespace llvm;
 
-static cl::opt
-AsSecureLogFileName("as-secure-log-file-name",
-cl::desc("As secure log file name (initialized from "
- "AS_SECURE_LOG_FILE env variable)"),
-cl::init(getenv("AS_SECURE_LOG_FILE")), cl::Hidden);
-
 static void defaultDiagHandler(const SMDiagnostic &SMD, bool, const SourceMgr &,
std::vector &) {
   SMD.print(nullptr, errs());
@@ -80,7 +74,7 @@
   InlineAsmUsedLabelNames(Allocator),
   CurrentDwarfLoc(0, 0, 0, DWARF2_FLAG_IS_STMT, 0, 0),
   AutoReset(DoAutoReset), TargetOptions(TargetOpts) {
-  SecureLogFile = AsSecureLogFileName;
+  SecureLogFile = TargetOptions ? TargetOptions->AsSecureLogFile : "";
 
   if (SrcMgr && SrcMgr->getNumBuffers())
 MainFileName = std::string(SrcMgr->getMemoryBuffer(SrcMgr->getMainFileID())

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

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

- --target=, remove -o per review
- Use "log_file" as path to try to avoid Windows path issue in driver test


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

https://reviews.llvm.org/D136888

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/as-secure-log-file.c
  clang/test/Driver/AS_SECURE_LOG_FILE.s
  clang/test/Misc/cc1as-as-secure-log-file.s
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/include/llvm/MC/MCTargetOptionsCommandFlags.h
  llvm/lib/MC/MCContext.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
  llvm/test/MC/AsmParser/secure_log_unique.s

Index: llvm/test/MC/AsmParser/secure_log_unique.s
===
--- llvm/test/MC/AsmParser/secure_log_unique.s
+++ llvm/test/MC/AsmParser/secure_log_unique.s
@@ -1,6 +1,6 @@
 // RUN: rm -f %t
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file %t -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file %t -triple x86_64-apple-darwin %s
 // RUN: FileCheck --input-file=%t %s
 .secure_log_unique "foobar"
 
Index: llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
===
--- llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
+++ llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
@@ -45,6 +45,7 @@
 MCOPT(bool, NoDeprecatedWarn)
 MCOPT(bool, NoTypeCheck)
 MCOPT(std::string, ABIName)
+MCOPT(std::string, AsSecureLogFile)
 
 llvm::mc::RegisterMCTargetOptionsFlags::RegisterMCTargetOptionsFlags() {
 #define MCBINDOPT(NAME)\
@@ -114,6 +115,10 @@
   cl::init(""));
   MCBINDOPT(ABIName);
 
+  static cl::opt AsSecureLogFile(
+  "as-secure-log-file", cl::desc("As secure log file name"), cl::Hidden);
+  MCBINDOPT(AsSecureLogFile);
+
 #undef MCBINDOPT
 }
 
@@ -130,6 +135,7 @@
   Options.MCNoDeprecatedWarn = getNoDeprecatedWarn();
   Options.MCNoTypeCheck = getNoTypeCheck();
   Options.EmitDwarfUnwind = getEmitDwarfUnwind();
+  Options.AsSecureLogFile = getAsSecureLogFile();
 
   return Options;
 }
Index: llvm/lib/MC/MCParser/DarwinAsmParser.cpp
===
--- llvm/lib/MC/MCParser/DarwinAsmParser.cpp
+++ llvm/lib/MC/MCParser/DarwinAsmParser.cpp
@@ -767,8 +767,8 @@
 return Error(IDLoc, ".secure_log_unique specified multiple times");
 
   // Get the secure log path.
-  const char *SecureLogFile = getContext().getSecureLogFile();
-  if (!SecureLogFile)
+  StringRef SecureLogFile = getContext().getSecureLogFile();
+  if (SecureLogFile.empty())
 return Error(IDLoc, ".secure_log_unique used but AS_SECURE_LOG_FILE "
  "environment variable unset.");
 
@@ -776,9 +776,8 @@
   raw_fd_ostream *OS = getContext().getSecureLog();
   if (!OS) {
 std::error_code EC;
-auto NewOS = std::make_unique(StringRef(SecureLogFile), EC,
-  sys::fs::OF_Append |
-  sys::fs::OF_TextWithCRLF);
+auto NewOS = std::make_unique(
+SecureLogFile, EC, sys::fs::OF_Append | sys::fs::OF_TextWithCRLF);
 if (EC)
return Error(IDLoc, Twine("can't open secure log file: ") +
SecureLogFile + " (" + EC.message() + ")");
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -59,12 +59,6 @@
 
 using namespace llvm;
 
-static cl::opt
-AsSecureLogFileName("as-secure-log-file-name",
-cl::desc("As secure log file name (initialized from "
- "AS_SECURE_LOG_FILE env variable)"),
-cl::init(getenv("AS_SECURE_LOG_FILE")), cl::Hidden);
-
 static void defaultDiagHandler(const SMDiagnostic &SMD, bool, const SourceMgr &,
std::vector &) {
   SMD.print(nullptr, errs());
@@ -80,7 +74,7 @@
   InlineAsmUsedLabelNames(Allocator),
   CurrentDwarfLoc(0, 0, 0, DWARF2_FLAG_IS_STMT, 0, 0),
   AutoReset(DoAutoReset), TargetOptions(TargetOpts) {
-  SecureLogFile = AsSecureLogFileName;
+  SecureLogFile = TargetOptions ? TargetOptions->AsSecureLogFile : "";
 
   if (SrcMgr && SrcMgr->getNumBuffers())
 MainFileName = std::string(SrcMgr->getMemoryBuffer(SrcMgr->getMainFileID())
Index: llvm/include/llvm/MC/MCTargetOptionsCommandFlags.h
===
--- llvm/include/llvm/MC/MCTargetOptionsCommandFlags.h
+++ llvm/include/llvm/MC/MCTa

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: llvm/lib/MC/MCContext.cpp:77
   AutoReset(DoAutoReset), TargetOptions(TargetOpts) {
-  SecureLogFile = AsSecureLogFileName;
+  SecureLogFile = TargetOptions ? TargetOptions->AsSecureLogFile : "";
 

MaskRay wrote:
> When is TargetOptions null? When it is TargetOptions, Does a value of `""` 
> matter?
> When is TargetOptions null? 

null is the default value for the parameter, and happens in various places that 
construct MCOptions. None of them require a secure log file as far as I can 
tell.

> Does a value of "" matter?

Like the other file path in MCTargetOptions  (SplitDwarfFile) empty indicates 
no file specified.


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

https://reviews.llvm.org/D136888

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

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: llvm/lib/MC/MCContext.cpp:77
   AutoReset(DoAutoReset), TargetOptions(TargetOpts) {
-  SecureLogFile = AsSecureLogFileName;
+  SecureLogFile = TargetOptions ? TargetOptions->AsSecureLogFile : "";
 

benlangmuir wrote:
> MaskRay wrote:
> > When is TargetOptions null? When it is TargetOptions, Does a value of `""` 
> > matter?
> > When is TargetOptions null? 
> 
> null is the default value for the parameter, and happens in various places 
> that construct MCOptions. None of them require a secure log file as far as I 
> can tell.
> 
> > Does a value of "" matter?
> 
> Like the other file path in MCTargetOptions  (SplitDwarfFile) empty indicates 
> no file specified.
@MaskRay was this just a question, or did you want me to change something here? 
Wanted to check before I merge this.


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

https://reviews.llvm.org/D136888

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

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[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 rGe1f998302276: Move getenv for AS_SECURE_LOG_FILE to clang 
(authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136888

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/as-secure-log-file.c
  clang/test/Driver/AS_SECURE_LOG_FILE.s
  clang/test/Misc/cc1as-as-secure-log-file.s
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/include/llvm/MC/MCTargetOptionsCommandFlags.h
  llvm/lib/MC/MCContext.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
  llvm/test/MC/AsmParser/secure_log_unique.s

Index: llvm/test/MC/AsmParser/secure_log_unique.s
===
--- llvm/test/MC/AsmParser/secure_log_unique.s
+++ llvm/test/MC/AsmParser/secure_log_unique.s
@@ -1,6 +1,6 @@
 // RUN: rm -f %t
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
-// RUN: env AS_SECURE_LOG_FILE=%t llvm-mc -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file %t -triple x86_64-apple-darwin %s
+// RUN: llvm-mc -as-secure-log-file %t -triple x86_64-apple-darwin %s
 // RUN: FileCheck --input-file=%t %s
 .secure_log_unique "foobar"
 
Index: llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
===
--- llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
+++ llvm/lib/MC/MCTargetOptionsCommandFlags.cpp
@@ -45,6 +45,7 @@
 MCOPT(bool, NoDeprecatedWarn)
 MCOPT(bool, NoTypeCheck)
 MCOPT(std::string, ABIName)
+MCOPT(std::string, AsSecureLogFile)
 
 llvm::mc::RegisterMCTargetOptionsFlags::RegisterMCTargetOptionsFlags() {
 #define MCBINDOPT(NAME)\
@@ -114,6 +115,10 @@
   cl::init(""));
   MCBINDOPT(ABIName);
 
+  static cl::opt AsSecureLogFile(
+  "as-secure-log-file", cl::desc("As secure log file name"), cl::Hidden);
+  MCBINDOPT(AsSecureLogFile);
+
 #undef MCBINDOPT
 }
 
@@ -130,6 +135,7 @@
   Options.MCNoDeprecatedWarn = getNoDeprecatedWarn();
   Options.MCNoTypeCheck = getNoTypeCheck();
   Options.EmitDwarfUnwind = getEmitDwarfUnwind();
+  Options.AsSecureLogFile = getAsSecureLogFile();
 
   return Options;
 }
Index: llvm/lib/MC/MCParser/DarwinAsmParser.cpp
===
--- llvm/lib/MC/MCParser/DarwinAsmParser.cpp
+++ llvm/lib/MC/MCParser/DarwinAsmParser.cpp
@@ -767,8 +767,8 @@
 return Error(IDLoc, ".secure_log_unique specified multiple times");
 
   // Get the secure log path.
-  const char *SecureLogFile = getContext().getSecureLogFile();
-  if (!SecureLogFile)
+  StringRef SecureLogFile = getContext().getSecureLogFile();
+  if (SecureLogFile.empty())
 return Error(IDLoc, ".secure_log_unique used but AS_SECURE_LOG_FILE "
  "environment variable unset.");
 
@@ -776,9 +776,8 @@
   raw_fd_ostream *OS = getContext().getSecureLog();
   if (!OS) {
 std::error_code EC;
-auto NewOS = std::make_unique(StringRef(SecureLogFile), EC,
-  sys::fs::OF_Append |
-  sys::fs::OF_TextWithCRLF);
+auto NewOS = std::make_unique(
+SecureLogFile, EC, sys::fs::OF_Append | sys::fs::OF_TextWithCRLF);
 if (EC)
return Error(IDLoc, Twine("can't open secure log file: ") +
SecureLogFile + " (" + EC.message() + ")");
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -59,12 +59,6 @@
 
 using namespace llvm;
 
-static cl::opt
-AsSecureLogFileName("as-secure-log-file-name",
-cl::desc("As secure log file name (initialized from "
- "AS_SECURE_LOG_FILE env variable)"),
-cl::init(getenv("AS_SECURE_LOG_FILE")), cl::Hidden);
-
 static void defaultDiagHandler(const SMDiagnostic &SMD, bool, const SourceMgr &,
std::vector &) {
   SMD.print(nullptr, errs());
@@ -80,7 +74,7 @@
   InlineAsmUsedLabelNames(Allocator),
   CurrentDwarfLoc(0, 0, 0, DWARF2_FLAG_IS_STMT, 0, 0),
   AutoReset(DoAutoReset), TargetOptions(TargetOpts) {
-  SecureLogFile = AsSecureLogFileName;
+  SecureLogFile = TargetOptions ? TargetOptions->AsSecureLogFile : "";
 
   if (SrcMgr && SrcMgr->getNumBuffers())
 MainFileName = std::string(SrcMgr->getMemoryBuffer(SrcMgr->getMainFileID())
Index: llvm/include/llvm/MC/MCTargetOptionsCommandFlags.h
===

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D135220#3899674 , @dexonsmith 
wrote:

> In D135220#3898849 , @hans wrote:
>
>> Relatedly, we ran into a problem with `clang-cl /showIncludes` not including 
>> all files in its output when they're linked: 
>> https://github.com/llvm/llvm-project/issues/58726
>
> Interestingly, that discussion points out that `-MD` works correctly:

I commented on the issue 
 
about what's going on here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135220

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

[PATCH] D131796: [clang][deps] Use a cc1 invocation in FullDependencies::getCommandLine()

[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 a project: clang.
Herald added a subscriber: cfe-commits.

Instead of trying to "fix" the original driver invocatin by appending
arguments to it, use a CompilerInvocation to give precise control over
the invocation, and serialize it to a -cc1 command.

This change should make it easier to (in the future) canonicalize the
command-line (e.g. to improve hits in something like ccache) or apply
optimizations, similar to what we do for module build commands.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131796

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  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
@@ -73,9 +73,10 @@
 // CHECK-NEXT: }
 // CHECK-NEXT:   ]
 // CHECK-NEXT:   "command-line": [
-// CHECK-NEXT: "-fsyntax-only",
+// CHECK-NEXT: "-cc1",
 // CHECK-NOT:  "-fmodules-cache-path=
 // CHECK-NOT:  "-fmodules-validate-once-per-build-session"
+// CHECK-NOT:  "-fbuild-session-timestamp=
 // CHECK-NOT:  "-fbuild-session-file=
 // CHECK-NOT:  "-fmodules-prune-interval=
 // CHECK-NOT:  "-fmodules-prune-after=
Index: clang/test/ClangScanDeps/modules-pch.c
===
--- clang/test/ClangScanDeps/modules-pch.c
+++ clang/test/ClangScanDeps/modules-pch.c
@@ -92,11 +92,11 @@
 // CHECK-PCH-NEXT: }
 // CHECK-PCH-NEXT:   ],
 // CHECK-PCH-NEXT:   "command-line": [
-// CHECK-PCH:  "-fno-implicit-modules",
-// CHECK-PCH-NEXT: "-fno-implicit-module-maps",
-// CHECK-PCH-NEXT: "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_COMMON_1]]/ModCommon1-{{.*}}.pcm",
-// CHECK-PCH-NEXT: "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_PCH]]/ModPCH-{{.*}}.pcm"
-// CHECK-PCH-NEXT:   ],
+// CHECK-PCH-NOT:  "-fimplicit-modules",
+// CHECK-PCH-NOT:  "-fmplicit-module-maps",
+// CHECK-PCH:  "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_COMMON_1]]/ModCommon1-{{.*}}.pcm",
+// CHECK-PCH:  "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_PCH]]/ModPCH-{{.*}}.pcm"
+// CHECK-PCH:],
 // CHECK-PCH-NEXT:   "file-deps": [
 // CHECK-PCH-NEXT: "[[PREFIX]]/pch.h"
 // CHECK-PCH-NEXT:   ],
@@ -154,10 +154,10 @@
 // CHECK-TU-NEXT: }
 // CHECK-TU-NEXT:   ],
 // CHECK-TU-NEXT:   "command-line": [
-// CHECK-TU:  "-fno-implicit-modules",
-// CHECK-TU-NEXT: "-fno-implicit-module-maps",
-// CHECK-TU-NEXT: "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_TU]]/ModTU-{{.*}}.pcm"
-// CHECK-TU-NEXT:   ],
+// CHECK-TU-NOT:  "-fimplicit-modules",
+// CHECK-TU-NOT:  "-fimplicit-module-maps",
+// CHECK-TU:  "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_TU]]/ModTU-{{.*}}.pcm"
+// CHECK-TU:],
 // CHECK-TU-NEXT:   "file-deps": [
 // CHECK-TU-NEXT: "[[PREFIX]]/tu.c",
 // CHECK-TU-NEXT: "[[PREFIX]]/pch.h.gch"
@@ -213,11 +213,11 @@
 // CHECK-TU-WITH-COMMON-NEXT: }
 // CHECK-TU-WITH-COMMON-NEXT:   ],
 // CHECK-TU-WITH-COMMON-NEXT:   "command-line": [
-// CHECK-TU-WITH-COMMON:  "-fno-implicit-modules",
-// CHECK-TU-WITH-COMMON-NEXT: "-fno-implicit-module-maps",
-// CHECK-TU-WITH-COMMON-NEXT: "-fmodule-file=[[PREFIX]]/build/{{.*}}/ModCommon2-{{.*}}.pcm",
-// CHECK-TU-WITH-COMMON-NEXT: "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_TU_WITH_COMMON]]/ModTUWithCommon-{{.*}}.pcm"
-// CHECK-TU-WITH-COMMON-NEXT:   ],
+// CHECK-TU-WITH-COMMON-NOT:  "-fimplicit-modules",
+// CHECK-TU-WITH-COMMON-NOT:  "-fimplicit-module-maps",
+// CHECK-TU-WITH-COMMON:  "-fmodule-file=[[PREFIX]]/build/{{.*}}/ModCommon2-{{.*}}.pcm",
+// CHECK-TU-WITH-COMMON:  "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_TU_WITH_COMMON]]/ModT

[PATCH] D131934: [clang][deps] Compute command-lines for dependencies immediately

[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.

Instead of delaying the generation of command-lines to after all
dependencies are reported, compute them immediately. This is partly in
preparation for splitting the TU driver command into its constituent cc1
and other jobs, but it also just simplifies working with the compiler
invocation for modules if they are not "without paths".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131934

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/ModuleDepCollector.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "clang/Driver/Driver.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
@@ -268,10 +269,7 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine =
-FD.getCommandLine([&](const ModuleID &MID, ModuleOutputKind MOK) {
-  return lookupModuleOutput(MID, MOK);
-});
+ID.CommandLine = FD.CommandLine;
 Inputs.push_back(std::move(ID));
   }
 
@@ -301,10 +299,7 @@
   {"file-deps", toJSONSorted(MD.FileDeps)},
   {"clang-module-deps", toJSONSorted(MD.ClangModuleDeps)},
   {"clang-modulemap-file", MD.ClangModuleMapFile},
-  {"command-line", MD.getCanonicalCommandLine(
-   [&](const ModuleID &MID, ModuleOutputKind MOK) {
- return lookupModuleOutput(MID, MOK);
-   })},
+  {"command-line", MD.getCanonicalCommandLine()},
   };
   OutModules.push_back(std::move(O));
 }
@@ -330,42 +325,6 @@
   }
 
 private:
-  std::string lookupModuleOutput(const ModuleID &MID, ModuleOutputKind MOK) {
-// Cache the PCM path, since it will be queried repeatedly for each module.
-// The other outputs are only queried once during getCanonicalCommandLine.
-auto PCMPath = PCMPaths.insert({MID, ""});
-if (PCMPath.second)
-  PCMPath.first->second = constructPCMPath(MID);
-switch (MOK) {
-case ModuleOutputKind::ModuleFile:
-  return PCMPath.first->second;
-case ModuleOutputKind::DependencyFile:
-  return PCMPath.first->second + ".d";
-case ModuleOutputKind::DependencyTargets:
-  // Null-separate the list of targets.
-  return join(ModuleDepTargets, StringRef("\0", 1));
-case ModuleOutputKind::DiagnosticSerializationFile:
-  return PCMPath.first->second + ".diag";
-}
-llvm_unreachable("Fully covered switch above!");
-  }
-
-  /// Construct a path for the explicitly built PCM.
-  std::string constructPCMPath(ModuleID MID) const {
-auto MDIt = Modules.find(IndexedModuleID{MID, 0});
-assert(MDIt != Modules.end());
-const ModuleDeps &MD = MDIt->second;
-
-StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
-StringRef ModuleCachePath = llvm::sys::path::parent_path(
-llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
-
-SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
- : ModuleCachePath);
-llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
-return std::string(ExplicitPCMPath);
-  }
-
   struct IndexedModuleID {
 ModuleID ID;
 mutable size_t InputIndex;
@@ -395,7 +354,6 @@
   std::mutex Lock;
   std::unordered_map
   Modules;
-  std::unordered_map PCMPaths;
   std::vector Inputs;
 };
 
@@ -417,6 +375,42 @@
   return false;
 }
 
+/// Construct a path for the explicitly built PCM.
+static std::string constructPCMPath(ModuleID MID, StringRef OutputDir) {
+  SmallString<256> ExplicitPCMPath(OutputDir);
+  llvm::sys::path::append(ExplicitPCMPath, MID.ContextHash,
+  MID.ModuleName + "-" + MID.ContextHash + ".pcm");
+  return std::string(ExplicitPCMPath);
+}
+
+static std::string lookupModuleOutput(const ModuleID &MID, ModuleOutputKind MOK,
+  StringRef OutputDir) {
+  std::string PCMPath = constructPCMPath(MID, OutputDir);
+  switch (MOK) {
+  case ModuleOutputKind::ModuleFile:
+