WIP: Add error handling to demangle API

[thieta]

Summary

As discussed in https://reviews.llvm.org/D138595 - the current demangler API
can't really report more detailed errors. This is an attempt to implement
that using std::optional and std::error_code

[thieta]

@dblaikie @jhenderson This is an attempt to implement the API discussed in the other Diff using std::error_code and std::optional. Let me know what you think before I convert the rest of the methods to have similar error handling.

I also think the demangle() and nonMicrosoftDemangle() should probably follow the same API if we decide this is what we want.

[dblaikie]

llvm/lib/Demangle/Demangle.cpp

75–78

Part of what I was hoping for was that we wouldn't need an explicit isItaniumEncoding check, even inside the implementation - that Demangler::parse could produce that as a separate error result if it parsed something without the desired prefix. Would that be possible?

[jhenderson]

llvm/include/llvm/Demangle/Demangle.h

27

I generally support the use of enum class, but the enums are generally being cast to an int, so maybe that's a sign that this should just be a plain enum?

28

Nit: comments should end with "." (here and below).

53

Nit: capitalization here is inconsistent.

FWIW, the LLVM coding standards state for warning and error messages to a) start with a lower case letter, and b) not to end with a period. Whether that applies in this context is debatable.

66

I'd just overload the function name, rather than appending "WithError". The fact that it takes a std::error_code reference indicates that it will do something to that field, and it's then similar to e.g. how std::filesystem overloads function.

llvm/lib/Demangle/Demangle.cpp

75–78

Not sure if this is what @dblaikie is suggesting, but it seems to me that you could have the underlying demangling code set a status of "NotThisFormat" or similar if it encounters something with the wrong prefix type. This would enable calling code to know whether to try the next format, or just stop. That might not work so well in the face of type demangling though.

[jhenderson]

Oh, and just to mention that this week is my last week working before a 6 week break, so I may not have the time to review/comment much more on this.

[thieta]

llvm/include/llvm/Demangle/Demangle.h

27

Ideally we should change the api to not return a int status from the demangling function instead using DemangleError() - as I noted in my other comment, I was trying to preserve the old API because I thought it was wide-spread. But that doesn't seem to be the case, so I might end up just fixing that instead of making this a normal int.

53

Hmm seems like it should apply here, I will make it consistent.

66

Agreed.

llvm/lib/Demangle/Demangle.cpp

75–78

We can do that - but it becomes a little more complex and we probably need to audit all the places where the demangling functions are used now in that case.

itaninumDemangle() calls: AbstractManglingParser::parse() which returns nullptr if there is any errors, but have no status reports here.

So we could either:

1. change the AbstractManglingParser API to return the error_code from there instead. This seems possible since it's only used internally in the itaniumDemangler - it's not exposed from there (wonder why the implementation lives in a header in that case). The function also seems to be aware when it's not starting with __Z (or rather it tries to find *Z and we can probably return InvalidFormat if it doesn't). We could change this and either keep the current itaniumDemangle() function which will change the error_code to the current status int.
2. change itaniumDemangle() to do something similar to what isItaniumEncoding does. but this seems to just move the responsibility for it.

I was going to say that it's harder to change the API of itaniumDemangle since it's used "everywhere" - but that doesn't seem to be true - searching for it only seems to happen in 10 places:

https://github.com/search?q=repo%3Allvm%2Fllvm-project+itaniumDemangle%28+language%3AC%2B%2B&type=code&l=C%2B%2B

I am going to give it a try to move the logic to AbstractManglingParser to return a error_code instead and see if it can work.

[thieta]

In D139298#3973355, @jhenderson wrote:

Oh, and just to mention that this week is my last week working before a 6 week break, so I may not have the time to review/comment much more on this.

Enjoy your vacation!

[thieta]

rebased and implemented some of the suggestions.

[thieta]

Hi - I finally got around to picking this one up again. This is rebased, passing all tests and now we pass the std::error_code directly to the API and changed all the callers to itaniumDemangle() and changed most of the places to actually check the error_code instead of the return const char*.

What do you guys think about this API change? If we like this in general, I can start changing the other demangle calls, either as part of this diff or in separate diffs per demangle API.

[thieta]

llvm/include/llvm/Demangle/Demangle.h

53

I missed this - but will fix in the next revision of the patch.

llvm/tools/llvm-undname/llvm-undname.cpp

80 ↗

(On Diff #496059)

just a temp change until I do the microsoftDemangle api.

[MaskRay]

Adding std::error_code looks good.

This is an attempt to implement that using std::optional and std::error_code

I don't see std::optional in the patch :)

llvm/include/llvm/Demangle/Demangle.h

53

If there isn't a precedent, as mentioned, no-capitalization is preferred (https://llvm.org/docs/CodingStandards.html#error-and-warning-messages).

Also, no-capitalization makes the message easily composable with a function that appends a prefix.

[dblaikie]

Could make the error_code out parameter optional (take by pointer, populate if non-null, have a null default parameter value) - then callers who don't benefit from/care about the specific type can continue using the null pointer result signal?

llvm/lib/ProfileData/ItaniumManglingCanonicalizer.cpp

290–296 ↗

(On Diff #496059)

This could be changed to parse, check the error and then do the make<NameType>(StringView thing if the parse failed as invalid, maybe? (potentially in a follow-up commit)

[jhenderson]

llvm/unittests/Demangle/DemangleTest.cpp

38

Here and below, can you use EXPECT_EQ(E, DemangleError::Success) (or equivalent)? This provides nicer error messages since it gives the actual value if the two aren't the same.

41

The comment seems to be wrapped a bit early too?

Also applies below.

llvm/include/llvm/Demangle/Demangle.h

 //===--- Demangle.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEMANGLE_DEMANGLE_H
 #define LLVM_DEMANGLE_DEMANGLE_H
 
 #include <cstddef>
 #include <string>
+#include <system_error>
 
 namespace llvm {
 /// This is a llvm local version of __cxa_demangle. Other than the name and
 /// being in the llvm namespace it is identical.
 ///
 /// The mangled_name is demangled into buf and returned. If the buffer is not
 /// large enough, realloc is used to expand it.
 ///
 /// The *status will be set to a value from the following enumeration
-enum : int {
-  demangle_unknown_error = -4,
-  demangle_invalid_args = -3,
-  demangle_invalid_mangled_name = -2,
-  demangle_memory_alloc_failure = -1,
-  demangle_success = 0,
+
+const std::error_category &demangleCategory();
+
+enum class DemangleError {
+  InvalidFormat = -5, // When a specific demangler can't handle this string
+  Unknown = -4,
+  InvalidArgs = -3, // Null args or other problems with input
+  InvalidMangledName =
+      -2, // Looks like a mangled string we can handle, but can't
+  MemoryAllocFailure = -1, // Any allocation failure.
+  Success = 0,
 };
 
+class DemangleErrorCategory : public std::error_category {
+public:
+  const char *name() const noexcept override { return "llvm.demangle"; }
+  std::string message(int EV) const override {
+    switch (static_cast<llvm::DemangleError>(EV)) {
+    case llvm::DemangleError::InvalidFormat:
+      return "Invalid mangled format.";
+    case llvm::DemangleError::InvalidArgs:
+      return "Invalid arguments.";
+    case llvm::DemangleError::InvalidMangledName:
+      return "Invalid mangled name.";
+    case llvm::DemangleError::MemoryAllocFailure:
+      return "Failed to allocate memory.";
+    case llvm::DemangleError::Unknown:
+      return "Unknown error.";
+    case llvm::DemangleError::Success:
+      return "success.";
+    }
+  };
+
+  bool equivalent(const std::error_code &E, int DE) const noexcept override {
+    return *this == E.category() && static_cast<int>(E.value()) == DE;
+  }
+};
+
+inline std::error_code make_error_code(llvm::DemangleError Err) {
+  return std::error_code(static_cast<int>(Err), llvm::demangleCategory());
+}
+
+std::optional<const char *> itaniumDemangleWithError(const char *M,
+                                                     std::error_code &E);
+
 char *itaniumDemangle(const char *mangled_name, char *buf, size_t *n,
                       int *status);
 
 enum MSDemangleFlags {
   MSDF_None = 0,
   MSDF_DumpBackrefs = 1 << 0,
   MSDF_NoAccessSpecifier = 1 << 1,
   MSDF_NoCallingConvention = 1 << 2,
struct ItaniumPartialDemangler {
   ~ItaniumPartialDemangler();
 
 private:
   void *RootNode;
   void *Context;
 };
 } // namespace llvm
 
+namespace std {
+template <> struct is_error_code_enum<llvm::DemangleError> : std::true_type {};
+} // namespace std
+
 #endif

llvm/lib/Demangle/Demangle.cpp

bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
 
   if (!Demangled)
     return false;
 
   Result = Demangled;
   std::free(Demangled);
   return true;
 }
+
+const std::error_category &llvm::demangleCategory() {
+  static llvm::DemangleErrorCategory E;
+  return E;
+}
+
+std::optional<const char *> llvm::itaniumDemangleWithError(const char *M,
+                                                           std::error_code &E) {
+  E = make_error_code(DemangleError::Success);
+
+  if (!isItaniumEncoding(M)) {
+    E = llvm::make_error_code(DemangleError::InvalidFormat);
+    return {};
+  }
+
+  int Status = (int)DemangleError::Unknown;
+  if (auto *D = itaniumDemangle(M, nullptr, nullptr, &Status)) {
+    return D;
+  }
+
+  E = llvm::make_error_code((DemangleError)Status);
+  return {};
+}

llvm/lib/Demangle/ItaniumDemangle.cpp

 //===----------------------------------------------------------------------===//
 
 using Demangler = itanium_demangle::ManglingParser<DefaultAllocator>;
 
 char *llvm::itaniumDemangle(const char *MangledName, char *Buf,
                             size_t *N, int *Status) {
   if (MangledName == nullptr || (Buf != nullptr && N == nullptr)) {
     if (Status)
-      *Status = demangle_invalid_args;
+      *Status = (int)DemangleError::InvalidArgs;
     return nullptr;
   }
 
-  int InternalStatus = demangle_success;
+  int InternalStatus = (int)DemangleError::Success;
   Demangler Parser(MangledName, MangledName + std::strlen(MangledName));
   Node *AST = Parser.parse();
 
   if (AST == nullptr)
-    InternalStatus = demangle_invalid_mangled_name;
+    InternalStatus = (int)DemangleError::InvalidMangledName;
   else {
     OutputBuffer OB(Buf, N);
     assert(Parser.ForwardTemplateRefs.empty());
     AST->print(OB);
     OB += '\0';
     if (N != nullptr)
       *N = OB.getCurrentPosition();
     Buf = OB.getBuffer();
   }
 
   if (Status)
     *Status = InternalStatus;
-  return InternalStatus == demangle_success ? Buf : nullptr;
+  return InternalStatus == (int)DemangleError::Success ? Buf : nullptr;
 }
 
 ItaniumPartialDemangler::ItaniumPartialDemangler()
     : RootNode(nullptr), Context(new Demangler{nullptr, nullptr}) {}
 
 ItaniumPartialDemangler::~ItaniumPartialDemangler() {
   delete static_cast<Demangler *>(Context);
 }

llvm/lib/Demangle/MicrosoftDemangle.cpp

if (Flags & MSDF_NoAccessSpecifier)
     OF = OutputFlags(OF | OF_NoAccessSpecifier);
   if (Flags & MSDF_NoReturnType)
     OF = OutputFlags(OF | OF_NoReturnType);
   if (Flags & MSDF_NoMemberType)
     OF = OutputFlags(OF | OF_NoMemberType);
   if (Flags & MSDF_NoVariableType)
     OF = OutputFlags(OF | OF_NoVariableType);
 
-  int InternalStatus = demangle_success;
+  int InternalStatus = (int)DemangleError::Success;
   if (D.Error)
-    InternalStatus = demangle_invalid_mangled_name;
+    InternalStatus = (int)DemangleError::InvalidMangledName;
   else {
     OutputBuffer OB(Buf, N);
     AST->output(OB, OF);
     OB += '\0';
     if (N != nullptr)
       *N = OB.getCurrentPosition();
     Buf = OB.getBuffer();
   }
 
   if (Status)
     *Status = InternalStatus;
-  return InternalStatus == demangle_success ? Buf : nullptr;
+  return InternalStatus == (int)DemangleError::Success ? Buf : nullptr;
 }

llvm/unittests/Demangle/DemangleTest.cpp

 //===-- DemangleTest.cpp --------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Demangle/Demangle.h"
 #include "gmock/gmock.h"
+#include <system_error>
 
 using namespace llvm;
 
 TEST(Demangle, demangleTest) {
   EXPECT_EQ(demangle("_"), "_");
   EXPECT_EQ(demangle("_Z3fooi"), "foo(int)");
   EXPECT_EQ(demangle("__Z3fooi"), "foo(int)");
   EXPECT_EQ(demangle("___Z3fooi_block_invoke"),
             "invocation function for block in foo(int)");
   EXPECT_EQ(demangle("____Z3fooi_block_invoke"),
             "invocation function for block in foo(int)");
   EXPECT_EQ(demangle("?foo@@YAXH@Z"), "void __cdecl foo(int)");
   EXPECT_EQ(demangle("foo"), "foo");
   EXPECT_EQ(demangle("_RNvC3foo3bar"), "foo::bar");
   EXPECT_EQ(demangle("__RNvC3foo3bar"), "foo::bar");
   EXPECT_EQ(demangle("_Dmain"), "D main");
 
   // Regression test for demangling of optional template-args for vendor
   // extended type qualifier (https://bugs.llvm.org/show_bug.cgi?id=48009)
   EXPECT_EQ(demangle("_Z3fooILi79EEbU7_ExtIntIXT_EEi"),
             "bool foo<79>(int _ExtInt<79>)");
 }
+
+TEST(Demangle, demangleWithError) {
+  std::error_code E;
+  EXPECT_TRUE(itaniumDemangleWithError("_Z3fooi", E));
+  EXPECT_TRUE(E == DemangleError::Success);
+
+  // A string that shouldn't be handled by this demangler
+  // should return InvaildFormat
+  EXPECT_FALSE(itaniumDemangleWithError("_", E));
+  EXPECT_TRUE(E == DemangleError::InvalidFormat);
+
+  // A string that should be handled by this demangler
+  // but is broken should return InvalidMangledNamed
+  EXPECT_FALSE(itaniumDemangleWithError("_Z3fooBroken@Y", E));
+  EXPECT_TRUE(E == DemangleError::InvalidMangledName);
+}