[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

79–81

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

79–81

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

89

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

[labath]

lldb/source/Core/Mangled.cpp

89

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

89

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

89

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/source/Core/Mangled.cpp

 }
 
 // Convert to pointer 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; }
+bool Mangled::operator!() 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.