Page MenuHomePhabricator

Add natvis files directly to the PDB instead of to the VS solution
AcceptedPublic

Authored by zturner on Jun 27 2018, 9:46 PM.

Details

Summary

Adding natvis files to the VS solution means that if you use CMake to generate a VS solution, natvis will just work. However, this is strictly inferior to putting the natvis files directly in the PDB. For starters, it only works if you generate a VS project, which not everyone does, but even then it's link.exe specific. PDB files support embedding natvis files, and this way it works everywhere.

I originally found out about this because I thought my natvis files weren't getting loaded because I wasn't getting visualization of Optional<T>. Turns out that visualizer was just broken, so I went ahead and fixed it, and also added a visualizer for Expected<T> at the same time.

Not really sure who to add for a review here, so + a couple of random people

Diff Detail

Event Timeline

zturner created this revision.Jun 27 2018, 9:46 PM
zturner updated this revision to Diff 153256.Jun 27 2018, 9:48 PM

Remove some debug prints I accidentally left in the cmake.

smeenai added inline comments.Jun 27 2018, 10:57 PM
clang/CMakeLists.txt
345

I'm wondering if it would be nicer to do a check_linker_flag_exists for /natvis. I'm fine with this though.

348

I don't think it makes too much of a difference right now, but for completeness, it would be nice to set CMAKE_MODULE_LINKER_FLAGS as well.

llvm/utils/LLVMVisualizers/llvm.natvis
198 ↗(On Diff #153256)

I think it would be better to split the changes to this file out into its own diff (perhaps even two diffs, one for the fix and one for the new visualizer).

zturner updated this revision to Diff 153353.Jun 28 2018, 10:53 AM

Removed the 2 natvis fixes and committed those independently. Also the CMakeLists.txt files that were in the LLVMVisualizers and ClangVisualizers folders are no longer needed, so I deleted those.

smeenai accepted this revision.Jun 28 2018, 2:19 PM

LGTM

This revision is now accepted and ready to land.Jun 28 2018, 2:19 PM