[libcxxabi] Update ItaniumDemangle.h from LLVM

[boomanaiden154]

76ba29b landed changes in ItaniumDemangle.h on the LLVM side that were not propagated over to the libcxxabi side. This patch fixes that.

[llvmbot]

@llvm/pr-subscribers-libcxxabi

Author: Aiden Grossman (boomanaiden154)

Changes

76ba29b landed changes in ItaniumDemangle.h on the LLVM side that were not propagated over to the libcxxabi side. This patch fixes that.

---

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

1 Files Affected:

• (modified) libcxxabi/src/demangle/ItaniumDemangle.h (+4-3)

diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 6acefeea8484b..4e7f92dd1991a 100644
--- a/libcxxabi/src/demangle/ItaniumDemangle.h
+++ b/libcxxabi/src/demangle/ItaniumDemangle.h
@@ -21,6 +21,7 @@
 #include "Utility.h"
 #include <algorithm>
 #include <cctype>
+#include <cstdint>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
@@ -164,18 +165,18 @@ class NodeArray;
 // traversed by the printLeft/Right functions to produce a demangled string.
 class Node {
 public:
-  enum Kind : unsigned char {
+  enum Kind : uint8_t {
 #define NODE(NodeKind) K##NodeKind,
 #include "ItaniumNodes.def"
   };
 
   /// Three-way bool to track a cached value. Unknown is possible if this node
   /// has an unexpanded parameter pack below it that may affect this cache.
-  enum class Cache : unsigned char { Yes, No, Unknown, };
+  enum class Cache : uint8_t { Yes, No, Unknown, };
 
   /// Operator precedence for expression nodes. Used to determine required
   /// parens in expression emission.
-  enum class Prec {
+  enum class Prec : uint8_t {
     Primary,
     Postfix,
     Unary,

[github-actions]

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

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h -- libcxxabi/src/demangle/ItaniumDemangle.h

View the diff from clang-format here.

diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 4e7f92dd1..cee0dd5d4 100644
--- a/libcxxabi/src/demangle/ItaniumDemangle.h
+++ b/libcxxabi/src/demangle/ItaniumDemangle.h
@@ -172,7 +172,11 @@ public:
 
   /// Three-way bool to track a cached value. Unknown is possible if this node
   /// has an unexpanded parameter pack below it that may affect this cache.
-  enum class Cache : uint8_t { Yes, No, Unknown, };
+  enum class Cache : uint8_t {
+    Yes,
+    No,
+    Unknown,
+  };
 
   /// Operator precedence for expression nodes. Used to determine required
   /// parens in expression emission.

[d0k]

Thanks!

[ojhunt]

@boomanaiden154 oh thanks, I didn't realize there was a clone of that file :-/ did you just know to watch for such changes or is there a test suite that catches it?

[boomanaiden154]

oh thanks, I didn't realize there was a clone of that file :-/ did you just know to watch for such changes or is there a test suite that catches it?

You added a dependency on llvm/Support so this ended up coming up during the Google integrate process and this stuck out to me when looking at the commit given I know how they're duplicated. I've been meaning to write a test to assert that they're the same but have not yet gotten around to it. I'll try and see if I can get a patch up today.

[ojhunt]

Oh, but the final version didn't need LLVM_PREFERRED_TYPE so we can remove that dependency (there were some fields that had been converted to unsigned, but in the end I changed the storage types of the enum instead, so we can remove that inclusion - just cleaning up an unneeded dependency on principle

[boomanaiden154]

That was fixed in 71d1b4c.

Here it's not just cleaning up an unneeded dependency. It's a layering violation to include from llvm/Support in ItaniumDemangle.h.

[ojhunt]

oh good to know :-O

[nico]

(There's libcxxabi/src/demangle/cp-to-llvm.sh to copy files over, but it's manual.)

[ojhunt]

That was fixed in 71d1b4c.

Here it's not just cleaning up an unneeded dependency. It's a layering violation to include from llvm/Support in ItaniumDemangle.h.

@boomanaiden154 was the issue including it in the lib headers? If so I wonder if we can make that trigger a style or bot error in future (if it already was reported I must have missed it because the format bot was already complaining about existing style so I didn't read the entire output :-/ )

[boomanaiden154]

was the issue including it in the lib headers?

Not sure what you mean here by lib headers. The issue was that ItaniumDemangle.h now referenced a header in llvm/Support which is not allowed. It is probably fine in LLVM given everything that includes ItaniumDemangle.h presumably also links/includes llvm/Support, but this is not the case in libcxxabi where the files need to match.

Layering violations in general are somewhat difficult to catch automatically. bazel has explicit checks for it, but CMake does not. I think they sometimes pop up in shared library builds, but not necessarily. This one is basically impossible to catch through a build failure. Automation to ensure the libcxxabi and llvm versions match though should be sufficient for this case as you would get a build error if a file in libcxxabi tried to include something from llvm/.

[ojhunt]

was the issue including it in the lib headers?

Not sure what you mean here by lib headers. The issue was that ItaniumDemangle.h now referenced a header in llvm/Support which is not allowed. It is probably fine in LLVM given everything that includes ItaniumDemangle.h presumably also links/includes llvm/Support, but this is not the case in libcxxabi where the files need to match.

Layering violations in general are somewhat difficult to catch automatically. bazel has explicit checks for it, but CMake does not. I think they sometimes pop up in shared library builds, but not necessarily. This one is basically impossible to catch through a build failure. Automation to ensure the libcxxabi and llvm versions match though should be sufficient for this case as you would get a build error if a file in libcxxabi tried to include something from llvm/.

Sorry, the problem is that from my pov ItaniumMangle was in llvm, so I'm trying to understand what the rules for limiting its inclusion of llvm headers.

By lib headers, I meant ItaniumMangle is in the llvm/include/llvm directory, and it sounds like that's for api/library headers, so including things like llvm/Support isn't safe/allowed as those are internal/implementation headers.

Is that correct?

[boomanaiden154]

Sorry, the problem is that from my pov ItaniumMangle was in llvm, so I'm trying to understand what the rules for limiting its inclusion of llvm headers.

It's the same across libcxxabi and LLVM (or at least should be), so cannot depend on stuff within LLVM.

By lib headers, I meant ItaniumMangle is in the llvm/include/llvm directory, and it sounds like that's for api/library headers, so including things like llvm/Support isn't safe/allowed as those are internal/implementation headers.

Is that correct?

You should be fine including things in llvm/include/llvm with the obvious exceptions like you cannot create circular dependencies. Demangling is a special case given it's shared across the monorepo.

[ojhunt]

You should be fine including things in llvm/include/llvm with the obvious exceptions like you cannot create circular dependencies. Demangling is a special case given it's shared across the monorepo.

Ah, I hadn't missed a general rule, there are just some cases where files are copied, thanks :D