[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

This commit seems to cause some regression for "-save-temps" as there is no new 
line before the pragma. See the below test case, -E will output
int test();#pragma clang assume_nonnull
It will fail the compilation on the preprocessed output with
error: expected unqualified-id
int test();#pragma clang assume_nonnull end

  ^

Thanks,

Manman
--

cat test-pragma.mm 
// RUN: %clang -E -o - %s | FileCheck %s

#define CF_ASSUME_NONNULL_BEGIN _Pragma("clang assume_nonnull begin")
#define CF_ASSUME_NONNULL_END   _Pragma("clang assume_nonnull end")

CF_ASSUME_NONNULL_BEGIN
int test();

CF_ASSUME_NONNULL_END
// CHECK-NOT: test();#pragma clang assume_nonnull


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104601

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

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

@dexonsmith @bruno: are you okay with this change? It looks good to me :]

Thanks,
Manman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109632

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

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

This looks good to me in general. Since it should not change functionality, it 
may not be possible to write a test case.

Manman




Comment at: clang/lib/Serialization/ASTReader.cpp:8194
+if (seen.insert(M).second) {
+  S.addMethodToGlobalList(, M);
+}

Does it make sense to check for duplication inside addMethodToGlobalList, as 
the function goes through the list as well? Maybe it is slower, as we will need 
to go through the list for each method, instead of a lookup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109632

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.



In D86049#2255738 , @rjmccall wrote:

> We just talk about it.  I agree with Nathan that we shouldn't just add this 
> as a short-term hack; we should design the ABI right and then do what we want.
>
> I think these are basically all the ABI questions:
>
> - Is there a good reason to preserve signature compatibility with normal ObjC 
> methods, or should we just drop the selector argument on direct methods?
> - Should we do the null test on the caller side or the callee side?
> - In direct class methods, is the caller's responsibility or the callee's to 
> ensure that `self` is initialized?
>
> For the first, I think dropping the argument is fine.

Dropping the argument sounds good to me! We are already passing the argument as 
undefined.

> For the second, I'm less certain.

It feels cleaner to do the null test on the callee side (i.e inside the 
wrapper). It may have binary size advantage compared to checking at each 
callsite.

> For the third, making it the callee's responsibility is generally better for 
> code size but harder to optimize.  But we only have the code-size cost on the 
> caller side when materializing from a global, and that's actually something 
> we can also eliminate as redundant.  So maybe the best approach is to 
> introduce a weak+hidden thunk that we use when making a call to a direct 
> class method on a specific class, and we have that thunk do the entire 
> materialization sequence.

Completely agree. For direct class method, making sure 'self' is initialized on 
the callee side is cleaner and better for code size. But caller has the context 
to know whether the class is already initialized.
Maybe we can start with doing it on callee's side and make an incremental 
improvement by implementing the approach that can get both benefits.

Do we need to support direct methods with variadic arguments for the wrapper 
implementation or are we going with the non-wrapper version? @plotfi

Thanks!
Manman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86049

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

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

In D75574#1925808 , @rjmccall wrote:

> This might also be interesting to @manmanren.


Thank you, John!

Manman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75574



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

[PATCH] D74813: [RFC] Add hash of block contents to function block names

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

There are a few concerns about the approach:
1> Compile time: dumping AST as string then hashing the string. Alex measured 
it on a synthetic benchmark, it shows insignificant impact.
2> Stability: from my understanding, the main goal of this patch is to increase 
stability of the symbol name so it will not change if the relevant code for the 
block is not changing. It may not be as stable when the compiler version 
changes.
Using per-function ID improves the stability compared to per-module ID. 
@alexbdv do you have rough ideas on how much better the proposed approach is in 
terms of stability, comparing to per-function ID?
3> Using a compiler flag may slow down the adoption.

@dexonsmith: How can we move this forward? Do you have any other suggestion?

Thanks!
Manman




Comment at: clang/lib/AST/Mangle.cpp:52
+
+  // Dump the statement IR to a text stream for hasing
+  stmt->dump(strStmtStream);

Should it be AST instead of IR in the comment here?



Comment at: clang/lib/AST/Mangle.cpp:56
+
+  // Strip out addresses
+  char *ptr = [1];

Is this needed to have deterministic behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



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

[PATCH] D58751: Order File Instrumentation: add clang support for -forder-file-instrumentation

[Manman Ren via Phabricator via cfe-commits]

manmanren closed this revision.
manmanren added a comment.

r355333


Repository:
  rC Clang

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

https://reviews.llvm.org/D58751



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

[PATCH] D58751: Order File Instrumentation: add clang support for -forder-file-instrumentation

[Manman Ren via Phabricator via cfe-commits]

manmanren marked 3 inline comments as done.
manmanren added inline comments.



Comment at: include/clang/Driver/Options.td:774
+Group, Flags<[CoreOption]>,
+HelpText<"Generate instrumented code to collect order file into 
default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env 
var)">;
 

modocache wrote:
> Would it make sense to add documentation to the Clang User's Manual for this 
> option? Some other profiling options are mentioned there, but not all.
Will do this in a followup!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58751



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

[PATCH] D58751: Order File Instrumentation: add clang support for -forder-file-instrumentation

[Manman Ren via Phabricator via cfe-commits]

manmanren created this revision.
manmanren added reviewers: davidxl, beanz, dexonsmith.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

When -forder-file-instrumentation is on, we pass llvm flag to enable the order 
file instrumentation pass.


Repository:
  rC Clang

https://reviews.llvm.org/D58751

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/clang_f_opts.c


Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -121,6 +121,7 @@
 // RUN: %clang -### -S -fcoverage-mapping -fno-coverage-mapping %s 2>&1 | 
FileCheck -check-prefix=CHECK-DISABLE-COVERAGE %s
 // RUN: %clang -### -S -fprofile-instr-generate -fcoverage-mapping 
-fno-coverage-mapping %s 2>&1 | FileCheck -check-prefix=CHECK-DISABLE-COVERAGE 
%s
 // RUN: %clang -### -S -fprofile-remapping-file foo/bar.txt %s 2>&1 | 
FileCheck -check-prefix=CHECK-PROFILE-REMAP %s
+// RUN: %clang -### -S -forder-file-instrumentation %s 2>&1 | FileCheck 
-check-prefix=CHECK-ORDERFILE-INSTR %s
 // CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
 // CHECK-PROFILE-GENERATE-LLVM: "-fprofile-instrument=llvm"
 // CHECK-PROFILE-GENERATE-DIR: 
"-fprofile-instrument-path=/some/dir{{/|}}{{.*}}"
@@ -132,6 +133,8 @@
 // CHECK-COVERAGE-AND-GEN: '-fcoverage-mapping' only allowed with 
'-fprofile-instr-generate'
 // CHECK-DISABLE-COVERAGE-NOT: "-fcoverage-mapping"
 // CHECK-PROFILE-REMAP: "-fprofile-remapping-file=foo/bar.txt"
+// CHECK-ORDERFILE-INSTR: "-forder-file-instrumentation"
+// CHECK-ORDERFILE-INSTR: "-enable-order-file-instrumentation"
 
 // RUN: %clang -### -S -fprofile-use %s 2>&1 | FileCheck 
-check-prefix=CHECK-PROFILE-USE %s
 // RUN: %clang -### -S -fprofile-instr-use %s 2>&1 | FileCheck 
-check-prefix=CHECK-PROFILE-USE %s
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -5307,6 +5307,17 @@
 }
   }
 
+  if (Args.hasArg(options::OPT_forder_file_instrumentation)) {
+ CmdArgs.push_back("-forder-file-instrumentation");
+ // Enable order file instrumentation when ThinLTO is not on. When ThinLTO 
is
+ // on, we need to pass these flags as linker flags and that will be 
handled
+ // outside of the compiler.
+ if (D.getLTOMode() != LTOK_Thin) {
+   CmdArgs.push_back("-mllvm");
+   CmdArgs.push_back("-enable-order-file-instrumentation");
+ }
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fforce_enable_int128,
options::OPT_fno_force_enable_int128)) {
 if (A->getOption().matches(options::OPT_fforce_enable_int128))
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -407,7 +407,8 @@
   Args.hasArg(options::OPT_fprofile_generate_EQ) ||
   Args.hasArg(options::OPT_fprofile_instr_generate) ||
   Args.hasArg(options::OPT_fprofile_instr_generate_EQ) ||
-  Args.hasArg(options::OPT_fcreate_profile))
+  Args.hasArg(options::OPT_fcreate_profile) ||
+  Args.hasArg(options::OPT_forder_file_instrumentation)) 
 return true;
 
   return false;
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -769,6 +769,9 @@
 def fprofile_exclude_files_EQ : Joined<["-"], "fprofile-exclude-files=">,
 Group, Flags<[CC1Option, CoreOption]>,
 HelpText<"Instrument only functions from files where names don't match all 
the regexes separated by a semi-colon">;
+def forder_file_instrumentation : Flag<["-"], "forder-file-instrumentation">,
+Group, Flags<[CoreOption]>,
+HelpText<"Generate instrumented code to collect order file into 
default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env 
var)">;
 
 def faddrsig : Flag<["-"], "faddrsig">, Group, Flags<[CoreOption, 
CC1Option]>,
   HelpText<"Emit an address-significance table">;


Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -121,6 +121,7 @@
 // RUN: %clang -### -S -fcoverage-mapping -fno-coverage-mapping %s 2>&1 | FileCheck -check-prefix=CHECK-DISABLE-COVERAGE %s
 // RUN: %clang -### -S -fprofile-instr-generate -fcoverage-mapping -fno-coverage-mapping %s 2>&1 | FileCheck -check-prefix=CHECK-DISABLE-COVERAGE %s
 // RUN: %clang -### -S -fprofile-remapping-file foo/bar.txt %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-REMAP %s
+// RUN: %clang -### -S -forder-file-instrumentation %s 2>&1 | FileCheck -check-prefix=CHECK-ORDERFILE-INSTR %s
 // CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
 // 

[PATCH] D51568: [modules] Add `-fno-absolute-module-directory` flag for relocatable modules

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

I am not sure if this is the best approach, but the implementation looks okay 
to me. @bruno @rsmith What do you think?

Manman




Comment at: include/clang/Serialization/ASTReader.h:2241
 SkipString(Record, Idx);
+Idx++;  // Relative
   }

Can you expand on this comment? Skip the relative bit?



Comment at: include/clang/Serialization/ASTWriter.h:642
   /// Convert a path from this build process into one that is appropriate
   /// for emission in the module file.
+  bool PreparePathForOutput(SmallVectorImpl ,

Can you add comment here on how we set IsRelativeModuleDirectory?



Comment at: include/clang/Serialization/ASTWriter.h:650
   /// Emit the current record with the given path as a blob.
-  void EmitRecordWithPath(unsigned Abbrev, RecordDataRef Record,
+  void EmitRecordWithPath(unsigned Abbrev, RecordDataImpl ,
   StringRef Path);

Can you update the comment to say that record is updated with the extra bit?


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568



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

[PATCH] D29923: Send UndefMacroDirective to MacroDefined callback

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

Please update the patch with context: 
http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Thanks,
Manman




Comment at: unittests/Basic/SourceManagerTest.cpp:251
   std::string Name;
-  bool isDefinition; // if false, it is expansion.
-  
-  MacroAction(SourceLocation Loc, StringRef Name, bool isDefinition)
-: Loc(Loc), Name(Name), isDefinition(isDefinition) { }
+  int Kind; // 0 expansion, 1 definition, 2 undefinition
+

Can we make this an enum?


https://reviews.llvm.org/D29923



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

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

Ping :]
Hoping to wrap this up this week.
Cheers,
Manman


https://reviews.llvm.org/D28299



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

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Manman Ren via Phabricator via cfe-commits]

manmanren marked an inline comment as done.
manmanren added a comment.

Ping :]
I am hoping to wrap this up this week. Thanks,
Manman


https://reviews.llvm.org/D27689



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

[PATCH] D28779: [ASTReader] Add a DeserializationListener callback for IMPORTED_MODULES

[Manman Ren via Phabricator via cfe-commits]

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

LGTM

Manman


https://reviews.llvm.org/D28779



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

[PATCH] D28790: [Modules] Correct test comment from obsolete earlier version of code. NFC

[Manman Ren via Phabricator via cfe-commits]

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

LGTM

Manman


https://reviews.llvm.org/D28790



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

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Manman Ren via Phabricator via cfe-commits]

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



Comment at: include/clang/Serialization/Module.h:19
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"

rsmith wrote:
> Redundant include?
Definition of ASTFileSignature is moved from Serialization/Module.h to 
Basic/Module.h so we need to have this include to get the definition.



Comment at: lib/Serialization/ASTWriter.cpp:1356
+  ASTFileSignature RetCode{{0}};
+  if (WritingModule && Context.getLangOpts().ImplicitModules)
+RetCode = WriteHash(0);

rsmith wrote:
> I don't think it makes sense to use `ImplicitModules` to control this. Doing 
> that for the old signature computation was at best a hack.
> 
> If there is no measurable performance difference from the hashing, we should 
> just do it unconditionally in all modes. Otherwise, there should be a 
> separate flag to control it; we should probably force that flag to be enabled 
> when the frontend implicitly builds a module and inserts it into the module 
> cache, but otherwise let the user control it as they see fit.
Use HSOpts.ModulesHashContent to guard the hashing of module file content. This 
flag is set to true when we implicitly build a module.


https://reviews.llvm.org/D27689



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

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

Bruno helped on collecting performance data for this patch:
$ cat cocoa-test.h
#import 

$ rm -rf tmp; clang -x objective-c -isysroot `xcrun --sdk macosx 
--show-sdk-path` -fblocks -fmodules -fmodules-cache-path=tmp cocoa-test.h 
-fsyntax-only

clang-release (10 runs)
---

name  avg min med max SD  SD%total
user 2.0023  1.9804  1.9994  2.0408  0.01490.74 20.0225
 sys 0.3601  0.3547  0.3505  0.4454  0.02877.98  3.6007
wall 2.5432  2.4291  2.4264  3.3604  0.2737   10.76 25.4324

clang-modhash-release (10 runs)
---

name  avg min med max SD  SD%total
user 2.1101  2.0935  2.1095  2.1274  0.01120.53 21.1012
 sys 0.3485  0.3462  0.3556  0.3476  0.00521.50  3.4854
wall 2.5614  2.5163  2.5605  2.6657  0.04191.63 25.6138

This ~5% slowdown for the total time of compiling all the modules (30+ modules).

A similar test for one object file in selfhost clang:

[TemplateName.cpp.o] clang-release (10 runs)


name  avg min med max SD  SD%total
user 7.4332  7.3716  7.4369  7.4976  0.04440.60 74.3318
 sys 0.5646  0.5512  0.5688  0.5570  0.01612.86  5.6464
wall 8.3357  8.1675  8.2991  8.2570  0.32403.89 83.3568

[TemplateName.cpp.o] clang-modhash-release (10 runs)


name  avg min med max SD  SD%total
user 7.6911  7.6202  7.6979  7.7779  0.04180.54 76.9114
 sys 0.5531  0.5437  0.5520  0.5622  0.01051.89  5.5315
wall 8.4598  8.3868  8.4839  8.5497  0.04550.54 84.5977

~3.5% slowdown for total of modules:

$ ls module.cache/1UKP65OUKXURH/
Clang_AST-1XMT5DOVQWGLJ.pcm   LLVM_C-2WKTHJHA6SDF0.pcm  
LLVM_Pass-HNKP05GYNHFQ.pcm
_Builtin_stddef_max_align_t-2T552E4NSAIDI.pcm
Clang_Basic-1XMT5DOVQWGLJ.pcm 
LLVM_Config_ABI_Breaking-2FT1AFJTZ51QZ.pcm
LLVM_Support_DataTypes-2FT1AFJTZ51QZ.pcm  modules.idx
Darwin-399Z2LWDSMSTV.pcm  LLVM_IR-HNKP05GYNHFQ.pcm  
LLVM_Utils-HNKP05GYNHFQ.pcm   std-1337JMP4UGPIP.pcm

Note that the cache was wiped out right before every tests instance.


Thanks Bruno. I am going to guard the hashing with a CC1 option (something like 
fmodules-hash-content) and force that flag to be enabled when the frontend 
implicitly builds a module. We will try to improve the hashing performance 
later (either improving SHA1 or using MD5 instead of SHA1).

Manman


https://reviews.llvm.org/D27689



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

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Manman Ren via Phabricator via cfe-commits]

manmanren updated this revision to Diff 84137.
manmanren added a comment.

Addressing review comments.


https://reviews.llvm.org/D28299

Files:
  include/clang/Basic/DiagnosticSerializationKinds.td
  include/clang/Basic/FileManager.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  include/clang/Serialization/Module.h
  include/clang/Serialization/ModuleManager.h
  lib/Basic/FileManager.cpp
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/ChainedIncludesSource.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GeneratePCH.cpp
  lib/Serialization/ModuleManager.cpp
  test/Modules/Inputs/system-out-of-date/X.h
  test/Modules/Inputs/system-out-of-date/Y.h
  test/Modules/Inputs/system-out-of-date/Z.h
  test/Modules/Inputs/system-out-of-date/module.map
  test/Modules/system-out-of-date-test.m

Index: test/Modules/system-out-of-date-test.m
===
--- /dev/null
+++ test/Modules/system-out-of-date-test.m
@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: echo '@import X;' | \
+// RUN:   %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date \
+// RUN: -fsyntax-only -x objective-c -
+
+// We have an version built with different diagnostic options.
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date -Wnon-modular-include-in-framework-module -Werror=non-modular-include-in-framework-module %s -fsyntax-only 2>&1 | FileCheck %s
+ @import X;
+ 
+ #import 
+// CHECK: While building module 'Z' imported from
+// CHECK: {{.*}}Y-{{.*}}pcm' was validated as a system module and is now being imported as a non-system module
Index: test/Modules/Inputs/system-out-of-date/module.map
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/module.map
@@ -0,0 +1,12 @@
+module X [system] {
+  header "X.h" // imports Y
+  export *
+}
+module Y {
+  header "Y.h"
+  export *
+}
+module Z {
+  header "Z.h" // imports Y
+  export *
+}
Index: test/Modules/Inputs/system-out-of-date/Z.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Z.h
@@ -0,0 +1 @@
+#import 
Index: test/Modules/Inputs/system-out-of-date/Y.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Y.h
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/system-out-of-date/X.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/X.h
@@ -0,0 +1 @@
+#import 
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,11 @@
 // Load the contents of the module
 if (std::unique_ptr Buffer = lookupBuffer(FileName)) {
   // The buffer was already provided for us.
-  ModuleEntry->Buffer = std::move(Buffer);
+  ModuleEntry->Buffer = Buffer.get();
+  FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(Buffer));
+} else if(llvm::MemoryBuffer *ConsistentB =
+  FileMgr.getPCMCache()->lookupConsistentBuffer(FileName)) {
+  ModuleEntry->Buffer = ConsistentB;
 } else {
   // Open the AST file.
   llvm::ErrorOr Buf(
@@ -131,7 +135,8 @@
 return Missing;
   }
 
-  ModuleEntry->Buffer = std::move(*Buf);
+  ModuleEntry->Buffer = (*Buf).get();
+  FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(*Buf));
 }
 
 // Initialize the stream.
@@ -148,8 +153,11 @@
   ErrorStr = ModuleEntry->Signature != ASTFileSignature({{0}}) ?
  "signature mismatch" : "could not read module signature";
 
-  if (NewModule)
+  if (NewModule) {
+FileMgr.getPCMCache()->removeFromConsistentBuffer(
+ModuleEntry->FileName);
 delete ModuleEntry;
+  }
   return OutOfDate;
 }
   }
@@ -184,7 +192,8 @@
 void ModuleManager::removeModules(
 ModuleIterator first, ModuleIterator last,
 llvm::SmallPtrSetImpl ,
-ModuleMap *modMap) {
+ModuleMap *modMap,
+llvm::SmallVectorImpl ) {
   if (first == last)
 return;
 
@@ -227,16 +236,76 @@
 // Files that didn't make it through ReadASTCore successfully will be
 // rebuilt (or there was an error). Invalidate them so that we can load the
 // new files that will be renamed over the old ones.
-if (LoadedSuccessfully.count(*victim) == 0)
-  FileMgr.invalidateCache((*victim)->File);
-
+if (LoadedSuccessfully.count(*victim) == 0) {
+  // Before removing the module file, check if it was validated in an
+  // ancestor 

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Manman Ren via Phabricator via cfe-commits]

manmanren updated this revision to Diff 83999.
manmanren added a comment.

Rebase on top of r291686.


https://reviews.llvm.org/D27689

Files:
  include/clang/AST/ExternalASTSource.h
  include/clang/Basic/Module.h
  include/clang/Frontend/PCHContainerOperations.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  include/clang/Serialization/Module.h
  lib/Basic/Module.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  lib/Frontend/ASTUnit.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GeneratePCH.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/Module.cpp
  lib/Serialization/ModuleManager.cpp
  test/Modules/diagnostic-options-out-of-date.m
  test/Modules/explicit-build.cpp
  test/Modules/module_file_info.m
  test/Modules/rebuild.m

Index: test/Modules/rebuild.m
===
--- test/Modules/rebuild.m
+++ test/Modules/rebuild.m
@@ -19,11 +19,10 @@
 // RUN: diff %t/Module.size %t/Module.size.saved
 // RUN: cp %t/Module.pcm %t/Module.pcm.saved.2
 
-// But the signature at least is expected to change, so we rebuild DependsOnModule.
-// NOTE: if we change how the signature is created, this test may need updating.
+// The signature is the hash of the PCM content, we will not rebuild rebuild DependsOnModule.
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -fsyntax-only -F %S/Inputs %s
 // RUN: diff %t/Module.pcm %t/Module.pcm.saved.2
-// RUN: not diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
+// RUN: diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
 
 // Rebuild Module, reset its timestamp, and verify its size hasn't changed
 // RUN: rm %t/Module.pcm
@@ -34,10 +33,9 @@
 // RUN: cp %t/Module.pcm %t/Module.pcm.saved.2
 
 // Verify again with Module pre-imported.
-// NOTE: if we change how the signature is created, this test may need updating.
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -fsyntax-only -F %S/Inputs %s
 // RUN: diff %t/Module.pcm %t/Module.pcm.saved.2
-// RUN: not diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
+// RUN: diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
 
 #ifdef PREIMPORT
 @import Module;
Index: test/Modules/module_file_info.m
===
--- test/Modules/module_file_info.m
+++ test/Modules/module_file_info.m
@@ -29,11 +29,6 @@
 // CHECK: CPU:
 // CHECK: ABI:
 
-// CHECK: Diagnostic options:
-// CHECK:   IgnoreWarnings: Yes
-// CHECK:   Diagnostic flags:
-// CHECK: -Wunused
-
 // CHECK: Header search options:
 // CHECK:   System root [-isysroot=]: '/'
 // CHECK:   Use builtin include directories [-nobuiltininc]: Yes
@@ -47,3 +42,8 @@
 // CHECK:   Predefined macros:
 // CHECK: -DBLARG
 // CHECK: -DWIBBLE=WOBBLE
+
+// CHECK: Diagnostic options:
+// CHECK:   IgnoreWarnings: Yes
+// CHECK:   Diagnostic flags:
+// CHECK: -Wunused
Index: test/Modules/explicit-build.cpp
===
--- test/Modules/explicit-build.cpp
+++ test/Modules/explicit-build.cpp
@@ -199,6 +199,6 @@
 // RUN:-fmodule-file=%t/c.pcm \
 // RUN:%s -DHAVE_A -DHAVE_B -DHAVE_C 2>&1 | FileCheck --check-prefix=CHECK-MISMATCHED-B %s
 //
-// CHECK-MISMATCHED-B:  fatal error: module file '{{.*}}b.pcm' is out of date and needs to be rebuilt: module file out of date
+// CHECK-MISMATCHED-B:  fatal error: module file '{{.*}}b.pcm' is out of date and needs to be rebuilt: signature mismatch
 // CHECK-MISMATCHED-B-NEXT: note: imported by module 'c'
 // CHECK-MISMATCHED-B-NOT:  note:
Index: test/Modules/diagnostic-options-out-of-date.m
===
--- test/Modules/diagnostic-options-out-of-date.m
+++ test/Modules/diagnostic-options-out-of-date.m
@@ -7,6 +7,16 @@
 // RUN: %clang_cc1 -Werror -Wconversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs %s -fmodules-disable-diagnostic-validation
 // Make sure we don't error out when using the pch
 // RUN: %clang_cc1 -Werror -Wno-conversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify -fmodules-disable-diagnostic-validation
+
+// Build A.pcm
+// RUN: %clang_cc1 -Werror -Wno-conversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs %s
+// Build pch that imports A.pcm
+// RUN: %clang_cc1 -Werror -Wno-conversion -emit-pch -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -o %t.pch -I %S/Inputs -x objective-c-header %S/Inputs/pch-import-module-out-of-date.pch
+// We will rebuild A.pcm and overwrite the original A.pcm that the pch imports, but the two versions have the same hash.
+// RUN: 

[PATCH] D28415: PCH: fix a regression that reports a module is defined in both pch and pcm

[Manman Ren via Phabricator via cfe-commits]

manmanren created this revision.
manmanren added reviewers: rsmith, benlangmuir, doug.gregor.
manmanren added a subscriber: cfe-commits.

In r276159, we started to say that a module X is defined in a pch if we specify 
-fmodule-name when building the pch.
This caused a regression that reports module X is defined in both pch and pcm 
if we generate the pch with -fmodule-name=X and then in a separate clang 
invocation, we include the pch and also import X.pcm.

This patch adds an option CompilingPCH similar to CompilingModule. When we use 
-fmodule-name=X while building a pch, modular headers in X will be textually 
included and the compiler knows that we are not building module X, so we don't 
put module X in SUBMODULE_DEFINITION of the pch.


https://reviews.llvm.org/D28415

Files:
  include/clang/Basic/LangOptions.def
  include/clang/Frontend/FrontendActions.h
  lib/Frontend/FrontendActions.cpp
  lib/Lex/PPDirectives.cpp
  lib/Serialization/ASTWriter.cpp
  test/Modules/Inputs/pch-with-module-name/A.h
  test/Modules/Inputs/pch-with-module-name/C.h
  test/Modules/Inputs/pch-with-module-name/C.m
  test/Modules/Inputs/pch-with-module-name/D.h
  test/Modules/Inputs/pch-with-module-name/module.modulemap
  test/Modules/Inputs/pch-with-module-name/test.h
  test/Modules/pch-with-module-name.m

Index: test/Modules/pch-with-module-name.m
===
--- test/Modules/pch-with-module-name.m
+++ test/Modules/pch-with-module-name.m
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/pch-with-module-name -emit-pch -o %t-A.pch %S/Inputs/pch-with-module-name/test.h -fmodule-name=Contacts -x objective-c-header
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/pch-with-module-name -include-pch %t-A.pch %s -fsyntax-only -fmodule-name=Contacts -verify
+// expected-no-diagnostics 
+#include "C.h"
Index: test/Modules/Inputs/pch-with-module-name/test.h
===
--- test/Modules/Inputs/pch-with-module-name/test.h
+++ test/Modules/Inputs/pch-with-module-name/test.h
@@ -0,0 +1 @@
+#include "A.h"
Index: test/Modules/Inputs/pch-with-module-name/module.modulemap
===
--- test/Modules/Inputs/pch-with-module-name/module.modulemap
+++ test/Modules/Inputs/pch-with-module-name/module.modulemap
@@ -0,0 +1,9 @@
+module CloudKit {
+  header "C.h"
+  export *
+}
+
+module Contacts {
+  header "D.h"
+  export *
+}
Index: test/Modules/Inputs/pch-with-module-name/D.h
===
--- test/Modules/Inputs/pch-with-module-name/D.h
+++ test/Modules/Inputs/pch-with-module-name/D.h
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/pch-with-module-name/C.m
===
--- test/Modules/Inputs/pch-with-module-name/C.m
+++ test/Modules/Inputs/pch-with-module-name/C.m
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/pch-with-module-name/C.h
===
--- test/Modules/Inputs/pch-with-module-name/C.h
+++ test/Modules/Inputs/pch-with-module-name/C.h
@@ -0,0 +1 @@
+#include "D.h"
Index: test/Modules/Inputs/pch-with-module-name/A.h
===
--- test/Modules/Inputs/pch-with-module-name/A.h
+++ test/Modules/Inputs/pch-with-module-name/A.h
@@ -0,0 +1 @@
+// in pch
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4654,17 +4654,6 @@
   // If we're emitting a module, write out the submodule information.  
   if (WritingModule)
 WriteSubmodules(WritingModule);
-  else if (!getLangOpts().CurrentModule.empty()) {
-// If we're building a PCH in the implementation of a module, we may need
-// the description of the current module.
-//
-// FIXME: We may need other modules that we did not load from an AST file,
-// such as if a module declares a 'conflicts' on a different module.
-Module *M = PP.getHeaderSearchInfo().getModuleMap().findModule(
-getLangOpts().CurrentModule);
-if (M && !M->IsFromModuleFile)
-  WriteSubmodules(M);
-  }
 
   Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes);
 
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1996,18 +1996,28 @@
 
   // Ask HeaderInfo if we should enter this #include file.  If not, #including
   // this file will have no effect.
+  bool SkipHeader = false;
   if (ShouldEnter &&
   !HeaderInfo.ShouldEnterIncludeFile(*this, File, isImport,
  SuggestedModule.getModule())) {
 ShouldEnter = 

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Manman Ren via Phabricator via cfe-commits]

manmanren added inline comments.



Comment at: include/clang/Basic/DiagnosticSerializationKinds.td:131
+  "diagnostic options now it is a non-system module">,
+  InGroup;
+

aprantl wrote:
> Is this better?
> 
> "module file '%0' was validated as a system module and is now being imported 
> as a non-system module; any differences in diagnostics options will be 
> ignored"
Thanks Adrian. Your wording sounds better.



Comment at: include/clang/Basic/DiagnosticSerializationKinds.td:135
+  "module file '%0' was validated in an ancestor thread, now it needs to "
+  "be invalidated">;
+

aprantl wrote:
> I wonder if there is a way to rephrase this message such that an end-user 
> could understan how to interpret this error?
I am not sure if we will emit this error diagnostic in some case, that is why I 
don't have a testing case. Maybe it is better to throw a hard error instead of 
a diagnostic that user can't understand?



https://reviews.llvm.org/D28299



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

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Manman Ren via Phabricator via cfe-commits]

manmanren updated this revision to Diff 83273.
manmanren added a comment.

Add testing case.


https://reviews.llvm.org/D28299

Files:
  include/clang/Basic/DiagnosticSerializationKinds.td
  include/clang/Basic/FileManager.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  include/clang/Serialization/Module.h
  include/clang/Serialization/ModuleManager.h
  lib/Basic/FileManager.cpp
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/ChainedIncludesSource.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GeneratePCH.cpp
  lib/Serialization/ModuleManager.cpp
  test/Modules/Inputs/system-out-of-date/X.h
  test/Modules/Inputs/system-out-of-date/Y.h
  test/Modules/Inputs/system-out-of-date/Z.h
  test/Modules/Inputs/system-out-of-date/module.map
  test/Modules/system-out-of-date-test.m

Index: test/Modules/system-out-of-date-test.m
===
--- /dev/null
+++ test/Modules/system-out-of-date-test.m
@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: echo '@import X;' | \
+// RUN:   %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date \
+// RUN: -fsyntax-only -x objective-c -
+
+// We have an version built with different diagnostic options.
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date -Wnon-modular-include-in-framework-module -Werror=non-modular-include-in-framework-module %s -fsyntax-only 2>&1 | FileCheck %s
+ @import X;
+ 
+ #import 
+// CHECK: While building module 'Z' imported from
+// CHECK: {{.*}}Y-{{.*}}pcm' was validated as a system module, ignoring differences in diagnostic options
Index: test/Modules/Inputs/system-out-of-date/module.map
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/module.map
@@ -0,0 +1,12 @@
+module X [system] {
+  header "X.h" // imports Y
+  export *
+}
+module Y {
+  header "Y.h"
+  export *
+}
+module Z {
+  header "Z.h" // imports Y
+  export *
+}
Index: test/Modules/Inputs/system-out-of-date/Z.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Z.h
@@ -0,0 +1 @@
+#import 
Index: test/Modules/Inputs/system-out-of-date/Y.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/Y.h
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/system-out-of-date/X.h
===
--- /dev/null
+++ test/Modules/Inputs/system-out-of-date/X.h
@@ -0,0 +1 @@
+#import 
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,11 @@
 // Load the contents of the module
 if (std::unique_ptr Buffer = lookupBuffer(FileName)) {
   // The buffer was already provided for us.
-  ModuleEntry->Buffer = std::move(Buffer);
+  ModuleEntry->Buffer = Buffer.get();
+  FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(Buffer));
+} else if(llvm::MemoryBuffer *ConsistentB =
+  FileMgr.getPCMCache()->lookupConsistentBuffer(FileName)) {
+  ModuleEntry->Buffer = ConsistentB;
 } else {
   // Open the AST file.
   llvm::ErrorOr Buf(
@@ -131,7 +135,8 @@
 return Missing;
   }
 
-  ModuleEntry->Buffer = std::move(*Buf);
+  ModuleEntry->Buffer = (*Buf).get();
+  FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(*Buf));
 }
 
 // Initialize the stream.
@@ -148,8 +153,11 @@
   ErrorStr = ModuleEntry->Signature != ASTFileSignature({{0}}) ?
  "signature mismatch" : "could not read module signature";
 
-  if (NewModule)
+  if (NewModule) {
+FileMgr.getPCMCache()->removeFromConsistentBuffer(
+ModuleEntry->FileName);
 delete ModuleEntry;
+  }
   return OutOfDate;
 }
   }
@@ -184,7 +192,8 @@
 void ModuleManager::removeModules(
 ModuleIterator first, ModuleIterator last,
 llvm::SmallPtrSetImpl ,
-ModuleMap *modMap) {
+ModuleMap *modMap,
+llvm::SmallVectorImpl ) {
   if (first == last)
 return;
 
@@ -227,16 +236,76 @@
 // Files that didn't make it through ReadASTCore successfully will be
 // rebuilt (or there was an error). Invalidate them so that we can load the
 // new files that will be renamed over the old ones.
-if (LoadedSuccessfully.count(*victim) == 0)
-  FileMgr.invalidateCache((*victim)->File);
-
+if (LoadedSuccessfully.count(*victim) == 0) {
+  // Before removing the module file, check if it was validated in an
+  // ancestor thread, if yes, throw 

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

I forgot to upload the testing case, I will add it now.

Manman




Comment at: include/clang/Basic/FileManager.h:176
+  /// Manage memory buffers associated with pcm files.
+  std::unique_ptr BufferMgr;
+

benlangmuir wrote:
> Why is this inside the FileManager? It isn't used by the FileManager.
I initially had another patch that puts PCMCache object at the top level (i.e 
whenever we create a FileManager, we create a PCMCache object). The initial 
patch is much bigger than this patch. PCMCache is cacheing the content of a PCM 
File, and is shared across threads that we spawned to build modules implicitly, 
just like FileManager. I had some discussions with Bruno and Adrian, we felt 
FileManager is a better place. But if you have other suggestions, please let me 
know!



Comment at: include/clang/Basic/FileManager.h:308
+  /// a thread, we pop a ThreadContext.
+  struct ThreadContext {
+/// Keep track of all module files that have been validated in this thread

benlangmuir wrote:
> Can we call this something like "ModuleLoadContext" or maybe 
> "ModuleCompilationContext"?  The word thread is misleading, since the threads 
> are not run concurrently, and are only an implementation detail of our crash 
> recovery.
> 
> Also, can you explain why it is only necessary to keep information about the 
> the current stack of contexts?  In a situation like
> 
> A imports B imports C
> A imports D imports C
> 
> We would no longer have information about C being validated, right?  What 
> happens if there's a mismatch there?  Can this never happen?
Sure, we can call it ModuleCompilationContext because we are spawning threads 
to build modules implicitly.

The issue we are trying to detect is use-after-free, i.e we don't want a thread 
to hold on to an invalid FileEntry. We error out before trying to delete the 
FileEntry that is live in another thread.

In a situation like
A imports B imports C
A imports D imports C

A will start a thread to build B, and B will start a thread to build C, C will 
be validated in B's compilation thread and A's compilation thread. Later on, if 
D tries to invalidate C, D knows that an ancestor thread A has validated C, so 
the compiler will error out. And it is okay for A to invalidate C in A's 
compilation thread, because we can clean up the references to C's FileEntry.



https://reviews.llvm.org/D28299



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

[PATCH] D26034: [CodeCompletion] Block property setters: Use dynamic priority heuristic

[Manman Ren via Phabricator via cfe-commits]

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

LGTM.

Manman


Repository:
  rL LLVM

https://reviews.llvm.org/D26034



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

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Manman Ren via Phabricator via cfe-commits]

manmanren created this revision.
manmanren added reviewers: rsmith, benlangmuir, dexonsmith.
manmanren added subscribers: cfe-commits, bruno.

We create a PCMCache class to manage memory buffers associated with pcm files. 
With implicit modules, we currently use lock files to make
sure we are making progress when a single clang invocation builds a module file 
and then loads the module file immediately after. This
implementation requires lock file for correctness and causes all kinds of 
locking issues.

This patch tries to use PCMCache to share the content between writes and reads 
within a single clang invocation, so we can remove the
correctness-side of the lock file, and only use lock file for performance i.e 
to reduce chance of building the same pcm file from multiple
clang invocations.

We also add ThreadContext in PCMCache to diagnose cases where an ancestor 
validates a module and later on, a child thread tries
to invalidate it. Without the patch, this scenario will cause use-after-free of 
the FileEntry held by the ancestor.


https://reviews.llvm.org/D28299

Files:
  include/clang/Basic/DiagnosticSerializationKinds.td
  include/clang/Basic/FileManager.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  include/clang/Serialization/Module.h
  include/clang/Serialization/ModuleManager.h
  lib/Basic/FileManager.cpp
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/ChainedIncludesSource.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GeneratePCH.cpp
  lib/Serialization/ModuleManager.cpp

Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -108,7 +108,11 @@
 // Load the contents of the module
 if (std::unique_ptr Buffer = lookupBuffer(FileName)) {
   // The buffer was already provided for us.
-  ModuleEntry->Buffer = std::move(Buffer);
+  ModuleEntry->Buffer = Buffer.get();
+  FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(Buffer));
+} else if(llvm::MemoryBuffer *ConsistentB =
+  FileMgr.getPCMCache()->lookupConsistentBuffer(FileName)) {
+  ModuleEntry->Buffer = ConsistentB;
 } else {
   // Open the AST file.
   llvm::ErrorOr Buf(
@@ -131,7 +135,8 @@
 return Missing;
   }
 
-  ModuleEntry->Buffer = std::move(*Buf);
+  ModuleEntry->Buffer = (*Buf).get();
+  FileMgr.getPCMCache()->addConsistentBuffer(FileName, std::move(*Buf));
 }
 
 // Initialize the stream.
@@ -148,8 +153,11 @@
   ErrorStr = ModuleEntry->Signature != ASTFileSignature({{0}}) ?
  "signature mismatch" : "could not read module signature";
 
-  if (NewModule)
+  if (NewModule) {
+FileMgr.getPCMCache()->removeFromConsistentBuffer(
+ModuleEntry->FileName);
 delete ModuleEntry;
+  }
   return OutOfDate;
 }
   }
@@ -184,7 +192,8 @@
 void ModuleManager::removeModules(
 ModuleIterator first, ModuleIterator last,
 llvm::SmallPtrSetImpl ,
-ModuleMap *modMap) {
+ModuleMap *modMap,
+llvm::SmallVectorImpl ) {
   if (first == last)
 return;
 
@@ -227,16 +236,76 @@
 // Files that didn't make it through ReadASTCore successfully will be
 // rebuilt (or there was an error). Invalidate them so that we can load the
 // new files that will be renamed over the old ones.
-if (LoadedSuccessfully.count(*victim) == 0)
-  FileMgr.invalidateCache((*victim)->File);
-
+if (LoadedSuccessfully.count(*victim) == 0) {
+  // Before removing the module file, check if it was validated in an
+  // ancestor thread, if yes, throw a hard error instead of causing
+  // use-after-free in the ancestor thread.
+  bool IsSystem;
+  if (FileMgr.getPCMCache()->isValidatedByAncestor((*victim)->FileName,
+   IsSystem))
+ValidationConflicts.push_back((*victim)->FileName);
+  else
+FileMgr.invalidateCache((*victim)->File);
+  FileMgr.getPCMCache()->removeFromConsistentBuffer((*victim)->FileName);
+}
 delete *victim;
   }
 
   // Remove the modules from the chain.
   Chain.erase(first, last);
 }
 
+llvm::MemoryBuffer*
+PCMCache::lookupConsistentBuffer(std::string Name) {
+  if (!ConsistentBuffers.count(Name))
+return nullptr;
+  return ConsistentBuffers[Name].first.get();
+}
+
+void
+PCMCache::addConsistentBuffer(std::string FileName,
+  std::unique_ptr Buffer) {
+  assert(!ConsistentBuffers.count(FileName) && "already has a buffer");
+  ConsistentBuffers[FileName] = std::make_pair(std::move(Buffer),
+   /*IsSystem*/false);
+}
+
+void PCMCache::removeFromConsistentBuffer(std::string FileName) {
+  

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Manman Ren via Phabricator via cfe-commits]

manmanren updated this revision to Diff 83068.
manmanren added a comment.

Addressing review comments!

I still need to measure the performance impact of calculating the hash then 
decide how to enable this.


https://reviews.llvm.org/D27689

Files:
  include/clang/AST/ExternalASTSource.h
  include/clang/Basic/Module.h
  include/clang/Frontend/PCHContainerOperations.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  include/clang/Serialization/Module.h
  lib/Basic/Module.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  lib/Frontend/ASTUnit.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GeneratePCH.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/Module.cpp
  lib/Serialization/ModuleManager.cpp
  test/Modules/diagnostic-options-out-of-date.m
  test/Modules/explicit-build.cpp
  test/Modules/module_file_info.m
  test/Modules/rebuild.m

Index: test/Modules/rebuild.m
===
--- test/Modules/rebuild.m
+++ test/Modules/rebuild.m
@@ -19,11 +19,10 @@
 // RUN: diff %t/Module.size %t/Module.size.saved
 // RUN: cp %t/Module.pcm %t/Module.pcm.saved.2
 
-// But the signature at least is expected to change, so we rebuild DependsOnModule.
-// NOTE: if we change how the signature is created, this test may need updating.
+// The signature is the hash of the PCM content, we will not rebuild rebuild DependsOnModule.
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -fsyntax-only -F %S/Inputs %s
 // RUN: diff %t/Module.pcm %t/Module.pcm.saved.2
-// RUN: not diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
+// RUN: diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
 
 // Rebuild Module, reset its timestamp, and verify its size hasn't changed
 // RUN: rm %t/Module.pcm
@@ -34,10 +33,9 @@
 // RUN: cp %t/Module.pcm %t/Module.pcm.saved.2
 
 // Verify again with Module pre-imported.
-// NOTE: if we change how the signature is created, this test may need updating.
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -fsyntax-only -F %S/Inputs %s
 // RUN: diff %t/Module.pcm %t/Module.pcm.saved.2
-// RUN: not diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
+// RUN: diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
 
 #ifdef PREIMPORT
 @import Module;
Index: test/Modules/module_file_info.m
===
--- test/Modules/module_file_info.m
+++ test/Modules/module_file_info.m
@@ -29,11 +29,6 @@
 // CHECK: CPU:
 // CHECK: ABI:
 
-// CHECK: Diagnostic options:
-// CHECK:   IgnoreWarnings: Yes
-// CHECK:   Diagnostic flags:
-// CHECK: -Wunused
-
 // CHECK: Header search options:
 // CHECK:   System root [-isysroot=]: '/'
 // CHECK:   Use builtin include directories [-nobuiltininc]: Yes
@@ -47,3 +42,8 @@
 // CHECK:   Predefined macros:
 // CHECK: -DBLARG
 // CHECK: -DWIBBLE=WOBBLE
+
+// CHECK: Diagnostic options:
+// CHECK:   IgnoreWarnings: Yes
+// CHECK:   Diagnostic flags:
+// CHECK: -Wunused
Index: test/Modules/explicit-build.cpp
===
--- test/Modules/explicit-build.cpp
+++ test/Modules/explicit-build.cpp
@@ -199,6 +199,6 @@
 // RUN:-fmodule-file=%t/c.pcm \
 // RUN:%s -DHAVE_A -DHAVE_B -DHAVE_C 2>&1 | FileCheck --check-prefix=CHECK-MISMATCHED-B %s
 //
-// CHECK-MISMATCHED-B:  fatal error: module file '{{.*}}b.pcm' is out of date and needs to be rebuilt: module file out of date
+// CHECK-MISMATCHED-B:  fatal error: module file '{{.*}}b.pcm' is out of date and needs to be rebuilt: signature mismatch
 // CHECK-MISMATCHED-B-NEXT: note: imported by module 'c'
 // CHECK-MISMATCHED-B-NOT:  note:
Index: test/Modules/diagnostic-options-out-of-date.m
===
--- test/Modules/diagnostic-options-out-of-date.m
+++ test/Modules/diagnostic-options-out-of-date.m
@@ -7,6 +7,16 @@
 // RUN: %clang_cc1 -Werror -Wconversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs %s -fmodules-disable-diagnostic-validation
 // Make sure we don't error out when using the pch
 // RUN: %clang_cc1 -Werror -Wno-conversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify -fmodules-disable-diagnostic-validation
+
+// Build A.pcm
+// RUN: %clang_cc1 -Werror -Wno-conversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs %s
+// Build pch that imports A.pcm
+// RUN: %clang_cc1 -Werror -Wno-conversion -emit-pch -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -o %t.pch -I %S/Inputs -x objective-c-header %S/Inputs/pch-import-module-out-of-date.pch
+// We will rebuild A.pcm and 

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Manman Ren via Phabricator via cfe-commits]

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

Thanks,
Manman


https://reviews.llvm.org/D27852



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

[PATCH] D27546: [ASTReader] Sort RawComments before merging

[Manman Ren via Phabricator via cfe-commits]

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

LGTM.

Manman


https://reviews.llvm.org/D27546



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

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Manman Ren via Phabricator via cfe-commits]

manmanren created this revision.
manmanren added reviewers: rsmith, benlangmuir, aprantl.
manmanren added subscribers: bruno, cfe-commits, dexonsmith.

We change ASTFileSignature from a random 32-bit number to the actual SHA hash 
of the pcm content.

1> Definition of ASTFileSignature is moved to Basic/Module.h so Module and 
ASTSourceDescriptor can use it. Before this patch both are using uint64_t as 
the type of the signature.
2> Add DIAGNOSTIC_OPTIONS_BLOCK and make sure it is the last block of the pcm 
file. This block contains DIAGNOSTIC_OPTIONS and SIGNATURE.
We are using this patch to solve a PCH+Modules issue: PCH are handled by the 
build system and modules are handled by the compiler, when the compiler 
overwrites a pcm file with the same content except the diagnostic differences, 
we can still treat the PCH as valid and continue using it. This requires us not 
hashing the diagnostic differences. Moving the diagnostic differences to the 
end of the pcm makes the task easy:

  In a few records, we save bit positions in the pcm file, if the size of the 
diagnostic options is different, the bit positions can be off.
  We only need to hash the content till the start of the 
DIAGNOSTIC_OPTIONS_BLOCK.

3> When we hash the pcm content, there is no point in saving the file size and 
modification time for the imported module file. The patch will use 0 for both. 
And the validation will not check the size differences or the modification time 
differences.
4> In GlobalModuleIndexBuilder, when the signature is non-zero, we check the 
signature instead of file size/modification time for imported modules.
5> Debug info has a 64-bit slot for dwo id, this patch uses the lower 64 bits 
of the SHA hash.

I want to gather some feedback on the general direction of this. Given that 
this is a big patch, I am open to separate it into smaller patches.

Cheers,
Manman


https://reviews.llvm.org/D27689

Files:
  include/clang/AST/ExternalASTSource.h
  include/clang/Basic/Module.h
  include/clang/Frontend/PCHContainerOperations.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  include/clang/Serialization/Module.h
  lib/Basic/Module.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  lib/Frontend/ASTUnit.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GeneratePCH.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/Module.cpp
  lib/Serialization/ModuleManager.cpp
  test/Modules/diagnostic-options-out-of-date.m
  test/Modules/explicit-build.cpp
  test/Modules/module_file_info.m
  test/Modules/rebuild.m

Index: test/Modules/rebuild.m
===
--- test/Modules/rebuild.m
+++ test/Modules/rebuild.m
@@ -19,11 +19,10 @@
 // RUN: diff %t/Module.size %t/Module.size.saved
 // RUN: cp %t/Module.pcm %t/Module.pcm.saved.2
 
-// But the signature at least is expected to change, so we rebuild DependsOnModule.
-// NOTE: if we change how the signature is created, this test may need updating.
+// The signature is the hash of the PCM content, we will not rebuild rebuild DependsOnModule.
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -fsyntax-only -F %S/Inputs %s
 // RUN: diff %t/Module.pcm %t/Module.pcm.saved.2
-// RUN: not diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
+// RUN: diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
 
 // Rebuild Module, reset its timestamp, and verify its size hasn't changed
 // RUN: rm %t/Module.pcm
@@ -34,10 +33,9 @@
 // RUN: cp %t/Module.pcm %t/Module.pcm.saved.2
 
 // Verify again with Module pre-imported.
-// NOTE: if we change how the signature is created, this test may need updating.
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash -fsyntax-only -F %S/Inputs %s
 // RUN: diff %t/Module.pcm %t/Module.pcm.saved.2
-// RUN: not diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
+// RUN: diff %t/DependsOnModule.pcm %t/DependsOnModule.pcm.saved
 
 #ifdef PREIMPORT
 @import Module;
Index: test/Modules/module_file_info.m
===
--- test/Modules/module_file_info.m
+++ test/Modules/module_file_info.m
@@ -29,11 +29,6 @@
 // CHECK: CPU:
 // CHECK: ABI:
 
-// CHECK: Diagnostic options:
-// CHECK:   IgnoreWarnings: Yes
-// CHECK:   Diagnostic flags:
-// CHECK: -Wunused
-
 // CHECK: Header search options:
 // CHECK:   System root [-isysroot=]: '/'
 // CHECK:   Use builtin include directories [-nobuiltininc]: Yes
@@ -47,3 +42,8 @@
 // CHECK:   Predefined macros:
 // CHECK: -DBLARG
 // CHECK: -DWIBBLE=WOBBLE
+
+// CHECK: Diagnostic options:
+// CHECK:   IgnoreWarnings: Yes
+// CHECK:   Diagnostic flags:
+// CHECK: -Wunused
Index: test/Modules/explicit-build.cpp

[PATCH] D27039: [CodeCompletion] Autocomplete NS_DESIGNATED_INITIALIZER in initializers with arguments

[Manman Ren via Phabricator via cfe-commits]

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

LGTM

Manman


Repository:
  rL LLVM

https://reviews.llvm.org/D27039



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

[PATCH] D27053: [CodeCompletion] Provide Objective-C class property completion results

[Manman Ren via Phabricator via cfe-commits]

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

LGTM .

Manman




Comment at: lib/Sema/SemaCodeComplete.cpp:3763
+if (!M->getSelector().isUnarySelector() ||
+M->getReturnType()->isVoidType() || M->isInstanceMethod())
+  continue;

Can you put a comment here on the conditions?


Repository:
  rL LLVM

https://reviews.llvm.org/D27053



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

[PATCH] D26503: [Parser][ObjC] Improve diagnostics and recovery when C++ keywords are used as identifiers in Objective-C++

[Manman Ren via Phabricator via cfe-commits]

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

LGTM

Manman


Repository:
  rL LLVM

https://reviews.llvm.org/D26503



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