Page MenuHomePhabricator

Add ___Z demangling to new common demangle function
ClosedPublic

Authored by jhenderson on Jan 17 2019, 7:49 AM.

Details

Summary

This is a follow-up to rL351448. This change adds support for the ___Z extension of Itanium demangling, requested by @erik.pilkington in the previous review, to the newly available demangle function.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Jan 17 2019, 7:49 AM
erik.pilkington accepted this revision.Jan 17 2019, 9:08 AM

LGTM, thanks for following up!

This revision is now accepted and ready to land.Jan 17 2019, 9:08 AM
rupprecht accepted this revision.Jan 17 2019, 10:55 AM
lib/Demangle/Demangle.cpp
18–19 ↗(On Diff #182290)

As of r351481, we allow leading underscores on these. Maybe it would be easier to flip this around so we call microsoftDemangle if the name starts with ?, then itaniumDemangle otherwise?

minor nit:

lib/Demangle/Demangle.cpp
19 ↗(On Diff #182290)

I wonder if

MangledName.compare(0, string::npos, "___Z")

would be a bit more readable way here?
("A value of string::npos indicates all characters until the end of the string.",
http://www.cplusplus.com/reference/string/string/compare/)

grimar added inline comments.Jan 18 2019, 2:30 AM
lib/Demangle/Demangle.cpp
19 ↗(On Diff #182290)

I think there is no much difference. I just had to count the amount of '_`, but the initial version is better I think now.

grimar accepted this revision.Jan 18 2019, 2:31 AM
grimar added inline comments.Jan 18 2019, 2:36 AM
lib/Demangle/Demangle.cpp
19 ↗(On Diff #182290)

Oh, and I just misread the spec :) Please ignore this comments.

jhenderson marked an inline comment as done.Jan 18 2019, 3:14 AM
jhenderson added inline comments.
lib/Demangle/Demangle.cpp
18–19 ↗(On Diff #182290)

@erik.pilkington, flipping it around would adversely affect the behaviour, because itaniumDemangle can also do demangling of types, which aren't strictly fully encoded mangled names. Based on code-inspection, I believe an input string of, say, 'v', would become 'void', which is clearly wrong behaviour for a variable with the name 'v'. At the same time, having to check for _Z, __Z, ___Z and ____Z seems silly. I'll see if I can come up with a better solution.

@grimar, my understanding is that it would compare the entirety of the first string against the other side, which would fail (unless the string was for example "_Z"). Is that what you understand after your re-read?

grimar added inline comments.Jan 18 2019, 3:18 AM
lib/Demangle/Demangle.cpp
18–19 ↗(On Diff #182290)

Yes, I mixed "compared string" with "comparing string". Sorry for the noise.

jhenderson updated this revision to Diff 182495.EditedJan 18 2019, 4:36 AM

Handle __Z and ____Z too.

grimar accepted this revision.Jan 18 2019, 4:57 AM

LGTM.

lib/Demangle/Demangle.cpp
17 ↗(On Diff #182495)

Using just size_t form seems to be much more common. But I see std::size_t is also used sometimes.
Up to you. FTR in LLD we use size_t.

20 ↗(On Diff #182495)

Up to you again, but I would use just I or Pos for shortness:

std::size_t I = MangledName.find_first_not_of('_');
return I > 0 && I <= 4 && MangledName[I] == 'Z';

jhenderson marked 4 inline comments as done.Jan 18 2019, 5:22 AM
jhenderson added inline comments.
lib/Demangle/Demangle.cpp
17 ↗(On Diff #182495)

Looks like ItaniumDemangle.cpp exclusively uses just size_t, so I'll do that.

20 ↗(On Diff #182495)

Pos is fine by me.

This revision was automatically updated to reflect the committed changes.
jhenderson marked 2 inline comments as done.