Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL271310: PCH + module: make sure we write out macros 
associated with builtin identifiers. (authored by mren).

Changed prior to commit:
  http://reviews.llvm.org/D20383?vs=57673=59103#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20383

Files:
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/Modules/Inputs/MacroFabs1.h
  cfe/trunk/test/Modules/Inputs/module.map
  cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
  cfe/trunk/test/Modules/pch-module-macro.m

Index: cfe/trunk/test/Modules/Inputs/module.map
===
--- cfe/trunk/test/Modules/Inputs/module.map
+++ cfe/trunk/test/Modules/Inputs/module.map
@@ -414,3 +414,7 @@
 }
 
 module Empty {}
+
+module MacroFabs1 {
+  header "MacroFabs1.h"
+}
Index: cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
===
--- cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
+++ cfe/trunk/test/Modules/Inputs/pch-import-module-with-macro.pch
@@ -0,0 +1,3 @@
+
+@import MacroFabs1;
+
Index: cfe/trunk/test/Modules/Inputs/MacroFabs1.h
===
--- cfe/trunk/test/Modules/Inputs/MacroFabs1.h
+++ cfe/trunk/test/Modules/Inputs/MacroFabs1.h
@@ -0,0 +1,6 @@
+
+#undef fabs
+#define fabs(x) (x)
+
+#undef my_fabs
+#define my_fabs(x) (x)
Index: cfe/trunk/test/Modules/pch-module-macro.m
===
--- cfe/trunk/test/Modules/pch-module-macro.m
+++ cfe/trunk/test/Modules/pch-module-macro.m
@@ -0,0 +1,9 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -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-with-macro.pch
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify
+// expected-no-diagnostics
+
+int test(int x) {
+  return my_fabs(x) + fabs(x);
+}
+
Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -2187,30 +2187,29 @@
 
 // Write out any exported module macros.
 bool EmittedModuleMacros = false;
-if (IsModule) {
-  auto Leafs = PP.getLeafModuleMacros(Name);
-  SmallVector Worklist(Leafs.begin(), Leafs.end());
-  llvm::DenseMap Visits;
-  while (!Worklist.empty()) {
-auto *Macro = Worklist.pop_back_val();
-
-// Emit a record indicating this submodule exports this macro.
-ModuleMacroRecord.push_back(
-getSubmoduleID(Macro->getOwningModule()));
-ModuleMacroRecord.push_back(getMacroRef(Macro->getMacroInfo(), Name));
-for (auto *M : Macro->overrides())
-  ModuleMacroRecord.push_back(getSubmoduleID(M->getOwningModule()));
-
-Stream.EmitRecord(PP_MODULE_MACRO, ModuleMacroRecord);
-ModuleMacroRecord.clear();
-
-// Enqueue overridden macros once we've visited all their ancestors.
-for (auto *M : Macro->overrides())
-  if (++Visits[M] == M->getNumOverridingMacros())
-Worklist.push_back(M);
+// We write out exported module macros for PCH as well.
+auto Leafs = PP.getLeafModuleMacros(Name);
+SmallVector Worklist(Leafs.begin(), Leafs.end());
+llvm::DenseMap Visits;
+while (!Worklist.empty()) {
+  auto *Macro = Worklist.pop_back_val();
+
+  // Emit a record indicating this submodule exports this macro.
+  ModuleMacroRecord.push_back(
+  getSubmoduleID(Macro->getOwningModule()));
+  ModuleMacroRecord.push_back(getMacroRef(Macro->getMacroInfo(), Name));
+  for (auto *M : Macro->overrides())
+ModuleMacroRecord.push_back(getSubmoduleID(M->getOwningModule()));
+
+  Stream.EmitRecord(PP_MODULE_MACRO, ModuleMacroRecord);
+  ModuleMacroRecord.clear();
+
+  // Enqueue overridden macros once we've visited all their ancestors.
+  for (auto *M : Macro->overrides())
+if (++Visits[M] == M->getNumOverridingMacros())
+  Worklist.push_back(M);
 
-EmittedModuleMacros = true;
-  }
+  EmittedModuleMacros = true;
 }
 
 if (Record.empty() && !EmittedModuleMacros)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Richard Smith via cfe-commits 
rsmith accepted this revision.
rsmith added a comment.

Looks good to me, too.


http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Doug Gregor via cfe-commits 
doug.gregor accepted this revision.
doug.gregor added a comment.
This revision is now accepted and ready to land.

Yes, that's a LGTM, sorry for being unclear.


http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-31 Thread Manman Ren via cfe-commits 
manmanren added a comment.

In http://reviews.llvm.org/D20383#443613, @doug.gregor wrote:

> Yeah, this looks like the right approach. PCH follows the same rules as 
> modules when it comes to newer information shadowing imported information.


Hi Doug,

Thanks for reviewing the patch! Can I take that as a "LGTM"? I will clean up 
the source change.

Manman


http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-30 Thread Doug Gregor via cfe-commits 
doug.gregor added a comment.

Yeah, this looks like the right approach. PCH follows the same rules as modules 
when it comes to newer information shadowing imported information.


http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-25 Thread Manman Ren via cfe-commits 
manmanren added a comment.

Doug and Richard,

Can you take a look at this when you have time?

Thanks,
Manman


http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-23 Thread Ben Langmuir via cfe-commits 
benlangmuir added a subscriber: doug.gregor.
benlangmuir added a comment.

I'd like to see Doug and/or Richard review this.  It seems reasonable to me to 
first blush, but I assume there was a good reason we weren't doing this 
already...


http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-19 Thread Manman Ren via cfe-commits 
manmanren added a comment.

Thanks Bruno



Comment at: lib/Serialization/ASTWriter.cpp:2191
@@ -2191,1 +2190,3 @@
+// We write out exported module macros for PCH as well.
+if (true) {
   auto Leafs = PP.getLeafModuleMacros(Name);

bruno wrote:
> Is this intended?
I was trying to make sure people get that it is the only thing this patch 
changes :) I will remove it if we are okay with this approach.



http://reviews.llvm.org/D20383



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

Re: [PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-18 Thread Bruno Cardoso Lopes via cfe-commits 
bruno added a subscriber: bruno.
bruno added a comment.

Hi Manman,



Comment at: lib/Serialization/ASTWriter.cpp:2191
@@ -2191,1 +2190,3 @@
+// We write out exported module macros for PCH as well.
+if (true) {
   auto Leafs = PP.getLeafModuleMacros(Name);

Is this intended?


http://reviews.llvm.org/D20383



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

[PATCH] D20383: PCH + Module: make sure we write out macros associated with builtin identifiers

2016-05-18 Thread Manman Ren via cfe-commits 
manmanren created this revision.
manmanren added reviewers: benlangmuir, rsmith.
manmanren added a subscriber: cfe-commits.

When we import a module that defines a builtin identifier from prefix header 
and precompile the prefix header, the macro information related to the 
identifier is lost.
If we don't precompile the prefix header, the source file can still see the 
macro information.
The reason is that we write out the identifier in the pch but not the macro 
information since the macro is not defined locally.

This seems to be related to r251565: If we read a builtin identifier from a 
module that wasn't "interesting" to that module, we will still write it out to 
a PCH that imports that module.

The fix proposed here is to write exported module macros for PCH as well. I am 
open to other suggestions.

http://reviews.llvm.org/D20383

Files:
  lib/Serialization/ASTWriter.cpp
  test/Modules/Inputs/MacroFabs1.h
  test/Modules/Inputs/module.map
  test/Modules/Inputs/pch-import-module-with-macro.pch
  test/Modules/pch-module-macro.m

Index: test/Modules/pch-module-macro.m
===
--- test/Modules/pch-module-macro.m
+++ test/Modules/pch-module-macro.m
@@ -0,0 +1,9 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -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-with-macro.pch
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps 
-fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify
+// expected-no-diagnostics
+
+int test(int x) {
+  return my_fabs(x) + fabs(x);
+}
+
Index: test/Modules/Inputs/pch-import-module-with-macro.pch
===
--- test/Modules/Inputs/pch-import-module-with-macro.pch
+++ test/Modules/Inputs/pch-import-module-with-macro.pch
@@ -0,0 +1,3 @@
+
+@import MacroFabs1;
+
Index: test/Modules/Inputs/module.map
===
--- test/Modules/Inputs/module.map
+++ test/Modules/Inputs/module.map
@@ -414,3 +414,7 @@
 }
 
 module Empty {}
+
+module MacroFabs1 {
+  header "MacroFabs1.h"
+}
Index: test/Modules/Inputs/MacroFabs1.h
===
--- test/Modules/Inputs/MacroFabs1.h
+++ test/Modules/Inputs/MacroFabs1.h
@@ -0,0 +1,6 @@
+
+#undef fabs
+#define fabs(x) (x)
+
+#undef my_fabs
+#define my_fabs(x) (x)
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -2187,7 +2187,8 @@
 
 // Write out any exported module macros.
 bool EmittedModuleMacros = false;
-if (IsModule) {
+// We write out exported module macros for PCH as well.
+if (true) {
   auto Leafs = PP.getLeafModuleMacros(Name);
   SmallVector Worklist(Leafs.begin(), Leafs.end());
   llvm::DenseMap Visits;


Index: test/Modules/pch-module-macro.m
===
--- test/Modules/pch-module-macro.m
+++ test/Modules/pch-module-macro.m
@@ -0,0 +1,9 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -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-with-macro.pch
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify
+// expected-no-diagnostics
+
+int test(int x) {
+  return my_fabs(x) + fabs(x);
+}
+
Index: test/Modules/Inputs/pch-import-module-with-macro.pch
===
--- test/Modules/Inputs/pch-import-module-with-macro.pch
+++ test/Modules/Inputs/pch-import-module-with-macro.pch
@@ -0,0 +1,3 @@
+
+@import MacroFabs1;
+
Index: test/Modules/Inputs/module.map
===
--- test/Modules/Inputs/module.map
+++ test/Modules/Inputs/module.map
@@ -414,3 +414,7 @@
 }
 
 module Empty {}
+
+module MacroFabs1 {
+  header "MacroFabs1.h"
+}
Index: test/Modules/Inputs/MacroFabs1.h
===
--- test/Modules/Inputs/MacroFabs1.h
+++ test/Modules/Inputs/MacroFabs1.h
@@ -0,0 +1,6 @@
+
+#undef fabs
+#define fabs(x) (x)
+
+#undef my_fabs
+#define my_fabs(x) (x)
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -2187,7 +2187,8 @@
 
 // Write out any exported module macros.
 bool EmittedModuleMacros = false;
-if (IsModule) {
+// We write out exported module macros for PCH as well.
+if (true) {
   auto Leafs = PP.getLeafModuleMacros(Name);
   SmallVector Worklist(Leafs.begin(),