[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-23 Thread Ben Barham via Phabricator via cfe-commits 
bnbarham added a comment.

Yep, that would be perfect :). Thanks @mibintc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-23 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added a comment.

Whoops, shouldn't mess with the mkdir line

diff --git a/clang/test/Index/preamble-reparse-changed-module.m 
b/clang/test/Index/preamble-reparse-changed-module.m
index 1c63e802ce0c..349ed0db27d0 100644

- a/clang/test/Index/preamble-reparse-changed-module.m

+++ b/clang/test/Index/preamble-reparse-changed-module.m
@@ -1,5 +1,6 @@
 // REQUIRES: shell

+// RUN: rm -rf %t
 // RUN: mkdir -p %t/mod
 // RUN: touch %t/empty.h
 // RUN: cp %S/Inputs/preamble-reparse-changed-module/module.modulemap %t/mod


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-23 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added a comment.

@bnbarham 
I think you mean a patch like this, is it right?  I'd like to fix the test in a 
pre-commit and then re-push my patch

   git diff
  diff --git a/clang/test/Index/preamble-reparse-changed-module.m 
b/clang/test/Index/preamble-reparse-changed-module.m
  index 1c63e802ce0c..fc336c6e0670 100644
  --- a/clang/test/Index/preamble-reparse-changed-module.m
  +++ b/clang/test/Index/preamble-reparse-changed-module.m
  @@ -1,6 +1,7 @@
   // REQUIRES: shell
  
  -// RUN: mkdir -p %t/mod
  +// RUN: rm -rf %t
  +// RUN: mkdir %t/mod
   // RUN: touch %t/empty.h
   // RUN: cp %S/Inputs/preamble-reparse-changed-module/module.modulemap %t/mod
   // RUN: cp %S/Inputs/preamble-reparse-changed-module/head.h %t/mod


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-22 Thread Ben Barham via Phabricator via cfe-commits 
bnbarham added a comment.

In D95159#2897941 , @mibintc wrote:

> Can you suggest how to find the problem?  Thanks a lot.

@mibintc the test is reusing the module cache directory and that's causing some 
issues between runs, sorry about that. You could add a `rm -rf %t` or `rm -rf 
%t/mcp` to this test to fix that. Looks like the same issue was run into in 
https://reviews.llvm.org/D96816#2572589. I'd prefer 
`clang/test/Index/preamble-reparse-changed-module.m` add the `rm` and another 
test added for the `LLVM_APPEND_VC_REV=NO` if we want that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-22 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added a subscriber: MaskRay.
mibintc added a comment.

More info about the test failure I'm seeing for 
clang/test/Index/preamble-reparse-changed-module.m

It appears it might be a non-deterministic failure, perhaps some test 
dependencies are missing?

Yesterday before I pushed my clang patch, I saw it fail when I used "ninja 
check-clang".  I quickly executed each step used by this test (llvm-lit -v ; 
then save all the RUN commands to a file ; then "source" the file -- nothing 
was printed to stdout and also the return code was 0).  I thought the fail 
report wasn't real at this point and I pushed the patch.  This was a Debug 
build.

Then I saw a bot failure, @MaskRay pinged my clang patch and said my patch had 
caused this test to fail.  He told me there was an assertion fail.

Then I created a new build area, configured with Debug and "enable llvm 
assertions", and "ninja check-clang".  This time 0 tests reported as fail.

Then I went back into the build-debug build where "ninja check-clang" had 
reported the fail and used gdb to debug.  In the course of debugging gdb told 
me that the build was out-of-date w.r.t. the source file, so I exitted gdb, 
"ninja clean" "ninja check-clang".  But now no tests are failing.  I don't know 
why gdb told me the build was out of date, I believe I did not change any of 
the files but I'm not 100% certain.

I removed the patch and created another build area, and rebuilt with Debug and 
asserts, everything is clean there too.

I captured the stdout from executing c-index-test, there is a difference 
between patched and unpatched. The __FLT_EVAL_METHOD__ line is not present in 
the patched, this is expected.

Can you suggest how to find the problem?  Thanks a lot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-21 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added a subscriber: aaron.ballman.
mibintc added a comment.

@aaron.ballman suggested I may have made a mistake adding the new option, I'll 
look there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-07-21 Thread Melanie Blower via Phabricator via cfe-commits 
mibintc added a comment.

The newly added test case clang/test/Index/preamble-reparse-changed-module.m is 
failing on the patch that I'm trying to commit, https://reviews.llvm.org/D93769

Actually the result of the test is a bit flaky, when I build the Debug version 
on Linux, running ninja check-clang does show the test fail, but when I do an 
identical build and in addition enabling llvm assertions, llvm-lit does not 
show this lit test failing.

I think the problem has something to do with the macro __FLT_EVAL_METHOD__.  
The patch now treats this macro specially, more like __LINE__ a macro that can 
vary depending on where it is seen in the source code.

In my patch, __FLT_EVAL_MACRO__ is not expanded in -E mode, it just appears as 
its name in the preprocessed output.  The reason is, the actual value of the 
macro depends on the floating point settings at a particular source code 
location (depends on #pragma float_control settings).  The pragma are 
determined in Sema, and -E runs before Sema so the actual value isn't available.

This macro is the only reason i can think why this test is failing.

Can you help me understand what to do about this test case/about my patch/ so 
that i can get my patch committed? I don't understand what this test is doing. 
In the debugger it hits a failed assertion, looks like index out-of-range. 
here's a little massaged traceback
__assert_fail
 llvm::SmallVectorTemplateCommon unsigned long, void ::indexing operator
clang::ASTReader::ReadString
clang::ASTReader::ParseLanguageOptions
clang::ASTReader::ReadOptionsBlock
clang::ASTReader::ReadControlBlock
clang::ASTReader::ReadASTCore
clang::ASTReader::ReadAST
clang::CompilerInstance::findOrCompileModuleAndReadAST
clang::CompilerInstance::loadModule
clang::Preprocessor::LexAfterModuleImport

Thanks a lot


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-01-21 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb0e89906f5b7: [ASTReader] Allow controlling separately 
whether validation should be disabled… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/ChainedIncludesSource.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Index/Inputs/preamble-reparse-changed-module/head.h
  clang/test/Index/Inputs/preamble-reparse-changed-module/module.modulemap
  clang/test/Index/Inputs/preamble-reparse-changed-module/new-head.h
  clang/test/Index/preamble-reparse-changed-module.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/c-index-test/core_main.cpp

Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -13,22 +13,25 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Support/PrettyStackTrace.h"
 
 using namespace clang;
 using namespace clang::index;
 using namespace llvm;
 
 extern "C" int indextest_core_main(int argc, const char **argv);
+extern "C" int indextest_perform_shell_execution(const char *command_line);
 
 namespace {
 
@@ -359,3 +362,21 @@
 
   return 0;
 }
+
+//===--===//
+// Utility functions
+//===--===//
+
+int indextest_perform_shell_execution(const char *command_line) {
+  BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  SmallVector Args;
+  llvm::cl::TokenizeGNUCommandLine(command_line, Saver, Args);
+  auto Program = llvm::sys::findProgramByName(Args[0]);
+  if (std::error_code ec = Program.getError()) {
+llvm::errs() << "command not found: " << Args[0] << "\n";
+return ec.value();
+  }
+  SmallVector execArgs(Args.begin(), Args.end());
+  return llvm::sys::ExecuteAndWait(*Program, execArgs);
+}
Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -24,6 +24,7 @@
 #endif
 
 extern int indextest_core_main(int argc, const char **argv);
+extern int indextest_perform_shell_execution(const char *command_line);
 
 /**/
 /* Utility functions. */
@@ -2095,6 +2096,8 @@
   enum CXErrorCode Err;
   int result, i;
   int trial;
+  int execute_after_trial = 0;
+  const char *execute_command = NULL;
   int remap_after_trial = 0;
   char *endptr = 0;
   
@@ -2133,12 +2136,26 @@
   if (checkForErrors(TU) != 0)
 return -1;
 
+  if (getenv("CINDEXTEST_EXECUTE_COMMAND")) {
+execute_command = getenv("CINDEXTEST_EXECUTE_COMMAND");
+  }
+  if (getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL")) {
+execute_after_trial =
+strtol(getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL"), , 10);
+  }
+
   if (getenv("CINDEXTEST_REMAP_AFTER_TRIAL")) {
 remap_after_trial =
 strtol(getenv("CINDEXTEST_REMAP_AFTER_TRIAL"), , 10);
   }
 
   for (trial = 0; trial < trials; ++trial) {
+if (execute_command && trial == execute_after_trial) {
+  result = indextest_perform_shell_execution(execute_command);
+  if (result != 0)
+return result;
+}
+
 free_remapped_files(unsaved_files, num_unsaved_files);
 if (parse_remapped_files_with_try(trial, argc, argv, 0,
   _files, _unsaved_files)) {
Index: clang/test/Index/preamble-reparse-changed-module.m
===
--- /dev/null
+++ clang/test/Index/preamble-reparse-changed-module.m
@@ -0,0 +1,18 @@
+// REQUIRES: shell
+
+// RUN: mkdir -p %t/mod
+// RUN:

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-01-21 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits 
akyrtzi updated this revision to Diff 318335.
akyrtzi edited the summary of this revision.
akyrtzi added a comment.

Fix typo in commit message, 'state' -> 'stale'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/ChainedIncludesSource.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Index/Inputs/preamble-reparse-changed-module/head.h
  clang/test/Index/Inputs/preamble-reparse-changed-module/module.modulemap
  clang/test/Index/Inputs/preamble-reparse-changed-module/new-head.h
  clang/test/Index/preamble-reparse-changed-module.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/c-index-test/core_main.cpp

Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -13,22 +13,25 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Support/PrettyStackTrace.h"
 
 using namespace clang;
 using namespace clang::index;
 using namespace llvm;
 
 extern "C" int indextest_core_main(int argc, const char **argv);
+extern "C" int indextest_perform_shell_execution(const char *command_line);
 
 namespace {
 
@@ -359,3 +362,21 @@
 
   return 0;
 }
+
+//===--===//
+// Utility functions
+//===--===//
+
+int indextest_perform_shell_execution(const char *command_line) {
+  BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  SmallVector Args;
+  llvm::cl::TokenizeGNUCommandLine(command_line, Saver, Args);
+  auto Program = llvm::sys::findProgramByName(Args[0]);
+  if (std::error_code ec = Program.getError()) {
+llvm::errs() << "command not found: " << Args[0] << "\n";
+return ec.value();
+  }
+  SmallVector execArgs(Args.begin(), Args.end());
+  return llvm::sys::ExecuteAndWait(*Program, execArgs);
+}
Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -24,6 +24,7 @@
 #endif
 
 extern int indextest_core_main(int argc, const char **argv);
+extern int indextest_perform_shell_execution(const char *command_line);
 
 /**/
 /* Utility functions. */
@@ -2095,6 +2096,8 @@
   enum CXErrorCode Err;
   int result, i;
   int trial;
+  int execute_after_trial = 0;
+  const char *execute_command = NULL;
   int remap_after_trial = 0;
   char *endptr = 0;
   
@@ -2133,12 +2136,26 @@
   if (checkForErrors(TU) != 0)
 return -1;
 
+  if (getenv("CINDEXTEST_EXECUTE_COMMAND")) {
+execute_command = getenv("CINDEXTEST_EXECUTE_COMMAND");
+  }
+  if (getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL")) {
+execute_after_trial =
+strtol(getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL"), , 10);
+  }
+
   if (getenv("CINDEXTEST_REMAP_AFTER_TRIAL")) {
 remap_after_trial =
 strtol(getenv("CINDEXTEST_REMAP_AFTER_TRIAL"), , 10);
   }
 
   for (trial = 0; trial < trials; ++trial) {
+if (execute_command && trial == execute_after_trial) {
+  result = indextest_perform_shell_execution(execute_command);
+  if (result != 0)
+return result;
+}
+
 free_remapped_files(unsaved_files, num_unsaved_files);
 if (parse_remapped_files_with_try(trial, argc, argv, 0,
   _files, _unsaved_files)) {
Index: clang/test/Index/preamble-reparse-changed-module.m
===
--- /dev/null
+++ clang/test/Index/preamble-reparse-changed-module.m
@@ -0,0 +1,18 @@
+// REQUIRES: shell
+
+// RUN: mkdir -p %t/mod
+// RUN: touch %t/empty.h
+// RUN: cp

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-01-21 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits 
akyrtzi updated this revision to Diff 318333.
akyrtzi added a comment.

Use `getValueOr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/ChainedIncludesSource.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Index/Inputs/preamble-reparse-changed-module/head.h
  clang/test/Index/Inputs/preamble-reparse-changed-module/module.modulemap
  clang/test/Index/Inputs/preamble-reparse-changed-module/new-head.h
  clang/test/Index/preamble-reparse-changed-module.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/c-index-test/core_main.cpp

Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -13,22 +13,25 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Support/PrettyStackTrace.h"
 
 using namespace clang;
 using namespace clang::index;
 using namespace llvm;
 
 extern "C" int indextest_core_main(int argc, const char **argv);
+extern "C" int indextest_perform_shell_execution(const char *command_line);
 
 namespace {
 
@@ -359,3 +362,21 @@
 
   return 0;
 }
+
+//===--===//
+// Utility functions
+//===--===//
+
+int indextest_perform_shell_execution(const char *command_line) {
+  BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  SmallVector Args;
+  llvm::cl::TokenizeGNUCommandLine(command_line, Saver, Args);
+  auto Program = llvm::sys::findProgramByName(Args[0]);
+  if (std::error_code ec = Program.getError()) {
+llvm::errs() << "command not found: " << Args[0] << "\n";
+return ec.value();
+  }
+  SmallVector execArgs(Args.begin(), Args.end());
+  return llvm::sys::ExecuteAndWait(*Program, execArgs);
+}
Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -24,6 +24,7 @@
 #endif
 
 extern int indextest_core_main(int argc, const char **argv);
+extern int indextest_perform_shell_execution(const char *command_line);
 
 /**/
 /* Utility functions. */
@@ -2095,6 +2096,8 @@
   enum CXErrorCode Err;
   int result, i;
   int trial;
+  int execute_after_trial = 0;
+  const char *execute_command = NULL;
   int remap_after_trial = 0;
   char *endptr = 0;
   
@@ -2133,12 +2136,26 @@
   if (checkForErrors(TU) != 0)
 return -1;
 
+  if (getenv("CINDEXTEST_EXECUTE_COMMAND")) {
+execute_command = getenv("CINDEXTEST_EXECUTE_COMMAND");
+  }
+  if (getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL")) {
+execute_after_trial =
+strtol(getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL"), , 10);
+  }
+
   if (getenv("CINDEXTEST_REMAP_AFTER_TRIAL")) {
 remap_after_trial =
 strtol(getenv("CINDEXTEST_REMAP_AFTER_TRIAL"), , 10);
   }
 
   for (trial = 0; trial < trials; ++trial) {
+if (execute_command && trial == execute_after_trial) {
+  result = indextest_perform_shell_execution(execute_command);
+  if (result != 0)
+return result;
+}
+
 free_remapped_files(unsaved_files, num_unsaved_files);
 if (parse_remapped_files_with_try(trial, argc, argv, 0,
   _files, _unsaved_files)) {
Index: clang/test/Index/preamble-reparse-changed-module.m
===
--- /dev/null
+++ clang/test/Index/preamble-reparse-changed-module.m
@@ -0,0 +1,18 @@
+// REQUIRES: shell
+
+// RUN: mkdir -p %t/mod
+// RUN: touch %t/empty.h
+// RUN: cp %S/Inputs/preamble-reparse-changed-module/module.modulemap %t/mod
+// RUN: cp

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-01-21 Thread Ben Barham via Phabricator via cfe-commits 
bnbarham accepted this revision.
bnbarham added a comment.
This revision is now accepted and ready to land.

LGTM, just the one minor comment




Comment at: clang/lib/Serialization/ASTReader.cpp:2221-
+  // validation for the PCH and the modules it loads.
+  ModuleKind K = CurrentDeserializingModuleKind.hasValue() ?
+ CurrentDeserializingModuleKind.getValue() : M.Kind;
+

There's a `getValueOr` method that you could use instead (ie. 
`CurrentDeserializingModuleKind.getValueOr(M.Kind)`).



Comment at: clang/test/Index/preamble-reparse-changed-module.m:8
+
+// RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_EXECUTE_COMMAND="cp 
%S/Inputs/preamble-reparse-changed-module/new-head.h %t/mod/head.h" 
CINDEXTEST_EXECUTE_AFTER_TRIAL=1 \
+// RUN: c-index-test -test-load-source-reparse 3 local %s -I %t -I %t/mod 
-fmodules -fmodules-cache-path=%t/mcp 2>&1 | FileCheck %s

For some reason I was expecting the trial's to be 1 based, not 0. I'm not sure 
why though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

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

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-01-21 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits 
akyrtzi updated this revision to Diff 318314.
akyrtzi added a comment.

clang-format changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95159

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/ChainedIncludesSource.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Index/Inputs/preamble-reparse-changed-module/head.h
  clang/test/Index/Inputs/preamble-reparse-changed-module/module.modulemap
  clang/test/Index/Inputs/preamble-reparse-changed-module/new-head.h
  clang/test/Index/preamble-reparse-changed-module.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/c-index-test/core_main.cpp

Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -13,22 +13,25 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Support/PrettyStackTrace.h"
 
 using namespace clang;
 using namespace clang::index;
 using namespace llvm;
 
 extern "C" int indextest_core_main(int argc, const char **argv);
+extern "C" int indextest_perform_shell_execution(const char *command_line);
 
 namespace {
 
@@ -359,3 +362,21 @@
 
   return 0;
 }
+
+//===--===//
+// Utility functions
+//===--===//
+
+int indextest_perform_shell_execution(const char *command_line) {
+  BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  SmallVector Args;
+  llvm::cl::TokenizeGNUCommandLine(command_line, Saver, Args);
+  auto Program = llvm::sys::findProgramByName(Args[0]);
+  if (std::error_code ec = Program.getError()) {
+llvm::errs() << "command not found: " << Args[0] << "\n";
+return ec.value();
+  }
+  SmallVector execArgs(Args.begin(), Args.end());
+  return llvm::sys::ExecuteAndWait(*Program, execArgs);
+}
Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -24,6 +24,7 @@
 #endif
 
 extern int indextest_core_main(int argc, const char **argv);
+extern int indextest_perform_shell_execution(const char *command_line);
 
 /**/
 /* Utility functions. */
@@ -2095,6 +2096,8 @@
   enum CXErrorCode Err;
   int result, i;
   int trial;
+  int execute_after_trial = 0;
+  const char *execute_command = NULL;
   int remap_after_trial = 0;
   char *endptr = 0;
   
@@ -2133,12 +2136,26 @@
   if (checkForErrors(TU) != 0)
 return -1;
 
+  if (getenv("CINDEXTEST_EXECUTE_COMMAND")) {
+execute_command = getenv("CINDEXTEST_EXECUTE_COMMAND");
+  }
+  if (getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL")) {
+execute_after_trial =
+strtol(getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL"), , 10);
+  }
+
   if (getenv("CINDEXTEST_REMAP_AFTER_TRIAL")) {
 remap_after_trial =
 strtol(getenv("CINDEXTEST_REMAP_AFTER_TRIAL"), , 10);
   }
 
   for (trial = 0; trial < trials; ++trial) {
+if (execute_command && trial == execute_after_trial) {
+  result = indextest_perform_shell_execution(execute_command);
+  if (result != 0)
+return result;
+}
+
 free_remapped_files(unsaved_files, num_unsaved_files);
 if (parse_remapped_files_with_try(trial, argc, argv, 0,
   _files, _unsaved_files)) {
Index: clang/test/Index/preamble-reparse-changed-module.m
===
--- /dev/null
+++ clang/test/Index/preamble-reparse-changed-module.m
@@ -0,0 +1,18 @@
+// REQUIRES: shell
+
+// RUN: mkdir -p %t/mod
+// RUN: touch %t/empty.h
+// RUN: cp %S/Inputs/preamble-reparse-changed-module/module.modulemap %t/mod
+// RUN: cp

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

2021-01-21 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits 
akyrtzi created this revision.
Herald added subscribers: dang, arphaman.
Herald added a reviewer: jansvoboda11.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This addresses an issue with how the PCH preable works, specifically:

1. When using a PCH/preamble the module hash changes and a different cache 
directory is used
2. When the preamble is used, PCH & PCM validation is disabled.

Due to combination of #1 and #2, reparsing with preamble enabled can end up 
loading a state module file before a header change and using it without 
updating it because validation is disabled and it doesn’t check that the header 
has changed and the module file is out-of-date.

rdar://72611253


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95159

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/ChainedIncludesSource.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Index/Inputs/preamble-reparse-changed-module/head.h
  clang/test/Index/Inputs/preamble-reparse-changed-module/module.modulemap
  clang/test/Index/Inputs/preamble-reparse-changed-module/new-head.h
  clang/test/Index/preamble-reparse-changed-module.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/c-index-test/core_main.cpp

Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -21,14 +21,17 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 
 using namespace clang;
 using namespace clang::index;
 using namespace llvm;
 
 extern "C" int indextest_core_main(int argc, const char **argv);
+extern "C" int indextest_perform_shell_execution(const char *command_line);
 
 namespace {
 
@@ -359,3 +362,21 @@
 
   return 0;
 }
+
+//===--===//
+// Utility functions
+//===--===//
+
+int indextest_perform_shell_execution(const char *command_line) {
+  BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  SmallVector Args;
+  llvm::cl::TokenizeGNUCommandLine(command_line, Saver, Args);
+  auto Program = llvm::sys::findProgramByName(Args[0]);
+  if (std::error_code ec = Program.getError()) {
+llvm::errs() << "command not found: " << Args[0] << "\n";
+return ec.value();
+  }
+  SmallVector execArgs(Args.begin(), Args.end());
+  return llvm::sys::ExecuteAndWait(*Program, execArgs);
+}
Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -24,6 +24,7 @@
 #endif
 
 extern int indextest_core_main(int argc, const char **argv);
+extern int indextest_perform_shell_execution(const char *command_line);
 
 /**/
 /* Utility functions. */
@@ -2095,6 +2096,8 @@
   enum CXErrorCode Err;
   int result, i;
   int trial;
+  int execute_after_trial = 0;
+  const char *execute_command = NULL;
   int remap_after_trial = 0;
   char *endptr = 0;
   
@@ -2133,12 +2136,26 @@
   if (checkForErrors(TU) != 0)
 return -1;
 
+  if (getenv("CINDEXTEST_EXECUTE_COMMAND")) {
+execute_command = getenv("CINDEXTEST_EXECUTE_COMMAND");
+  }
+  if (getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL")) {
+execute_after_trial =
+strtol(getenv("CINDEXTEST_EXECUTE_AFTER_TRIAL"), , 10);
+  }
+
   if (getenv("CINDEXTEST_REMAP_AFTER_TRIAL")) {
 remap_after_trial =
 strtol(getenv("CINDEXTEST_REMAP_AFTER_TRIAL"), , 10);
   }
 
   for (trial = 0; trial < trials; ++trial) {
+if (execute_command && trial == execute_after_trial) {
+  result = indextest_perform_shell_execution(execute_command);
+  if (result != 0)
+return result;
+}
+
 free_remapped_files(unsaved_files, num_unsaved_files);
 if (parse_remapped_files_with_try(trial, argc, argv, 0,
   _files, _unsaved_files)) {
Index: clang/test/Index/preamble-reparse-changed-module.m
===
--- /dev/null
+++ clang/test/Index/preamble-reparse-changed-module.m
@@ -0,0 +1,18 @@
+//