This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators
ClosedPublic

Authored by shafik on Mar 3 2020, 10:49 AM.

Details

Summary

Currently dsymutil when generating accelerator tables will attempt to strip the template parameters from names for subroutines. For some overload operators which contain < in their names e.g. operator< the current method ends up stripping the operator name as well, so we just end up with the name operator in the table for each case.

Diff Detail

Event Timeline

shafik created this revision.Mar 3 2020, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2020, 10:49 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
shafik edited the summary of this revision. (Show Details)Mar 3 2020, 10:49 AM
JDevlieghere added inline comments.Mar 3 2020, 1:39 PM
llvm/lib/DWARFLinker/DWARFLinker.cpp
177–181

s/auto/StringRef/

181

This comment adds more confusion than it clarifies. The angle brackets being balanced doesn't really matter as this point. I'd say something like:

To strip the template we need to find at least one left angle bracket
183

Please follow the LLVM coding style. This applies to all variables. Also I'd rename this to LeftAngleBracketsToSkip.

187

Can this operator occur more than once?

194

The comment says not balanced, but you use > instead of !=. Does that matter?

202

Given that we always strip the end, can we simplify the algorithm to something like

size_t RightAngleCount = name.count('>');
size_t LeftAngleCount = name.count('<');
if (RightAngleCount != LeftAngleCount) {
	size_t AngleDifference = std::abs(RightAngleCount - LeftAngleCount);
	while (AngleDifference) {
		name = name.take_front(name.rfind('<'));
	}
}

That way we don't have to hardcode the <=> stuff.

202

Forgot to decrement AngleDifference: while (AngleDifference--) {

203

We should extract this logic in a static function, something like StringRef stripTemplateFromName(StringRef Str);.

llvm/test/tools/dsymutil/X86/template_operators.test
24

Please test operator<=> as well.

shafik updated this revision to Diff 248072.Mar 3 2020, 4:58 PM
shafik marked 11 inline comments as done.
  • Addressing comments
  • Fixing bug, I was setting NameWithoutTemplate incorrectly in some cases.
shafik added inline comments.Mar 3 2020, 5:01 PM
llvm/lib/DWARFLinker/DWARFLinker.cpp
187

No

194

I clarified the comment (hopefully)

202

As discussed in person I am ignoring this comment.

llvm/test/tools/dsymutil/X86/template_operators.test
24

Good call, I caught a bug where I was setting NameWithoutTemplate incorrectly in this case.

shafik marked an inline comment as done.Mar 3 2020, 5:02 PM
shafik added inline comments.
llvm/lib/DWARFLinker/DWARFLinker.cpp
203

Just realized I missed this comment, I will extract this out in the next update.

shafik updated this revision to Diff 248221.Mar 4 2020, 9:39 AM
shafik marked an inline comment as done.

Factored out template parameter stripping into its own function.

JDevlieghere added inline comments.Mar 4 2020, 9:43 AM
llvm/lib/DWARFLinker/DWARFLinker.cpp
123

For consistency with the other free functions in the cpp file, can you just make it static? This seems to be the default in llvm.

135

I still think that Skip would be more meaningful than Count

152

Please clang format your patch before landing.

178

You can move this into the if:

if (Optional<StringRef> StrippedName = StripTemplateParameters(Name))
shafik updated this revision to Diff 248239.Mar 4 2020, 10:32 AM
shafik marked 4 inline comments as done.

Addressing comment

  • Converting function to static Vs anon namespace
  • Updating naming
  • moving assignment into if
  • clang-format
JDevlieghere accepted this revision.Mar 4 2020, 1:52 PM

Thanks. LGTM

This revision is now accepted and ready to land.Mar 4 2020, 1:52 PM
This revision was automatically updated to reflect the committed changes.