[Lldb-commits] [PATCH] D68134: [LLDB] Use the llvm microsoft demangler instead of the windows dbghelp api

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo reopened this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

In D68134#1687500 <https://reviews.llvm.org/D68134#1687500>, @labath wrote:

> In D68134#1687031 <https://reviews.llvm.org/D68134#1687031>, @mstorsjo wrote:
>
> > In D68134#1686970 <https://reviews.llvm.org/D68134#1686970>, @thakis wrote:
> >
> > > We can add flags for omitting access specifiers etc if it's critical for lldb. Or maybe we can just change the test that caused the revert.
> >
> >
> > Yeah I doubt it's critical to maintain the exact same form as before, but I need to get the tests running in my cross compile setup to verify exactly how to update them.
>
>
> I'm not sure what failed here exactly, but there are some places in lldb that parse the demangled names. These might get confused by additional things appearing in the name. Though it's possible to also fix that, so the main question might be: what is the name we want to display to the users? I guess it would be the best if this matched what is displayed by other tools ?


In this case, the output becomes the same as what has been chosen to be used by the common llvm demangler, which probably should be a good choice in general. In this case, a few more identifiers are added. Or I guess it can be discussed if that one should be changed (or add options to it, as @thakis said was possible).

> As for tests, you should at least be able to run the tests in the regular "host" setup, right ?

In principle, but I normally don't run windows except in a very underpowered VM for testing things, and building there is no fun. I can spin up some more powerful VMs for some better testing though.

In this case, much of the functionality of these tests require having the MS DIA SDK available (which also implies building in MSVC/clang-cl mode, not mingw mode), for reading PDB files. Probably just another step to do, but I don't have it set up right now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68134/new/

https://reviews.llvm.org/D68134