[clang] Use string tables for static diagnostic descriptions

[froydnj]

Summary

Using a pointer for the description string in StaticDiagInfoRec causes several problems:

1. We don't need to use a whole pointer to represent the string;
2. The use of pointers incurs runtime relocations for those pointers; the relocations take up space on disk and represent runtime overhead;
3. The need to relocate data implies that, on some platforms, the entire array containing StaticDiagInfoRecs cannot be shared between processes.

This patch changes the storage scheme for the diagnostic descriptions to
avoid these problems. We instead generate (effectively) one large
string and then StaticDiagInfoRec conceptually holds offsets into the
string. We elected to also move the storage of those offsets into a
separate array to further reduce the space required.

On x86-64 Linux, this change removes about 120KB of relocations and
moves about 60KB from the non-shareable .data.rel.ro section to
shareable .rodata. (The array is about 80KB before this, but we
eliminated 4 bytes/entry by using offsets rather than pointers.) We
actually reap this benefit twice, because these tables show up in both
libclang.so and libclang-cpp.so and we get the reduction in both places.

[froydnj]

I'd like to apply the same sort of techniques to the builtin tables, which are enormous, but this is baby steps.

[froydnj]

Ping.

[riccibruno]

Add some reviewers based on git diff --name-only | xargs -n 1 git blame --porcelain | grep "^author " | sort | uniq -c | sort -nr | head -30.

[bkramer]

Nice, those relocations have annoyed me for years. I'm worried about whether the way you're accessing StaticDiagInfoDescriptionStringTable might be undefined behavior. I won't block this change on that though.

[froydnj]

In D81865#2176014, @bkramer wrote:

Nice, those relocations have annoyed me for years. I'm worried about whether the way you're accessing StaticDiagInfoDescriptionStringTable might be undefined behavior. I won't block this change on that though.

Is there somebody who should review that particular bit? My understanding is that it is OK, but my understanding of the C++ standard is not necessarily complete, and I'd like to get a second opinion.

[dblaikie]

In D81865#2176148, @froydnj wrote:

In D81865#2176014, @bkramer wrote:

Nice, those relocations have annoyed me for years. I'm worried about whether the way you're accessing StaticDiagInfoDescriptionStringTable might be undefined behavior. I won't block this change on that though.

Is there somebody who should review that particular bit? My understanding is that it is OK, but my understanding of the C++ standard is not necessarily complete, and I'd like to get a second opinion.

I believe this falls under the (using cppreference ( https://en.cppreference.com/w/cpp/language/union ) , since it's a bit easier to read) UB clause: " It's undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union."

Last member written to was the "StringTable" member, but then it's read from the "String" member, so that'd be UB. Commonly supported, but UB - not sure if we have a general statement that we depend on this behavior in LLVM, even though it's non-standard (but it's possible that we do make such an assumption about the compiler that's building LLVM). It'd be nice to avoid that, though - and not too difficult, I think - I /believe/ it's valid to take a pointer to an object, cast it to char*, compute a pointer to some specific member and then cast it back to the right type and access. But I could be wrong there. @rsmith would be the person to give an authoritative answer.

[froydnj]

In D81865#2176589, @dblaikie wrote:

I believe this falls under the (using cppreference ( https://en.cppreference.com/w/cpp/language/union ) , since it's a bit easier to read) UB clause: " It's undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union."

Last member written to was the "StringTable" member, but then it's read from the "String" member, so that'd be UB. Commonly supported, but UB - not sure if we have a general statement that we depend on this behavior in LLVM, even though it's non-standard (but it's possible that we do make such an assumption about the compiler that's building LLVM). It'd be nice to avoid that, though - and not too difficult, I think - I /believe/ it's valid to take a pointer to an object, cast it to char*, compute a pointer to some specific member and then cast it back to the right type and access. But I could be wrong there. @rsmith would be the person to give an authoritative answer.

Thanks for the explanation. Is the language of "writing" applicable here, given that this is a constant blob of storage? (I suppose the compiler is permitted to designate a particular member as having been "written"?)

Would it be more palatable to write:

struct StaticDiagInfoDescriptionStringTable {
  // members as char[] for each diagnostic
};

const StaticDiagInfoDescriptionStringTable StaticDiagInfoDescriptions = {
  // define all the members
};

...

struct StaticDiagInfoRec {
  ...
  StringRef getDescription() const {
    size_t MyIndex = this - &StaticDiagInfo[0];
    uint32_t StringOffset = StaticDiagInfoDescriptionOffsets[MyIndex];
    // Defined as a pointer to the first member, and (presumably) there is no internal padding.
    const char *StringTable = reinterpret_cast<const char*>(&StaticDiagInfoDescriptions);
    return StringRef(&StringTable[StringOffset], DescriptionLen);
};

and then we don't have to care about how the host compiler interprets access to different members of unions?

[dblaikie]

In D81865#2178542, @froydnj wrote:

In D81865#2176589, @dblaikie wrote:

I believe this falls under the (using cppreference ( https://en.cppreference.com/w/cpp/language/union ) , since it's a bit easier to read) UB clause: " It's undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union."

Last member written to was the "StringTable" member, but then it's read from the "String" member, so that'd be UB. Commonly supported, but UB - not sure if we have a general statement that we depend on this behavior in LLVM, even though it's non-standard (but it's possible that we do make such an assumption about the compiler that's building LLVM). It'd be nice to avoid that, though - and not too difficult, I think - I /believe/ it's valid to take a pointer to an object, cast it to char*, compute a pointer to some specific member and then cast it back to the right type and access. But I could be wrong there. @rsmith would be the person to give an authoritative answer.

Thanks for the explanation. Is the language of "writing" applicable here, given that this is a constant blob of storage? (I suppose the compiler is permitted to designate a particular member as having been "written"?)

I /believe/ it is applicable, though I could be wrong.

Ah, cppreference's example supports that theory at least: S s = {0x12345678}; // initializes the first member, s.n is now the active member - that the initialization itself does set the active member of the union.

(oh, and a later example on cppreference, which is lifted from the C++ spec (though not sure exactly which version) says similarly: "Y y = { { 1, 2 } }; // OK, y.x is active union member (9.2)")

Would it be more palatable to write:

struct StaticDiagInfoDescriptionStringTable {
  // members as char[] for each diagnostic
};

const StaticDiagInfoDescriptionStringTable StaticDiagInfoDescriptions = {
  // define all the members
};

...

struct StaticDiagInfoRec {
  ...
  StringRef getDescription() const {
    size_t MyIndex = this - &StaticDiagInfo[0];
    uint32_t StringOffset = StaticDiagInfoDescriptionOffsets[MyIndex];
    // Defined as a pointer to the first member, and (presumably) there is no internal padding.
    const char *StringTable = reinterpret_cast<const char*>(&StaticDiagInfoDescriptions);
    return StringRef(&StringTable[StringOffset], DescriptionLen);
};

and then we don't have to care about how the host compiler interprets access to different members of unions?

I think so? I guess that's essentially the point of offsetof.

The comment about padding probably isn't needed, I think? Even if there was padding, the StringOffset comes from "offsetof" so it describes the offset including any padding involved?

Be great if @rsmith got a chance to weigh in here.

[froydnj]

Ping for an opinion on the recent discussion of how to move forward with this patch.

[froydnj]

Ping for an opinion on the recent discussion of how to move forward with this patch.

[froydnj]

Updated to use a struct instead of a union for the actual table in an effort to avoid UB.

[froydnj]

Throwing this back into the review queue.

[dblaikie]

Sounds good to me - thanks!

[MaskRay]

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

[dblaikie]

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

[MaskRay]

In D81865#2293066, @dblaikie wrote:

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

Sorry for the noise. What I saw previously was a mere difference in the DIAG macro and the new isDeferable... Maybe Phab presented the diff between two Diffs to me. The updated view seems good.

[froydnj]

In D81865#2293146, @MaskRay wrote:

In D81865#2293066, @dblaikie wrote:

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

Sorry for the noise. What I saw previously was a mere difference in the DIAG macro and the new isDeferable... Maybe Phab presented the diff between two Diffs to me. The updated view seems good.

I assumed that "add another parameter to a macro due to rebasing" was not a significant enough change to warrant reposting...but as this is the first patch I was committing myself, I probably should have been a bit more explicit in what I was committing (even re-asking for review? I'm not sure of the norms around rebasing in the LLVM project). My mistake!

[MaskRay]

In D81865#2293158, @froydnj wrote:

In D81865#2293146, @MaskRay wrote:

In D81865#2293066, @dblaikie wrote:

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

Sorry for the noise. What I saw previously was a mere difference in the DIAG macro and the new isDeferable... Maybe Phab presented the diff between two Diffs to me. The updated view seems good.

I assumed that "add another parameter to a macro due to rebasing" was not a significant enough change to warrant reposting...but as this is the first patch I was committing myself, I probably should have been a bit more explicit in what I was committing (even re-asking for review? I'm not sure of the norms around rebasing in the LLVM project). My mistake!

I think that is fine. It seems that you just rebased on top of Recommit "[CUDA][HIP] Defer overloading resolution diagnostics for host device functions" (which actually added isDeferable). I somewhat read the diff between two Diffs and found it different from the commit and commented here. Sorry!

clang/lib/Basic/DiagnosticIDs.cpp

 using namespace clang;
 
 //===----------------------------------------------------------------------===//
 // Builtin Diagnostic information
 //===----------------------------------------------------------------------===//
 
 namespace {
 
+struct StaticDiagInfoRec;
+
+// Store the descriptions in a separate table to avoid pointers that need to
+// be relocated, and also decrease the amount of data needed on 64-bit
+// platforms. See "How To Write Shared Libraries" by Ulrich Drepper.
+struct StaticDiagInfoDescriptionStringTable {
+#define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR,     \
+             SHOWINSYSHEADER, DEFERRABLE, CATEGORY)                            \
+  char ENUM##_desc[sizeof(DESC)];
+  // clang-format off
+#include "clang/Basic/DiagnosticCommonKinds.inc"
+#include "clang/Basic/DiagnosticDriverKinds.inc"
+#include "clang/Basic/DiagnosticFrontendKinds.inc"
+#include "clang/Basic/DiagnosticSerializationKinds.inc"
+#include "clang/Basic/DiagnosticLexKinds.inc"
+#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"
+  // clang-format on
+#undef DIAG
+};
+
+const StaticDiagInfoDescriptionStringTable StaticDiagInfoDescriptions = {
+#define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR,     \
+             SHOWINSYSHEADER, DEFERRABLE, CATEGORY)                            \
+  DESC,
+  // clang-format off
+#include "clang/Basic/DiagnosticCommonKinds.inc"
+#include "clang/Basic/DiagnosticDriverKinds.inc"
+#include "clang/Basic/DiagnosticFrontendKinds.inc"
+#include "clang/Basic/DiagnosticSerializationKinds.inc"
+#include "clang/Basic/DiagnosticLexKinds.inc"
+#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"
+  // clang-format on
+#undef DIAG
+};
+
+extern const StaticDiagInfoRec StaticDiagInfo[];
+
+// Stored separately from StaticDiagInfoRec to pack better.  Otherwise,
+// StaticDiagInfoRec would have extra padding on 64-bit platforms.
+const uint32_t StaticDiagInfoDescriptionOffsets[] = {
+#define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR,     \
+             SHOWINSYSHEADER, DEFERRABLE, CATEGORY)                            \
+  offsetof(StaticDiagInfoDescriptionStringTable, ENUM##_desc),
+  // clang-format off
+#include "clang/Basic/DiagnosticCommonKinds.inc"
+#include "clang/Basic/DiagnosticDriverKinds.inc"
+#include "clang/Basic/DiagnosticFrontendKinds.inc"
+#include "clang/Basic/DiagnosticSerializationKinds.inc"
+#include "clang/Basic/DiagnosticLexKinds.inc"
+#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"
+  // clang-format on
+#undef DIAG
+};
+
 // Diagnostic classes.
 enum {
   CLASS_NOTE       = 0x01,
   CLASS_REMARK     = 0x02,
   CLASS_WARNING    = 0x03,
   CLASS_EXTENSION  = 0x04,
   CLASS_ERROR      = 0x05
 };
 
 struct StaticDiagInfoRec {
   uint16_t DiagID;
   unsigned DefaultSeverity : 3;
   unsigned Class : 3;
   unsigned SFINAE : 2;
   unsigned WarnNoWerror : 1;
   unsigned WarnShowInSystemHeader : 1;
   unsigned Deferrable : 1;
   unsigned Category : 6;
 
   uint16_t OptionGroupIndex;
 
   uint16_t DescriptionLen;
-  const char *DescriptionStr;
 
   unsigned getOptionGroupIndex() const {
     return OptionGroupIndex;
   }
 
   StringRef getDescription() const {
-    return StringRef(DescriptionStr, DescriptionLen);
+    size_t MyIndex = this - &StaticDiagInfo[0];
+    uint32_t StringOffset = StaticDiagInfoDescriptionOffsets[MyIndex];
+    const char* Table = reinterpret_cast<const char*>(&StaticDiagInfoDescriptions);
+    return StringRef(&Table[StringOffset], DescriptionLen);
   }
 
   diag::Flavor getFlavor() const {
     return Class == CLASS_REMARK ? diag::Flavor::Remark
                                  : diag::Flavor::WarningOrError;
   }
 
   bool operator<(const StaticDiagInfoRec &RHS) const {

 VALIDATE_DIAG_SIZE(COMMENT)
 VALIDATE_DIAG_SIZE(CROSSTU)
 VALIDATE_DIAG_SIZE(SEMA)
 VALIDATE_DIAG_SIZE(ANALYSIS)
 VALIDATE_DIAG_SIZE(REFACTORING)
 #undef VALIDATE_DIAG_SIZE
 #undef STRINGIFY_NAME
 
-} // namespace anonymous
-
-static const StaticDiagInfoRec StaticDiagInfo[] = {
+const StaticDiagInfoRec StaticDiagInfo[] = {
 #define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR,     \
              SHOWINSYSHEADER, DEFERRABLE, CATEGORY)                            \
-  {diag::ENUM, DEFAULT_SEVERITY,         CLASS,      DiagnosticIDs::SFINAE,    \
-   NOWERROR,   SHOWINSYSHEADER,          DEFERRABLE, CATEGORY,                 \
-   GROUP,      STR_SIZE(DESC, uint16_t), DESC},
+  {                                                                            \
+      diag::ENUM,                                                              \
+      DEFAULT_SEVERITY,                                                        \
+      CLASS,                                                                   \
+      DiagnosticIDs::SFINAE,                                                   \
+      NOWERROR,                                                                \
+      SHOWINSYSHEADER,                                                         \
+      DEFERRABLE,                                                              \
+      CATEGORY,                                                                \
+      GROUP,                                                                   \
+      STR_SIZE(DESC, uint16_t)},
+  // clang-format off
 #include "clang/Basic/DiagnosticCommonKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
 #include "clang/Basic/DiagnosticFrontendKinds.inc"
 #include "clang/Basic/DiagnosticSerializationKinds.inc"
 #include "clang/Basic/DiagnosticLexKinds.inc"
 #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"
+  // clang-format on
 #undef DIAG
 };
 
+} // namespace
+
 static const unsigned StaticDiagInfoSize = llvm::array_lengthof(StaticDiagInfo);
 
 /// GetDiagInfo - Return the StaticDiagInfoRec entry for the specified DiagID,
 /// or null if the ID is invalid.
 static const StaticDiagInfoRec *GetDiagInfo(unsigned DiagID) {
   // Out of bounds diag. Can't be in the table.
   using namespace diag;
   if (DiagID >= DIAG_UPPER_LIMIT || DiagID <= DIAG_START_COMMON)