This is an archive of the discontinued LLVM Phabricator instance.

Add new LLDB setting: use-source-cache
ClosedPublic

Authored by emrekultursay on Mar 25 2020, 2:54 PM.

Details

Summary

LLDB memory-maps large source files, and at the same time, caches
all source files in the Source Cache.

On Windows, memory-mapped source files are not writeable, causing
bad user experience in IDEs (such as errors when saving edited files).
IDEs should have the ability to disable the Source Cache at LLDB
startup, so that users can edit source files while debugging.

Bug: llvm.org/PR45310

Diff Detail

Event Timeline

emrekultursay created this revision.Mar 25 2020, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 2:54 PM

Testing the effect of this change might be hard, but please add or extend an existing test to check that you can set and read the new property.

I did not see any existing tests that validate setting/reading of properties. Can you point to me if you know any that exists?

Yeah, the differences between windows&posix handling of memory mapped files is quite annoying (though I can't really say that the windows behavior is worse than randomly sending out SIGBUSes).

The alternative (which is used in other tools like clangd, I believe) is to not use mmap when opening these kinds of files. But I am not sure if that is actually better than not caching at all.

I did not see any existing tests that validate setting/reading of properties. Can you point to me if you know any that exists?

TestSettings.py does various setting-related things, but I'm not sure how much would reading/writing of this specific setting be valueable, as it would literally only test the single declaration in CoreProperties.td. It would be better to have a test which sets this setting to false and then demonstrates that one is able to read/write to the source file. This is something that already works elsewhere, but it would at least test this functionality on windows (and given that we don't have a failing test, we probably don't have a test for this functionality anyway).

lldb/source/Core/Debugger.cpp
355

this looks like copy-paste gone wrong. SetPrompt is present in the use-color setting because that actually affects the value of the prompt -- this doesn't.

But this does raise the question of whether we should nuke the existing cache when this setting is set to false (I think we should).

emrekultursay edited the summary of this revision. (Show Details)Mar 26 2020, 9:39 AM
  • Apply code review comments from labath@
  • Wipe source cache when user sets the property to false
  • Also expose set/get methods for use-source-cache in SBDebugger

I added an end-to-end test to the entire use-source-cache feature in https://reviews.llvm.org/D76806

labath accepted this revision.Mar 30 2020, 4:37 AM
This revision is now accepted and ready to land.Mar 30 2020, 4:37 AM
This revision was automatically updated to reflect the committed changes.