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 @@
// Check whether we know anything about this macro name or not.
llvm::StringMap<std::pair<StringRef, bool /*IsUndef*/>>::iterator Known =
ASTFileMacros.find(MacroName);
- if (!Validate || Known == ASTFileMacros.end()) {
- // 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?
-
+ if (!Validate) {
if (Existing.second) {
SuggestedPredefines += "#undef ";
SuggestedPredefines += MacroName.str();
@@ -671,6 +667,12 @@
}
continue;
}
+ if (Known == ASTFileMacros.end()) {
+ if (Diags) {
+ Diags->Report(diag::err_pch_macro_def_undef) << MacroName << true;
+ }
+ return true;
+ }
// If the macro was defined in one but undef'd in the other, we have a
// conflict.
@@ -684,8 +686,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 +698,12 @@
}
return true;
}
+ for (const auto &MacroName : ASTFileMacros.keys()) {
+ if (Diags) {
+ Diags->Report(diag::err_pch_macro_def_undef) << MacroName << false;
+ }
+ return true;
+ }
// Check whether we're using predefines.
if (PPOpts.UsePredefines != ExistingPPOpts.UsePredefines && Validate) {
@@ -5171,7 +5181,7 @@
bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts,
bool Complain,
std::string &SuggestedPredefines) override {
- return checkPreprocessorOptions(ExistingPPOpts, PPOpts, nullptr, FileMgr,
+ return checkPreprocessorOptions(PPOpts, ExistingPPOpts, nullptr, FileMgr,
SuggestedPredefines, ExistingLangOpts);
}
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
