This is an archive of the discontinued LLVM Phabricator instance.

Check for lack of C++ context first when demangling
ClosedPublic

Authored by scott.smith on May 1 2017, 11:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

scott.smith created this revision.May 1 2017, 11:11 AM

FYI, moving this code changes how two symbols (out of 2M+) are handled on a test program I have. I noticed this because when I changed the loop to set it up for parallelization, I effectively deferred all handling of C++ names to later. It simplifies the code to handle !const_context situations first, and then handle the rest. At a high level I assume that should be equivalent, but it actually changed two symbols.

I send this review out so you can either:

  1. Tell me it's wrong to move the if statement, because of some situation I don't understand, or
  2. Tell me this is correct, and the two symbols that are handled differently actually should be handled differently, or
  3. Tell me this is correct, and the two symbols that are handled differently are due to a bug somewhere else.

FWIW the two symbols demangle to roughly "thing<lots_of_stuff>". lots_of_stuff includes lambdas, vectors, and some other things. It wouldn't surprise me if I stumbled across a bug in the mangler or demangler (there are lots of repeated sections, so it could be a bug in back references).

clayborg accepted this revision.May 1 2017, 11:31 AM
This revision is now accepted and ready to land.May 1 2017, 11:31 AM

Can someone please commit this? Thank you!

labath added a subscriber: labath.May 2 2017, 3:11 AM

I am having trouble applying this. Do you need to rebase or something?

I am having trouble applying this. Do you need to rebase or something?

It was based on another commit that you committed for me, but committed after trying to commit this one. It should apply now that you committed D32316.

This revision was automatically updated to reflect the committed changes.