This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Use the llvm microsoft demangler instead of the windows dbghelp api
ClosedPublic

Authored by mstorsjo on Sep 27 2019, 5:54 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 27 2019, 5:54 AM
This revision is now accepted and ready to land.Sep 27 2019, 5:58 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2019, 2:33 AM
thakis added a subscriber: thakis.Sep 28 2019, 5:47 AM

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.

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.

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 ?

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

(See CPlusPlusLanguage::MethodName for one such parsing instance.)

mstorsjo updated this revision to Diff 222358.Sep 30 2019, 12:44 AM
mstorsjo edited the summary of this revision. (Show Details)
mstorsjo added a reviewer: thakis.

I tried to update the testcases based on the log output (http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9306/steps/test/logs/stdio).

Even after fudging the tests to somehow run on windows, both of the tests that failed seem to require the DIA SDK which I don't have readily available (for reading PDB files; there's some native support for PDB files but I'm not sure what the state of it is and running those tests with it).

mstorsjo reopened this revision.Sep 30 2019, 12:50 AM

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.

This revision is now accepted and ready to land.Sep 30 2019, 12:50 AM
labath accepted this revision.Sep 30 2019, 5:46 AM

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.

That makes sense to me, but I am not really a windows person. Maybe wait a while to see if any of the real "windows people" have any thoughts.

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.

So, you're developing windows arm64 support, without even a real windows x86 around? That's brave. :)

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.

That makes sense to me, but I am not really a windows person. Maybe wait a while to see if any of the real "windows people" have any thoughts.

@thakis - Do you happen to have an opinion here on what's the best course forward? I guess you're the one currently most familiar with the MS demangler.

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.

So, you're developing windows arm64 support, without even a real windows x86 around? That's brave. :)

That's nothing; I brought up most of the windows arm64 support in llvm/clang/lld before I even had a real windows arm64 device to test on (and without access to MSVC targeting arm64 as well for most of the time), just testing in wine, which was capable of running a handful of binaries that were available :-)

amccarth accepted this revision.Oct 8 2019, 2:19 PM

LGTM after one question.

lldb/lit/SymbolFile/PDB/udt-layout.test
1 ↗(On Diff #222358)

Is system-windows still required after you've removed the dependency on dbghelp?

mstorsjo marked an inline comment as done.Oct 9 2019, 12:38 AM

LGTM after one question.

Thanks! I'll hold off committing for a bit still, as I might try to add more options to the microsoft demangler, to match the previous output.

lldb/lit/SymbolFile/PDB/udt-layout.test
1 ↗(On Diff #222358)

Yes, this test depends on the system-provided PDB support, so it needs to run on windows.

mstorsjo updated this revision to Diff 224969.Oct 15 2019, 1:57 AM

Using newly added demangler options to (hopefully) avoid any changes to test outputs (at least the differences seen when applied earlier shouldn't be present any longer).

mstorsjo requested review of this revision.Oct 16 2019, 6:15 AM

Does anyone want to ack this one after updating it to not need changes to tests (as far as I know), by using new demangler options?

labath accepted this revision.Oct 16 2019, 6:23 AM
This revision is now accepted and ready to land.Oct 16 2019, 6:23 AM
This revision was automatically updated to reflect the committed changes.