[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese requested changes to this revision.
Bigcheese added a comment.
This revision now requires changes to proceed.

I have a bit more review to do, but this is what I've found so far.  The naming 
comments are just suggestions, but the digit separators' are actually an issue.




Comment at: include/clang/Lex/DependencyDirectivesSourceMinimizer.h:25
+namespace clang {
+namespace minimize_source_to_dependency_directives {
+

This is a really long namespace name, not sure what else to call it though.



Comment at: include/clang/Lex/DependencyDirectivesSourceMinimizer.h:36
+  pp_import,
+  pp_at_import,
+  pp_pragma_import,

Is `@import` actually a preprocessor directive? For C++20 modules all the 
modules bits end up being declarations.



Comment at: lib/Frontend/FrontendActions.cpp:923-924
+   Toks)) {
+CI.getDiagnostics().Report(
+diag::err_minimize_source_to_dependency_directives_failed);
+return;

Can we give better error messages here?  Should 
`minimizeSourceToDependencyDirectives` take a `DiagnosticEngine`?



Comment at: lib/Lex/DependencyDirectivesSourceMinimizer.cpp:28
+
+struct Lexer {
+  SmallVectorImpl &Out;

I'm not sure `Lexer` is the best name for this class.  It's really a combined 
lexer and minimizer and I was a bit confused by some things until I realized 
that.  I think it would make more sense to name this `Minimizer` and the 
associated `lex` as `minimize`.



Comment at: lib/Lex/DependencyDirectivesSourceMinimizer.cpp:203
+
+static const char *reverseOverSpaces(const char *First, const char *Last) {
+  while (First != Last && isHorizontalWhitespace(Last[-1]))

`assert(First <= Last)` to match the other checks?



Comment at: lib/Lex/DependencyDirectivesSourceMinimizer.cpp:241
+  // Iterate over strings correctly to avoid comments and newlines.
+  if (*First == '\"' || *First == '\'') {
+if (isRawStringLiteral(Start, First))

I don't think this handles digit separators correctly.
```
#include 
int a = 0'1;
#include 
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D55463



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

[PATCH] D63101: [Frontend] SetUpDiagnosticLog should handle unowned diagnostic consumer in the compiler

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D63101



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

[PATCH] D65493: Modernize atomic detection and usage

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

This looks like a good simplification, but I think `call_once` could be 
simplified more.




Comment at: llvm/cmake/modules/CheckAtomic.cmake:46
+  if (NOT HAVE_ATOMICS_WITH_LIB)
+   message(FATAL_ERROR "Host compiler must support atomic!")
   endif()

Indentation?



Comment at: llvm/cmake/modules/CheckAtomic.cmake:75
 
-## TODO: This define is only used for the legacy atomic operations in
-## llvm's Atomic.h, which should be replaced.  Other code simply
-## assumes C++11  works.
-CHECK_CXX_SOURCE_COMPILES("
-#ifdef _MSC_VER
-#include 
-#endif
-int main() {
-#ifdef _MSC_VER
-volatile LONG val = 1;
-MemoryBarrier();
-InterlockedCompareExchange(&val, 0, 1);
-InterlockedIncrement(&val);
-InterlockedDecrement(&val);
-#else
-volatile unsigned long val = 1;
-__sync_synchronize();
-__sync_val_compare_and_swap(&val, 1, 0);
-__sync_add_and_fetch(&val, 1);
-__sync_sub_and_fetch(&val, 1);
-#endif
-return 0;
-  }
-" LLVM_HAS_ATOMICS)
-
 if( NOT LLVM_HAS_ATOMICS )
   message(STATUS "Warning: LLVM will be built thread-unsafe because atomic 
builtins are missing")

Does anything else set `LLVM_HAS_ATOMICS`?



Comment at: llvm/include/llvm/Support/Threading.h:111
+  std::atomic_thread_fence(std::memory_order_seq_cst);
   TsanIgnoreWritesBegin();
   TsanHappensBefore(&flag.status);

Not sure we still need the tsan hooks when using `std::atomic`.



Comment at: llvm/include/llvm/Support/Threading.h:118
+  InitStatus tmp = flag.status;
+  std::atomic_thread_fence(std::memory_order_seq_cst);
   while (tmp != Done) {

Do we actually need thread fences here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65493



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

[PATCH] D65989: [clang-scan-deps] Add minimizer support for C++20 modules.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added a reviewer: arphaman.
Herald added subscribers: tschuett, dexonsmith.
Herald added a project: clang.

This only adds support to the minimizer, it doesn't actually capture the 
dependencies yet.


Repository:
  rC Clang

https://reviews.llvm.org/D65989

Files:
  include/clang/Lex/DependencyDirectivesSourceMinimizer.h
  lib/Lex/DependencyDirectivesSourceMinimizer.cpp
  unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

Index: unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
===
--- unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
+++ unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
@@ -60,7 +60,9 @@
"#__include_macros \n"
"#import \n"
"@import A;\n"
-   "#pragma clang module import A\n",
+   "#pragma clang module import A\n"
+   "export module m;\n"
+   "import m;\n",
Out, Tokens));
   EXPECT_EQ(pp_define, Tokens[0].K);
   EXPECT_EQ(pp_undef, Tokens[1].K);
@@ -76,7 +78,10 @@
   EXPECT_EQ(pp_import, Tokens[11].K);
   EXPECT_EQ(decl_at_import, Tokens[12].K);
   EXPECT_EQ(pp_pragma_import, Tokens[13].K);
-  EXPECT_EQ(pp_eof, Tokens[14].K);
+  EXPECT_EQ(cxx_export_decl, Tokens[14].K);
+  EXPECT_EQ(cxx_module_decl, Tokens[15].K);
+  EXPECT_EQ(cxx_import_decl, Tokens[16].K);
+  EXPECT_EQ(pp_eof, Tokens[17].K);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
@@ -568,4 +573,48 @@
   EXPECT_STREQ("#pragma once\n#include \n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, CxxModules) {
+SmallVector Out;
+SmallVector Tokens;
+
+StringRef Source = R"(
+  module;
+  #include "textual-header.h"
+
+  export module m;
+  exp\
+ort \
+import \
+:l [[rename]];
+
+  export void f();
+
+  void h() {
+import.a = 3;
+import = 3;
+import <<= 3;
+import->a = 3;
+import();
+import . a();
+
+import a b d e d e f e;
+import foo [[no_unique_address]];
+import foo();
+import f(:sefse);
+import f(->a = 3);
+  }
+  )";
+ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out, Tokens));
+EXPECT_STREQ("#include \"textual-header.h\"\nexport module m;\n"
+ "export import :l [[rename]];\n"
+ "import <<= 3;\nimport a b d e d e f e;\n"
+ "import foo [[no_unique_address]];\nimport foo();\n"
+ "import f(:sefse);\nimport f(->a = 3);\n", Out.data());
+ASSERT_EQ(Tokens.size(), 12u);
+EXPECT_EQ(Tokens[0].K,
+  minimize_source_to_dependency_directives::pp_include);
+EXPECT_EQ(Tokens[2].K,
+  minimize_source_to_dependency_directives::cxx_module_decl);
+}
+
 } // end anonymous namespace
Index: lib/Lex/DependencyDirectivesSourceMinimizer.cpp
===
--- lib/Lex/DependencyDirectivesSourceMinimizer.cpp
+++ lib/Lex/DependencyDirectivesSourceMinimizer.cpp
@@ -59,6 +59,7 @@
   LLVM_NODISCARD bool minimizeImpl(const char *First, const char *const End);
   LLVM_NODISCARD bool lexPPLine(const char *&First, const char *const End);
   LLVM_NODISCARD bool lexAt(const char *&First, const char *const End);
+  LLVM_NODISCARD bool lexModule(const char *&First, const char *const End);
   LLVM_NODISCARD bool lexDefine(const char *&First, const char *const End);
   LLVM_NODISCARD bool lexPragma(const char *&First, const char *const End);
   LLVM_NODISCARD bool lexEndif(const char *&First, const char *const End);
@@ -576,6 +577,59 @@
   return false;
 }
 
+bool Minimizer::lexModule(const char *&First, const char *const End) {
+  IdInfo Id = lexIdentifier(First, End);
+  First = Id.Last;
+  bool Export = false;
+  if (Id.Name == "export") {
+Export = true;
+skipWhitespace(First, End);
+if (!isIdentifierBody(*First)) {
+  skipLine(First, End);
+  return false;
+}
+Id = lexIdentifier(First, End);
+First = Id.Last;
+  }
+
+  if (Id.Name != "module" && Id.Name != "import") {
+skipLine(First, End);
+return false;
+  }
+
+  skipWhitespace(First, End);
+
+  // Ignore this as a module directive if the next character can't be part of
+  // an import.
+
+  switch (*First) {
+  case ':':
+  case '<':
+  case '"':
+break;
+  default:
+if (!isIdentifierBody(*First)) {
+  skipLine(First, End);
+  return false;
+}
+  }
+
+  if (Export) {
+makeToken(cxx_export_decl);
+append("export ");
+  }
+
+  if (Id.Name == "module")
+makeToken(cxx_module_decl);
+  else
+makeToken(cxx_import_decl);
+  app

[PATCH] D65989: [clang-scan-deps] Add minimizer support for C++20 modules.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese closed this revision.
Bigcheese added a comment.

Fixed and committed as r368381.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65989



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

This fix works, but we could also use openat to get around max path length 
issues.  Windows also has an API that can be used similarly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:130
+
+  const DirectoryEntry *getDir() const { return Entry.getDir(); }
+

Isn't this incorrect in the case of symlinks?



Comment at: clang/include/clang/Basic/FileManager.h:249-251
+  /// This function is deprecated and will be removed at some point in the
+  /// future, new clients should use
+  ///  \c getFileRef.

`LLVM_DEPRECATED()`?  (or w/e the name is of our depreciation attribute macro).


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

https://reviews.llvm.org/D65907



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

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese requested changes to this revision.
Bigcheese added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Basic/SourceManager.h:1024
+  Optional getFileEntryRefForID(FileID FID) const {
+bool MyInvalid = false;
+const SrcMgr::SLocEntry &Entry = getSLocEntry(FID, &MyInvalid);

`Invalid`?  I realize this is copied from above, but i'd fix the name.



Comment at: clang/lib/Lex/PPDirectives.cpp:1757
 
-  if (!File) {
+  auto LookupHeaderFile = [&]() -> Optional {
+Optional File = LookupFile(

This lambda is huge and I only see it used once.  Should this be a separate 
function?


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

https://reviews.llvm.org/D65907



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

[PATCH] D63518: BitStream reader: propagate errors

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

This is a pretty big behavior change.  Is clang-doc expecting to get files with 
unknown blocks?



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

[PATCH] D64149: [clang-scan-deps] use `-Wno-error` when scanning for dependencies

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D64149



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

[PATCH] D64164: [clang][ARM] Fix msvc arm{64} builtins to use int on LLP64 systems.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added reviewers: bruno, rnk, bkelley.
Herald added subscribers: kristina, jfb, dexonsmith, kristof.beyls, javed.absar.
Herald added a project: clang.

The `InterlockedX_{acq,nf,rel}` functions deal with 32 bits which is long on
MSVC, but int on most other systems.

This also checks that `ReadStatusRegister` and `WriteStatusRegister` have
the correct type on aarch64-darwin.


Repository:
  rC Clang

https://reviews.llvm.org/D64164

Files:
  include/clang/Basic/BuiltinsAArch64.def
  include/clang/Basic/BuiltinsARM.def
  test/CodeGen/arm64-microsoft-status-reg.cpp
  test/CodeGen/ms-intrinsics-other.c

Index: test/CodeGen/ms-intrinsics-other.c
===
--- test/CodeGen/ms-intrinsics-other.c
+++ test/CodeGen/ms-intrinsics-other.c
@@ -4,6 +4,15 @@
 // RUN: %clang_cc1 -ffreestanding -fms-extensions \
 // RUN: -triple x86_64--linux -Oz -emit-llvm %s -o - \
 // RUN: | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -fms-extensions \
+// RUN: -triple aarch64--darwin -Oz -emit-llvm %s -o - \
+// RUN: | FileCheck %s --check-prefix=CHECK-ARM-ARM64
+// RUN: %clang_cc1 -ffreestanding -fms-extensions \
+// RUN: -triple aarch64--darwin -Oz -emit-llvm %s -o - \
+// RUN: | FileCheck %s --check-prefix=CHECK-ARM
+// RUN: %clang_cc1 -ffreestanding -fms-extensions \
+// RUN: -triple armv7--darwin -Oz -emit-llvm %s -o - \
+// RUN: | FileCheck %s --check-prefix=CHECK-ARM
 
 // LP64 targets use 'long' as 'int' for MS intrinsics (-fms-extensions)
 #ifdef __LP64__
@@ -196,3 +205,214 @@
 // CHECK:  [[RESULT:%[0-9]+]] = tail call i64 @llvm.ctpop.i64(i64 %x)
 // CHECK: ret i64 [[RESULT]]
 // CHECK: }
+
+#if defined(__aarch64__)
+LONG test_InterlockedAdd(LONG volatile *Addend, LONG Value) {
+  return _InterlockedAdd(Addend, Value);
+}
+
+// CHECK-ARM-ARM64: define{{.*}}i32 @test_InterlockedAdd(i32*{{[a-z_ ]*}}%Addend, i32 %Value) {{.*}} {
+// CHECK-ARM-ARM64: %[[OLDVAL:[0-9]+]] = atomicrmw add i32* %Addend, i32 %Value seq_cst
+// CHECK-ARM-ARM64: %[[NEWVAL:[0-9]+]] = add i32 %[[OLDVAL:[0-9]+]], %Value
+// CHECK-ARM-ARM64: ret i32 %[[NEWVAL:[0-9]+]]
+#endif
+
+#if defined(__arm__) || defined(__aarch64__)
+LONG test_InterlockedExchangeAdd_acq(LONG volatile *value, LONG mask) {
+  return _InterlockedExchangeAdd_acq(value, mask);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedExchangeAdd_acq(i32*{{[a-z_ ]*}}%value, i32{{[a-z_ ]*}}%mask){{.*}}{
+// CHECK-ARM:   [[RESULT:%[0-9]+]] = atomicrmw add i32* %value, i32 %mask acquire
+// CHECK-ARM:   ret i32 [[RESULT:%[0-9]+]]
+// CHECK-ARM: }
+LONG test_InterlockedExchangeAdd_rel(LONG volatile *value, LONG mask) {
+  return _InterlockedExchangeAdd_rel(value, mask);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedExchangeAdd_rel(i32*{{[a-z_ ]*}}%value, i32{{[a-z_ ]*}}%mask){{.*}}{
+// CHECK-ARM:   [[RESULT:%[0-9]+]] = atomicrmw add i32* %value, i32 %mask release
+// CHECK-ARM:   ret i32 [[RESULT:%[0-9]+]]
+// CHECK-ARM: }
+LONG test_InterlockedExchangeAdd_nf(LONG volatile *value, LONG mask) {
+  return _InterlockedExchangeAdd_nf(value, mask);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedExchangeAdd_nf(i32*{{[a-z_ ]*}}%value, i32{{[a-z_ ]*}}%mask){{.*}}{
+// CHECK-ARM:   [[RESULT:%[0-9]+]] = atomicrmw add i32* %value, i32 %mask monotonic
+// CHECK-ARM:   ret i32 [[RESULT:%[0-9]+]]
+// CHECK-ARM: }
+
+LONG test_InterlockedExchange_acq(LONG volatile *value, LONG mask) {
+  return _InterlockedExchange_acq(value, mask);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedExchange_acq(i32*{{[a-z_ ]*}}%value, i32{{[a-z_ ]*}}%mask){{.*}}{
+// CHECK-ARM:   [[RESULT:%[0-9]+]] = atomicrmw xchg i32* %value, i32 %mask acquire
+// CHECK-ARM:   ret i32 [[RESULT:%[0-9]+]]
+// CHECK-ARM: }
+LONG test_InterlockedExchange_rel(LONG volatile *value, LONG mask) {
+  return _InterlockedExchange_rel(value, mask);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedExchange_rel(i32*{{[a-z_ ]*}}%value, i32{{[a-z_ ]*}}%mask){{.*}}{
+// CHECK-ARM:   [[RESULT:%[0-9]+]] = atomicrmw xchg i32* %value, i32 %mask release
+// CHECK-ARM:   ret i32 [[RESULT:%[0-9]+]]
+// CHECK-ARM: }
+LONG test_InterlockedExchange_nf(LONG volatile *value, LONG mask) {
+  return _InterlockedExchange_nf(value, mask);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedExchange_nf(i32*{{[a-z_ ]*}}%value, i32{{[a-z_ ]*}}%mask){{.*}}{
+// CHECK-ARM:   [[RESULT:%[0-9]+]] = atomicrmw xchg i32* %value, i32 %mask monotonic
+// CHECK-ARM:   ret i32 [[RESULT:%[0-9]+]]
+// CHECK-ARM: }
+
+LONG test_InterlockedCompareExchange_acq(LONG volatile *Destination, LONG Exchange, LONG Comperand) {
+  return _InterlockedCompareExchange_acq(Destination, Exchange, Comperand);
+}
+// CHECK-ARM: define{{.*}}i32 @test_InterlockedCompareExchange_acq(i32*{{[a-z_ ]*}}%Destination, i32{{[a-z_ ]*}}%Exchange, i32{{[a-z_ ]*}}%Comperand){{.*}}{
+// CHECK-ARM: [[TMP:%[0-9]+]] = cmpxchg volatile i32* %Destination, i3

[PATCH] D64164: [clang][ARM] Fix msvc arm{64} builtins to use int on LLP64 systems.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D64164#1569442 , @efriedma wrote:

> Do the changes to BuiltinsARM.def have any practical effect?  long should be 
> 32 bits on all 32-bit ARM targets.


I don't think they do right now.  I updated them there as that's what the 
original patch did.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64164



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

[PATCH] D64164: [clang][ARM] Fix msvc arm{64} builtins to use int on LP64 systems.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D64164#1569679 , @rnk wrote:

> Please check the commit message:
>
> > [clang][ARM] Fix msvc arm{64} builtins to use int on LLP64 systems.
>
> I think you mean "use int on LP64 systems", since long is 32-bits on LLP64, 
> right?
>
> Otherwise, sounds good.


Thanks, I did indeed mean LP64.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64164



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

[PATCH] D64525: [clang-scan-deps] Dependency directives source minimizer: single quotes are not digit separators after a valid character literal prefix

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:270
+return false;
+  if (Prev == '8' && (Cur - 1 != Start) && *(Cur - 2) == 'u')
+return false;

Are we sure at this point that it's always safe to jump back 2?


Repository:
  rC Clang

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

https://reviews.llvm.org/D64525



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

[PATCH] D64525: [clang-scan-deps] Dependency directives source minimizer: single quotes are not digit separators after a valid character literal prefix

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added inline comments.



Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:270
+return false;
+  if (Prev == '8' && (Cur - 1 != Start) && *(Cur - 2) == 'u')
+return false;

arphaman wrote:
> Bigcheese wrote:
> > Are we sure at this point that it's always safe to jump back 2?
> It should be, because otherwise `Start` would've been equals to `Cur - 1` 
> which we check for right before the dereference.
Oh, obviously.  I missed that check.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64525



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

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D55463



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

[PATCH] D62271: [Driver] Fix -working-directory issues

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added reviewers: dexonsmith, arphaman, jkorous.
Herald added a project: clang.

Currently the `-working-directory` option does not actually impact the working
directory for all of the clang driver, it only impacts how files are looked up
to make sure they exist. This means that that clang passes the wrong paths
to -fdebug-compilation-dir and -coverage-notes-file.

This patch fixes that by changing all the places in the driver where we convert
to absolute paths to use the VFS, and then calling setCurrentWorkingDirectory on
the VFS. This also changes the default VFS for `Driver` to use a virtualized
working directory, instead of changing the process's working directory.


Repository:
  rC Clang

https://reviews.llvm.org/D62271

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/working-directory.c

Index: test/Driver/working-directory.c
===
--- test/Driver/working-directory.c
+++ test/Driver/working-directory.c
@@ -1,3 +1,11 @@
 // RUN: %clang -### -working-directory /no/such/dir/ input 2>&1 | FileCheck %s
+// RUN: %clang -### -working-directory %p/Inputs no_such_file.cpp -c 2>&1 | FileCheck %s --check-prefix=CHECK_NO_FILE
+// RUN: %clang -### -working-directory %p/Inputs pchfile.cpp -c 2>&1 | FileCheck %s --check-prefix=CHECK_WORKS
 
-//CHECK: no such file or directory: '/no/such/dir/input'
+// CHECK: unable to set working directory: /no/such/dir/
+
+// CHECK_NO_FILE: no such file or directory: 'no_such_file.cpp'
+
+// CHECK_WORKS: "-coverage-notes-file" "{{[^"]+}}test/Driver/Inputs/pchfile.gcno"
+// CHECK_WORKS: "-working-directory" "{{[^"]+}}test/Driver/Inputs"
+// CHECK_WORKS: "-fdebug-compilation-dir" "{{[^"]+}}test/Driver/Inputs"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -616,11 +616,11 @@
 }
 
 /// Add a CC1 option to specify the debug compilation directory.
-static void addDebugCompDirArg(const ArgList &Args, ArgStringList &CmdArgs) {
-  SmallString<128> cwd;
-  if (!llvm::sys::fs::current_path(cwd)) {
+static void addDebugCompDirArg(const ArgList &Args, ArgStringList &CmdArgs,
+   const llvm::vfs::FileSystem &VFS) {
+  if (llvm::ErrorOr CWD = VFS.getCurrentWorkingDirectory()) {
 CmdArgs.push_back("-fdebug-compilation-dir");
-CmdArgs.push_back(Args.MakeArgString(cwd));
+CmdArgs.push_back(Args.MakeArgString(*CWD));
   }
 }
 
@@ -885,13 +885,8 @@
   else
 OutputFilename = llvm::sys::path::filename(Output.getBaseInput());
   SmallString<128> CoverageFilename = OutputFilename;
-  if (llvm::sys::path::is_relative(CoverageFilename)) {
-SmallString<128> Pwd;
-if (!llvm::sys::fs::current_path(Pwd)) {
-  llvm::sys::path::append(Pwd, CoverageFilename);
-  CoverageFilename.swap(Pwd);
-}
-  }
+  if (llvm::sys::path::is_relative(CoverageFilename))
+(void)D.getVFS().makeAbsolute(CoverageFilename);
   llvm::sys::path::replace_extension(CoverageFilename, "gcno");
   CmdArgs.push_back(Args.MakeArgString(CoverageFilename));
 
@@ -4354,7 +4349,7 @@
 CmdArgs.push_back("-fno-autolink");
 
   // Add in -fdebug-compilation-dir if necessary.
-  addDebugCompDirArg(Args, CmdArgs);
+  addDebugCompDirArg(Args, CmdArgs, D.getVFS());
 
   addDebugPrefixMapArg(D, Args, CmdArgs);
 
@@ -6063,7 +6058,7 @@
 DebugInfoKind = (WantDebug ? codegenoptions::LimitedDebugInfo
: codegenoptions::NoDebugInfo);
 // Add the -fdebug-compilation-dir flag if needed.
-addDebugCompDirArg(Args, CmdArgs);
+addDebugCompDirArg(Args, CmdArgs, C.getDriver().getVFS());
 
 addDebugPrefixMapArg(getToolChain().getDriver(), Args, CmdArgs);
 
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -133,7 +133,7 @@
 
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
-this->VFS = llvm::vfs::getRealFileSystem();
+this->VFS = llvm::vfs::createPhysicalFileSystem().release();
 
   Name = llvm::sys::path::filename(ClangExecutable);
   Dir = llvm::sys::path::parent_path(ClangExecutable);
@@ -1005,6 +1005,11 @@
 }
   }
 
+  // Check for working directory option before accessing any files
+  if (Arg *WD = Args.getLastArg(options::OPT_working_directory))
+if (std::error_code EC = VFS->setCurrentWorkingDirectory(WD->getValue()))
+  Diag(diag::err_drv_unable_to_set_working_directory) << WD->getValue();
+
   // FIXME: This stuff needs to go into the Compilation, not the driver.
   bool CCCPrintPhases;
 
@@ -1986,20 +1991,11 @@
   if (Value == "-")
 return true;
 
-  SmallString<64> Path(Value);
-  if (Arg *WorkDir = Args.getLastArg(options::OPT_

[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added reviewers: rsmith, bruno.
Herald added a subscriber: dexonsmith.
Herald added a project: clang.

Repository:
  rC Clang

https://reviews.llvm.org/D68528

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Lex/HeaderSearchOptions.h
  lib/Frontend/CompilerInvocation.cpp
  test/Modules/context-hash.c

Index: test/Modules/context-hash.c
===
--- /dev/null
+++ test/Modules/context-hash.c
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t1
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t2
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s \
+// RUN:   -fmodules-strict-hash -Rmodule-build 2> %t3
+// RUN: echo %t | cat - %t1 %t2 %t3 | FileCheck %s
+
+// This test verifies that only strict hashing includes search paths in the
+// module context hash.
+
+#include 
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK: building module 'cstd' as '[[PREFIX]]{{[/\\]}}[[CONTEXT_HASH:[A-Z0-9]+]]{{[/\\]}}cstd-[[AST_HASH:[A-Z0-9]+]].pcm'
+// CHECK: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2060,6 +2060,7 @@
 Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
+  Opts.ModulesStrictHash = Args.hasArg(OPT_fmodules_strict_hash);
   Opts.ModulesValidateDiagnosticOptions =
   !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
@@ -3519,6 +3520,7 @@
   using llvm::hash_code;
   using llvm::hash_value;
   using llvm::hash_combine;
+  using llvm::hash_combine_range;
 
   // Start the signature with the compiler version.
   // FIXME: We'd rather use something more cryptographically sound than
@@ -3573,6 +3575,15 @@
   hsOpts.ModulesValidateDiagnosticOptions);
   code = hash_combine(code, hsOpts.ResourceDir);
 
+  if (hsOpts.ModulesStrictHash) {
+hash_code SHPC = hash_combine_range(hsOpts.SystemHeaderPrefixes.begin(),
+hsOpts.SystemHeaderPrefixes.end());
+hash_code UEC = hash_combine_range(hsOpts.UserEntries.begin(),
+   hsOpts.UserEntries.end());
+code = hash_combine(code, hsOpts.SystemHeaderPrefixes.size(), SHPC,
+hsOpts.UserEntries.size(), UEC);
+  }
+
   // Extend the signature with the user build path.
   code = hash_combine(code, hsOpts.ModuleUserBuildPath);
 
Index: include/clang/Lex/HeaderSearchOptions.h
===
--- include/clang/Lex/HeaderSearchOptions.h
+++ include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringRef.h"
 #include 
@@ -202,6 +203,12 @@
 
   unsigned ModulesHashContent : 1;
 
+  /// Weather we should include all things that could impact the module in the
+  /// hash.
+  ///
+  /// This includes things like the full header search path.
+  unsigned ModulesStrictHash : 1;
+
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
 ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
@@ -209,7 +216,8 @@
 UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
 ModulesValidateOncePerBuildSession(false),
 ModulesValidateSystemHeaders(false), UseDebugInfo(false),
-ModulesValidateDiagnosticOptions(true), ModulesHashContent(false) {}
+ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+ModulesStrictHash(false) {}
 
   /// AddPath - Add the \p Path path to the specified \p Group list.
   void AddPath(StringRef Path, frontend::IncludeDirGroup Group,
@@ -233,6 +241,15 @@
   }
 };
 
+inline llvm::hash_code hash_value(const HeaderSearchOptions::Entry &E) {
+  return llvm::hash_combine(E.Path, E.Group, E.IsFramework, E.IgnoreSysRoot);
+}
+
+

[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 223345.
Bigcheese retitled this revision from "[Implicit Modules] Add -cc1 option 
-fmodules-strict-hash which includes search paths." to "[Implicit Modules] Add 
-cc1 option -fmodules-strict-hash which includes search paths and diagnostics.".
Bigcheese added a comment.

Add diagnostics.


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

https://reviews.llvm.org/D68528

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Lex/HeaderSearchOptions.h
  lib/Frontend/CompilerInvocation.cpp
  test/Modules/context-hash.c

Index: test/Modules/context-hash.c
===
--- /dev/null
+++ test/Modules/context-hash.c
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t1
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t2
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s \
+// RUN:   -fmodules-strict-hash -Rmodule-build 2> %t3
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fmodules-strict-hash \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t4
+// RUN: echo %t | cat - %t1 %t2 %t3 %t4 | FileCheck %s
+
+// This test verifies that only strict hashing includes search paths and
+// diagnostics in the module context hash.
+
+#include 
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK: building module 'cstd' as '[[PREFIX]]{{[/\\]}}[[CONTEXT_HASH:[A-Z0-9]+]]{{[/\\]}}cstd-[[AST_HASH:[A-Z0-9]+]].pcm'
+// CHECK: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2060,6 +2060,7 @@
 Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
+  Opts.ModulesStrictHash = Args.hasArg(OPT_fmodules_strict_hash);
   Opts.ModulesValidateDiagnosticOptions =
   !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
@@ -3519,6 +3520,7 @@
   using llvm::hash_code;
   using llvm::hash_value;
   using llvm::hash_combine;
+  using llvm::hash_combine_range;
 
   // Start the signature with the compiler version.
   // FIXME: We'd rather use something more cryptographically sound than
@@ -3573,6 +3575,24 @@
   hsOpts.ModulesValidateDiagnosticOptions);
   code = hash_combine(code, hsOpts.ResourceDir);
 
+  if (hsOpts.ModulesStrictHash) {
+hash_code SHPC = hash_combine_range(hsOpts.SystemHeaderPrefixes.begin(),
+hsOpts.SystemHeaderPrefixes.end());
+hash_code UEC = hash_combine_range(hsOpts.UserEntries.begin(),
+   hsOpts.UserEntries.end());
+code = hash_combine(code, hsOpts.SystemHeaderPrefixes.size(), SHPC,
+hsOpts.UserEntries.size(), UEC);
+
+const DiagnosticOptions &diagOpts = getDiagnosticOpts();
+#define DIAGOPT(Name, Bits, Default) \
+  code = hash_combine(code, diagOpts.Name);
+#define ENUM_DIAGOPT(Name, Type, Bits, Default) \
+  code = hash_combine(code, diagOpts.get##Name());
+#include "clang/Basic/DiagnosticOptions.def"
+#undef DIAGOPT
+#undef ENUM_DIAGOPT
+  }
+
   // Extend the signature with the user build path.
   code = hash_combine(code, hsOpts.ModuleUserBuildPath);
 
Index: include/clang/Lex/HeaderSearchOptions.h
===
--- include/clang/Lex/HeaderSearchOptions.h
+++ include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringRef.h"
 #include 
@@ -202,6 +203,12 @@
 
   unsigned ModulesHashContent : 1;
 
+  /// Weather we should include all things that could impact the module in the
+  /// hash.
+  ///
+  /// This includes things like the full 

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D68193#1701426 , @kousikk wrote:

> @arphaman I don't mind changing this if there are race conditions as you say, 
> but isn't the assumption of the tool that the filesystem remains unchanged 
> for a single run of the tool? If so, should we actually throw error 
> conditions instead of crashing in those cases?


This is true for a normal build, but it is not true with modules. All file 
system access goes through this, even for modules. I'm currently fixing an 
issue where we cache the non-existence of the module cache directory, causing 
the subsequent module load to fail. There is also an issue due to minimization 
where if we return the underlying stat result, the file size after minimization 
is different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68193



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

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:83
 
+  std::error_code getError() const {
+assert(isValid() && "not initialized");

Is this actually called anywhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68193



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

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

lgtm, but wait for Alex or Duncan to also take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68193



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

[PATCH] D68835: [clang-scan-deps] Add basic support for Clang modules.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added a reviewer: arphaman.
Herald added subscribers: tschuett, dexonsmith.
Herald added a project: clang.

This fixes two issues that prevent simple uses of Clang modules from working.

- We would previously minimize _every_ file opened by clang, even module maps 
and module pcm files. Now we only minimize files with known extensions. It 
would be better if we knew which files clang intended to open as a source file, 
but this works for now.

- We previously cached every lookup, even failed lookups. This is a problem 
because clang stats the module cache directory before building a module and 
creating that directory. If we cache that failure then the subsequent pcm load 
doesn't see the module cache and fails.

Overall this still leaves us building minmized modules on disk during scanning.
This will need to be improved eventually for performance, but this is correct,
and works for now.


Repository:
  rC Clang

https://reviews.llvm.org/D68835

Files:
  lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  test/ClangScanDeps/Inputs/module.modulemap
  test/ClangScanDeps/Inputs/modules_cdb.json
  test/ClangScanDeps/modules.cpp

Index: test/ClangScanDeps/modules.cpp
===
--- /dev/null
+++ test/ClangScanDeps/modules.cpp
@@ -0,0 +1,41 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: rm -rf %t.module-cache
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/modules_cdb_input.cpp
+// RUN: cp %s %t.dir/modules_cdb_input2.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: cp %S/Inputs/header2.h %t.dir/Inputs/header2.h
+// RUN: cp %S/Inputs/module.modulemap %t.dir/Inputs/module.modulemap
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/modules_cdb.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO %s
+//
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs. Note that the 'NOT' check is not used
+// as it might fail if the results for `modules_cdb_input.cpp` are reported before
+// `modules_cdb_input2.cpp`.
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+
+#include "header.h"
+
+// CHECK1: modules_cdb_input2.cpp
+// CHECK1-NEXT: modules_cdb_input2.cpp
+// CHECK1-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK1-NEXT: Inputs{{/|\\}}header2.h
+// CHECK1: Inputs{{/|\\}}header.h
+
+// CHECK2: modules_cdb_input.cpp
+// CHECK2-NEXT: Inputs/module.modulemap
+// CHECK2-NEXT: Inputs{{/|\\}}header.h
+// CHECK2NO-NOT: header2
Index: test/ClangScanDeps/Inputs/modules_cdb.json
===
--- /dev/null
+++ test/ClangScanDeps/Inputs/modules_cdb.json
@@ -0,0 +1,13 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/modules_cdb_input2.cpp -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb2.d -fmodules -fmodules-cache-path=DIR/module-cache",
+  "file": "DIR/modules_cdb_input2.cpp"
+},
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fmodules-cache-path=DIR/module-cache",
+  "file": "DIR/modules_cdb_input.cpp"
+}
+]
+
Index: test/ClangScanDeps/Inputs/module.modulemap
===
--- /dev/null
+++ test/ClangScanDeps/Inputs/module.modulemap
@@ -0,0 +1,7 @@
+module header1 {
+  header "header.h"
+}
+
+module header2 {
+header "header2.h"
+}
Index: lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,6 +122,31 @@
   return It.first->getValue();
 }
 
+/// Whitelist file extensions that should be minimized, treating no extension as
+/// a source file that should be minimized.
+///
+/// This is kinda hacky, it would be better if we knew what kind of file Clang
+/// was expecting instead.
+static bool shouldMinimize(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return true; // C++ standard library
+  return llvm::StringSwitch(Ext)
+.CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true)
+.CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true)
+.C

[PATCH] D68835: [clang-scan-deps] Add basic support for Clang modules.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 224507.
Bigcheese marked 2 inline comments as done.
Bigcheese added a comment.

Addressed review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D68835

Files:
  lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  test/ClangScanDeps/Inputs/module.modulemap
  test/ClangScanDeps/Inputs/modules_cdb.json
  test/ClangScanDeps/modules.cpp

Index: test/ClangScanDeps/modules.cpp
===
--- /dev/null
+++ test/ClangScanDeps/modules.cpp
@@ -0,0 +1,41 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: rm -rf %t.module-cache
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/modules_cdb_input.cpp
+// RUN: cp %s %t.dir/modules_cdb_input2.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: cp %S/Inputs/header2.h %t.dir/Inputs/header2.h
+// RUN: cp %S/Inputs/module.modulemap %t.dir/Inputs/module.modulemap
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/modules_cdb.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO %s
+//
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs. Note that the 'NOT' check is not used
+// as it might fail if the results for `modules_cdb_input.cpp` are reported before
+// `modules_cdb_input2.cpp`.
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+
+#include "header.h"
+
+// CHECK1: modules_cdb_input2.cpp
+// CHECK1-NEXT: modules_cdb_input2.cpp
+// CHECK1-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK1-NEXT: Inputs{{/|\\}}header2.h
+// CHECK1: Inputs{{/|\\}}header.h
+
+// CHECK2: modules_cdb_input.cpp
+// CHECK2-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK2-NEXT: Inputs{{/|\\}}header.h
+// CHECK2NO-NOT: header2
Index: test/ClangScanDeps/Inputs/modules_cdb.json
===
--- /dev/null
+++ test/ClangScanDeps/Inputs/modules_cdb.json
@@ -0,0 +1,13 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/modules_cdb_input2.cpp -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb2.d -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/modules_cdb_input2.cpp"
+},
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/modules_cdb_input.cpp"
+}
+]
+
Index: test/ClangScanDeps/Inputs/module.modulemap
===
--- /dev/null
+++ test/ClangScanDeps/Inputs/module.modulemap
@@ -0,0 +1,7 @@
+module header1 {
+  header "header.h"
+}
+
+module header2 {
+header "header2.h"
+}
Index: lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,6 +122,31 @@
   return It.first->getValue();
 }
 
+/// Whitelist file extensions that should be minimized, treating no extension as
+/// a source file that should be minimized.
+///
+/// This is kinda hacky, it would be better if we knew what kind of file Clang
+/// was expecting instead.
+static bool shouldMinimize(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return true; // C++ standard library
+  return llvm::StringSwitch(Ext)
+.CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true)
+.CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true)
+.CasesLower(".m", ".mm", true)
+.CasesLower(".def", ".inc", true)
+.Default(false);
+}
+
+
+static bool shouldCacheStatFailures(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return false; // This may be the module cache directory.
+  return shouldMinimize(Filename); // Only cache stat failures on source files.
+}
+
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 const StringRef Filename) {
@@ -132,7 +157,8 @@
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
 
-  bool KeepOriginalSource = IgnoredFiles.count(Fil

[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.

[Michael Spencer via Phabricator via cfe-commits]

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



Comment at: include/clang/Lex/HeaderSearchOptions.h:209
+  ///
+  /// This includes things like the full header search path.
+  unsigned ModulesStrictHash : 1;

bruno wrote:
> What else do you plan to add in the future as part of "all the things that 
> could impact"? It seems to me that by default this should always be the case, 
> but header search related things are special because one might want to  
> handwave on correctness to have smaller caches (default behavior right now). 
> 
> I wonder if we should instead have `fmodules-strict-header-seach` and later 
> on add a more generic thing that group such cases? WDYT?
This also includes diagnostic options. I'm not sure it's useful for users to 
pick each individual thing they care about for the hash. The intent here is 
just to capture every possible difference we find.


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

https://reviews.llvm.org/D68528



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

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm. Jan may want to take a look as I believe he was looking at a related 
issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69017



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

[PATCH] D68835: [clang-scan-deps] Add basic support for Clang modules.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 225311.
Bigcheese added a comment.

Added .i, .ii, .mi, and .mmi as files to minimize.


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

https://reviews.llvm.org/D68835

Files:
  lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  test/ClangScanDeps/Inputs/module.modulemap
  test/ClangScanDeps/Inputs/modules_cdb.json
  test/ClangScanDeps/modules.cpp

Index: test/ClangScanDeps/modules.cpp
===
--- /dev/null
+++ test/ClangScanDeps/modules.cpp
@@ -0,0 +1,41 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: rm -rf %t.module-cache
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/modules_cdb_input.cpp
+// RUN: cp %s %t.dir/modules_cdb_input2.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: cp %S/Inputs/header2.h %t.dir/Inputs/header2.h
+// RUN: cp %S/Inputs/module.modulemap %t.dir/Inputs/module.modulemap
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/modules_cdb.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO %s
+//
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs. Note that the 'NOT' check is not used
+// as it might fail if the results for `modules_cdb_input.cpp` are reported before
+// `modules_cdb_input2.cpp`.
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+
+#include "header.h"
+
+// CHECK1: modules_cdb_input2.cpp
+// CHECK1-NEXT: modules_cdb_input2.cpp
+// CHECK1-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK1-NEXT: Inputs{{/|\\}}header2.h
+// CHECK1: Inputs{{/|\\}}header.h
+
+// CHECK2: modules_cdb_input.cpp
+// CHECK2-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK2-NEXT: Inputs{{/|\\}}header.h
+// CHECK2NO-NOT: header2
Index: test/ClangScanDeps/Inputs/modules_cdb.json
===
--- /dev/null
+++ test/ClangScanDeps/Inputs/modules_cdb.json
@@ -0,0 +1,13 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/modules_cdb_input2.cpp -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb2.d -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/modules_cdb_input2.cpp"
+},
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/modules_cdb_input.cpp"
+}
+]
+
Index: test/ClangScanDeps/Inputs/module.modulemap
===
--- /dev/null
+++ test/ClangScanDeps/Inputs/module.modulemap
@@ -0,0 +1,7 @@
+module header1 {
+  header "header.h"
+}
+
+module header2 {
+header "header2.h"
+}
Index: lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,6 +122,32 @@
   return It.first->getValue();
 }
 
+/// Whitelist file extensions that should be minimized, treating no extension as
+/// a source file that should be minimized.
+///
+/// This is kinda hacky, it would be better if we knew what kind of file Clang
+/// was expecting instead.
+static bool shouldMinimize(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return true; // C++ standard library
+  return llvm::StringSwitch(Ext)
+.CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true)
+.CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true)
+.CasesLower(".m", ".mm", true)
+.CasesLower(".i", ".ii", ".mi", ".mmi", true)
+.CasesLower(".def", ".inc", true)
+.Default(false);
+}
+
+
+static bool shouldCacheStatFailures(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return false; // This may be the module cache directory.
+  return shouldMinimize(Filename); // Only cache stat failures on source files.
+}
+
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 const StringRef Filename) {
@@ -132,7 +158,8 @@
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
 
-  bool KeepOriginalSource = IgnoredFiles.co

[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 225326.
Bigcheese marked an inline comment as done.
Bigcheese added a comment.

Fixed spelling and updated comment.


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

https://reviews.llvm.org/D68528

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Lex/HeaderSearchOptions.h
  lib/Frontend/CompilerInvocation.cpp
  test/Modules/context-hash.c

Index: test/Modules/context-hash.c
===
--- /dev/null
+++ test/Modules/context-hash.c
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t1
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t2
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s \
+// RUN:   -fmodules-strict-hash -Rmodule-build 2> %t3
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fmodules-strict-hash \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t4
+// RUN: echo %t | cat - %t1 %t2 %t3 %t4 | FileCheck %s
+
+// This test verifies that only strict hashing includes search paths and
+// diagnostics in the module context hash.
+
+#include 
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK: building module 'cstd' as '[[PREFIX]]{{[/\\]}}[[CONTEXT_HASH:[A-Z0-9]+]]{{[/\\]}}cstd-[[AST_HASH:[A-Z0-9]+]].pcm'
+// CHECK: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2065,6 +2065,7 @@
 Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
+  Opts.ModulesStrictHash = Args.hasArg(OPT_fmodules_strict_hash);
   Opts.ModulesValidateDiagnosticOptions =
   !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
@@ -3544,6 +3545,7 @@
   using llvm::hash_code;
   using llvm::hash_value;
   using llvm::hash_combine;
+  using llvm::hash_combine_range;
 
   // Start the signature with the compiler version.
   // FIXME: We'd rather use something more cryptographically sound than
@@ -3598,6 +3600,24 @@
   hsOpts.ModulesValidateDiagnosticOptions);
   code = hash_combine(code, hsOpts.ResourceDir);
 
+  if (hsOpts.ModulesStrictHash) {
+hash_code SHPC = hash_combine_range(hsOpts.SystemHeaderPrefixes.begin(),
+hsOpts.SystemHeaderPrefixes.end());
+hash_code UEC = hash_combine_range(hsOpts.UserEntries.begin(),
+   hsOpts.UserEntries.end());
+code = hash_combine(code, hsOpts.SystemHeaderPrefixes.size(), SHPC,
+hsOpts.UserEntries.size(), UEC);
+
+const DiagnosticOptions &diagOpts = getDiagnosticOpts();
+#define DIAGOPT(Name, Bits, Default) \
+  code = hash_combine(code, diagOpts.Name);
+#define ENUM_DIAGOPT(Name, Type, Bits, Default) \
+  code = hash_combine(code, diagOpts.get##Name());
+#include "clang/Basic/DiagnosticOptions.def"
+#undef DIAGOPT
+#undef ENUM_DIAGOPT
+  }
+
   // Extend the signature with the user build path.
   code = hash_combine(code, hsOpts.ModuleUserBuildPath);
 
Index: include/clang/Lex/HeaderSearchOptions.h
===
--- include/clang/Lex/HeaderSearchOptions.h
+++ include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringRef.h"
 #include 
@@ -206,6 +207,13 @@
 
   unsigned ModulesHashContent : 1;
 
+  /// Whether we should include all things that could impact the module in the
+  /// hash.
+  ///
+  /// This includes things like the full header search path, and enabled
+  /// diagnostics.
+  unsigned ModulesStrictHash : 1;
+
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleF

[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

While adding the documentation I realized that a better name for this option 
would be `-fmodules-strict-context-hash` to make it clear which hash it's 
referring to.


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

https://reviews.llvm.org/D68528



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

[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.

[Michael Spencer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14a3f77ba103: [Implicit Modules] Add -cc1 option 
-fmodules-strict-context-hash which includes… (authored by Bigcheese).

Changed prior to commit:
  https://reviews.llvm.org/D68528?vs=225326&id=225731#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68528

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Modules/context-hash.c

Index: clang/test/Modules/context-hash.c
===
--- /dev/null
+++ clang/test/Modules/context-hash.c
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t1
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t2
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s \
+// RUN:   -fmodules-strict-context-hash -Rmodule-build 2> %t3
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fmodules-strict-context-hash \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t4
+// RUN: echo %t | cat - %t1 %t2 %t3 %t4 | FileCheck %s
+
+// This test verifies that only strict hashing includes search paths and
+// diagnostics in the module context hash.
+
+#include 
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK: building module 'cstd' as '[[PREFIX]]{{[/\\]}}[[CONTEXT_HASH:[A-Z0-9]+]]{{[/\\]}}cstd-[[AST_HASH:[A-Z0-9]+]].pcm'
+// CHECK: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2067,6 +2067,7 @@
 Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
+  Opts.ModulesStrictContextHash = Args.hasArg(OPT_fmodules_strict_context_hash);
   Opts.ModulesValidateDiagnosticOptions =
   !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
@@ -3546,6 +3547,7 @@
   using llvm::hash_code;
   using llvm::hash_value;
   using llvm::hash_combine;
+  using llvm::hash_combine_range;
 
   // Start the signature with the compiler version.
   // FIXME: We'd rather use something more cryptographically sound than
@@ -3600,6 +3602,24 @@
   hsOpts.ModulesValidateDiagnosticOptions);
   code = hash_combine(code, hsOpts.ResourceDir);
 
+  if (hsOpts.ModulesStrictContextHash) {
+hash_code SHPC = hash_combine_range(hsOpts.SystemHeaderPrefixes.begin(),
+hsOpts.SystemHeaderPrefixes.end());
+hash_code UEC = hash_combine_range(hsOpts.UserEntries.begin(),
+   hsOpts.UserEntries.end());
+code = hash_combine(code, hsOpts.SystemHeaderPrefixes.size(), SHPC,
+hsOpts.UserEntries.size(), UEC);
+
+const DiagnosticOptions &diagOpts = getDiagnosticOpts();
+#define DIAGOPT(Name, Bits, Default) \
+  code = hash_combine(code, diagOpts.Name);
+#define ENUM_DIAGOPT(Name, Type, Bits, Default) \
+  code = hash_combine(code, diagOpts.get##Name());
+#include "clang/Basic/DiagnosticOptions.def"
+#undef DIAGOPT
+#undef ENUM_DIAGOPT
+  }
+
   // Extend the signature with the user build path.
   code = hash_combine(code, hsOpts.ModuleUserBuildPath);
 
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringRef.h"
 #include 
@@ -206,6 +207,13 @@
 
   unsigned ModulesHashContent : 1;
 

[PATCH] D69186: Refactor DependencyScanningTool to its own file

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM with style nit. I like that this decouples `DependencyScanningTool` from 
printing the results.




Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:105-107
+  llvm::Expected &MaybeFile,
+  SharedStream &OS,
+  SharedStream &Errs) {

This looks like it needs to be clang-formatted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69186



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

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

The current assumption is that the clang-scan-deps binary is the one that comes 
next to the clang binary you are using. There are lots of other differences 
between clang versions than just the resource-dir.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69122



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

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a reviewer: klimek.
Bigcheese added a comment.

I've added Manuel as a reviewer as this patch is also changing the tooling APIs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69122



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

[PATCH] D68835: [clang-scan-deps] Add basic support for Clang modules.

[Michael Spencer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ab6d8236b17: [clang-scan-deps] Add basic support for 
modules. (authored by Bigcheese).

Changed prior to commit:
  https://reviews.llvm.org/D68835?vs=225311&id=226352#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68835

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/test/ClangScanDeps/Inputs/module.modulemap
  clang/test/ClangScanDeps/Inputs/modules_cdb.json
  clang/test/ClangScanDeps/modules.cpp

Index: clang/test/ClangScanDeps/modules.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules.cpp
@@ -0,0 +1,41 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: rm -rf %t.module-cache
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/modules_cdb_input.cpp
+// RUN: cp %s %t.dir/modules_cdb_input2.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: cp %S/Inputs/header2.h %t.dir/Inputs/header2.h
+// RUN: cp %S/Inputs/module.modulemap %t.dir/Inputs/module.modulemap
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/modules_cdb.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO %s
+//
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs. Note that the 'NOT' check is not used
+// as it might fail if the results for `modules_cdb_input.cpp` are reported before
+// `modules_cdb_input2.cpp`.
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
+// RUN:   FileCheck --check-prefix=CHECK2 %s
+
+#include "header.h"
+
+// CHECK1: modules_cdb_input2.cpp
+// CHECK1-NEXT: modules_cdb_input2.cpp
+// CHECK1-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK1-NEXT: Inputs{{/|\\}}header2.h
+// CHECK1: Inputs{{/|\\}}header.h
+
+// CHECK2: modules_cdb_input.cpp
+// CHECK2-NEXT: Inputs{{/|\\}}module.modulemap
+// CHECK2-NEXT: Inputs{{/|\\}}header.h
+// CHECK2NO-NOT: header2
Index: clang/test/ClangScanDeps/Inputs/modules_cdb.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/modules_cdb.json
@@ -0,0 +1,13 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/modules_cdb_input2.cpp -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/modules_cdb2.d -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/modules_cdb_input2.cpp"
+},
+{
+  "directory": "DIR",
+  "command": "clang -E DIR/modules_cdb_input.cpp -IInputs -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/modules_cdb_input.cpp"
+}
+]
+
Index: clang/test/ClangScanDeps/Inputs/module.modulemap
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/module.modulemap
@@ -0,0 +1,7 @@
+module header1 {
+  header "header.h"
+}
+
+module header2 {
+header "header2.h"
+}
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -122,6 +122,32 @@
   return It.first->getValue();
 }
 
+/// Whitelist file extensions that should be minimized, treating no extension as
+/// a source file that should be minimized.
+///
+/// This is kinda hacky, it would be better if we knew what kind of file Clang
+/// was expecting instead.
+static bool shouldMinimize(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return true; // C++ standard library
+  return llvm::StringSwitch(Ext)
+.CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true)
+.CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true)
+.CasesLower(".m", ".mm", true)
+.CasesLower(".i", ".ii", ".mi", ".mmi", true)
+.CasesLower(".def", ".inc", true)
+.Default(false);
+}
+
+
+static bool shouldCacheStatFailures(StringRef Filename) {
+  StringRef Ext = llvm::sys::path::extension(Filename);
+  if (Ext.empty())
+return false; // This may be the module cache directory.
+  return shouldMinimize(Filename); // Only cache stat failures on source files.
+}

[PATCH] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added reviewers: arphaman, kousikk.
Bigcheese added a project: clang.
Herald added subscribers: tschuett, dexonsmith, mgrang, mgorny.

This adds experimental support for extracting a Clang module dependency graph
from a compilation database. The output format is experimental and will change.
It is currently a concatenation of JSON outputs for each compilation. Future
patches will change this to deduplicate modules between compilations.

This patch doesn't implement deduplication as the patch was already getting 
pretty
large. This also doesn't properly support C++20 modules yet.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69420

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
  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/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-full.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
@@ -59,6 +59,17 @@
 llvm::cl::init(ScanningMode::MinimizedSourcePreprocessing),
 llvm::cl::cat(DependencyScannerCategory));
 
+static llvm::cl::opt Format(
+"format", llvm::cl::desc("The output format for the dependencies"),
+llvm::cl::values(clEnumValN(ScanningFormat::Make, "make",
+"Makefile compatible dep file"),
+ clEnumValN(ScanningFormat::Full, "experimental-full",
+"Full dependency graph suitable"
+" for explicitly building modules. This format "
+"is experimental and will change.")),
+llvm::cl::init(ScanningFormat::Make),
+llvm::cl::cat(DependencyScannerCategory));
+
 llvm::cl::opt
 NumThreads("j", llvm::cl::Optional,
llvm::cl::desc("Number of worker threads to use (default: use "
@@ -200,7 +211,7 @@
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());
 
-  DependencyScanningService Service(ScanMode, ReuseFileManager,
+  DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
 SkipExcludedPPRanges);
 #if LLVM_ENABLE_THREADS
   unsigned NumWorkers =
Index: clang/test/ClangScanDeps/modules-full.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-full.cpp
@@ -0,0 +1,74 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: rm -rf %t.module-cache
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/modules_cdb_input.cpp
+// RUN: cp %s %t.dir/modules_cdb_input2.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: cp %S/Inputs/header2.h %t.dir/Inputs/header2.h
+// RUN: cp %S/Inputs/module.modulemap %t.dir/Inputs/module.modulemap
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/modules_cdb.json > %t.cdb
+//
+// RUN: echo %t.dir > %t.result
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 \
+// RUN:   -mode preprocess-minimized-sources -format experimental-full >> %t.result
+// RUN: cat %t.result | FileCheck --check-prefixes=CHECK %s
+
+#include "header.h"
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK-NEXT: {
+// CHECK-NEXT:  "clang-context-hash": "[[CONTEXT_HASH:[A-Z0-9]+]]",
+// CHECK-NEXT:  "clang-module-deps": [
+// CHECK-NEXT:"header1"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "clang-modules": [
+// CHECK-NEXT:{
+// CHECK-NEXT:  "clang-module-deps": [
+// CHECK-NEXT:"header2"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "clang-modulemap-file": "[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap",
+// CHECK-NEXT:  "file-deps": [
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap",
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}header.h"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "name": "header1"
+// CHECK-NEXT:},
+// CHECK-NEXT:{
+// CHECK-NEXT:  "clang-module-deps": [],
+// CHECK-NEXT:  "clang-modulemap-file": "[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap",
+// CHECK-NEXT:  "file-deps": [
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}header2.h",
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "name": "header2"

[PATCH] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:94
+
+void printDependencies(std::string &S, StringRef MainFile) {
+  // Sort the modules by name to get a deterministic order.

kousikk wrote:
> Should output arguments be at the end?
The point was to match up with `MakeDependencyPrinterConsumer`'s 
`printDependencies`. This will also be going away with the patch to merge the 
dependencies.


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

https://reviews.llvm.org/D69420



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

[PATCH] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 226508.
Bigcheese marked 3 inline comments as done.
Bigcheese added a comment.

Address review comments.


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

https://reviews.llvm.org/D69420

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
  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/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-full.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
@@ -59,6 +59,17 @@
 llvm::cl::init(ScanningMode::MinimizedSourcePreprocessing),
 llvm::cl::cat(DependencyScannerCategory));
 
+static llvm::cl::opt Format(
+"format", llvm::cl::desc("The output format for the dependencies"),
+llvm::cl::values(clEnumValN(ScanningOutputFormat::Make, "make",
+"Makefile compatible dep file"),
+ clEnumValN(ScanningOutputFormat::Full, "experimental-full",
+"Full dependency graph suitable"
+" for explicitly building modules. This format "
+"is experimental and will change.")),
+llvm::cl::init(ScanningOutputFormat::Make),
+llvm::cl::cat(DependencyScannerCategory));
+
 llvm::cl::opt
 NumThreads("j", llvm::cl::Optional,
llvm::cl::desc("Number of worker threads to use (default: use "
@@ -200,7 +211,7 @@
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());
 
-  DependencyScanningService Service(ScanMode, ReuseFileManager,
+  DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
 SkipExcludedPPRanges);
 #if LLVM_ENABLE_THREADS
   unsigned NumWorkers =
Index: clang/test/ClangScanDeps/modules-full.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-full.cpp
@@ -0,0 +1,74 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: rm -rf %t.module-cache
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/modules_cdb_input.cpp
+// RUN: cp %s %t.dir/modules_cdb_input2.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: cp %S/Inputs/header2.h %t.dir/Inputs/header2.h
+// RUN: cp %S/Inputs/module.modulemap %t.dir/Inputs/module.modulemap
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/modules_cdb.json > %t.cdb
+//
+// RUN: echo %t.dir > %t.result
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 \
+// RUN:   -mode preprocess-minimized-sources -format experimental-full >> %t.result
+// RUN: cat %t.result | FileCheck --check-prefixes=CHECK %s
+
+#include "header.h"
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK-NEXT: {
+// CHECK-NEXT:  "clang-context-hash": "[[CONTEXT_HASH:[A-Z0-9]+]]",
+// CHECK-NEXT:  "clang-module-deps": [
+// CHECK-NEXT:"header1"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "clang-modules": [
+// CHECK-NEXT:{
+// CHECK-NEXT:  "clang-module-deps": [
+// CHECK-NEXT:"header2"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "clang-modulemap-file": "[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap",
+// CHECK-NEXT:  "file-deps": [
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap",
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}header.h"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "name": "header1"
+// CHECK-NEXT:},
+// CHECK-NEXT:{
+// CHECK-NEXT:  "clang-module-deps": [],
+// CHECK-NEXT:  "clang-modulemap-file": "[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap",
+// CHECK-NEXT:  "file-deps": [
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}header2.h",
+// CHECK-NEXT:"[[PREFIX]]{{[/\\]}}Inputs{{[/\\]}}module.modulemap"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "name": "header2"
+// CHECK-NEXT:}
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "file-deps": [
+// CHECK-NEXT:"header.h"
+// CHECK-NEXT:  ],
+// CHECK-NEXT:  "input-file": "[[PREFIX]]{{[/\\]}}modules_cdb_input2.cpp"
+// CHECK-NEXT:},
+// CHECK-NEXT:{
+// CHECK-NOT:   "clang-context-hash": "[[CONTEXT_HASH]]",
+// CHECK-NEXT:  "clang-context-hash": "{{[A-Z0-9]+}}",
+// CHECK-NEXT:  "clang-module-deps": [
+// CHECK-NEXT:"header1"
+// CHECK-NEXT:  ],
+

[PATCH] D79998: Add AST_SIGNATURE record to unhashed control block of pcm files (Patch series 2/3)

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D79998#2038430 , @dang wrote:

> Should I add the test here or in the clang-scan-deps patch?


It's best to have a test in every non-nfc patch. You should be able to test 
this with llvm-bcanalyzer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79998



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

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/include/clang/Frontend/CompilerInvocation.h:156
   /// \param [out] Res - The resulting invocation.
+  /// \param [in] CommandLineArgs - Array of argument strings, this should not
+  /// contain "-cc1".

Is this really a should not, or is it must not?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3604-3610
+#define OPTION_WITH_MARSHALLING(PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, 
\
+ALIASARGS, FLAGS, PARAM, HELPTEXT, METAVAR,
\
+VALUES, KEYPATH, IS_POSITIVE, DEFAULT_VALUE)   
\
+  if (Option::KIND##Class == Option::FlagClass)
\
+Res.KEYPATH = Args.hasArg(OPT_##ID) && IS_POSITIVE;
+#include "clang/Driver/Options.inc"
+#undef OPTION_WITH_MARSHALLING

This should probably go in a separate function as it will grow. I would 
recommend something like `ParseSimpleArgs` and call it right before 
`ParseAnalyzerArgs `.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3607
+VALUES, KEYPATH, IS_POSITIVE, DEFAULT_VALUE)   
\
+  if (Option::KIND##Class == Option::FlagClass)
\
+Res.KEYPATH = Args.hasArg(OPT_##ID) && IS_POSITIVE;

How would this handle other option classes? I think it would be good to include 
a few different types of options in the first patch.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3845
+  IS_POSITIVE != DEFAULT_VALUE && this->KEYPATH != DEFAULT_VALUE)  
\
+Args.push_back(StringAllocator(Twine(PREFIX_TYPE[0]) + NAME));
+#include "clang/Driver/Options.inc"

It's a little sad that we need to allocation every string just because of the 
`-`. We definitely need to be able to allocate strings for options with data, 
but it would be good if we could just have the strings with `-` prefixed in the 
option table if that's reasonable to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79796



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

[PATCH] D84185: Better defaults for MarshallingInfoString

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84185



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

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

LGTM for llvm/{lib,include/llvm}/Object/* and llvm/tools/llvm-readobj/*.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76572



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

[PATCH] D77697: libc++: adjust modulemap for non-modular C

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D77697#1970586 , @dexonsmith wrote:

> In D77697#1969998 , @compnerd wrote:
>
> > @dexonsmith - yeah, sadly I dont think that there is a good way to audit 
> > that - any change to the public headers can cause issues.  Furthermore, the 
> > libc headers themselves also influence this.
>
>
> For auditing, can you use `llvm-bcanalyze` to see which headers are claimed 
> by which PCM?  If not, we should probably add a `clang-pcm` tool or something 
> to help inspect module contents.  @Bigcheese, thoughts?
>
> I guess the obvious concern about this is that this is a game of 
> whack-a-mole.  If the headers change, you may need to shuffle module order 
> again, at which point, which version of libc should we make libc++ work 
> against?  And different libc implementations could need different orders.  
> But I'm just pointing it out; I don't have a problem with this patch landing.


That would be useful for debugging this kind of issue and for testing a 
specific set of system headers + libc++ headers.

I'm also fine with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77697



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

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added reviewers: dexonsmith, vsapsai, rdhindsa.
Bigcheese added a project: clang.
Herald added a subscriber: hiraditya.

This is needed to fix the reason
0a2be46cfdb698fe (Modules: Invalidate out-of-date PCMs as they're
discovered) and 5b44a4b07fc1d ([modules] Do not cache invalid state for
modules that we attempted to load.) were reverted.

These patches changed Clang to use `isVolatile` when loading modules.
This had the side effect of not using mmap when loading modules, and
thus greatly increased memory usage.

The reason it wasn't using mmap is because `MemoryBuffer` plays some
games with file size when you request null termination, and it has to
disable these when `isVolatile` is set as the size may change by the
time it's mmapped. Clang by default passes
`RequiresNullTerminator = true`, and `shouldUseMmap` ignored if
`RequiresNullTerminator` was even requested.

This patch adds `RequiresNullTerminator` to the `FileManager` interface
so Clang can use it when loading modules, and changes `shouldUseMmap` to
only take volatility into account if `RequiresNullTerminator` is true.
This is fine as both `mmap` and a `read` loop are vulnerable to
modifying the file while reading, but are immune to the rename Clang
does when replacing a module file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D2

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  llvm/lib/Support/MemoryBuffer.cpp


Index: llvm/lib/Support/MemoryBuffer.cpp
===
--- llvm/lib/Support/MemoryBuffer.cpp
+++ llvm/lib/Support/MemoryBuffer.cpp
@@ -329,7 +329,7 @@
   // mmap may leave the buffer without null terminator if the file size changed
   // by the time the last page is mapped in, so avoid it if the file size is
   // likely to change.
-  if (IsVolatile)
+  if (IsVolatile && RequiresNullTerminator)
 return false;
 
   // We don't use mmap for small files because this can severely fragment our
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -458,7 +458,8 @@
 }
 
 llvm::ErrorOr>
-FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
+FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
+  bool RequiresNullTerminator) {
   uint64_t FileSize = Entry->getSize();
   // If there's a high enough chance that the file have changed since we
   // got its size, force a stat before opening it.
@@ -468,28 +469,29 @@
   StringRef Filename = Entry->getName();
   // If the file is already open, use the open file descriptor.
   if (Entry->File) {
-auto Result =
-Entry->File->getBuffer(Filename, FileSize,
-   /*RequiresNullTerminator=*/true, isVolatile);
+auto Result = Entry->File->getBuffer(Filename, FileSize,
+ RequiresNullTerminator, isVolatile);
 Entry->closeFile();
 return Result;
   }
 
   // Otherwise, open the file.
-  return getBufferForFileImpl(Filename, FileSize, isVolatile);
+  return getBufferForFileImpl(Filename, FileSize, isVolatile,
+  RequiresNullTerminator);
 }
 
 llvm::ErrorOr>
 FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
-  bool isVolatile) {
+  bool isVolatile,
+  bool RequiresNullTerminator) {
   if (FileSystemOpts.WorkingDir.empty())
-return FS->getBufferForFile(Filename, FileSize,
-/*RequiresNullTerminator=*/true, isVolatile);
+return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
+isVolatile);
 
   SmallString<128> FilePath(Filename);
   FixupRelativePath(FilePath);
-  return FS->getBufferForFile(FilePath, FileSize,
-  /*RequiresNullTerminator=*/true, isVolatile);
+  return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
+  isVolatile);
 }
 
 /// getStatValue - Get the 'stat' information for the specified path,
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -378,15 +378,19 @@
   /// Open the specified file as a MemoryBuffer, returning a new
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::ErrorOr>
-  getBufferForFile(const FileEntry *Entry, bool isVolatile = false);
+  getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
+   bool RequiresNullTerminator = true);
   llvm::ErrorOr>
-  getBufferForFile(StringRef Filename, bool isVolatile = false) {
-return getBufferForFileI

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

Not really. It's a static function in MemoryBuffer.cpp, and the `MemoryBuffer` 
class doesn't have a `Kind` member so we can't check for `MemoryBufferMMapFile`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D2



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

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 256722.
Bigcheese added a comment.

Added a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D2

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/unittests/Support/MemoryBufferTest.cpp

Index: llvm/unittests/Support/MemoryBufferTest.cpp
===
--- llvm/unittests/Support/MemoryBufferTest.cpp
+++ llvm/unittests/Support/MemoryBufferTest.cpp
@@ -380,4 +380,24 @@
   ASSERT_EQ(16u, MB.getBufferSize());
   EXPECT_EQ("", MB.getBuffer());
 }
+
+TEST_F(MemoryBufferTest, mmapVolatileNoNull) {
+  int FD;
+  SmallString<64> TestPath;
+  ASSERT_NO_ERROR(sys::fs::createTemporaryFile(
+  "MemoryBufferTest_mmapVolatileNoNull", "temp", FD, TestPath));
+  FileRemover Cleanup(TestPath);
+  raw_fd_ostream OF(FD, true);
+  // Create a file large enough to mmap. A 32KiB file should be enough.
+  for (unsigned i = 0; i < 0x1000; ++i)
+OF << "01234567";
+  OF.flush();
+
+  auto MBOrError = MemoryBuffer::getOpenFile(FD, TestPath, -1, false, true);
+  ASSERT_NO_ERROR(MBOrError.getError())
+  OwningBuffer MB = std::move(*MBOrError);
+  EXPECT_EQ(MB->getBufferKind(), MemoryBuffer::MemoryBuffer_MMap);
+  EXPECT_EQ(MB->getBufferSize(), std::size_t(0x8000));
+  EXPECT_TRUE(MB->getBuffer().startswith("01234567"));
+}
 }
Index: llvm/lib/Support/MemoryBuffer.cpp
===
--- llvm/lib/Support/MemoryBuffer.cpp
+++ llvm/lib/Support/MemoryBuffer.cpp
@@ -329,7 +329,7 @@
   // mmap may leave the buffer without null terminator if the file size changed
   // by the time the last page is mapped in, so avoid it if the file size is
   // likely to change.
-  if (IsVolatile)
+  if (IsVolatile && RequiresNullTerminator)
 return false;
 
   // We don't use mmap for small files because this can severely fragment our
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -458,7 +458,8 @@
 }
 
 llvm::ErrorOr>
-FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
+FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
+  bool RequiresNullTerminator) {
   uint64_t FileSize = Entry->getSize();
   // If there's a high enough chance that the file have changed since we
   // got its size, force a stat before opening it.
@@ -468,28 +469,29 @@
   StringRef Filename = Entry->getName();
   // If the file is already open, use the open file descriptor.
   if (Entry->File) {
-auto Result =
-Entry->File->getBuffer(Filename, FileSize,
-   /*RequiresNullTerminator=*/true, isVolatile);
+auto Result = Entry->File->getBuffer(Filename, FileSize,
+ RequiresNullTerminator, isVolatile);
 Entry->closeFile();
 return Result;
   }
 
   // Otherwise, open the file.
-  return getBufferForFileImpl(Filename, FileSize, isVolatile);
+  return getBufferForFileImpl(Filename, FileSize, isVolatile,
+  RequiresNullTerminator);
 }
 
 llvm::ErrorOr>
 FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
-  bool isVolatile) {
+  bool isVolatile,
+  bool RequiresNullTerminator) {
   if (FileSystemOpts.WorkingDir.empty())
-return FS->getBufferForFile(Filename, FileSize,
-/*RequiresNullTerminator=*/true, isVolatile);
+return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
+isVolatile);
 
   SmallString<128> FilePath(Filename);
   FixupRelativePath(FilePath);
-  return FS->getBufferForFile(FilePath, FileSize,
-  /*RequiresNullTerminator=*/true, isVolatile);
+  return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
+  isVolatile);
 }
 
 /// getStatValue - Get the 'stat' information for the specified path,
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -378,15 +378,19 @@
   /// Open the specified file as a MemoryBuffer, returning a new
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::ErrorOr>
-  getBufferForFile(const FileEntry *Entry, bool isVolatile = false);
+  getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
+   bool RequiresNullTerminator = true);
   llvm::ErrorOr>
-  getBufferForFile(StringRef Filename, bool isVolatile = false) {
-return getBufferForFileImp

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

[Michael Spencer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG92e8af0ecbe7: [Clang] Expose RequiresNullTerminator in 
FileManager. (authored by Bigcheese).

Changed prior to commit:
  https://reviews.llvm.org/D2?vs=256722&id=257852#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D2

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/unittests/Support/MemoryBufferTest.cpp

Index: llvm/unittests/Support/MemoryBufferTest.cpp
===
--- llvm/unittests/Support/MemoryBufferTest.cpp
+++ llvm/unittests/Support/MemoryBufferTest.cpp
@@ -380,4 +380,32 @@
   ASSERT_EQ(16u, MB.getBufferSize());
   EXPECT_EQ("", MB.getBuffer());
 }
+
+TEST_F(MemoryBufferTest, mmapVolatileNoNull) {
+  // Verify that `MemoryBuffer::getOpenFile` will use mmap when
+  // `RequiresNullTerminator = false`, `IsVolatile = true`, and the file is
+  // large enough to use mmap.
+  //
+  // This is done because Clang should use this mode to open module files, and
+  // falling back to malloc for them causes a huge memory usage increase.
+
+  int FD;
+  SmallString<64> TestPath;
+  ASSERT_NO_ERROR(sys::fs::createTemporaryFile(
+  "MemoryBufferTest_mmapVolatileNoNull", "temp", FD, TestPath));
+  FileRemover Cleanup(TestPath);
+  raw_fd_ostream OF(FD, true);
+  // Create a file large enough to mmap. A 32KiB file should be enough.
+  for (unsigned i = 0; i < 0x1000; ++i)
+OF << "01234567";
+  OF.flush();
+
+  auto MBOrError = MemoryBuffer::getOpenFile(FD, TestPath,
+  /*FileSize=*/-1, /*RequiresNullTerminator=*/false, /*IsVolatile=*/true);
+  ASSERT_NO_ERROR(MBOrError.getError())
+  OwningBuffer MB = std::move(*MBOrError);
+  EXPECT_EQ(MB->getBufferKind(), MemoryBuffer::MemoryBuffer_MMap);
+  EXPECT_EQ(MB->getBufferSize(), std::size_t(0x8000));
+  EXPECT_TRUE(MB->getBuffer().startswith("01234567"));
+}
 }
Index: llvm/lib/Support/MemoryBuffer.cpp
===
--- llvm/lib/Support/MemoryBuffer.cpp
+++ llvm/lib/Support/MemoryBuffer.cpp
@@ -329,7 +329,7 @@
   // mmap may leave the buffer without null terminator if the file size changed
   // by the time the last page is mapped in, so avoid it if the file size is
   // likely to change.
-  if (IsVolatile)
+  if (IsVolatile && RequiresNullTerminator)
 return false;
 
   // We don't use mmap for small files because this can severely fragment our
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -458,7 +458,8 @@
 }
 
 llvm::ErrorOr>
-FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
+FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
+  bool RequiresNullTerminator) {
   uint64_t FileSize = Entry->getSize();
   // If there's a high enough chance that the file have changed since we
   // got its size, force a stat before opening it.
@@ -468,28 +469,29 @@
   StringRef Filename = Entry->getName();
   // If the file is already open, use the open file descriptor.
   if (Entry->File) {
-auto Result =
-Entry->File->getBuffer(Filename, FileSize,
-   /*RequiresNullTerminator=*/true, isVolatile);
+auto Result = Entry->File->getBuffer(Filename, FileSize,
+ RequiresNullTerminator, isVolatile);
 Entry->closeFile();
 return Result;
   }
 
   // Otherwise, open the file.
-  return getBufferForFileImpl(Filename, FileSize, isVolatile);
+  return getBufferForFileImpl(Filename, FileSize, isVolatile,
+  RequiresNullTerminator);
 }
 
 llvm::ErrorOr>
 FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
-  bool isVolatile) {
+  bool isVolatile,
+  bool RequiresNullTerminator) {
   if (FileSystemOpts.WorkingDir.empty())
-return FS->getBufferForFile(Filename, FileSize,
-/*RequiresNullTerminator=*/true, isVolatile);
+return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
+isVolatile);
 
   SmallString<128> FilePath(Filename);
   FixupRelativePath(FilePath);
-  return FS->getBufferForFile(FilePath, FileSize,
-  /*RequiresNullTerminator=*/true, isVolatile);
+  return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
+  isVolatile);
 }
 
 /// getStatValue - Get the 'stat' information for the specified path,
Index: clang/include/clang/Basic/FileManager.h

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/include/clang/Frontend/CompilerInvocation.h:248
+  /// \returns - True if parsing was successful, false otherwise
+  bool parseSimpleArgs(const llvm::opt::ArgList &Args,
+   DiagnosticsEngine &Diags);

Is there a reason for this to be a member of `CompilerInvocation`? The rest of 
the argument parsing functions are static file local functions.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3658
  MissingArgCount, IncludedFlagsBitmask);
+
   LangOptions &LangOpts = *Res.getLangOpts();

Put formatting fixups in a separate commit.



Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:41
+TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineFlag) {
+  const char *Args[] = {"clang", "-xc++", "-fmodules-strict-context-hash", 
"-"};
+

This reminded me that some of the -cc1 arguments are positional such as 
`-xc++`. I think we'll need custom handling for `INPUT` arguments, although we 
don't need them (or want them) for the modules use case. Do you have any 
thoughts on how to handle them? I don't think answering that should block the 
initial patch though.



Comment at: llvm/include/llvm/Option/OptParser.td:153
+}
+class MarshallingFlagRequired
+  : MarshallingFlag {

I'm not sure Required is a great name here. I initially read that as the option 
was required, not that it should always be emitted.



Comment at: llvm/include/llvm/Option/OptParser.td:167-171
+class MarshallingEnum enumvalues>
+  : OptionMarshallingInfo<"enum", keypath> {
+  code DefaultValue = defaultvalue;
+  listEnumValues = enumvalues;
+}

I noticed that this isn't being used for the enum case you added 
(`-mrelocation-model`). Do you intend to use it? You should either use it in 
this patch or remove it until it actually is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79796



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

[PATCH] D83315: Turn arcmt-* options into a single option

[Michael Spencer via Phabricator via cfe-commits]

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

I have a slight preference for `-arcmt-action=`, but up to you if you want to 
change it. Otherwise LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83315



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

[PATCH] D82574: Merge TableGen files used for clang options

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM. We can look at splitting it up again once we know what the dependencies 
are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82574



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

[PATCH] D83690: Port Migator option flags to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83690



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

[PATCH] D83406: Remove NormalizerRetTy and use the decltype of the KeyPath instead

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83406



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

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/include/clang/Frontend/CompilerInvocation.h:193
+  /// \param [out] Args - The generated arguments. Note that the caller is
+  /// responsible for insersting the path to the clang executable and "-cc1" if
+  /// desired.

inserting



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:160-161
+
+  llvm::report_fatal_error("The simple enum value was not correctly defined in 
"
+   "the tablegen option description");
+}

`llvm::report_fatal_error` is only used for code paths we actually expect to 
potentially be hit. For programming errors we instead use `assert` or 
`llvm_unreachable`. See 
https://llvm.org/docs/ProgrammersManual.html#error-handling .



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:430-440
+  std::vector> OptsWithMarshalling;
+  for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
+const Record &R = *Opts[I];
 
+// Start a single option entry.
+OS << "OPTION(";
+WriteOptRecordFields(OS, R);

Just to verify, this doesn't change the output for options that don't have 
marshalling info, right? Just want to make sure this doesn't change anything 
for other users of libOption.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469
+  OS << "};\n";
+  OS << "static const unsigned SimpleEnumValueTablesSize = "
+"sizeof(SimpleEnumValueTables) / sizeof(SimpleEnumValueTable);\n";
+

What happens if there are none?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79796



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

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM with `KeyPathPrefix` moved to the patch that actually uses it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79796



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

[PATCH] D82874: Add diagnostic option backing field for -fansi-escape-codes

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm with the fix I mentioned.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3629
 #undef MERGE_ASSIGN_VALUE
+  llvm::sys::Process::UseANSIEscapeCodes(DiagnosticOpts->UseANSIEscapeCodes);
   return true;

This should probably be moved out so that this function only parses args and 
doesn't modify outside state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82874



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

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/docs/Modules.rst:231
+  prebuilt modules paths (specified via ``-fprebuilt-module-path``), we will
+  look for a matching implicit modules in the prebuilt modules paths.
+





Comment at: clang/docs/Modules.rst:295
+
+A trick to prebuilt required modules in one command is to generate implicit 
modules using the ``-fdisable-module-hash`` option.
+

I also think it's important to point out here that disabling the module hash 
means you are responsible for making sure the modules are compatible. How to do 
this is mentioned below, but it's never stated why you would want to do this.



Comment at: clang/test/Modules/prebuilt-implicit-modules.m:9
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module 
-fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap 
-fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//

Is this actually supposed to be `module_e`? I would expect this to be verifying 
that it didn't add `module_a` to `%t1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68997

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

[PATCH] D83071: Add support for options with two flags for controlling the same field.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83071

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

[PATCH] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

I mostly just need to rebase this patch now. I'll try to get to that soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70351



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

[PATCH] D80172: Revert "Re-fix _lrotl/_lrotr to always take Long, no matter the platform."

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese created this revision.
Bigcheese added reviewers: bruno, rnk, erichkeane.
Bigcheese added a project: clang.
Herald added a subscriber: dexonsmith.

This reverts commit 92146ce399cdb26c3a9aa4d68af8cacb7c1c0fef.

This commit broke users targeting LP64 systems that expect these
intrinsics to match the MSVC behavior of working on 32 bit integers.
These users use a `LONG` macro which is defined as `long` on Windows
(LLP64), and `int` when targeting LP64 systems. Without this behavior
there is no intrinsic for 32 bit rotates on these platforms.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80172

Files:
  clang/include/clang/Basic/Builtins.def
  clang/test/CodeGen/ms-intrinsics-rotations.c


Index: clang/test/CodeGen/ms-intrinsics-rotations.c
===
--- clang/test/CodeGen/ms-intrinsics-rotations.c
+++ clang/test/CodeGen/ms-intrinsics-rotations.c
@@ -12,10 +12,17 @@
 // RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
 // RUN: -triple x86_64--linux -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
 // RUN: %clang_cc1 -ffreestanding -fms-extensions \
 // RUN: -triple x86_64--darwin -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
+
+// LP64 targets use 'long' as 'int' for MS intrinsics (-fms-extensions)
+#ifdef __LP64__
+#define LONG int
+#else
+#define LONG long
+#endif
 
 // rotate left
 
@@ -40,15 +47,12 @@
 // CHECK:   [[R:%.*]] = call i32 @llvm.fshl.i32(i32 [[X:%.*]], i32 [[X]], i32 
[[Y:%.*]])
 // CHECK:   ret i32 [[R]]
 
-unsigned long test_lrotl(unsigned long value, int shift) {
+unsigned LONG test_lrotl(unsigned LONG value, int shift) {
   return _lrotl(value, shift);
 }
 // CHECK-32BIT-LONG: i32 @test_lrotl
 // CHECK-32BIT-LONG:   [[R:%.*]] = call i32 @llvm.fshl.i32(i32 [[X:%.*]], i32 
[[X]], i32 [[Y:%.*]])
 // CHECK-32BIT-LONG:   ret i32 [[R]]
-// CHECK-64BIT-LONG: i64 @test_lrotl
-// CHECK-64BIT-LONG:   [[R:%.*]] = call i64 @llvm.fshl.i64(i64 [[X:%.*]], i64 
[[X]], i64 [[Y:%.*]])
-// CHECK-64BIT-LONG:   ret i64 [[R]]
 
 unsigned __int64 test_rotl64(unsigned __int64 value, int shift) {
   return _rotl64(value, shift);
@@ -80,15 +84,12 @@
 // CHECK:   [[R:%.*]] = call i32 @llvm.fshr.i32(i32 [[X:%.*]], i32 [[X]], i32 
[[Y:%.*]])
 // CHECK:   ret i32 [[R]]
 
-unsigned long test_lrotr(unsigned long value, int shift) {
+unsigned LONG test_lrotr(unsigned LONG value, int shift) {
   return _lrotr(value, shift);
 }
 // CHECK-32BIT-LONG: i32 @test_lrotr
 // CHECK-32BIT-LONG:   [[R:%.*]] = call i32 @llvm.fshr.i32(i32 [[X:%.*]], i32 
[[X]], i32 [[Y:%.*]])
 // CHECK-32BIT-LONG:   ret i32 [[R]]
-// CHECK-64BIT-LONG: i64 @test_lrotr
-// CHECK-64BIT-LONG:   [[R:%.*]] = call i64 @llvm.fshr.i64(i64 [[X:%.*]], i64 
[[X]], i64 [[Y:%.*]])
-// CHECK-64BIT-LONG:   ret i64 [[R]]
 
 unsigned __int64 test_rotr64(unsigned __int64 value, int shift) {
   return _rotr64(value, shift);
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -874,12 +874,12 @@
 LANGBUILTIN(_rotl8,  "UcUcUc","n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotl16, "UsUsUc","n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotl,   "UiUii", "n", ALL_MS_LANGUAGES)
-LANGBUILTIN(_lrotl,  "ULiULii",   "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(_lrotl,  "UNiUNii",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotl64, "UWiUWii",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotr8,  "UcUcUc","n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotr16, "UsUsUc","n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotr,   "UiUii", "n", ALL_MS_LANGUAGES)
-LANGBUILTIN(_lrotr,  "ULiULii",   "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(_lrotr,  "UNiUNii",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_rotr64, "UWiUWii",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(__va_start,   "vc**.", "nt", ALL_MS_LANGUAGES)
 LANGBUILTIN(__fastfail, "vUi","nr", ALL_MS_LANGUAGES)


Index: clang/test/CodeGen/ms-intrinsics-rotations.c
===
--- clang/test/CodeGen/ms-intrinsics-rotations.c
+++ clang/test/CodeGen/ms-intrinsics-rotations.c
@@ -12,10 +12,17 @@
 // RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
 // RUN: -triple x86_64--linux -emit-llvm %s -o - \
-// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
 // RUN: %clang_cc1 -ffreestanding -fms-extensions \
 // RUN:  

[PATCH] D80172: Revert "Re-fix _lrotl/_lrotr to always take Long, no matter the platform."

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D80172#2042946 , @rnk wrote:

> I'd be fine going back to the behavior from before.
>
> > Without this behavior there is no intrinsic for 32 bit rotates on these 
> > platforms.
>
> Strictly speaking, there seems to be `__builtin_rotate(left|right)32`, which 
> is portable to all clang platforms.


Sorry, I meant there's no MSVC intrinsic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80172



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

[PATCH] D132971: [clang][modules] Don't hard code [no_undeclared_includes] for the Darwin module

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

I'm fine with this change, but do we actually have a backwards compatibility 
policy anywhere in Clang? Would be good to know what range of SDKs a compiler 
release is expected to support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132971

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

[PATCH] D105527: libclang.so: Make SONAME independent from LLVM version

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

For future reference this was very difficult to merge into external changes. It 
looks like you resorted this at the same time as renaming it, and that messes 
up git's rename logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105527

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

[PATCH] D105527: libclang.so: Make SONAME independent from LLVM version

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

Also, this python script just doesn't work. It's missing a sys import, a "w" 
flag, and a new line after each write. It also dumps the output into the source 
directory. Was this actually tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105527

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

[PATCH] D105527: libclang.so: Make SONAME independent from LLVM version

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

Here's a version that actually works (python 3, not sure if it's valid in 2), 
although I would much prefer we not write to the source directory during a 
build.

  import re
  import os
  import sys
  
  input_file = open(sys.argv[1])
  with open(sys.argv[2], "w") as output_file:
  for line in input_file:
  m = re.search('clang_[^;]+', line)
  if m:
  output_file.write(m.group(0) + '\n')


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105527

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

[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/lib/Lex/ModuleMap.cpp:2355
+parseModuleMembers();
+  }
+}

bruno wrote:
> Too many curly braces?
This is correct. It closes the block on line 2353.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118311

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

[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese updated this revision to Diff 405482.
Bigcheese added a comment.

- Fixed documentation typo.
- Made missing '{' diagnostic more clear.
- Use `ModuleMapParser::skipUntil`.

Emitting an actual fixup is kind of difficult from `ModuleMapParser` because 
the way it handles tokens makes it hard to get the source location of the end 
of the line containing the requires feature list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118311

Files:
  clang/docs/Modules.rst
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/include/clang/Basic/Module.h
  clang/lib/Basic/Module.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/test/Modules/requires-block-errors.m
  clang/test/Modules/requires-block.m

Index: clang/test/Modules/requires-block.m
===
--- /dev/null
+++ clang/test/Modules/requires-block.m
@@ -0,0 +1,73 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/A.mm
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/include %t/B.mm -verify
+
+//--- include/module.modulemap
+
+requires !cplusplus {
+  module A {
+header "A.h"
+  }
+}
+
+requires cplusplus {
+  module A {
+header "A.h"
+header "A.hpp"
+  }
+}
+
+// Test empty blocks
+requires cplusplus {}
+requires !cplusplus {}
+
+// Test nested requires
+requires objc {
+  module B {
+requires !cplusplus {
+  header "B.h"
+}
+requires cplusplus {
+  header "B.hpp"
+}
+  }
+}
+
+//--- include/A.h
+
+typedef int A_t;
+
+//--- include/A.hpp
+
+using Adrgdrg_t = long;
+
+//--- include/B.h
+
+typedef int B_t;
+
+//--- include/B.hpp
+
+using Bdrgdrg_t = long;
+
+//--- A.m
+@import A;
+A_t a;
+Adrgdrg_t b; // expected-error {{unknown type name 'Adrgdrg_t'}}
+
+//--- A.mm
+@import A;
+A_t a;
+Adrgdrg_t b;
+
+//--- B.m
+@import B;
+B_t a;
+Bdrgdrg_t b; // expected-error {{unknown type name 'Bdrgdrg_t'}}
+
+//--- B.mm
+@import B;
+B_t a; // expected-error {{unknown type name 'B_t'}}
+Bdrgdrg_t b;
Index: clang/test/Modules/requires-block-errors.m
===
--- /dev/null
+++ clang/test/Modules/requires-block-errors.m
@@ -0,0 +1,31 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-closing-brace %t/missing-closing-brace.m -verify
+// RUN: %clang_cc1 -fmodules-cache-path=%t/module-cache -fmodules -fimplicit-module-maps -I %t/missing-opening-brace %t/missing-opening-brace.m -verify
+
+//--- missing-closing-brace/module.modulemap
+
+requires !cplusplus {
+  module Pony {
+header "Pony.h"
+  }
+
+//--- missing-closing-brace/Pony.h
+
+//--- missing-closing-brace.m
+// expected-error@module.modulemap:7 {{expected '}'}}
+// expected-note@module.modulemap:2 {{to match this '{'}}
+@import Pony // expected-error {{could not build module 'Pony'}}
+
+//--- missing-opening-brace/module.modulemap
+
+requires !cplusplus
+module Pony {
+  header "Pony.h"
+}
+
+//--- missing-opening-brace/Pony.h
+
+//--- missing-opening-brace.m
+// expected-error@module.modulemap:2 {{invalid requires declaration outside of a module declaration, did you mean to add '{' to open a requires block?}}
+@import Pony // expected-error {{could not build module 'Pony'}}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1468,10 +1468,19 @@
 
 using ModuleId = SmallVector, 2>;
 
+struct RequiresFeature {
+  bool RequiredState;
+  std::string Feature;
+};
+using RequiresFeatureList = SmallVector;
+
 bool parseModuleId(ModuleId &Id);
 void parseModuleDecl();
+void parseModuleMembers();
 void parseExternModuleDecl();
-void parseRequiresDecl();
+void parseRequiresFeatureList(RequiresFeatureList &RFL);
+void parseRequiresDecl(bool TopLevel = false);
+void parseRequiresBlockBody(const RequiresFeatureList &RFL);
 void parseHeaderDecl(MMToken::TokenKind, SourceLocation LeadingLoc);
 void parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
 void parseExportDecl();
@@ -1825,6 +1834,7 @@
 ///
 ///   module-member:
 /// requires-declaration
+/// requires-block-declaration
 /// header-declaration
 /// submodule-declaration
 /// export-declaration
@@ -2030,6 +2040,38 @@
   ActiveModule->ModuleMapIsPrivate)
 diagnosePrivateModules(ExplicitLoc, FrameworkLoc);
 
+  parseModuleMembers();
+
+  if (To

[PATCH] D118915: [clang][deps] Generate '-fmodule-file=' only for direct dependencies

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118915

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

[PATCH] D118890: [clang][deps] Disable global module index

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm. I agree that testing this isn't really necessary given the difficulty of 
even constructing one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118890

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

[PATCH] D118986: [clang][deps] Return the whole TU command line

[Michael Spencer via Phabricator via cfe-commits]

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

This does require build systems to be able to totally replace a command line 
rather than just add to it, but it seems like that's a requirement unless we 
add a way to modify the command line to Clang. lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118986

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

[PATCH] D118855: [modules] Add a flag for TagDecl if it was a definition demoted to a declaration.

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118855

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

[PATCH] D66989: FileManager: Remove ShouldCloseOpenFile argument from getBufferForFile, NFC

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


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

https://reviews.llvm.org/D66989



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

[PATCH] D67127: [clang-scan-deps] add skip excluded conditional preprocessor block preprocessing optimization

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.
Herald added a subscriber: wuzish.



Comment at: clang/include/clang/Frontend/CompilerInstance.h:256
+  /// preprocessor.
+  void setAdditionalPPCallbacks(std::unique_ptr PPC);
+

This kinda sounds like it can be called multiple times.  Is there any way you 
can use chained pp callbacks?



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:171-177
+llvm::cl::opt SkipExcludedPPRanges(
+"skip-excluded-pp-ranges",
+llvm::cl::desc(
+"Use the preprocessor optimization that skips excluded conditionals by 
"
+"bumping the buffer pointer in the lexer instead of lexing the tokens  
"
+"until reaching the end directive."),
+llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));

Is there any reason for this to be configurable other than for perf testing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67127



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

[PATCH] D67127: [clang-scan-deps] add skip excluded conditional preprocessor block preprocessing optimization

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

Looking more into this I'm not sure you need the PPCallbacks.  The Preprocessor 
should just own the PreprocessorSkippedMappings data structure and the 
dependency scanner can update it live.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67127



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

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68997

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

[PATCH] D82860: Port ObjCMTAction to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82860

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

[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3938
+  if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);  
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE) 
\

Will this ever have an issue with lifetime? I can see various values for 
`EXTRACTOR` causing issues here. https://abseil.io/tips/107


It would be good to at least document somewhere the restrictions on `EXTRACTOR`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211

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

[PATCH] D83298: Add ability to make fixups to CompilerInvocation after option parsing

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83298

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

[PATCH] D86290: Move all fields of '-cc1' option related classes into def file databases

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86290

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

[PATCH] D92775: [clang][cli] Add flexible TableGen multiclass for boolean options

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:226
   ASSERT_THAT(GeneratedArgs,
-  Contains(StrEq("-fno-experimental-new-pass-manager")));
+  Not(Contains(StrEq("-fno-experimental-new-pass-manager";
   ASSERT_THAT(GeneratedArgs,

dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > Can you clarify why this was dropped? Was it previously emitted due to a 
> > > limitation in the implementation, or are we no longer supporting options 
> > > that always get emitted for clarity?
> > This option was the only one using the old infrastructure 
> > (`BooleanMarshalledFFlag`).
> > It was set up to always generate the flag, even when it wasn't necessary 
> > (this flag sets the keypath to its default value).
> > 
> > I think we should aim to generate only command line arguments that are 
> > necessary to preserve the original invocation semantics.
> > I imagine this will be useful when debugging: one won't need to scan 
> > hundreds of flags that don't have any effect on CompilerInvocation.
> This is a change in direction; the original thinking was that some options 
> should always be emitted for human readability. I don’t feel too strongly 
> about it, but I think this should be changed / dropped independently of other 
> work if it’s done. I suggest splitting this out; I’d also like to hear 
> @Bigcheese’s thoughts on that change since he did more of the original 
> reviews. 
In general there are some options that should always be kept, such as the 
triple, but I don't see any reason to always keep 
`-fno-experimental-new-pass-manager`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92775

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

[PATCH] D92857: [clang][cli] Don't always emit -f[no-]experimental-new-pass-manager

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM. I don't see any reason to always emit this option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92857

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

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm. Thanks for the patch.


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

https://reviews.llvm.org/D92191

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

[PATCH] D93008: [clang][cli] Do not marshall only CC1Option flags in BoolOption

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93008

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

[PATCH] D93094: [clang][cli] Prevent double denormalization

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm with the test fix.




Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:267
 
-  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-fdebug-pass-manager")));
+  ASSERT_EQ(count(GeneratedArgs, "-fdebug-pass-manager"), 1);
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fno-debug-pass-manager";

jansvoboda11 wrote:
> dexonsmith wrote:
> > jansvoboda11 wrote:
> > > Is it wise to rely on pointer comparison here? The call to `count` 
> > > returns 2 before changing the denormalizer and 1 after, but I'm not sure 
> > > if it will work on all platforms.
> > Does this compile / avoid the pointer comparison?
> > ```
> > ASSERT_EQ(count(GeneratedArgs, StringRef("-fdebug-pass-manager")), 1);
> > ```
> Yes, this forces the `const char *` from GeneratedArgs to be converted into 
> `StringRef`, which does comparison via `::memcmp`.
> 
> If we decide comparing `StringRef`s is a better solution, I'd be inclined to 
> create a helper function or custom matcher that does the conversion to 
> `StringRef`. That way, we don't have to worry about doing the right thing in 
> test cases.
I believe that if you have shared libraries enabled this is guaranteed to be 
different. We should do a string comparison here, and I think a helper makes 
sense if we expect to do this more often.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93094

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

[PATCH] D84669: [clang][cli] Port CodeGenOpts simple string flags to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm with the comment.




Comment at: clang/include/clang/Driver/Options.td:2123-2125
 def fpatchable_function_entry_EQ : Joined<["-"], 
"fpatchable-function-entry=">, Group, Flags<[CC1Option]>,
-  MetaVarName<"">, HelpText<"Generate M NOPs before function entry and 
N-M NOPs after function entry">;
+  MetaVarName<"">, HelpText<"Generate M NOPs before function entry and 
N-M NOPs after function entry">,
+  MarshallingInfoStringInt<"CodeGenOpts.PatchableFunctionEntryCount">;

This should have a comment saying that the cc1 semantics are different from the 
driver semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84669

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

[PATCH] D84668: [clang][cli] Port TargetOpts simple string based options to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm with the fix above.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:264-265
 
+template ::value, bool> = true>
+static void denormalizeSimpleEnum(SmallVectorImpl &Args,

jansvoboda11 wrote:
> dexonsmith wrote:
> > Once the template is gone from the `unsigned` overload above, I wonder if 
> > we can use `!std::is_convertible` here, and let the `unsigned` 
> > overload directly catch any enums that aren't strongly typed.
> Unfortunately, `std::is_convertible::value == false` when `T` is 
> an `enum class` (and it's the same for `std::is_constructible T>::value`): .
> 
> I didn't find any type trait in the standard library that would have the same 
> semantics as `static_cast`, but we could use something like 
> this: .
I would suggest instead to just rename the `unsigned` version to 
`denormalizeSimpleEnumImpl` and removing the constraints on this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84668

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

[PATCH] D84670: [clang][cli] Port HeaderSearch simple string options to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm with nit.




Comment at: clang/include/clang/Driver/Options.td:1815
+  HelpText<"Specify the interval (in seconds) after which a module file will 
be considered unused">,
+  MarshallingInfoStringInt<"HeaderSearchOpts->ModuleCachePruneAfter", "31 * 24 
*60 * 60">;
 def fmodules_search_all : Flag <["-"], "fmodules-search-all">, Group,

Missing a space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84670

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

[PATCH] D84671: [clang][cli] Port LangOpts simple string based options to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84671

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

[PATCH] D84672: [clang][cli] Port PreprocessorOpts simple string based options to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84672

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

[PATCH] D93395: [clang][cli] Remove -f[no-]trapping-math from -cc1 command line

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

This looks fine to be as that code is definitely dead. I would wait for 
@SjoerdMeijer in case there's a bug in the existing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93395

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

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

LGTM


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

https://reviews.llvm.org/D82756

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

[PATCH] D82860: Port ObjCMTAction to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D82860

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

[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3938
+  if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);  
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE) 
\

jansvoboda11 wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > jansvoboda11 wrote:
> > > > Bigcheese wrote:
> > > > > Will this ever have an issue with lifetime? I can see various values 
> > > > > for `EXTRACTOR` causing issues here. https://abseil.io/tips/107
> > > > > 
> > > > > 
> > > > > It would be good to at least document somewhere the restrictions on 
> > > > > `EXTRACTOR`.
> > > > Mentioned the reference lifetime extension in a comment near extractor 
> > > > definitions.
> > > It might be safer to refactor as:
> > > ```
> > > // Capture the extracted value as a lambda argument to avoid potential
> > > // lifetime extension issues.
> > > [&](const auto &Extracted) {
> > >   if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
> > > DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
> > > }(EXTRACTOR(this->KEYPATH));
> > > ```
> > > 
> > Might be even better to avoid the generic lambda:
> > ```
> > // Capture the extracted value as a lambda argument to avoid potential
> > // lifetime extension issues.
> > using ExtractedType =
> > std::remove_const_t > decltype(EXTRACTOR(this->KEYPATH))>>
> > [&](const ExtractedType &Extracted) {
> >   if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
> > DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
> > }(EXTRACTOR(this->KEYPATH));
> > ```
> > (since generic vs. non-generic could affect compile-time of 
> > CompilerInvocation.cpp given how many instances there will be).
> Thanks for the suggestions @dexonsmith. I'm having trouble writing a test 
> case where the lambda workaround produces a different result than `const auto 
> &` variable.
> @Bigcheese, could you show a concrete example of an extractor that causes 
> issues so I can test it out?
I think I was confused about when this can happen. The `const auto &` will 
never cause a conversion that would prevent lifetime extension. The only place 
this would break is if the copy constructor of the type is rather broken, which 
isn't a reasonable case to support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211

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

[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3938
+  if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);  
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE) 
\

Bigcheese wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > dexonsmith wrote:
> > > > jansvoboda11 wrote:
> > > > > Bigcheese wrote:
> > > > > > Will this ever have an issue with lifetime? I can see various 
> > > > > > values for `EXTRACTOR` causing issues here. 
> > > > > > https://abseil.io/tips/107
> > > > > > 
> > > > > > 
> > > > > > It would be good to at least document somewhere the restrictions on 
> > > > > > `EXTRACTOR`.
> > > > > Mentioned the reference lifetime extension in a comment near 
> > > > > extractor definitions.
> > > > It might be safer to refactor as:
> > > > ```
> > > > // Capture the extracted value as a lambda argument to avoid potential
> > > > // lifetime extension issues.
> > > > [&](const auto &Extracted) {
> > > >   if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
> > > > DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
> > > > }(EXTRACTOR(this->KEYPATH));
> > > > ```
> > > > 
> > > Might be even better to avoid the generic lambda:
> > > ```
> > > // Capture the extracted value as a lambda argument to avoid potential
> > > // lifetime extension issues.
> > > using ExtractedType =
> > > std::remove_const_t > > decltype(EXTRACTOR(this->KEYPATH))>>
> > > [&](const ExtractedType &Extracted) {
> > >   if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
> > > DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
> > > }(EXTRACTOR(this->KEYPATH));
> > > ```
> > > (since generic vs. non-generic could affect compile-time of 
> > > CompilerInvocation.cpp given how many instances there will be).
> > Thanks for the suggestions @dexonsmith. I'm having trouble writing a test 
> > case where the lambda workaround produces a different result than `const 
> > auto &` variable.
> > @Bigcheese, could you show a concrete example of an extractor that causes 
> > issues so I can test it out?
> I think I was confused about when this can happen. The `const auto &` will 
> never cause a conversion that would prevent lifetime extension. The only 
> place this would break is if the copy constructor of the type is rather 
> broken, which isn't a reasonable case to support.
Now I remember what the issue was, it's if the `EXTRACTOR` itself creates an 
owning temporary and converts back to a reference type: 
https://godbolt.org/z/xsvh4f

This is kind of weird to do, the `EXTRACTOR` would need to take an owning type 
with an implicit conversion from the type of `KEYPATH`, and then return a 
reference type. I'm not sure how realistic that situation is, but it seems fine 
to defend against with Duncan's suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211

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

[PATCH] D83698: [clang][cli] Port Target option flags to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83698

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

[PATCH] D92330: [clang-scan-deps] Improve argument parsing to find target object file path.

[Michael Spencer via Phabricator via cfe-commits]

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

lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92330

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

[PATCH] D102923: [clang][lex] Remark for used header search paths

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a subscriber: akyrtzi.
Bigcheese added a comment.

I think this is looking good, but would like either @dexonsmith, @akyrtzi, or 
someone else familiar with this area to take a look. Only other comment I have 
is that the new functions you add should use the current LLVM naming convention.

I also wonder how we should handle other things that are found via include 
paths such as module maps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

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

[PATCH] D94472: [WIP][clang][cli] Command line round-trip for HeaderSearch options

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D94472#2519838 , @jansvoboda11 
wrote:

> In D94472#2508018 , @dexonsmith 
> wrote:
>
>> `strict` mode additionally uses the `GeneratedArgs1` to fill 
>> CompilerInvocation, indirectly checking both directions by requiring tests 
>> to pass both with and without this round-trip. However, during development 
>> people generally only run the tests one way and the failure mode won't be 
>> ideal.
>
> So people build without assertions during development? In that case, I agree 
> that erroring out on `GeneratedArgs1 != GeneratedArgs2` (in all kinds of 
> builds) would improve the experience. I don't think there's anything 
> preventing us to incorporate this into the current patch.

The only issue I have with this if always parsing twice has a noticeable 
performance impact for any users of Clang. Can we measure the impact? If it's 
small (< 100ms ?) then that's fine. I'm also concerned about users like cling 
and clangd.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472

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

[PATCH] D95514: [clang][cli] Teach CompilerInvocation to allocate strings on its own

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

In D95514#2528324 , @jansvoboda11 
wrote:

> In D95514#2526064 , @dexonsmith 
> wrote:
>
>> Well, the compiler developers are the users, since `-cc1` isn't for 
>> end-users. I acknowledge it's a tradeoff. IMO, without actually seeing it in 
>> practice, it seems a bit nicer to design this away entirely. But maybe 
>> having a `=` is more valuable than I think.
>
>
>
>> For string options:
>>
>> - If `-cc1` accepts only `-opt string`, no need for allocation.
>> - If `-cc1` accepts only `-opt=string`, the fully-spelled option is on the 
>> original command-line. `ExistingStrings.find()` should turn it up when 
>> calling `Strings.save()`.
>> - If `-cc1` accepts both `-opt=string` and `-opt string` canonicalize to 
>> `-opt string` when generating. No need for allocation.
>>
>> For numeric / enum options we'll need to allocate, but the allocation 
>> doesn't have to live past running the parser.
>>
>> The only problem would be if `string` gets canonicalized / rewritten somehow 
>> during parse + generate, or if an enum value is additionally saved as a 
>> string value.
>
> We can definitely ask around how would people feel about that.
>
 Consider that some callers may parse the full command-line and then drop 
 everything but one options class. That pattern wouldn’t work anymore since 
 the StringRefs won’t be valid.
>>>
>>> Ah, that's right. So if we don't go with the allocation-less approach, we'd 
>>> need to take `StringAllocator` from the client that also ensures proper 
>>> lifetimes.
>>
>> Maybe it wouldn't be terrible to have a `RoundTrip` arg on each options 
>> class. When you assign to the CompilerInvocation, it copies the shared ptr 
>> into each of the options classes, so that the pool stays alive as long as 
>> one of them has it.
>
> I think this is the most robust solution.
>
> Moving `BumpPtrAllocator`/`StringSaver` into `CompilerInvocation` will become 
> problematic once we start round-tripping more option classes.
>
> I've looked through all option classes in `CompilerInvocation`, and 
> `AnalyzerOptions::Config` seems to be the only member that stores non-owning 
> references to command line arguments.
> I think the best path forward is to change `AnalyzerOptions` to take 
> ownership and avoid dealing with complicated lifetimes. We can look into 
> removing allocations later, as an optional optimization.
> What do you think?

I think that makes sense given the rest of `CompilerInvocation` is owning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95514

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

[PATCH] D95790: [clang][cli] Documentation of CompilerInvocation parsing/generation

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang/docs/InternalsManual.rst:711
+required for parsing or generating the command line argument.
+
+**Positive Flag**

You should create a separate section here for listing the classes.



Comment at: clang/docs/InternalsManual.rst:770
+The key path defaults to an empty ``std::vector``. Values 
specified
+with each appearance of the option on command line are appended to the vector.
+





Comment at: clang/docs/InternalsManual.rst:843
+implementation is still possible.
+
 The Lexer and Preprocessor Library

Past this you should add a section providing instructions on how to actually 
add marshaling for an option, and cover what to do if the automatic 
infrastructure isn't enough.



Comment at: clang/docs/InternalsManual.rst:590
+the driver options in ``clang/Driver/Options.td``. The information making up an
+option definition include the name and prefix (for example ``-std=``), form and
+position of the option value, help text, aliases and more. Each option may




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95790

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

[PATCH] D95790: [clang][cli] Documentation of CompilerInvocation parsing/generation

[Michael Spencer via Phabricator via cfe-commits]

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

Needs an example in the "Creating new Command Line Option" section, but with 
that it looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95790

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

[PATCH] D97461: [clang][cli] Implement '-cuid=' marshalling

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97461

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