[diagtool] Make the BuiltinDiagnosticsByID table sorted

[karka228]

When building with -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON with a recent libstdc++ (e.g. from gcc 13.3.0) the testcase
clang/test/Misc/warning-flags-tree.c fail with the message:

+ diagtool tree --internal
 .../include/c++/13.3.0/bits/stl_algo.h:2013:
In function:
    _ForwardIterator std::lower_bound(_ForwardIterator, _ForwardIterator,
    const _Tp &, _Compare) [_ForwardIterator = const
    diagtool::DiagnosticRecord *, _Tp = diagtool::DiagnosticRecord, _Compare
    = bool (*)(const diagtool::DiagnosticRecord &, const
    diagtool::DiagnosticRecord &)]

Error: elements in iterator range [first, last) are not partitioned by the predicate __comp and value __val.

Objects involved in the operation:
    iterator "first" @ 0x7ffea8ef2fd8 {
    }
    iterator "last" @ 0x7ffea8ef2fd0 {
    }

The reason for this error is that std::lower_bound is called on BuiltinDiagnosticsByID without it being entirely sorted. Calling std::lower_bound If the range is not sorted, the behavior of this function is undefined. This is detected when building with expensive checks.

To make BuiltinDiagnosticsByID sorted we need to slightly change the order the inc-files are included. The include of DiagnosticCrossTUKinds.inc in DiagnosticNames.cpp is included too early and should be moved down directly after DiagnosticCommentKinds.inc.

As a part of pull request the includes that build up BuiltinDiagnosticsByID table are extracted into a common wrapper header file AllDiagnosticKinds.inc that is used by both clang and diagtool.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Karl-Johan Karlsson (karka228)

Changes

When building with -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON with a recent libstdc++ (e.g. from gcc 13.3.0) the testcase
clang/test/Misc/warning-flags-tree.c fail with the message:

+ diagtool tree --internal
 .../include/c++/13.3.0/bits/stl_algo.h:2013:
In function:
    _ForwardIterator std::lower_bound(_ForwardIterator, _ForwardIterator,
    const _Tp &, _Compare) [_ForwardIterator = const
    diagtool::DiagnosticRecord *, _Tp = diagtool::DiagnosticRecord, _Compare
    = bool (*)(const diagtool::DiagnosticRecord &, const
    diagtool::DiagnosticRecord &)]

Error: elements in iterator range [first, last) are not partitioned by the predicate __comp and value __val.

Objects involved in the operation:
    iterator "first" @ 0x7ffea8ef2fd8 {
    }
    iterator "last" @ 0x7ffea8ef2fd0 {
    }

The reason for this error is that std::lower_bound is called on BuiltinDiagnosticsByID without it being entirely sorted. Calling std::lower_bound If the range is not sorted, the behavior of this function is undefined. This is detected when building with expensive checks.

To make BuiltinDiagnosticsByID sorted we need to slightly change the order the inc-files are included.

---

Full diff: https://github.com/llvm/llvm-project/pull/120321.diff

1 Files Affected:

• (modified) clang/tools/diagtool/DiagnosticNames.cpp (+1-1)

diff --git a/clang/tools/diagtool/DiagnosticNames.cpp b/clang/tools/diagtool/DiagnosticNames.cpp
index eb90f082437b33..d6c80ddf72cb27 100644
--- a/clang/tools/diagtool/DiagnosticNames.cpp
+++ b/clang/tools/diagtool/DiagnosticNames.cpp
@@ -31,7 +31,6 @@ static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
              SHOWINSYSHEADER, SHOWINSYSMACRO, DEFER, CATEGORY)                 \
   {#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
 #include "clang/Basic/DiagnosticCommonKinds.inc"
-#include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
 #include "clang/Basic/DiagnosticFrontendKinds.inc"
 #include "clang/Basic/DiagnosticSerializationKinds.inc"
@@ -39,6 +38,7 @@ static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
 #include "clang/Basic/DiagnosticParseKinds.inc"
 #include "clang/Basic/DiagnosticASTKinds.inc"
 #include "clang/Basic/DiagnosticCommentKinds.inc"
+#include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
 #include "clang/Basic/DiagnosticRefactoringKinds.inc"

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[bjope]

Added some reviewers based on people that have touched code related to "diagnostics" (but don't really know much about the fix, nor who might know anything about this particular code in diagtool). Feel free to redirect to someone else (or let me know if I should try to find other reviewers).

[karka228]

Add a comment about why we turn off clang-format:

// Turn off clang-format, as the order of the includes are important
// to make sure the table is sorted.

[kadircet]

also it's unclear where the "ground truth" for this order is coming from. AFAICT, order is defined by https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticIDs.h#L49-L65
and enforced by very subtle interactions between tablegen and how particular headers include these.

since this order needs to be preserved in multiple places, can you put together a wrapper header, which includes individual diagnostic kinds in the specific order and explain the relationship between these pieces? (I'd rather have a solution that puts all of this logic into tablegen instead, generating enums with a particular order once, but I can see that's a much bigger change that you might not want to sign up).

[karka228]

"since this order needs to be preserved in multiple places, can you put together a wrapper header, which includes individual diagnostic kinds in the specific order and explain the relationship between these pieces?"
@kadircet I added a new file DiagnosticsID.inc as a "wrapper header".

[karka228]

Verify the changed include order by adding an assert in getDiagnosticForID below:

  // The requirement for lower_bound to produce a valid result it is
  // enough if the BuiltinDiagnosticsByID is partitioned (by DiagID),
  // but as we want this function to work for all possible values of
  // DiagID sent in as argument it is better to right away check if
  // BuiltinDiagnosticsByID is sorted.
  assert(llvm::is_sorted(BuiltinDiagnosticsByID, orderByID) &&
	 "IDs in BuiltinDiagnosticsByID must be sorted.");

[cor3ntin]

This seems brittle - Maybe we should call std::sort somewhere instead?

[karka228]

This seems brittle - Maybe we should call std::sort somewhere instead?

I agree that it is a bit brittle, but it is hard to find a good place where to insert a std::sort. As @kadircet wrote it is probably better in the future to solve this in a better way in tablegen.

[kadircet]

also it's unclear where the "ground truth" for this order is coming from. AFAICT, order is defined by https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticIDs.h#L49-L65
and enforced by very subtle interactions between tablegen and how particular headers include these.

since this order needs to be preserved in multiple places, can you put together a wrapper header, which includes individual diagnostic kinds in the specific order and explain the relationship between these pieces? (I'd rather have a solution that puts all of this logic into tablegen instead, generating enums with a particular order once, but I can see that's a much bigger change that you might not want to sign up).

[kadircet]

thanks!