[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc843c921a1a3: [clang] Require strict matches for defines for 
PCH in GCC style directories (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D126676?vs=451434&id=451605#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/PCH/pch-dir.c

Index: clang/test/PCH/pch-dir.c
===
--- clang/test/PCH/pch-dir.c
+++ clang/test/PCH/pch-dir.c
@@ -6,7 +6,7 @@
 // RUN: %clang -x c++-header -std=c++98 %S/pch-dir.h -o %t.h.gch/cpp.gch 
 // RUN: %clang -include %t.h -DFOO=foo -fsyntax-only %s -Xclang -print-stats 2> %t.clog
 // RUN: FileCheck -check-prefix=CHECK-C %s < %t.clog
-// RUN: %clang -include %t.h -DFOO=bar -DBAR=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
+// RUN: %clang -include %t.h -DFOO=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
 // RUN: FileCheck -check-prefix=CHECK-CBAR %s < %t.cbarlog
 // RUN: %clang -x c++ -include %t.h -std=c++98 -fsyntax-only %s -Xclang -print-stats 2> %t.cpplog
 // RUN: FileCheck -check-prefix=CHECK-CPP %s < %t.cpplog
@@ -14,6 +14,11 @@
 // RUN: not %clang -x c++ -std=c++11 -include %t.h -fsyntax-only %s 2> %t.cpp11log
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.cpp11log
 
+// RUN: not %clang -include %t.h -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+// RUN: not %clang -include %t.h -DFOO=foo -DBAR=bar -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+
 // Don't crash if the precompiled header file is missing.
 // RUN: not %clang_cc1 -include-pch %t.h.gch -DFOO=baz -fsyntax-only %s -print-stats 2> %t.missinglog
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -624,20 +624,28 @@
   }
 }
 
+enum OptionValidation {
+  OptionValidateNone,
+  OptionValidateContradictions,
+  OptionValidateStrictMatches,
+};
+
 /// Check the preprocessor options deserialized from the control block
 /// against the preprocessor options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-/// \param Validate If true, validate preprocessor options. If false, allow
-///macros defined by \p ExistingPPOpts to override those defined by
-///\p PPOpts in SuggestedPredefines.
-static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
- const PreprocessorOptions &ExistingPPOpts,
- DiagnosticsEngine *Diags,
- FileManager &FileMgr,
- std::string &SuggestedPredefines,
- const LangOptions &LangOpts,
- bool Validate = true) {
+/// \param Validation If set to OptionValidateNone, ignore differences in
+///preprocessor options. If set to OptionValidateContradictions,
+///require that options passed both in the AST file and on the command
+///line (-D or -U) match, but tolerate options missing in one or the
+///other. If set to OptionValidateContradictions, require that there
+///are no differences in the options between the two.
+static bool checkPreprocessorOptions(
+const PreprocessorOptions &PPOpts,
+const PreprocessorOptions &ExistingPPOpts, DiagnosticsEngine *Diags,
+FileManager &FileMgr, std::string &SuggestedPredefines,
+const LangOptions &LangOpts,
+OptionValidation Validation = OptionValidateContradictions) {
   // Check macro definitions.
   MacroDefinitionsMap ASTFileMacros;
   collectMacroDefinitions(PPOpts, ASTFileMacros);
@@ -653,7 +661,15 @@
 // Check whether we know anything about this macro name or not.
 llvm::StringMap>::iterator Known =
 ASTFileMacros.find(MacroName);
-if (!Validate || Known == ASTFileMacros.end()) {
+if (Validation == OptionValidateNone || Known == ASTFileMacros.end()) {
+  if (Validation == OptionValidateStrictMatches) {
+// If strict matches are requested, don't tolerate any extra defines on
+// the command line that are missing in the AST file.
+if (Diags) {
+  Diags->Report(diag::err_pch_macro_def_undef) << MacroName << true;
+}
+return true;
+  }
   // FIXME: Check whether this identifier was referenced anywhere in the
   // AST

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 accepted this revision.
jansvoboda11 added a comment.

LGTM, no issues on our side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 451434.
mstorsjo added a comment.

Rebased, to fix (trivial) conflicts in the release notes file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/PCH/pch-dir.c

Index: clang/test/PCH/pch-dir.c
===
--- clang/test/PCH/pch-dir.c
+++ clang/test/PCH/pch-dir.c
@@ -6,7 +6,7 @@
 // RUN: %clang -x c++-header -std=c++98 %S/pch-dir.h -o %t.h.gch/cpp.gch 
 // RUN: %clang -include %t.h -DFOO=foo -fsyntax-only %s -Xclang -print-stats 2> %t.clog
 // RUN: FileCheck -check-prefix=CHECK-C %s < %t.clog
-// RUN: %clang -include %t.h -DFOO=bar -DBAR=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
+// RUN: %clang -include %t.h -DFOO=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
 // RUN: FileCheck -check-prefix=CHECK-CBAR %s < %t.cbarlog
 // RUN: %clang -x c++ -include %t.h -std=c++98 -fsyntax-only %s -Xclang -print-stats 2> %t.cpplog
 // RUN: FileCheck -check-prefix=CHECK-CPP %s < %t.cpplog
@@ -14,6 +14,11 @@
 // RUN: not %clang -x c++ -std=c++11 -include %t.h -fsyntax-only %s 2> %t.cpp11log
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.cpp11log
 
+// RUN: not %clang -include %t.h -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+// RUN: not %clang -include %t.h -DFOO=foo -DBAR=bar -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+
 // Don't crash if the precompiled header file is missing.
 // RUN: not %clang_cc1 -include-pch %t.h.gch -DFOO=baz -fsyntax-only %s -print-stats 2> %t.missinglog
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -624,20 +624,28 @@
   }
 }
 
+enum OptionValidation {
+  OptionValidateNone,
+  OptionValidateContradictions,
+  OptionValidateStrictMatches,
+};
+
 /// Check the preprocessor options deserialized from the control block
 /// against the preprocessor options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-/// \param Validate If true, validate preprocessor options. If false, allow
-///macros defined by \p ExistingPPOpts to override those defined by
-///\p PPOpts in SuggestedPredefines.
-static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
- const PreprocessorOptions &ExistingPPOpts,
- DiagnosticsEngine *Diags,
- FileManager &FileMgr,
- std::string &SuggestedPredefines,
- const LangOptions &LangOpts,
- bool Validate = true) {
+/// \param Validation If set to OptionValidateNone, ignore differences in
+///preprocessor options. If set to OptionValidateContradictions,
+///require that options passed both in the AST file and on the command
+///line (-D or -U) match, but tolerate options missing in one or the
+///other. If set to OptionValidateContradictions, require that there
+///are no differences in the options between the two.
+static bool checkPreprocessorOptions(
+const PreprocessorOptions &PPOpts,
+const PreprocessorOptions &ExistingPPOpts, DiagnosticsEngine *Diags,
+FileManager &FileMgr, std::string &SuggestedPredefines,
+const LangOptions &LangOpts,
+OptionValidation Validation = OptionValidateContradictions) {
   // Check macro definitions.
   MacroDefinitionsMap ASTFileMacros;
   collectMacroDefinitions(PPOpts, ASTFileMacros);
@@ -653,7 +661,15 @@
 // Check whether we know anything about this macro name or not.
 llvm::StringMap>::iterator Known =
 ASTFileMacros.find(MacroName);
-if (!Validate || Known == ASTFileMacros.end()) {
+if (Validation == OptionValidateNone || Known == ASTFileMacros.end()) {
+  if (Validation == OptionValidateStrictMatches) {
+// If strict matches are requested, don't tolerate any extra defines on
+// the command line that are missing in the AST file.
+if (Diags) {
+  Diags->Report(diag::err_pch_macro_def_undef) << MacroName << true;
+}
+return true;
+  }
   // FIXME: Check whether this identifier was referenced anywhere in the
   // AST file. If so, we should reject the AST file. Unfortunately, this
   // information isn't in the control block. What shall we do about it?
@@ -684,8 +70

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D126676#3703024 , @jansvoboda11 
wrote:

> Also, I'd like to test this patch internally to see if it affects anything on 
> our end. I should be able to do so next week.

Ping @jansvoboda11 - any update on testing this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Reid Kleckner via Phabricator via cfe-commits]

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

I don't have any concerns, this seems reasonable to me.

Since Jan offered to test it, please wait to hear back before landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D126676#3703024 , @jansvoboda11 
wrote:

> Do you think it would make sense to introduce a command-line flag that would 
> control this behavior?

Hmm - I don’t think it’d be necessary. If using the gcc style PCH directory, I 
would also kinda expect it to be used in a setting where it’s occasionally 
built with GCC (which requires strict matches) or with a build system that 
enforces such a setup anyway.

> Also, I'd like to test this patch internally to see if it affects anything on 
> our end. I should be able to do so next week.

Thanks, that’d be very much appreciated!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Do you think it would make sense to introduce a command-line flag that would 
control this behavior?

Also, I'd like to test this patch internally to see if it affects anything on 
our end. I should be able to do so next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

@rnk Can you have another look here? There doesn’t really seem to be anyone 
else who’s able to give it a review at the moment, but @dexonsmith seemed ok 
with the idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D126676#3687491 , @mstorsjo wrote:

> In D126676#3687227 , @dexonsmith 
> wrote:
>
>> There are cases where it’s safe to have mismatched defines (e.g., if a 
>> define can be proven during a cheap dependency scan not to matter for a 
>> given PCH, it’s better to canonicalize away the define and share the 
>> artifact more broadly) but if I understand correctly this patch won’t 
>> preclude compiler smarts like that.
>
> Yup. Any implementation of such logic hasn’t materialized during the 10 years 
> since todos hinting that we should implement that, but it doesn’t seem to be 
> a big practical issue either, outside of false positive matches with gcc PCH 
> directories - so I guess it’s not a high priority in practice.

Note, FWIW, that clang-scan-deps is being used to canonicalize command-lines as 
of the last year; e.g., there are patches up (maybe committed?) to remove 
unused search paths for implicitly-discovered explicit modules. Finding unused 
defines is a bit harder and less urgent (search path explosion had to be fixed 
for performance parity with on-demand implicit modules, which have an unsound 
optimization of ignoring search path differences), but there’s now a framework 
where it can be reasonably implemented if/when it’s identified as a major 
bottleneck in builds.

(Fallout from unsoundness in on-demand implicit modules is a big reason 
progress has been slow in this area.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 448708.
mstorsjo added a comment.

Update the parameters to the internal `checkPreprocessorOptions` function to 
use a three-state enum instead of having two separate `bool` flags.

The 15.x release already has been branched, but I'd like to have this 
backported and included if possible (the regression risk should be quite small 
as long as it's restricted to GCC PCH directories). To handle that, I'd omit 
the release notes when pushing to the main branch (as the release notes 
document differs between the branches) and manually add that to the release 
branch, if this is accepted for backport.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/PCH/pch-dir.c

Index: clang/test/PCH/pch-dir.c
===
--- clang/test/PCH/pch-dir.c
+++ clang/test/PCH/pch-dir.c
@@ -6,7 +6,7 @@
 // RUN: %clang -x c++-header -std=c++98 %S/pch-dir.h -o %t.h.gch/cpp.gch 
 // RUN: %clang -include %t.h -DFOO=foo -fsyntax-only %s -Xclang -print-stats 2> %t.clog
 // RUN: FileCheck -check-prefix=CHECK-C %s < %t.clog
-// RUN: %clang -include %t.h -DFOO=bar -DBAR=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
+// RUN: %clang -include %t.h -DFOO=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
 // RUN: FileCheck -check-prefix=CHECK-CBAR %s < %t.cbarlog
 // RUN: %clang -x c++ -include %t.h -std=c++98 -fsyntax-only %s -Xclang -print-stats 2> %t.cpplog
 // RUN: FileCheck -check-prefix=CHECK-CPP %s < %t.cpplog
@@ -14,6 +14,11 @@
 // RUN: not %clang -x c++ -std=c++11 -include %t.h -fsyntax-only %s 2> %t.cpp11log
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.cpp11log
 
+// RUN: not %clang -include %t.h -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+// RUN: not %clang -include %t.h -DFOO=foo -DBAR=bar -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+
 // Don't crash if the precompiled header file is missing.
 // RUN: not %clang_cc1 -include-pch %t.h.gch -DFOO=baz -fsyntax-only %s -print-stats 2> %t.missinglog
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -624,20 +624,28 @@
   }
 }
 
+enum OptionValidation {
+  OptionValidateNone,
+  OptionValidateContradictions,
+  OptionValidateStrictMatches,
+};
+
 /// Check the preprocessor options deserialized from the control block
 /// against the preprocessor options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-/// \param Validate If true, validate preprocessor options. If false, allow
-///macros defined by \p ExistingPPOpts to override those defined by
-///\p PPOpts in SuggestedPredefines.
-static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
- const PreprocessorOptions &ExistingPPOpts,
- DiagnosticsEngine *Diags,
- FileManager &FileMgr,
- std::string &SuggestedPredefines,
- const LangOptions &LangOpts,
- bool Validate = true) {
+/// \param Validation If set to OptionValidateNone, ignore differences in
+///preprocessor options. If set to OptionValidateContradictions,
+///require that options passed both in the AST file and on the command
+///line (-D or -U) match, but tolerate options missing in one or the
+///other. If set to OptionValidateContradictions, require that there
+///are no differences in the options between the two.
+static bool checkPreprocessorOptions(
+const PreprocessorOptions &PPOpts,
+const PreprocessorOptions &ExistingPPOpts, DiagnosticsEngine *Diags,
+FileManager &FileMgr, std::string &SuggestedPredefines,
+const LangOptions &LangOpts,
+OptionValidation Validation = OptionValidateContradictions) {
   // Check macro definitions.
   MacroDefinitionsMap ASTFileMacros;
   collectMacroDefinitions(PPOpts, ASTFileMacros);
@@ -653,7 +661,15 @@
 // Check whether we know anything about this macro name or not.
 llvm::StringMap>::iterator Known =
 ASTFileMacros.find(MacroName);
-if (!Validate || Known == ASTFileMacros.end()) {
+if (Validation == OptionValidateNone || Known == ASTFileMacros.end()) {
+  if (Validation == OptionValidateStrictMatches) {
+// If strict match

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D126676#3687227 , @dexonsmith 
wrote:

> I haven’t reviewed the details of the patch and won’t have time to do so (at 
> least for a while), but the description of the intended (more narrow) scope 
> SGTM. With the scope limited to GCC directories this should be fine.
>
> There are cases where it’s safe to have mismatched defines (e.g., if a define 
> can be proven during a cheap dependency scan not to matter for a given PCH, 
> it’s better to canonicalize away the define and share the artifact more 
> broadly) but if I understand correctly this patch won’t preclude compiler 
> smarts like that.

Yup. Any implementation of such logic hasn’t materialized during the 10 years 
since todos hinting that we should implement that, but it doesn’t seem to be a 
big practical issue either, outside of false positive matches with gcc PCH 
directories - so I guess it’s not a high priority in practice.

> If I’m right, then I think this can go in once @rnk is happy. (If I’m wrong, 
> maybe better to wait for one of the people I pinged.)

Yeah, I’d be happy to add a validation level enum to make things a bit clearer 
- I’ll try to revise the patch soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I haven’t reviewed the details of the patch and won’t have time to do so (at 
least for a while), but the description of the intended (more narrow) scope 
SGTM. With the scope limited to GCC directories this should be fine.

There are cases where it’s safe to have mismatched defines (e.g., if a define 
can be proven during a cheap dependency scan not to matter for a given PCH, 
it’s better to canonicalize away the define and share the artifact more 
broadly) but if I understand correctly this patch won’t preclude compiler 
smarts like that.

If I’m right, then I think this can go in once @rnk is happy. (If I’m wrong, 
maybe better to wait for one of the people I pinged.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Ping @vsapsai @Bigcheese @akyrtzi @benlangmuir @arphaman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: arphaman, dexonsmith.
rnk added a comment.

Seeking additional opinions on the PCH change: @dexonsmith @arphaman

The use case makes sense to me. It seems reasonable to me that the compiler 
shouldn't use the same PCH file with mismatched defines on the command line.




Comment at: clang/lib/Serialization/ASTReader.cpp:640-641
  const LangOptions &LangOpts,
- bool Validate = true) {
+ bool Validate = true,
+ bool ValidateStrict = false) {
   // Check macro definitions.

These feel like they should be a "validation level" enum. Multiple optional 
boolean parameters in sequence is usually a hazard for future refactorings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 443461.
mstorsjo added a comment.

Reduce the scope and impact of the change: When telling Clang to include a 
specifically named PCH file, keep tolerating the same set of mismatches as 
before. (Clang has been tolerating such differences for over 10 years, and 
there are a number of tests specifically for such fuzzy matches.) When using a 
GCC style directory with multiple alternative PCH files, require an exact 
match, to avoid the case where multiple ones are acceptable and Clang 
erroneously picks the first seemingly acceptable one.

In this form, the patch no longer breaks other testcases than the one that is 
meant to be fixed/changed.

This still has a small risk of breaking someone's setup, but as this makes 
Clang match GCC's behaviour for the compatible option, the risk is probably 
quite small.

(A more comprehensive solution to avoid breaking that case, would be to iterate 
over the PCH directory, first requiring a strict match, and if none is found, 
iterate over the directory again, tolerating inexact matches like before. But I 
don't think that's necessary.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/PCH/pch-dir.c

Index: clang/test/PCH/pch-dir.c
===
--- clang/test/PCH/pch-dir.c
+++ clang/test/PCH/pch-dir.c
@@ -6,7 +6,7 @@
 // RUN: %clang -x c++-header -std=c++98 %S/pch-dir.h -o %t.h.gch/cpp.gch 
 // RUN: %clang -include %t.h -DFOO=foo -fsyntax-only %s -Xclang -print-stats 2> %t.clog
 // RUN: FileCheck -check-prefix=CHECK-C %s < %t.clog
-// RUN: %clang -include %t.h -DFOO=bar -DBAR=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
+// RUN: %clang -include %t.h -DFOO=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
 // RUN: FileCheck -check-prefix=CHECK-CBAR %s < %t.cbarlog
 // RUN: %clang -x c++ -include %t.h -std=c++98 -fsyntax-only %s -Xclang -print-stats 2> %t.cpplog
 // RUN: FileCheck -check-prefix=CHECK-CPP %s < %t.cpplog
@@ -14,6 +14,11 @@
 // RUN: not %clang -x c++ -std=c++11 -include %t.h -fsyntax-only %s 2> %t.cpp11log
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.cpp11log
 
+// RUN: not %clang -include %t.h -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+// RUN: not %clang -include %t.h -DFOO=foo -DBAR=bar -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+
 // Don't crash if the precompiled header file is missing.
 // RUN: not %clang_cc1 -include-pch %t.h.gch -DFOO=baz -fsyntax-only %s -print-stats 2> %t.missinglog
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -637,7 +637,8 @@
  FileManager &FileMgr,
  std::string &SuggestedPredefines,
  const LangOptions &LangOpts,
- bool Validate = true) {
+ bool Validate = true,
+ bool ValidateStrict = false) {
   // Check macro definitions.
   MacroDefinitionsMap ASTFileMacros;
   collectMacroDefinitions(PPOpts, ASTFileMacros);
@@ -654,6 +655,14 @@
 llvm::StringMap>::iterator Known =
 ASTFileMacros.find(MacroName);
 if (!Validate || Known == ASTFileMacros.end()) {
+  if (ValidateStrict) {
+// If strict matches are requested, don't tolerate any extra defines on
+// the command line that are missing in the AST file.
+if (Diags) {
+  Diags->Report(diag::err_pch_macro_def_undef) << MacroName << true;
+}
+return true;
+  }
   // FIXME: Check whether this identifier was referenced anywhere in the
   // AST file. If so, we should reject the AST file. Unfortunately, this
   // information isn't in the control block. What shall we do about it?
@@ -684,8 +693,10 @@
 
 // If the macro was #undef'd in both, or if the macro bodies are identical,
 // it's fine.
-if (Existing.second || Existing.first == Known->second.first)
+if (Existing.second || Existing.first == Known->second.first) {
+  ASTFileMacros.erase(Known);
   continue;
+}
 
 // The macro bodies differ; complain.
 if (Diags) {
@@ -694,6 +705,16 @@
 }
 return true;
   }
+  if (ValidateStrict) {
+// If strict matches are requested, don't tolerate any extra defines in
+// the AST file that are missing on the command line.
+for

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D126676#3587243 , @rnk wrote:

> I don't have a great answer here, but yes, dllexport macro norms sort of run 
> directly counter to the normal ways that people use PCH. It seems like 
> projects will need to have a library module / PCH file for building a DLL and 
> then a PCH / module for consuming the DLL.

This isn’t about sharing a PCH between building and consuming a DLL - that’d be 
highly unusual use of PCHs indeed.

It’s about building a library as both DLL and static library at the same time. 
For that, you’d have a PCH directory containing two versions of the same PCH, 
one built with `-DDLLEXPORT` and one without. When building the object files 
for the two copies of the library (for the static or DLL version), the compiler 
gets called with or without `-DDLLEXPORT`, and is pointed at the directory 
containing the two copies of the PCH.

GCC successfully picks the right one out of the directory, while Clang just 
picks the first one it tries. Clang also does try to pick the right one out of 
a directory with multiple copies (picking the right one, if you have one built 
with `-DFOO=1` and one with `-DFOO=2`), but Clang considers that if you have a 
define entirely missing (PCH built with `-DDLLEXPORT` but the same define 
absent when including the header, or vice versa) that they’re probably 
compatible, and pick the first one out of the directory.

I can somewhat understand the rationale - if not using a PCH directory, but 
only one single file, Clang does allow you to include that while defining 
`-DUNRELATED`. (Clang’s implementation does have a 10 year old todo about 
checking that the differing define indeed was unrelated and not used by the PCH 
in any way.) GCC doesn’t allow that, but requires there to be an exact match 
between all `-D` arguments between when the PCH was built and consumed.

So with GCC’s stricter logic here, I think it would make sense to make Clang 
equally strict, but I wonder if that’d break Clang-only usecases that have 
ended up relying on Clang’s more tolerant behaviour here. (There even are tests 
testing specifically the fuzzy allowing of differences.) Maybe the safest would 
be to only make it strict in the case when picking the right one out of a 
directory with alternatives?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I don't have a great answer here, but yes, dllexport macro norms sort of run 
directly counter to the normal ways that people use PCH. It seems like projects 
will need to have a library module / PCH file for building a DLL and then a PCH 
/ module for consuming the DLL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126676

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

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: aaron.ballman, rnk.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

Some amount of differences between defines when creating and using
a PCH were intentionally allowed in
c379c072405f39bca1d3552408fc0427328e8b6d (and later extended in
b63687519610a73dd565be1fec28332211b4df5b).

When using a PCH (or when picking a PCH out of a directory containing
multiple candidates) Clang used to accept the header if there were
defines on the command line when creating the PCH that are missing
when using the PCH, or vice versa, defines only set when using the
PCH.

The only cases where Clang explicitly rejected the use of a PCH
is if there was an explicit conflict between the options, e.g.
-DFOO=1 vs -DFOO=2, or -DFOO vs -UFOO.

The latter commit added a FIXME that we really should check whether
mismatched defines actually were used somewhere in the PCH, so that
the define would affect the outcome. This FIXME has stood unaddressed
since 2012.

This differs from GCC, which rejects PCH files if the defines differ
at all.

This is also problematic when using a directory containing multiple
candidate PCH files, one built with -DFOO and one without. When
attempting to include a PCH and iterating over the candidates in the
directory, Clang will essentially pick the first one out of the two,
even if there existed a better, exact match in the directory.

This fixes https://github.com/lhmouse/mcfgthread/issues/63.

This is likely to be controversial. In tree, 84 tests break by
making this change. This patch only fixes up the core tests for
this feature (to bring it up for discussion); out of the rest, some
probably are easy to fix, while others seem to be designed to specifically
test cases with such discrepancies.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126676

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/test/PCH/fuzzy-pch.c
  clang/test/PCH/pch-dir.c

Index: clang/test/PCH/pch-dir.c
===
--- clang/test/PCH/pch-dir.c
+++ clang/test/PCH/pch-dir.c
@@ -6,7 +6,7 @@
 // RUN: %clang -x c++-header -std=c++98 %S/pch-dir.h -o %t.h.gch/cpp.gch 
 // RUN: %clang -include %t.h -DFOO=foo -fsyntax-only %s -Xclang -print-stats 2> %t.clog
 // RUN: FileCheck -check-prefix=CHECK-C %s < %t.clog
-// RUN: %clang -include %t.h -DFOO=bar -DBAR=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
+// RUN: %clang -include %t.h -DFOO=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
 // RUN: FileCheck -check-prefix=CHECK-CBAR %s < %t.cbarlog
 // RUN: %clang -x c++ -include %t.h -std=c++98 -fsyntax-only %s -Xclang -print-stats 2> %t.cpplog
 // RUN: FileCheck -check-prefix=CHECK-CPP %s < %t.cpplog
@@ -14,6 +14,11 @@
 // RUN: not %clang -x c++ -std=c++11 -include %t.h -fsyntax-only %s 2> %t.cpp11log
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.cpp11log
 
+// RUN: not %clang -include %t.h -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+// RUN: not %clang -include %t.h -DFOO=foo -DBAR=bar -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+
 // Don't crash if the precompiled header file is missing.
 // RUN: not %clang_cc1 -include-pch %t.h.gch -DFOO=baz -fsyntax-only %s -print-stats 2> %t.missinglog
 // RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog
Index: clang/test/PCH/fuzzy-pch.c
===
--- clang/test/PCH/fuzzy-pch.c
+++ clang/test/PCH/fuzzy-pch.c
@@ -1,7 +1,9 @@
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -DFOO -o %t %S/variables.h
-// RUN: %clang_cc1 -DBAR=int -include-pch %t -fsyntax-only -pedantic %s
-// RUN: %clang_cc1 -DFOO -DBAR=int -include-pch %t %s
+// RUN: not %clang_cc1 -DBAR=int -include-pch %t -fsyntax-only -pedantic %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-BAR %s < %t.err
+// RUN: not %clang_cc1 -DFOO -DBAR=int -include-pch %t %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-BAR %s < %t.err
 // RUN: not %clang_cc1 -DFOO=blah -DBAR=int -include-pch %t %s 2> %t.err
 // RUN: FileCheck -check-prefix=CHECK-FOO %s < %t.err
 // RUN: not %clang_cc1 -UFOO -include-pch %t %s 2> %t.err
@@ -26,6 +28,7 @@
 
 // CHECK-FOO: definition of macro 'FOO' differs between the precompiled header ('1') and the command line ('blah')
 // CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd on the command line
+// CHECK-BAR: macro 'BAR' was undef'd in the precompiled header but defined on the command line
 
 // CHECK-UNDEF: command line contains '-undef' but precompiled header was not built with it
 
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -653,11 +653,7