From 6c2b5a8ff0b4ea3a9c5dcaf1ffe80c949845f500 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 9 Feb 2018 01:15:13 +0000 Subject: [PATCH] [modules] Fix incorrect diagnostic mapping computation when a module changes diagnostic settings using _Pragma within a macro. The AST writer had previously been assuming that all diagnostic state transitions would occur within a FileID corresponding to a file. When a diagnostic state change occured within a macro, it was unable to form a location for that state change and would instead corrupt the diagnostic state of the "root" node (and thus that of the main compilation). Also introduce a "#pragma clang __debug diag_mapping" debugging utility that I added to track this issue down. llvm-svn: 324695 --- clang/include/clang/Basic/Diagnostic.h | 9 ++++ clang/include/clang/Basic/SourceManager.h | 12 +++++ clang/lib/Basic/Diagnostic.cpp | 90 +++++++++++++++++++++++++++++++ clang/lib/Lex/Pragma.cpp | 14 +++++ clang/lib/Serialization/ASTReader.cpp | 3 ++ clang/lib/Serialization/ASTWriter.cpp | 7 ++- clang/test/Modules/Inputs/diag_flags.h | 3 ++ clang/test/Modules/diag-flags.cpp | 34 ++++++++---- 8 files changed, 159 insertions(+), 13 deletions(-) diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index d87d25f..a40a78d 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -262,6 +262,10 @@ private: CurDiagStateLoc = SourceLocation(); } + /// Produce a debugging dump of the diagnostic state. + LLVM_DUMP_METHOD void dump(SourceManager &SrcMgr, + StringRef DiagName = StringRef()) const; + /// Grab the most-recently-added state point. DiagState *getCurDiagState() const { return CurDiagState; } /// Get the location at which a diagnostic state was last added. @@ -409,6 +413,11 @@ public: DiagnosticsEngine &operator=(const DiagnosticsEngine &) = delete; ~DiagnosticsEngine(); + LLVM_DUMP_METHOD void dump() const { DiagStatesByLoc.dump(*SourceMgr); } + LLVM_DUMP_METHOD void dump(StringRef DiagName) const { + DiagStatesByLoc.dump(*SourceMgr, DiagName); + } + const IntrusiveRefCntPtr &getDiagnosticIDs() const { return Diags; } diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h index 397ad2e..fe5aeda 100644 --- a/clang/include/clang/Basic/SourceManager.h +++ b/clang/include/clang/Basic/SourceManager.h @@ -1137,6 +1137,18 @@ public: /// be used by clients. SourceLocation getImmediateSpellingLoc(SourceLocation Loc) const; + /// \brief Form a SourceLocation from a FileID and Offset pair. + SourceLocation getComposedLoc(FileID FID, unsigned Offset) const { + bool Invalid = false; + const SrcMgr::SLocEntry &Entry = getSLocEntry(FID, &Invalid); + if (Invalid) + return SourceLocation(); + + unsigned GlobalOffset = Entry.getOffset() + Offset; + return Entry.isFile() ? SourceLocation::getFileLoc(GlobalOffset) + : SourceLocation::getMacroLoc(GlobalOffset); + } + /// \brief Decompose the specified location into a raw FileID + Offset pair. /// /// The first element is the FileID, the second is the offset from the diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp index 5903d72..83e61dd9 100644 --- a/clang/lib/Basic/Diagnostic.cpp +++ b/clang/lib/Basic/Diagnostic.cpp @@ -236,6 +236,96 @@ DiagnosticsEngine::DiagStateMap::getFile(SourceManager &SrcMgr, return &F; } +void DiagnosticsEngine::DiagStateMap::dump(SourceManager &SrcMgr, + StringRef DiagName) const { + llvm::errs() << "diagnostic state at "; + CurDiagStateLoc.dump(SrcMgr); + llvm::errs() << ": " << CurDiagState << "\n"; + + for (auto &F : Files) { + FileID ID = F.first; + File &File = F.second; + + bool PrintedOuterHeading = false; + auto PrintOuterHeading = [&] { + if (PrintedOuterHeading) return; + PrintedOuterHeading = true; + + llvm::errs() << "File " << &File << " : " << SrcMgr.getBuffer(ID)->getBufferIdentifier(); + if (F.second.Parent) { + std::pair Decomp = + SrcMgr.getDecomposedIncludedLoc(ID); + assert(File.ParentOffset == Decomp.second); + llvm::errs() << " parent " << File.Parent << " "; + SrcMgr.getLocForStartOfFile(Decomp.first) + .getLocWithOffset(Decomp.second) + .dump(SrcMgr); + } + if (File.HasLocalTransitions) + llvm::errs() << " has_local_transitions"; + llvm::errs() << "\n"; + }; + + if (DiagName.empty()) + PrintOuterHeading(); + + for (DiagStatePoint &Transition : File.StateTransitions) { + bool PrintedInnerHeading = false; + auto PrintInnerHeading = [&] { + if (PrintedInnerHeading) return; + PrintedInnerHeading = true; + + PrintOuterHeading(); + llvm::errs() << " "; + SrcMgr.getLocForStartOfFile(ID) + .getLocWithOffset(Transition.Offset) + .dump(SrcMgr); + llvm::errs() << ": state " << Transition.State << ":\n"; + }; + + if (DiagName.empty()) + PrintInnerHeading(); + + for (auto &Mapping : *Transition.State) { + StringRef Option = + DiagnosticIDs::getWarningOptionForDiag(Mapping.first); + if (!DiagName.empty() && DiagName != Option) + continue; + + PrintInnerHeading(); + llvm::errs() << " "; + if (Option.empty()) + llvm::errs() << ""; + else + llvm::errs() << Option; + llvm::errs() << ": "; + + switch (Mapping.second.getSeverity()) { + case diag::Severity::Ignored: llvm::errs() << "ignored"; break; + case diag::Severity::Remark: llvm::errs() << "remark"; break; + case diag::Severity::Warning: llvm::errs() << "warning"; break; + case diag::Severity::Error: llvm::errs() << "error"; break; + case diag::Severity::Fatal: llvm::errs() << "fatal"; break; + } + + if (!Mapping.second.isUser()) + llvm::errs() << " default"; + if (Mapping.second.isPragma()) + llvm::errs() << " pragma"; + if (Mapping.second.hasNoWarningAsError()) + llvm::errs() << " no-error"; + if (Mapping.second.hasNoErrorAsFatal()) + llvm::errs() << " no-fatal"; + if (Mapping.second.wasUpgradedFromWarning()) + llvm::errs() << " overruled"; + llvm::errs() << "\n"; + } + } + } +} + void DiagnosticsEngine::PushDiagStatePoint(DiagState *State, SourceLocation Loc) { assert(Loc.isValid() && "Adding invalid loc point"); diff --git a/clang/lib/Lex/Pragma.cpp b/clang/lib/Lex/Pragma.cpp index b9be03f..db32e45 100644 --- a/clang/lib/Lex/Pragma.cpp +++ b/clang/lib/Lex/Pragma.cpp @@ -1053,6 +1053,20 @@ struct PragmaDebugHandler : public PragmaHandler { PP.Diag(Identifier, diag::warn_pragma_debug_missing_argument) << II->getName(); } + } else if (II->isStr("diag_mapping")) { + Token DiagName; + PP.LexUnexpandedToken(DiagName); + if (DiagName.is(tok::eod)) + PP.getDiagnostics().dump(); + else if (DiagName.is(tok::string_literal) && !DiagName.hasUDSuffix()) { + StringLiteralParser Literal(DiagName, PP); + if (Literal.hadError) + return; + PP.getDiagnostics().dump(Literal.GetString()); + } else { + PP.Diag(DiagName, diag::warn_pragma_debug_missing_argument) + << II->getName(); + } } else if (II->isStr("llvm_fatal_error")) { llvm::report_fatal_error("#pragma clang __debug llvm_fatal_error"); } else if (II->isStr("llvm_unreachable")) { diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index b62c47c..979ff0c 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -5761,6 +5761,8 @@ void ASTReader::ReadPragmaDiagnosticMappings(DiagnosticsEngine &Diag) { Initial.ExtBehavior = (diag::Severity)Flags; FirstState = ReadDiagState(Initial, SourceLocation(), true); + assert(F.OriginalSourceFileID.isValid()); + // Set up the root buffer of the module to start with the initial // diagnostic state of the module itself, to cover files that contain no // explicit transitions (for which we did not serialize anything). @@ -5781,6 +5783,7 @@ void ASTReader::ReadPragmaDiagnosticMappings(DiagnosticsEngine &Diag) { "Invalid data, missing pragma diagnostic states"); SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]); auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc); + assert(IDAndOffset.first.isValid() && "invalid FileID for transition"); assert(IDAndOffset.second == 0 && "not a start location for a FileID"); unsigned Transitions = Record[Idx++]; diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 50c60fe..8c5863e 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -3092,8 +3092,11 @@ void ASTWriter::WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag, !FileIDAndFile.second.HasLocalTransitions) continue; ++NumLocations; - AddSourceLocation(Diag.SourceMgr->getLocForStartOfFile(FileIDAndFile.first), - Record); + + SourceLocation Loc = Diag.SourceMgr->getComposedLoc(FileIDAndFile.first, 0); + assert(!Loc.isInvalid() && "start loc for valid FileID is invalid"); + AddSourceLocation(Loc, Record); + Record.push_back(FileIDAndFile.second.StateTransitions.size()); for (auto &StatePoint : FileIDAndFile.second.StateTransitions) { Record.push_back(StatePoint.Offset); diff --git a/clang/test/Modules/Inputs/diag_flags.h b/clang/test/Modules/Inputs/diag_flags.h index 3b85c84..55eeab1 100644 --- a/clang/test/Modules/Inputs/diag_flags.h +++ b/clang/test/Modules/Inputs/diag_flags.h @@ -1 +1,4 @@ +#define pragma _Pragma("clang diagnostic push") _Pragma("clang diagnostic error \"-Wpadded\"") _Pragma("clang diagnostic pop") +pragma + struct Padded { char x; int y; }; diff --git a/clang/test/Modules/diag-flags.cpp b/clang/test/Modules/diag-flags.cpp index ada90d2..14067c6 100644 --- a/clang/test/Modules/diag-flags.cpp +++ b/clang/test/Modules/diag-flags.cpp @@ -3,24 +3,24 @@ // For an implicit module, all that matters are the warning flags in the user. // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -emit-module -fmodules-cache-path=%t -fmodule-name=diag_flags -x c++ %S/Inputs/module.map -fmodules-ts // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -Wpadded -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DERROR -Wpadded -Werror -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DERROR -Werror=padded +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -DLOCAL_WARNING -Wpadded +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DERROR -DLOCAL_ERROR -Wpadded -Werror +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DERROR -DLOCAL_ERROR -Werror=padded // // For an explicit module, all that matters are the warning flags in the module build. // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -emit-module -fmodule-name=diag_flags -x c++ %S/Inputs/module.map -fmodules-ts -o %t/nodiag.pcm -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -fmodule-file=%t/nodiag.pcm -Wpadded -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -fmodule-file=%t/nodiag.pcm -Werror -Wpadded +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -fmodule-file=%t/nodiag.pcm -Wpadded -DLOCAL_WARNING +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -fmodule-file=%t/nodiag.pcm -Werror -Wpadded -DLOCAL_ERROR // // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -emit-module -fmodule-name=diag_flags -x c++ %S/Inputs/module.map -fmodules-ts -o %t/warning.pcm -Wpadded // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/warning.pcm -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/warning.pcm -Werror=padded +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/warning.pcm -Werror=padded -DLOCAL_ERROR // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/warning.pcm -Werror // // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -emit-module -fmodule-name=diag_flags -x c++ %S/Inputs/module.map -fmodules-ts -o %t/werror-no-error.pcm -Werror -Wpadded -Wno-error=padded // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/werror-no-error.pcm // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/werror-no-error.pcm -Wno-padded -// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/werror-no-error.pcm -Werror=padded +// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DWARNING -fmodule-file=%t/werror-no-error.pcm -Werror=padded -DLOCAL_ERROR // // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -emit-module -fmodule-name=diag_flags -x c++ %S/Inputs/module.map -fmodules-ts -o %t/error.pcm -Werror=padded // RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules -fimplicit-module-maps -verify -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DERROR -fmodule-file=%t/error.pcm -Wno-padded @@ -35,10 +35,22 @@ import diag_flags; // Diagnostic flags from the module user make no difference to diagnostics // emitted within the module when using an explicitly-loaded module. #if ERROR -// expected-error@diag_flags.h:14 {{padding struct}} +// expected-error@diag_flags.h:4 {{padding struct}} #elif WARNING -// expected-warning@diag_flags.h:14 {{padding struct}} -#else -// expected-no-diagnostics +// expected-warning@diag_flags.h:4 {{padding struct}} #endif int arr[sizeof(Padded)]; + +// Diagnostic flags from the module make no difference to diagnostics emitted +// in the module user. +#if LOCAL_ERROR +// expected-error@+4 {{padding struct}} +#elif LOCAL_WARNING +// expected-warning@+2 {{padding struct}} +#endif +struct Padded2 { char x; int y; }; +int arr2[sizeof(Padded2)]; + +#if !ERROR && !WARNING && !LOCAL_ERROR && !LOCAL_WARNING +// expected-no-diagnostics +#endif -- 2.7.4