Page MenuHomePhabricator

[lldb] Fix PR52702 by fixing bool conversion of Mangled

Authored by rZhBoYao on Dec 23 2021, 4:32 AM.



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 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 .

Diff Detail

Event Timeline

rZhBoYao requested review of this revision.Dec 23 2021, 4:32 AM
rZhBoYao created this revision.
clayborg requested changes to this revision.Dec 23 2021, 8:38 AM

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


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; }
This revision now requires changes to proceed.Dec 23 2021, 8:38 AM
rZhBoYao added inline comments.Dec 26 2021, 8:10 PM

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 updated this revision to Diff 396261.Dec 26 2021, 9:09 PM
rZhBoYao updated this revision to Diff 396263.Dec 26 2021, 9:27 PM
rZhBoYao added inline comments.Dec 26 2021, 9:30 PM

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

rZhBoYao marked an inline comment as done and an inline comment as not done.Dec 26 2021, 9:31 PM
labath added a subscriber: labath.Dec 28 2021, 5:41 AM
labath added inline comments.

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

rZhBoYao added inline comments.Dec 28 2021, 6:57 AM

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

labath added inline comments.Dec 28 2021, 7:06 AM

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

rZhBoYao updated this revision to Diff 396395.Dec 28 2021, 7:36 AM
rZhBoYao retitled this revision from [lldb] Fix PR52702 by fixing Mangled::operator! to [lldb] Fix PR52702 by fixing bool conversion of Mangled.
rZhBoYao edited the summary of this revision. (Show Details)
rZhBoYao marked 2 inline comments as done.
labath accepted this revision.Dec 29 2021, 1:10 AM

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

This revision was not accepted when it landed; it landed in state Needs Review.Dec 29 2021, 1:19 AM
This revision was automatically updated to reflect the committed changes.

Thank you all for spending time reviewing this!