[lldb] Fix PR52702 by fixing bool conversion of Mangled

[rZhBoYao]

Summary

Remove the Mangled::operator! and Mangled::operator void* where the comments in header and implementation files disagree and replace them with operator bool.

This fix PR52702 as https://reviews.llvm.org/D106837 used the buggy Mangled::operator! in Symbol::SynthesizeNameIfNeeded. For example, consider the symbol "puts" in a hello world C program:

// Inside Symbol::SynthesizeNameIfNeeded
(lldb) p m_mangled
(lldb_private::Mangled) $0 = (m_mangled = None, m_demangled = "puts")
(lldb) p !m_mangled
(bool) $1 = true          # should be false!!

This leads to Symbol::SynthesizeNameIfNeeded overwriting m_demangled part of Mangled (in this case "puts").

In conclusion, this patch turns
callq 0x401030 ; symbol stub for: ___lldb_unnamed_symbol36
back into
callq 0x401030 ; symbol stub for: puts .

[clayborg]

We should add a unit test for this to verify this doesn't regress in lldb/unittests/Core/MangledTest.cpp for both the operator bool and the ! operator

lldb/source/Core/Mangled.cpp

78–79

Looks like there is a similar bug here in this function as well. We should convert the convert to pointer function here to "operator bool" and add a unit test.

explicit operator bool() const { return m_mangled || m_demangled; }

[rZhBoYao]

lldb/source/Core/Mangled.cpp

78–79

This change breaks this test: lldb-shell :: SymbolFile/DWARF/x86/find-qualified-variable.cpp because the comment in the header file says: "This allows code to check a Mangled object to see if it contains a valid mangled name", which differs from the comment here, and apparently that's what people's intent was. Modifying lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2143 solves this.

[rZhBoYao]

lldb/source/Core/Mangled.cpp

79

Should we remove Mangled::operator! altogether? I find it less confusing without.

[labath]

lldb/source/Core/Mangled.cpp

79

Does !mangled work without it? Would it be sufficient to implement the ! operator in terms of bool conversion function?

[rZhBoYao]

lldb/source/Core/Mangled.cpp

79

Yes, I tried removing it and the unit test I added (TEST(MangledTest, LogicalNotOperator)) passed. Keeping it this way is fine tho.

[labath]

lldb/source/Core/Mangled.cpp

79

In that case, you should definitely remove it. We don't want to have two implementations that need to be kept in sync.

[labath]

Cool. Thanks for fixing this. Do you need someone to commit this for you?

[rZhBoYao]

Thank you all for spending time reviewing this!

lldb/include/lldb/Core/Mangled.h

bool operator==(const Mangled &rhs) const {
     return m_mangled == rhs.m_mangled &&
            GetDemangledName() == rhs.GetDemangledName();
   }
 
   bool operator!=(const Mangled &rhs) const {
     return !(*this == rhs);
   }
 
-  /// Convert to pointer operator.
+  /// Convert to bool operator.
   ///
-  /// This allows code to check a Mangled object to see if it contains a valid
-  /// mangled name using code such as:
+  /// This allows code to check any Mangled objects to see if they contain
+  /// anything valid using code such as:
   ///
   /// \code
   /// Mangled mangled(...);
   /// if (mangled)
   /// { ...
   /// \endcode
   ///
   /// \return
-  ///     A pointer to this object if either the mangled or unmangled
-  ///     name is set, NULL otherwise.
-  operator void *() const;
-
-  /// Logical NOT operator.
-  ///
-  /// This allows code to check a Mangled object to see if it contains an
-  /// empty mangled name using code such as:
-  ///
-  /// \code
-  /// Mangled mangled(...);
-  /// if (!mangled)
-  /// { ...
-  /// \endcode
-  ///
-  /// \return
-  ///     Returns \b true if the object has an empty mangled and
-  ///     unmangled name, \b false otherwise.
-  bool operator!() const;
+  ///     Returns \b true if either the mangled or unmangled name is set,
+  ///     \b false if the object has an empty mangled and unmangled name.
+  explicit operator bool() const;
 
   /// Clear the mangled and demangled values.
   void Clear();
 
   /// Compare the mangled string values
   ///
   /// Compares the Mangled::GetName() string in \a lhs and \a rhs.
   ///

lldb/source/Core/Mangled.cpp

if (s)
     SetValue(s);
 }
 
 Mangled::Mangled(llvm::StringRef name) {
   if (!name.empty())
     SetValue(ConstString(name));
 }
 
-// Convert to pointer operator. This allows code to check any Mangled objects
+// Convert to bool operator. This allows code to check any Mangled objects
 // to see if they contain anything valid using code such as:
 //
 //  Mangled mangled(...);
 //  if (mangled)
 //  { ...
-Mangled::operator void *() const {
-  return (m_mangled) ? const_cast<Mangled *>(this) : nullptr;
-}
-
-// Logical NOT operator. This allows code to check any Mangled objects to see
-// if they are invalid using code such as:
-//
-//  Mangled mangled(...);
-//  if (!file_spec)
-//  { ...
-bool Mangled::operator!() const { return !m_mangled; }
+Mangled::operator bool() const { return m_mangled || m_demangled; }
 
 // Clear the mangled and demangled values.
 void Mangled::Clear() {
   m_mangled.Clear();
   m_demangled.Clear();
 }
 
 // Compare the string values.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

void SymbolFileDWARF::FindGlobalVariables(
   if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
     return;
 
   // Remember how many variables are in the list before we search.
   const uint32_t original_size = variables.GetSize();
 
   llvm::StringRef basename;
   llvm::StringRef context;
-  bool name_is_mangled = (bool)Mangled(name);
+  bool name_is_mangled = Mangled::GetManglingScheme(name.GetStringRef()) !=
+                         Mangled::eManglingSchemeNone;
 
   if (!CPlusPlusLanguage::ExtractContextAndIdentifier(name.GetCString(),
                                                       context, basename))
     basename = name.GetStringRef();
 
   // Loop invariant: Variables up to this index have been checked for context
   // matches.
   uint32_t pruned_idx = original_size;

lldb/unittests/Core/MangledTest.cpp

 TEST(MangledTest, EmptyForInvalidDLangName) {
   ConstString mangled_name("_DDD");
   Mangled the_mangled(mangled_name);
   ConstString the_demangled = the_mangled.GetDemangledName();
 
   EXPECT_STREQ("", the_demangled.GetCString());
 }
 
+TEST(MangledTest, BoolConversionOperator) {
+  {
+    ConstString MangledName("_ZN1a1b1cIiiiEEvm");
+    Mangled TheMangled(MangledName);
+    EXPECT_EQ(true, bool(TheMangled));
+    EXPECT_EQ(false, !TheMangled);
+  }
+  {
+    ConstString UnmangledName("puts");
+    Mangled TheMangled(UnmangledName);
+    EXPECT_EQ(true, bool(TheMangled));
+    EXPECT_EQ(false, !TheMangled);
+  }
+  {
+    Mangled TheMangled{};
+    EXPECT_EQ(false, bool(TheMangled));
+    EXPECT_EQ(true, !TheMangled);
+  }
+}
 
 TEST(MangledTest, NameIndexes_FindFunctionSymbols) {
   SubsystemRAII<FileSystem, HostInfo, ObjectFileELF, SymbolFileSymtab>
       subsystems;
 
   auto ExpectedFile = TestFile::fromYaml(R"(
 --- !ELF
 FileHeader: