This is an archive of the discontinued LLVM Phabricator instance.

[llvm] lldb abi_tag support 1/3 - Add equality comparison API to itanium_demangle::Node
Needs ReviewPublic

Authored by Michael137 on Aug 6 2022, 10:41 AM.

Details

Reviewers
aprantl
ldionne
urnathan
Group Reviewers
Restricted Project
Summary

In LLDB we currently rely on hand-parsing demangled function
prototypes when selecting a candidate symbol for expression evaluation.
We now plan to use the Itanium mangle tree API to compare function
encodings instead.

This patch adds the ability to compare nodes of the Itanium mangle tree.

Testing

  • Added unit-tests for each Node's new operator==

Diff Detail

Event Timeline

Michael137 created this revision.Aug 6 2022, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 10:41 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
  • Fixed PointerToMemberType::operator==
  • Add necessary null-checks
Michael137 added a comment.EditedAug 6 2022, 11:28 AM

Planning to add unit-tests once we are fine with the approach in https://reviews.llvm.org/D131335

Michael137 added a reviewer: urnathan.
Michael137 updated this revision to Diff 450970.Aug 8 2022, 3:21 PM
  • Fix NodeArray::operator==()
  • Fix FunctionEncoding::operator==()
  • Start adding operator!=()
Michael137 updated this revision to Diff 450974.Aug 8 2022, 3:26 PM
  • Start adding unit-tests
Michael137 updated this revision to Diff 450977.Aug 8 2022, 3:29 PM
  • Remove debug logging
Michael137 published this revision for review.Aug 8 2022, 3:32 PM

Still WIP, but general approach is there. Before I add more unit-tests (i.e., around 75 test-cases, one for each Node type), are people ok with the general implementation/testing strategy?

Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 3:32 PM
Michael137 updated this revision to Diff 450979.Aug 8 2022, 3:34 PM
  • Remove mistakenly added header
Michael137 updated this revision to Diff 450990.Aug 8 2022, 4:04 PM
  • Reword commit message
Michael137 retitled this revision from WIP: [llvm] lldb abi_tag support 1/4 - Add equality comparison API to itanium_demangle::Node to WIP: [llvm] lldb abi_tag support 1/3 - Add equality comparison API to itanium_demangle::Node.Aug 8 2022, 4:07 PM
Michael137 updated this revision to Diff 451007.Aug 8 2022, 5:12 PM
  • Add more tests
Michael137 edited the summary of this revision. (Show Details)Aug 9 2022, 1:19 AM
Michael137 updated this revision to Diff 451084.Aug 9 2022, 3:10 AM
  • Start adding asserts to operator==
Michael137 updated this revision to Diff 451086.Aug 9 2022, 3:11 AM
  • Add more tests

Once you use the libcxxabi sources as a base, the libcxxabi review group should kick in and our CI should get triggered.

llvm/include/llvm/Demangle/ItaniumDemangle.h
233

I think I don't understand why there is both operator== and equals. Do we really need both?

Instead, you could consider defining a virtual bool equals(Node const* Root) const = 0; and override it in subclasses, which seems to be a bit more in-line with what's done elsewhere in this file (e.g. printRight). Thoughts? It does mean that all overrides are going to have to check that Root->Kind == this->Kind, but that may be acceptable.

372

If you keep defining operator== after my comment above, I feel like all classes that get an operator== should also get an operator!= defined with the usual !(a == b).

435–440

Just a suggestion, non-blocking. I find that clearer but it may just be preference.

llvm/unittests/Demangle/ItaniumDemangleTestUtils.h
2

I suspect cp_to_llvm.sh will have to be updated to know about this new file.

Michael137 added inline comments.Aug 10 2022, 5:29 PM
llvm/include/llvm/Demangle/ItaniumDemangle.h
233

That's a reasonable alternative. It does give us some more flexibility in comparing concrete vs. abstract nodes. With my implementation we'd have to cast a concrete node back to Node* to compare for equality which isn't very ergonomic.

372

Will do

435–440

I find your approach more readable :)
will do that instead, thanks

Michael137 added inline comments.Aug 12 2022, 6:11 AM
llvm/unittests/Demangle/ItaniumDemangleTestUtils.h
2

It looks like the unit tests are exclusively in llvm/? I'll keep the tests in this directory for now

  • Move changes to libcxxabi/
  • Add operator!=()
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 10:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
  • Remove redundant accessors
Michael137 marked 3 inline comments as done.Aug 12 2022, 10:20 AM
Michael137 retitled this revision from WIP: [llvm] lldb abi_tag support 1/3 - Add equality comparison API to itanium_demangle::Node to [llvm] lldb abi_tag support 1/3 - Add equality comparison API to itanium_demangle::Node.Aug 15 2022, 11:27 AM