Skip to content

Conversation

fmayer
Copy link
Contributor

@fmayer fmayer commented Sep 13, 2024

This reverts commit e7f782e.

This had UBSan failures:

[----------] 1 test from ConfigCompileTests
[ RUN ] ConfigCompileTests.DiagnosticSuppression
Config fragment: compiling :0 -> 0x00007B8366E2F7D8 (trusted=false)
/usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33: runtime error: reference binding to null pointer of type 'clang::DiagnosticIDs'

UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33

Created using spr 1.3.4
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:as-a-library libclang and C++ API clang:static analyzer flang:driver flang Flang issues not falling into any other category labels Sep 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clangd

Author: Florian Mayer (fmayer)

Changes

This reverts commit e7f782e.

This had UBSan failures:

[----------] 1 test from ConfigCompileTests
[ RUN ] ConfigCompileTests.DiagnosticSuppression
Config fragment: compiling <unknown>:0 -> 0x00007B8366E2F7D8 (trusted=false)
/usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33: runtime error: reference binding to null pointer of type 'clang::DiagnosticIDs'

UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33


Patch is 65.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/108645.diff

29 Files Affected:

  • (modified) clang-tools-extra/clangd/Diagnostics.cpp (+7-20)
  • (modified) clang-tools-extra/clangd/Diagnostics.h (+4-4)
  • (modified) clang-tools-extra/clangd/ParsedAST.cpp (+3-3)
  • (modified) clang-tools-extra/clangd/Preamble.cpp (+2-2)
  • (modified) clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp (+13-34)
  • (modified) clang/include/clang/Basic/Attr.td (+7-5)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+2-6)
  • (modified) clang/include/clang/Basic/DiagnosticCategories.h (+2-3)
  • (modified) clang/include/clang/Basic/DiagnosticIDs.h (+18-135)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (-6)
  • (modified) clang/lib/Basic/Diagnostic.cpp (+8-14)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+117-163)
  • (modified) clang/lib/Frontend/LogDiagnosticPrinter.cpp (+2-2)
  • (modified) clang/lib/Frontend/SerializedDiagnosticPrinter.cpp (+5-7)
  • (modified) clang/lib/Frontend/TextDiagnosticPrinter.cpp (+3-7)
  • (modified) clang/lib/Sema/Sema.cpp (+2-3)
  • (modified) clang/lib/Sema/SemaCUDA.cpp (+2-2)
  • (modified) clang/lib/Sema/SemaDeclAttr.cpp (+5-21)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+5-27)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+1-2)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+1-1)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (+1-1)
  • (modified) clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp (+1)
  • (modified) clang/test/Sema/diagnose_if.c (+4-4)
  • (removed) clang/test/SemaCXX/diagnose_if-warning-group.cpp (-63)
  • (modified) clang/tools/diagtool/ListWarnings.cpp (+4-3)
  • (modified) clang/tools/diagtool/ShowEnabledWarnings.cpp (+3-3)
  • (modified) clang/tools/libclang/CXStoredDiagnostic.cpp (+1-3)
  • (modified) flang/lib/Frontend/TextDiagnosticPrinter.cpp (+2-2)
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 552dd36b6900bf..d5eca083eb6512 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -579,17 +579,7 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
   for (auto &Diag : Output) {
     if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) {
       // Warnings controlled by -Wfoo are better recognized by that name.
-      const StringRef Warning = [&] {
-        if (OrigSrcMgr) {
-          return OrigSrcMgr->getDiagnostics()
-              .getDiagnosticIDs()
-              ->getWarningOptionForDiag(Diag.ID);
-        }
-        if (!DiagnosticIDs::IsCustomDiag(Diag.ID))
-          return DiagnosticIDs{}.getWarningOptionForDiag(Diag.ID);
-        return StringRef{};
-      }();
-
+      StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(Diag.ID);
       if (!Warning.empty()) {
         Diag.Name = ("-W" + Warning).str();
       } else {
@@ -906,23 +896,20 @@ void StoreDiags::flushLastDiag() {
   Output.push_back(std::move(*LastDiag));
 }
 
-bool isDiagnosticSuppressed(const clang::Diagnostic &Diag,
-                            const llvm::StringSet<> &Suppress,
-                            const LangOptions &LangOpts) {
+bool isBuiltinDiagnosticSuppressed(unsigned ID,
+                                   const llvm::StringSet<> &Suppress,
+                                   const LangOptions &LangOpts) {
   // Don't complain about header-only stuff in mainfiles if it's a header.
   // FIXME: would be cleaner to suppress in clang, once we decide whether the
   //        behavior should be to silently-ignore or respect the pragma.
-  if (Diag.getID() == diag::pp_pragma_sysheader_in_main_file &&
-      LangOpts.IsHeaderFile)
+  if (ID == diag::pp_pragma_sysheader_in_main_file && LangOpts.IsHeaderFile)
     return true;
 
-  if (const char *CodePtr = getDiagnosticCode(Diag.getID())) {
+  if (const char *CodePtr = getDiagnosticCode(ID)) {
     if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
       return true;
   }
-  StringRef Warning =
-      Diag.getDiags()->getDiagnosticIDs()->getWarningOptionForDiag(
-          Diag.getID());
+  StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
   if (!Warning.empty() && Suppress.contains(Warning))
     return true;
   return false;
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index c45d8dc3aa6ce6..d4c0478c63a5cf 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -181,11 +181,11 @@ class StoreDiags : public DiagnosticConsumer {
 };
 
 /// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
-bool isDiagnosticSuppressed(const clang::Diagnostic &Diag,
-                            const llvm::StringSet<> &Suppressed,
-                            const LangOptions &);
+bool isBuiltinDiagnosticSuppressed(unsigned ID,
+                                   const llvm::StringSet<> &Suppressed,
+                                   const LangOptions &);
 /// Take a user-specified diagnostic code, and convert it to a normalized form
-/// stored in the config and consumed by isDiagnosticsSuppressed.
+/// stored in the config and consumed by isBuiltinDiagnosticsSuppressed.
 ///
 /// (This strips err_ and -W prefix so we can match with or without them.)
 llvm::StringRef normalizeSuppressedCode(llvm::StringRef);
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 4491be9aa0362b..a8b6cc8dd0a46f 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -340,7 +340,7 @@ void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
       if (Enable) {
         if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
             DiagnosticsEngine::Warning) {
-          auto Group = Diags.getDiagnosticIDs()->getGroupForDiag(ID);
+          auto Group = DiagnosticIDs::getGroupForDiag(ID);
           if (!Group || !EnabledGroups(*Group))
             continue;
           Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
@@ -583,8 +583,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
       if (Cfg.Diagnostics.SuppressAll ||
-          isDiagnosticSuppressed(Info, Cfg.Diagnostics.Suppress,
-                                 Clang->getLangOpts()))
+          isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                        Clang->getLangOpts()))
         return DiagnosticsEngine::Ignored;
 
       auto It = OverriddenSeverity.find(Info.getID());
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 84e8fec342829c..dd13b1a9e5613d 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -621,8 +621,8 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                            const clang::Diagnostic &Info) {
     if (Cfg.Diagnostics.SuppressAll ||
-        isDiagnosticSuppressed(Info, Cfg.Diagnostics.Suppress,
-                               CI.getLangOpts()))
+        isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                      CI.getLangOpts()))
       return DiagnosticsEngine::Ignored;
     switch (Info.getID()) {
     case diag::warn_no_newline_eof:
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index a812bac0338aa7..4ecfdf0184ab40 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -298,41 +298,20 @@ TEST_F(ConfigCompileTests, DiagnosticSuppression) {
                                    "unreachable-code", "unused-variable",
                                    "typecheck_bool_condition",
                                    "unexpected_friend", "warn_alloca"));
-  clang::DiagnosticsEngine DiagEngine(nullptr, nullptr,
-                                      new clang::IgnoringDiagConsumer);
-
-  using Diag = clang::Diagnostic;
-  {
-    auto D = DiagEngine.Report(diag::warn_unreachable);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unreachable, Conf.Diagnostics.Suppress, LangOptions()));
   // Subcategory not respected/suppressed.
-  {
-    auto D = DiagEngine.Report(diag::warn_unreachable_break);
-    EXPECT_FALSE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::warn_unused_variable);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::err_typecheck_bool_condition);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::err_unexpected_friend);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::warn_alloca);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
+  EXPECT_FALSE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unreachable_break, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unused_variable, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::err_typecheck_bool_condition,
+                                            Conf.Diagnostics.Suppress,
+                                            LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::err_unexpected_friend, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_alloca, Conf.Diagnostics.Suppress, LangOptions()));
 
   Frag.Diagnostics.Suppress.emplace_back("*");
   EXPECT_TRUE(compileAndApply());
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 70fad60d4edbb5..9a7b163b2c6da8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3358,16 +3358,18 @@ def DiagnoseIf : InheritableAttr {
   let Spellings = [GNU<"diagnose_if">];
   let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
   let Args = [ExprArgument<"Cond">, StringArgument<"Message">,
-              EnumArgument<"DefaultSeverity",
-                           "DefaultSeverity",
+              EnumArgument<"DiagnosticType", "DiagnosticType",
                            /*is_string=*/true,
-                           ["error",    "warning"],
-                           ["DS_error", "DS_warning"]>,
-              StringArgument<"WarningGroup", /*optional*/ 1>,
+                           ["error", "warning"],
+                           ["DT_Error", "DT_Warning"]>,
               BoolArgument<"ArgDependent", 0, /*fake*/ 1>,
               DeclArgument<Named, "Parent", 0, /*fake*/ 1>];
   let InheritEvenIfAlreadyPresent = 1;
   let LateParsed = LateAttrParseStandard;
+  let AdditionalMembers = [{
+    bool isError() const { return diagnosticType == DT_Error; }
+    bool isWarning() const { return diagnosticType == DT_Warning; }
+  }];
   let TemplateDependent = 1;
   let Documentation = [DiagnoseIfDocs];
 }
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 54b69e98540239..0c7836c2ea569c 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -336,12 +336,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
     // Map extensions to warnings or errors?
     diag::Severity ExtBehavior = diag::Severity::Ignored;
 
-    DiagnosticIDs &DiagIDs;
-
-    DiagState(DiagnosticIDs &DiagIDs)
+    DiagState()
         : IgnoreAllWarnings(false), EnableAllWarnings(false),
           WarningsAsErrors(false), ErrorsAsFatal(false),
-          SuppressSystemWarnings(false), DiagIDs(DiagIDs) {}
+          SuppressSystemWarnings(false) {}
 
     using iterator = llvm::DenseMap<unsigned, DiagnosticMapping>::iterator;
     using const_iterator =
@@ -872,8 +870,6 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   /// \param FormatString A fixed diagnostic format string that will be hashed
   /// and mapped to a unique DiagID.
   template <unsigned N>
-  // TODO: Deprecate this once all uses are removed from LLVM
-  // [[deprecated("Use a CustomDiagDesc instead of a Level")]]
   unsigned getCustomDiagID(Level L, const char (&FormatString)[N]) {
     return Diags->getCustomDiagID((DiagnosticIDs::Level)L,
                                   StringRef(FormatString, N - 1));
diff --git a/clang/include/clang/Basic/DiagnosticCategories.h b/clang/include/clang/Basic/DiagnosticCategories.h
index 839f8dee3ca89f..14be326f7515f8 100644
--- a/clang/include/clang/Basic/DiagnosticCategories.h
+++ b/clang/include/clang/Basic/DiagnosticCategories.h
@@ -21,12 +21,11 @@ namespace clang {
     };
 
     enum class Group {
-#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups, Docs)    \
-      GroupName,
+#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups, Docs)        \
+  GroupName,
 #include "clang/Basic/DiagnosticGroups.inc"
 #undef CATEGORY
 #undef DIAG_ENTRY
-      NUM_GROUPS
     };
   }  // end namespace diag
 }  // end namespace clang
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index 2402996ece5c94..8b976bdac6dc51 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -14,7 +14,6 @@
 #ifndef LLVM_CLANG_BASIC_DIAGNOSTICIDS_H
 #define LLVM_CLANG_BASIC_DIAGNOSTICIDS_H
 
-#include "clang/Basic/DiagnosticCategories.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
@@ -83,7 +82,7 @@ namespace clang {
     /// to either Ignore (nothing), Remark (emit a remark), Warning
     /// (emit a warning) or Error (emit as an error).  It allows clients to
     /// map ERRORs to Error or Fatal (stop emitting diagnostics after this one).
-    enum class Severity : uint8_t {
+    enum class Severity {
       // NOTE: 0 means "uncomputed".
       Ignored = 1, ///< Do not present this diagnostic, ignore it.
       Remark = 2,  ///< Present this diagnostic as a remark.
@@ -180,96 +179,13 @@ class DiagnosticMapping {
 class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
 public:
   /// The level of the diagnostic, after it has been through mapping.
-  enum Level : uint8_t { Ignored, Note, Remark, Warning, Error, Fatal };
-
-  // Diagnostic classes.
-  enum Class {
-    CLASS_INVALID = 0x00,
-    CLASS_NOTE = 0x01,
-    CLASS_REMARK = 0x02,
-    CLASS_WARNING = 0x03,
-    CLASS_EXTENSION = 0x04,
-    CLASS_ERROR = 0x05
-  };
-
-  static bool IsCustomDiag(diag::kind Diag) {
-    return Diag >= diag::DIAG_UPPER_LIMIT;
-  }
-
-  class CustomDiagDesc {
-    LLVM_PREFERRED_TYPE(diag::Severity)
-    unsigned DefaultSeverity : 3;
-    LLVM_PREFERRED_TYPE(Class)
-    unsigned DiagClass : 3;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned ShowInSystemHeader : 1;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned ShowInSystemMacro : 1;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned HasGroup : 1;
-    diag::Group Group;
-    std::string Description;
-
-    auto get_as_tuple() const {
-      return std::tuple(DefaultSeverity, DiagClass, ShowInSystemHeader,
-                        ShowInSystemMacro, HasGroup, Group,
-                        std::string_view{Description});
-    }
-
-  public:
-    CustomDiagDesc(diag::Severity DefaultSeverity, std::string Description,
-                   unsigned Class = CLASS_WARNING,
-                   bool ShowInSystemHeader = false,
-                   bool ShowInSystemMacro = false,
-                   std::optional<diag::Group> Group = std::nullopt)
-        : DefaultSeverity(static_cast<unsigned>(DefaultSeverity)),
-          DiagClass(Class), ShowInSystemHeader(ShowInSystemHeader),
-          ShowInSystemMacro(ShowInSystemMacro), HasGroup(Group != std::nullopt),
-          Group(Group.value_or(diag::Group{})),
-          Description(std::move(Description)) {}
-
-    std::optional<diag::Group> GetGroup() const {
-      if (HasGroup)
-        return Group;
-      return std::nullopt;
-    }
-
-    diag::Severity GetDefaultSeverity() const {
-      return static_cast<diag::Severity>(DefaultSeverity);
-    }
-
-    Class GetClass() const { return static_cast<Class>(DiagClass); }
-    std::string_view GetDescription() const { return Description; }
-    bool ShouldShowInSystemHeader() const { return ShowInSystemHeader; }
-
-    friend bool operator==(const CustomDiagDesc &lhs,
-                           const CustomDiagDesc &rhs) {
-      return lhs.get_as_tuple() == rhs.get_as_tuple();
-    }
-
-    friend bool operator<(const CustomDiagDesc &lhs,
-                          const CustomDiagDesc &rhs) {
-      return lhs.get_as_tuple() < rhs.get_as_tuple();
-    }
-  };
-
-  struct GroupInfo {
-    LLVM_PREFERRED_TYPE(diag::Severity)
-    unsigned Severity : 3;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned HasNoWarningAsError : 1;
+  enum Level {
+    Ignored, Note, Remark, Warning, Error, Fatal
   };
 
 private:
   /// Information for uniquing and looking up custom diags.
   std::unique_ptr<diag::CustomDiagInfo> CustomDiagInfo;
-  std::unique_ptr<GroupInfo[]> GroupInfos = []() {
-    auto GIs = std::make_unique<GroupInfo[]>(
-        static_cast<size_t>(diag::Group::NUM_GROUPS));
-    for (size_t i = 0; i != static_cast<size_t>(diag::Group::NUM_GROUPS); ++i)
-      GIs[i] = {{}, false};
-    return GIs;
-  }();
 
 public:
   DiagnosticIDs();
@@ -284,34 +200,7 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
   // FIXME: Replace this function with a create-only facilty like
   // createCustomDiagIDFromFormatString() to enforce safe usage. At the time of
   // writing, nearly all callers of this function were invalid.
-  unsigned getCustomDiagID(CustomDiagDesc Diag);
-
-  // TODO: Deprecate this once all uses are removed from LLVM
-  // [[deprecated("Use a CustomDiagDesc instead of a Level")]]
-  unsigned getCustomDiagID(Level Level, StringRef Message) {
-    return getCustomDiagID([&]() -> CustomDiagDesc {
-      switch (Level) {
-      case DiagnosticIDs::Level::Ignored:
-        return {diag::Severity::Ignored, std::string(Message), CLASS_WARNING,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Note:
-        return {diag::Severity::Fatal, std::string(Message), CLASS_NOTE,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Remark:
-        return {diag::Severity::Remark, std::string(Message), CLASS_REMARK,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Warning:
-        return {diag::Severity::Warning, std::string(Message), CLASS_WARNING,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Error:
-        return {diag::Severity::Error, std::string(Message), CLASS_ERROR,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Fatal:
-        return {diag::Severity::Fatal, std::string(Message), CLASS_ERROR,
-                /*ShowInSystemHeader*/ true};
-      }
-    }());
-  }
+  unsigned getCustomDiagID(Level L, StringRef FormatString);
 
   //===--------------------------------------------------------------------===//
   // Diagnostic classification and reporting interfaces.
@@ -323,36 +212,35 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
   /// Return true if the unmapped diagnostic levelof the specified
   /// diagnostic ID is a Warning or Extension.
   ///
-  /// This is not legal to call on NOTEs.
-  bool isWarningOrExtension(unsigned DiagID) const;
+  /// This only works on builtin diagnostics, not custom ones, and is not
+  /// legal to call on NOTEs.
+  static bool isBuiltinWarningOrExtension(unsigned DiagID);
 
   /// Return true if the specified diagnostic is mapped to errors by
   /// default.
-  bool isDefaultMappingAsError(unsigned DiagID) const;
+  static bool isDefaultMappingAsError(unsigned DiagID);
 
   /// Get the default mapping for this diagnostic.
-  DiagnosticMapping getDefaultMapping(unsigned DiagID) const;
-
-  void initCustomDiagMapping(DiagnosticMapping &, unsigned DiagID);
+  static DiagnosticMapping getDefaultMapping(unsigned DiagID);
 
-  /// Determine whether the given diagnostic ID is a Note.
-  bool isNote(unsigned DiagID) const;
+  /// Determine whether the given built-in diagnostic ID is a Note.
+  static bool isBuiltinNote(unsigned DiagID);
 
-  /// Determine whether the given diagnostic ID is for an
+  /// Determine whether the given built-in diagnostic ID is for an
   /// extension of some sort.
-  bool isExtensionDiag(unsigned DiagID) const {
+  static bool isBuiltinExtensionDiag(unsigned DiagID) {
     bool ignored;
-    return isExtensionDiag(DiagID, ignored);
+    return isBuiltinExtensionDiag(DiagID, ignored);
   }
 
-  /// Determine whether the given diagnostic ID is for an
+  /// Determine whether the given built-in diagnostic ID is for an
   /// extension of some sort, and whether it is enabled by default.
   ///
   /// This also returns EnabledByDefault, which is set to indicate whether the
   /// diagnostic is ignored by default (in which case -pedantic enables it) or
   /// treated as a warning/error by default.
   ///
-  bool isExtensionDiag(unsigned DiagID, bool &EnabledByDefault) const;
+  static bool isBuiltinExtensionDiag(unsigned DiagID, bool &EnabledByDefault);
 
   /// Given a group ID, returns the flag that toggles the group.
   /// For example, for Group::DeprecatedDeclarations, returns
@@ -362,22 +250,19 @@ class DiagnosticID...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-clang-modules

Author: Florian Mayer (fmayer)

Changes

This reverts commit e7f782e.

This had UBSan failures:

[----------] 1 test from ConfigCompileTests
[ RUN ] ConfigCompileTests.DiagnosticSuppression
Config fragment: compiling <unknown>:0 -> 0x00007B8366E2F7D8 (trusted=false)
/usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33: runtime error: reference binding to null pointer of type 'clang::DiagnosticIDs'

UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33


Patch is 65.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/108645.diff

29 Files Affected:

  • (modified) clang-tools-extra/clangd/Diagnostics.cpp (+7-20)
  • (modified) clang-tools-extra/clangd/Diagnostics.h (+4-4)
  • (modified) clang-tools-extra/clangd/ParsedAST.cpp (+3-3)
  • (modified) clang-tools-extra/clangd/Preamble.cpp (+2-2)
  • (modified) clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp (+13-34)
  • (modified) clang/include/clang/Basic/Attr.td (+7-5)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+2-6)
  • (modified) clang/include/clang/Basic/DiagnosticCategories.h (+2-3)
  • (modified) clang/include/clang/Basic/DiagnosticIDs.h (+18-135)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (-6)
  • (modified) clang/lib/Basic/Diagnostic.cpp (+8-14)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+117-163)
  • (modified) clang/lib/Frontend/LogDiagnosticPrinter.cpp (+2-2)
  • (modified) clang/lib/Frontend/SerializedDiagnosticPrinter.cpp (+5-7)
  • (modified) clang/lib/Frontend/TextDiagnosticPrinter.cpp (+3-7)
  • (modified) clang/lib/Sema/Sema.cpp (+2-3)
  • (modified) clang/lib/Sema/SemaCUDA.cpp (+2-2)
  • (modified) clang/lib/Sema/SemaDeclAttr.cpp (+5-21)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+5-27)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+1-2)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+1-1)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (+1-1)
  • (modified) clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp (+1)
  • (modified) clang/test/Sema/diagnose_if.c (+4-4)
  • (removed) clang/test/SemaCXX/diagnose_if-warning-group.cpp (-63)
  • (modified) clang/tools/diagtool/ListWarnings.cpp (+4-3)
  • (modified) clang/tools/diagtool/ShowEnabledWarnings.cpp (+3-3)
  • (modified) clang/tools/libclang/CXStoredDiagnostic.cpp (+1-3)
  • (modified) flang/lib/Frontend/TextDiagnosticPrinter.cpp (+2-2)
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 552dd36b6900bf..d5eca083eb6512 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -579,17 +579,7 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
   for (auto &Diag : Output) {
     if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) {
       // Warnings controlled by -Wfoo are better recognized by that name.
-      const StringRef Warning = [&] {
-        if (OrigSrcMgr) {
-          return OrigSrcMgr->getDiagnostics()
-              .getDiagnosticIDs()
-              ->getWarningOptionForDiag(Diag.ID);
-        }
-        if (!DiagnosticIDs::IsCustomDiag(Diag.ID))
-          return DiagnosticIDs{}.getWarningOptionForDiag(Diag.ID);
-        return StringRef{};
-      }();
-
+      StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(Diag.ID);
       if (!Warning.empty()) {
         Diag.Name = ("-W" + Warning).str();
       } else {
@@ -906,23 +896,20 @@ void StoreDiags::flushLastDiag() {
   Output.push_back(std::move(*LastDiag));
 }
 
-bool isDiagnosticSuppressed(const clang::Diagnostic &Diag,
-                            const llvm::StringSet<> &Suppress,
-                            const LangOptions &LangOpts) {
+bool isBuiltinDiagnosticSuppressed(unsigned ID,
+                                   const llvm::StringSet<> &Suppress,
+                                   const LangOptions &LangOpts) {
   // Don't complain about header-only stuff in mainfiles if it's a header.
   // FIXME: would be cleaner to suppress in clang, once we decide whether the
   //        behavior should be to silently-ignore or respect the pragma.
-  if (Diag.getID() == diag::pp_pragma_sysheader_in_main_file &&
-      LangOpts.IsHeaderFile)
+  if (ID == diag::pp_pragma_sysheader_in_main_file && LangOpts.IsHeaderFile)
     return true;
 
-  if (const char *CodePtr = getDiagnosticCode(Diag.getID())) {
+  if (const char *CodePtr = getDiagnosticCode(ID)) {
     if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
       return true;
   }
-  StringRef Warning =
-      Diag.getDiags()->getDiagnosticIDs()->getWarningOptionForDiag(
-          Diag.getID());
+  StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
   if (!Warning.empty() && Suppress.contains(Warning))
     return true;
   return false;
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index c45d8dc3aa6ce6..d4c0478c63a5cf 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -181,11 +181,11 @@ class StoreDiags : public DiagnosticConsumer {
 };
 
 /// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
-bool isDiagnosticSuppressed(const clang::Diagnostic &Diag,
-                            const llvm::StringSet<> &Suppressed,
-                            const LangOptions &);
+bool isBuiltinDiagnosticSuppressed(unsigned ID,
+                                   const llvm::StringSet<> &Suppressed,
+                                   const LangOptions &);
 /// Take a user-specified diagnostic code, and convert it to a normalized form
-/// stored in the config and consumed by isDiagnosticsSuppressed.
+/// stored in the config and consumed by isBuiltinDiagnosticsSuppressed.
 ///
 /// (This strips err_ and -W prefix so we can match with or without them.)
 llvm::StringRef normalizeSuppressedCode(llvm::StringRef);
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 4491be9aa0362b..a8b6cc8dd0a46f 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -340,7 +340,7 @@ void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
       if (Enable) {
         if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
             DiagnosticsEngine::Warning) {
-          auto Group = Diags.getDiagnosticIDs()->getGroupForDiag(ID);
+          auto Group = DiagnosticIDs::getGroupForDiag(ID);
           if (!Group || !EnabledGroups(*Group))
             continue;
           Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
@@ -583,8 +583,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
       if (Cfg.Diagnostics.SuppressAll ||
-          isDiagnosticSuppressed(Info, Cfg.Diagnostics.Suppress,
-                                 Clang->getLangOpts()))
+          isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                        Clang->getLangOpts()))
         return DiagnosticsEngine::Ignored;
 
       auto It = OverriddenSeverity.find(Info.getID());
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 84e8fec342829c..dd13b1a9e5613d 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -621,8 +621,8 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                            const clang::Diagnostic &Info) {
     if (Cfg.Diagnostics.SuppressAll ||
-        isDiagnosticSuppressed(Info, Cfg.Diagnostics.Suppress,
-                               CI.getLangOpts()))
+        isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                      CI.getLangOpts()))
       return DiagnosticsEngine::Ignored;
     switch (Info.getID()) {
     case diag::warn_no_newline_eof:
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index a812bac0338aa7..4ecfdf0184ab40 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -298,41 +298,20 @@ TEST_F(ConfigCompileTests, DiagnosticSuppression) {
                                    "unreachable-code", "unused-variable",
                                    "typecheck_bool_condition",
                                    "unexpected_friend", "warn_alloca"));
-  clang::DiagnosticsEngine DiagEngine(nullptr, nullptr,
-                                      new clang::IgnoringDiagConsumer);
-
-  using Diag = clang::Diagnostic;
-  {
-    auto D = DiagEngine.Report(diag::warn_unreachable);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unreachable, Conf.Diagnostics.Suppress, LangOptions()));
   // Subcategory not respected/suppressed.
-  {
-    auto D = DiagEngine.Report(diag::warn_unreachable_break);
-    EXPECT_FALSE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::warn_unused_variable);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::err_typecheck_bool_condition);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::err_unexpected_friend);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
-  {
-    auto D = DiagEngine.Report(diag::warn_alloca);
-    EXPECT_TRUE(isDiagnosticSuppressed(
-        Diag{&DiagEngine}, Conf.Diagnostics.Suppress, LangOptions()));
-  }
+  EXPECT_FALSE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unreachable_break, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unused_variable, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::err_typecheck_bool_condition,
+                                            Conf.Diagnostics.Suppress,
+                                            LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::err_unexpected_friend, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_alloca, Conf.Diagnostics.Suppress, LangOptions()));
 
   Frag.Diagnostics.Suppress.emplace_back("*");
   EXPECT_TRUE(compileAndApply());
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 70fad60d4edbb5..9a7b163b2c6da8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3358,16 +3358,18 @@ def DiagnoseIf : InheritableAttr {
   let Spellings = [GNU<"diagnose_if">];
   let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
   let Args = [ExprArgument<"Cond">, StringArgument<"Message">,
-              EnumArgument<"DefaultSeverity",
-                           "DefaultSeverity",
+              EnumArgument<"DiagnosticType", "DiagnosticType",
                            /*is_string=*/true,
-                           ["error",    "warning"],
-                           ["DS_error", "DS_warning"]>,
-              StringArgument<"WarningGroup", /*optional*/ 1>,
+                           ["error", "warning"],
+                           ["DT_Error", "DT_Warning"]>,
               BoolArgument<"ArgDependent", 0, /*fake*/ 1>,
               DeclArgument<Named, "Parent", 0, /*fake*/ 1>];
   let InheritEvenIfAlreadyPresent = 1;
   let LateParsed = LateAttrParseStandard;
+  let AdditionalMembers = [{
+    bool isError() const { return diagnosticType == DT_Error; }
+    bool isWarning() const { return diagnosticType == DT_Warning; }
+  }];
   let TemplateDependent = 1;
   let Documentation = [DiagnoseIfDocs];
 }
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 54b69e98540239..0c7836c2ea569c 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -336,12 +336,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
     // Map extensions to warnings or errors?
     diag::Severity ExtBehavior = diag::Severity::Ignored;
 
-    DiagnosticIDs &DiagIDs;
-
-    DiagState(DiagnosticIDs &DiagIDs)
+    DiagState()
         : IgnoreAllWarnings(false), EnableAllWarnings(false),
           WarningsAsErrors(false), ErrorsAsFatal(false),
-          SuppressSystemWarnings(false), DiagIDs(DiagIDs) {}
+          SuppressSystemWarnings(false) {}
 
     using iterator = llvm::DenseMap<unsigned, DiagnosticMapping>::iterator;
     using const_iterator =
@@ -872,8 +870,6 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   /// \param FormatString A fixed diagnostic format string that will be hashed
   /// and mapped to a unique DiagID.
   template <unsigned N>
-  // TODO: Deprecate this once all uses are removed from LLVM
-  // [[deprecated("Use a CustomDiagDesc instead of a Level")]]
   unsigned getCustomDiagID(Level L, const char (&FormatString)[N]) {
     return Diags->getCustomDiagID((DiagnosticIDs::Level)L,
                                   StringRef(FormatString, N - 1));
diff --git a/clang/include/clang/Basic/DiagnosticCategories.h b/clang/include/clang/Basic/DiagnosticCategories.h
index 839f8dee3ca89f..14be326f7515f8 100644
--- a/clang/include/clang/Basic/DiagnosticCategories.h
+++ b/clang/include/clang/Basic/DiagnosticCategories.h
@@ -21,12 +21,11 @@ namespace clang {
     };
 
     enum class Group {
-#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups, Docs)    \
-      GroupName,
+#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups, Docs)        \
+  GroupName,
 #include "clang/Basic/DiagnosticGroups.inc"
 #undef CATEGORY
 #undef DIAG_ENTRY
-      NUM_GROUPS
     };
   }  // end namespace diag
 }  // end namespace clang
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index 2402996ece5c94..8b976bdac6dc51 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -14,7 +14,6 @@
 #ifndef LLVM_CLANG_BASIC_DIAGNOSTICIDS_H
 #define LLVM_CLANG_BASIC_DIAGNOSTICIDS_H
 
-#include "clang/Basic/DiagnosticCategories.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
@@ -83,7 +82,7 @@ namespace clang {
     /// to either Ignore (nothing), Remark (emit a remark), Warning
     /// (emit a warning) or Error (emit as an error).  It allows clients to
     /// map ERRORs to Error or Fatal (stop emitting diagnostics after this one).
-    enum class Severity : uint8_t {
+    enum class Severity {
       // NOTE: 0 means "uncomputed".
       Ignored = 1, ///< Do not present this diagnostic, ignore it.
       Remark = 2,  ///< Present this diagnostic as a remark.
@@ -180,96 +179,13 @@ class DiagnosticMapping {
 class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
 public:
   /// The level of the diagnostic, after it has been through mapping.
-  enum Level : uint8_t { Ignored, Note, Remark, Warning, Error, Fatal };
-
-  // Diagnostic classes.
-  enum Class {
-    CLASS_INVALID = 0x00,
-    CLASS_NOTE = 0x01,
-    CLASS_REMARK = 0x02,
-    CLASS_WARNING = 0x03,
-    CLASS_EXTENSION = 0x04,
-    CLASS_ERROR = 0x05
-  };
-
-  static bool IsCustomDiag(diag::kind Diag) {
-    return Diag >= diag::DIAG_UPPER_LIMIT;
-  }
-
-  class CustomDiagDesc {
-    LLVM_PREFERRED_TYPE(diag::Severity)
-    unsigned DefaultSeverity : 3;
-    LLVM_PREFERRED_TYPE(Class)
-    unsigned DiagClass : 3;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned ShowInSystemHeader : 1;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned ShowInSystemMacro : 1;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned HasGroup : 1;
-    diag::Group Group;
-    std::string Description;
-
-    auto get_as_tuple() const {
-      return std::tuple(DefaultSeverity, DiagClass, ShowInSystemHeader,
-                        ShowInSystemMacro, HasGroup, Group,
-                        std::string_view{Description});
-    }
-
-  public:
-    CustomDiagDesc(diag::Severity DefaultSeverity, std::string Description,
-                   unsigned Class = CLASS_WARNING,
-                   bool ShowInSystemHeader = false,
-                   bool ShowInSystemMacro = false,
-                   std::optional<diag::Group> Group = std::nullopt)
-        : DefaultSeverity(static_cast<unsigned>(DefaultSeverity)),
-          DiagClass(Class), ShowInSystemHeader(ShowInSystemHeader),
-          ShowInSystemMacro(ShowInSystemMacro), HasGroup(Group != std::nullopt),
-          Group(Group.value_or(diag::Group{})),
-          Description(std::move(Description)) {}
-
-    std::optional<diag::Group> GetGroup() const {
-      if (HasGroup)
-        return Group;
-      return std::nullopt;
-    }
-
-    diag::Severity GetDefaultSeverity() const {
-      return static_cast<diag::Severity>(DefaultSeverity);
-    }
-
-    Class GetClass() const { return static_cast<Class>(DiagClass); }
-    std::string_view GetDescription() const { return Description; }
-    bool ShouldShowInSystemHeader() const { return ShowInSystemHeader; }
-
-    friend bool operator==(const CustomDiagDesc &lhs,
-                           const CustomDiagDesc &rhs) {
-      return lhs.get_as_tuple() == rhs.get_as_tuple();
-    }
-
-    friend bool operator<(const CustomDiagDesc &lhs,
-                          const CustomDiagDesc &rhs) {
-      return lhs.get_as_tuple() < rhs.get_as_tuple();
-    }
-  };
-
-  struct GroupInfo {
-    LLVM_PREFERRED_TYPE(diag::Severity)
-    unsigned Severity : 3;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned HasNoWarningAsError : 1;
+  enum Level {
+    Ignored, Note, Remark, Warning, Error, Fatal
   };
 
 private:
   /// Information for uniquing and looking up custom diags.
   std::unique_ptr<diag::CustomDiagInfo> CustomDiagInfo;
-  std::unique_ptr<GroupInfo[]> GroupInfos = []() {
-    auto GIs = std::make_unique<GroupInfo[]>(
-        static_cast<size_t>(diag::Group::NUM_GROUPS));
-    for (size_t i = 0; i != static_cast<size_t>(diag::Group::NUM_GROUPS); ++i)
-      GIs[i] = {{}, false};
-    return GIs;
-  }();
 
 public:
   DiagnosticIDs();
@@ -284,34 +200,7 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
   // FIXME: Replace this function with a create-only facilty like
   // createCustomDiagIDFromFormatString() to enforce safe usage. At the time of
   // writing, nearly all callers of this function were invalid.
-  unsigned getCustomDiagID(CustomDiagDesc Diag);
-
-  // TODO: Deprecate this once all uses are removed from LLVM
-  // [[deprecated("Use a CustomDiagDesc instead of a Level")]]
-  unsigned getCustomDiagID(Level Level, StringRef Message) {
-    return getCustomDiagID([&]() -> CustomDiagDesc {
-      switch (Level) {
-      case DiagnosticIDs::Level::Ignored:
-        return {diag::Severity::Ignored, std::string(Message), CLASS_WARNING,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Note:
-        return {diag::Severity::Fatal, std::string(Message), CLASS_NOTE,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Remark:
-        return {diag::Severity::Remark, std::string(Message), CLASS_REMARK,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Warning:
-        return {diag::Severity::Warning, std::string(Message), CLASS_WARNING,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Error:
-        return {diag::Severity::Error, std::string(Message), CLASS_ERROR,
-                /*ShowInSystemHeader*/ true};
-      case DiagnosticIDs::Level::Fatal:
-        return {diag::Severity::Fatal, std::string(Message), CLASS_ERROR,
-                /*ShowInSystemHeader*/ true};
-      }
-    }());
-  }
+  unsigned getCustomDiagID(Level L, StringRef FormatString);
 
   //===--------------------------------------------------------------------===//
   // Diagnostic classification and reporting interfaces.
@@ -323,36 +212,35 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
   /// Return true if the unmapped diagnostic levelof the specified
   /// diagnostic ID is a Warning or Extension.
   ///
-  /// This is not legal to call on NOTEs.
-  bool isWarningOrExtension(unsigned DiagID) const;
+  /// This only works on builtin diagnostics, not custom ones, and is not
+  /// legal to call on NOTEs.
+  static bool isBuiltinWarningOrExtension(unsigned DiagID);
 
   /// Return true if the specified diagnostic is mapped to errors by
   /// default.
-  bool isDefaultMappingAsError(unsigned DiagID) const;
+  static bool isDefaultMappingAsError(unsigned DiagID);
 
   /// Get the default mapping for this diagnostic.
-  DiagnosticMapping getDefaultMapping(unsigned DiagID) const;
-
-  void initCustomDiagMapping(DiagnosticMapping &, unsigned DiagID);
+  static DiagnosticMapping getDefaultMapping(unsigned DiagID);
 
-  /// Determine whether the given diagnostic ID is a Note.
-  bool isNote(unsigned DiagID) const;
+  /// Determine whether the given built-in diagnostic ID is a Note.
+  static bool isBuiltinNote(unsigned DiagID);
 
-  /// Determine whether the given diagnostic ID is for an
+  /// Determine whether the given built-in diagnostic ID is for an
   /// extension of some sort.
-  bool isExtensionDiag(unsigned DiagID) const {
+  static bool isBuiltinExtensionDiag(unsigned DiagID) {
     bool ignored;
-    return isExtensionDiag(DiagID, ignored);
+    return isBuiltinExtensionDiag(DiagID, ignored);
   }
 
-  /// Determine whether the given diagnostic ID is for an
+  /// Determine whether the given built-in diagnostic ID is for an
   /// extension of some sort, and whether it is enabled by default.
   ///
   /// This also returns EnabledByDefault, which is set to indicate whether the
   /// diagnostic is ignored by default (in which case -pedantic enables it) or
   /// treated as a warning/error by default.
   ///
-  bool isExtensionDiag(unsigned DiagID, bool &EnabledByDefault) const;
+  static bool isBuiltinExtensionDiag(unsigned DiagID, bool &EnabledByDefault);
 
   /// Given a group ID, returns the flag that toggles the group.
   /// For example, for Group::DeprecatedDeclarations, returns
@@ -362,22 +250,19 @@ class DiagnosticID...
[truncated]

Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 514fc798270e23f8a117869c0961e52fa1d4bae8 0163ed813f97fc0f83cfbf0b720f993fc96eddc0 --extensions c,h,cpp -- clang-tools-extra/clangd/Diagnostics.cpp clang-tools-extra/clangd/Diagnostics.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp clang/include/clang/Basic/Diagnostic.h clang/include/clang/Basic/DiagnosticCategories.h clang/include/clang/Basic/DiagnosticIDs.h clang/lib/Basic/Diagnostic.cpp clang/lib/Basic/DiagnosticIDs.cpp clang/lib/Frontend/LogDiagnosticPrinter.cpp clang/lib/Frontend/SerializedDiagnosticPrinter.cpp clang/lib/Frontend/TextDiagnosticPrinter.cpp clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaCUDA.cpp clang/lib/Sema/SemaDeclAttr.cpp clang/lib/Sema/SemaOverload.cpp clang/lib/Sema/SemaTemplateInstantiateDecl.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp clang/test/Sema/diagnose_if.c clang/tools/diagtool/ListWarnings.cpp clang/tools/diagtool/ShowEnabledWarnings.cpp clang/tools/libclang/CXStoredDiagnostic.cpp flang/lib/Frontend/TextDiagnosticPrinter.cpp
View the diff from clang-format here.
diff --git a/clang/include/clang/Basic/DiagnosticCategories.h b/clang/include/clang/Basic/DiagnosticCategories.h
index 14be326f75..b2c2259062 100644
--- a/clang/include/clang/Basic/DiagnosticCategories.h
+++ b/clang/include/clang/Basic/DiagnosticCategories.h
@@ -21,8 +21,8 @@ namespace clang {
     };
 
     enum class Group {
-#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups, Docs)        \
-  GroupName,
+#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups, Docs)    \
+      GroupName,
 #include "clang/Basic/DiagnosticGroups.inc"
 #undef CATEGORY
 #undef DIAG_ENTRY
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index 8b976bdac6..1aa72a008f 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -179,9 +179,7 @@ public:
 class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
 public:
   /// The level of the diagnostic, after it has been through mapping.
-  enum Level {
-    Ignored, Note, Remark, Warning, Error, Fatal
-  };
+  enum Level { Ignored, Note, Remark, Warning, Error, Fatal };
 
 private:
   /// Information for uniquing and looking up custom diags.
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 66776daa5e..edfc849952 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -353,8 +353,7 @@ void DiagnosticsEngine::PushDiagStatePoint(DiagState *State,
 
 void DiagnosticsEngine::setSeverity(diag::kind Diag, diag::Severity Map,
                                     SourceLocation L) {
-  assert(Diag < diag::DIAG_UPPER_LIMIT &&
-         "Can only map builtin diagnostics");
+  assert(Diag < diag::DIAG_UPPER_LIMIT && "Can only map builtin diagnostics");
   assert((Diags->isBuiltinWarningOrExtension(Diag) ||
           (Map == diag::Severity::Fatal || Map == diag::Severity::Error)) &&
          "Cannot map errors into warnings!");
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index cd42573968..8adb161dd3 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -104,11 +104,11 @@ const uint32_t StaticDiagInfoDescriptionOffsets[] = {
 
 // Diagnostic classes.
 enum DiagnosticClass {
-  CLASS_NOTE       = 0x01,
-  CLASS_REMARK     = 0x02,
-  CLASS_WARNING    = 0x03,
-  CLASS_EXTENSION  = 0x04,
-  CLASS_ERROR      = 0x05
+  CLASS_NOTE = 0x01,
+  CLASS_REMARK = 0x02,
+  CLASS_WARNING = 0x03,
+  CLASS_EXTENSION = 0x04,
+  CLASS_ERROR = 0x05
 };
 
 struct StaticDiagInfoRec {
@@ -356,47 +356,46 @@ static unsigned getBuiltinDiagClass(unsigned DiagID) {
 //===----------------------------------------------------------------------===//
 
 namespace clang {
-  namespace diag {
-    class CustomDiagInfo {
-      typedef std::pair<DiagnosticIDs::Level, std::string> DiagDesc;
-      std::vector<DiagDesc> DiagInfo;
-      std::map<DiagDesc, unsigned> DiagIDs;
-    public:
-
-      /// getDescription - Return the description of the specified custom
-      /// diagnostic.
-      StringRef getDescription(unsigned DiagID) const {
-        assert(DiagID - DIAG_UPPER_LIMIT < DiagInfo.size() &&
-               "Invalid diagnostic ID");
-        return DiagInfo[DiagID-DIAG_UPPER_LIMIT].second;
-      }
-
-      /// getLevel - Return the level of the specified custom diagnostic.
-      DiagnosticIDs::Level getLevel(unsigned DiagID) const {
-        assert(DiagID - DIAG_UPPER_LIMIT < DiagInfo.size() &&
-               "Invalid diagnostic ID");
-        return DiagInfo[DiagID-DIAG_UPPER_LIMIT].first;
-      }
-
-      unsigned getOrCreateDiagID(DiagnosticIDs::Level L, StringRef Message,
-                                 DiagnosticIDs &Diags) {
-        DiagDesc D(L, std::string(Message));
-        // Check to see if it already exists.
-        std::map<DiagDesc, unsigned>::iterator I = DiagIDs.lower_bound(D);
-        if (I != DiagIDs.end() && I->first == D)
-          return I->second;
-
-        // If not, assign a new ID.
-        unsigned ID = DiagInfo.size()+DIAG_UPPER_LIMIT;
-        DiagIDs.insert(std::make_pair(D, ID));
-        DiagInfo.push_back(D);
-        return ID;
-      }
-    };
-
-  } // end diag namespace
-} // end clang namespace
+namespace diag {
+class CustomDiagInfo {
+  typedef std::pair<DiagnosticIDs::Level, std::string> DiagDesc;
+  std::vector<DiagDesc> DiagInfo;
+  std::map<DiagDesc, unsigned> DiagIDs;
+
+public:
+  /// getDescription - Return the description of the specified custom
+  /// diagnostic.
+  StringRef getDescription(unsigned DiagID) const {
+    assert(DiagID - DIAG_UPPER_LIMIT < DiagInfo.size() &&
+           "Invalid diagnostic ID");
+    return DiagInfo[DiagID - DIAG_UPPER_LIMIT].second;
+  }
 
+  /// getLevel - Return the level of the specified custom diagnostic.
+  DiagnosticIDs::Level getLevel(unsigned DiagID) const {
+    assert(DiagID - DIAG_UPPER_LIMIT < DiagInfo.size() &&
+           "Invalid diagnostic ID");
+    return DiagInfo[DiagID - DIAG_UPPER_LIMIT].first;
+  }
+
+  unsigned getOrCreateDiagID(DiagnosticIDs::Level L, StringRef Message,
+                             DiagnosticIDs &Diags) {
+    DiagDesc D(L, std::string(Message));
+    // Check to see if it already exists.
+    std::map<DiagDesc, unsigned>::iterator I = DiagIDs.lower_bound(D);
+    if (I != DiagIDs.end() && I->first == D)
+      return I->second;
+
+    // If not, assign a new ID.
+    unsigned ID = DiagInfo.size() + DIAG_UPPER_LIMIT;
+    DiagIDs.insert(std::make_pair(D, ID));
+    DiagInfo.push_back(D);
+    return ID;
+  }
+};
+
+} // namespace diag
+} // namespace clang
 
 //===----------------------------------------------------------------------===//
 // Common Diagnostic implementation
@@ -418,7 +417,6 @@ unsigned DiagnosticIDs::getCustomDiagID(Level L, StringRef FormatString) {
   return CustomDiagInfo->getOrCreateDiagID(L, FormatString, *this);
 }
 
-
 /// isBuiltinWarningOrExtension - Return true if the unmapped diagnostic
 /// level of the specified diagnostic ID is a Warning or Extension.
 /// This only works on builtin diagnostics, not custom ones, and is not legal to
@@ -432,7 +430,7 @@ bool DiagnosticIDs::isBuiltinWarningOrExtension(unsigned DiagID) {
 /// Note.
 bool DiagnosticIDs::isBuiltinNote(unsigned DiagID) {
   return DiagID < diag::DIAG_UPPER_LIMIT &&
-    getBuiltinDiagClass(DiagID) == CLASS_NOTE;
+         getBuiltinDiagClass(DiagID) == CLASS_NOTE;
 }
 
 /// isBuiltinExtensionDiag - Determine whether the given built-in diagnostic
@@ -441,7 +439,7 @@ bool DiagnosticIDs::isBuiltinNote(unsigned DiagID) {
 /// which case -pedantic enables it) or treated as a warning/error by default.
 ///
 bool DiagnosticIDs::isBuiltinExtensionDiag(unsigned DiagID,
-                                        bool &EnabledByDefault) {
+                                           bool &EnabledByDefault) {
   if (DiagID >= diag::DIAG_UPPER_LIMIT ||
       getBuiltinDiagClass(DiagID) != CLASS_EXTENSION)
     return false;
@@ -693,8 +691,8 @@ static bool getDiagnosticsInGroup(diag::Flavor Flavor,
   // Add the members of the subgroups.
   const int16_t *SubGroups = DiagSubGroups + Group->SubGroups;
   for (; *SubGroups != (int16_t)-1; ++SubGroups)
-    NotFound &= getDiagnosticsInGroup(Flavor, &OptionTable[(short)*SubGroups],
-                                      Diags);
+    NotFound &=
+        getDiagnosticsInGroup(Flavor, &OptionTable[(short)*SubGroups], Diags);
 
   return NotFound;
 }
diff --git a/clang/lib/Frontend/LogDiagnosticPrinter.cpp b/clang/lib/Frontend/LogDiagnosticPrinter.cpp
index 469d1c2263..f7c82881c8 100644
--- a/clang/lib/Frontend/LogDiagnosticPrinter.cpp
+++ b/clang/lib/Frontend/LogDiagnosticPrinter.cpp
@@ -160,4 +160,3 @@ void LogDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
   // Record the diagnostic entry.
   Entries.push_back(DE);
 }
-
diff --git a/clang/tools/diagtool/ListWarnings.cpp b/clang/tools/diagtool/ListWarnings.cpp
index a71f6e3a66..f14fef2bbd 100644
--- a/clang/tools/diagtool/ListWarnings.cpp
+++ b/clang/tools/diagtool/ListWarnings.cpp
@@ -97,4 +97,3 @@ int ListWarnings::run(unsigned int argc, char **argv, llvm::raw_ostream &out) {
 
   return 0;
 }
-

@philnik777
Copy link
Contributor

@fmayer Could you maybe provide some information? What test is this? How do I run it? I haven't received a single mail from a buildbot regarding this commit.

@fmayer
Copy link
Contributor Author

fmayer commented Sep 13, 2024

@fmayer Could you maybe provide some information? What test is this? How do I run it? I haven't received a single mail from a buildbot regarding this commit.

It's this buildbot: https://lab.llvm.org/buildbot/#/builders/169/builds/3168 (sorry, had the wrong link before)

I took the cmake command lines from there, and ran check-clangd before and after your change.

fmayer added a commit that referenced this pull request Sep 13, 2024
…arning information (#70976)" (#108453)"

This reverts commit e7f782e.

This had UBSan failures:

[----------] 1 test from ConfigCompileTests
[ RUN      ] ConfigCompileTests.DiagnosticSuppression
Config fragment: compiling <unknown>:0 -> 0x00007B8366E2F7D8 (trusted=false)
/usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33: runtime error: reference binding to null pointer of type 'clang::DiagnosticIDs'

UndefinedBehaviorSanitizer: undefined-behavior /usr/local/google/home/fmayer/large/llvm-project/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:203:33

Pull Request: #108645
@fmayer
Copy link
Contributor Author

fmayer commented Sep 13, 2024

I submitted this to get the buildbot back to green, happy to help if you have more questions on how to reproduce!

@fmayer fmayer closed this Sep 13, 2024
@fmayer fmayer deleted the users/fmayer/spr/revert-reapply-clang-extend-diagnose_if-to-accept-more-detailed-warning-information-70976-108453-1 branch September 13, 2024 23:00
@fmayer
Copy link
Contributor Author

fmayer commented Sep 13, 2024

The error is gone in the first buildbot build with this revert: https://lab.llvm.org/buildbot/#/builders/169/builds/3191/steps/9/logs/stdio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:as-a-library libclang and C++ API clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:static analyzer clang Clang issues not falling into any other category clang-tools-extra clangd flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants