[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGacae69d08c88: [lldb] Add new LLDB setting: use-source-cache 
(authored by emrekultursay, committed by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804

Files:
  lldb/include/lldb/API/SBDebugger.h
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/SourceManager.cpp

Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -72,7 +72,7 @@
   FileSP file_sp;
   if (same_as_previous)
 file_sp = m_last_file_sp;
-  else if (debugger_sp)
+  else if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +95,7 @@
 else
   file_sp = std::make_shared(file_spec, debugger_sp);
 
-if (debugger_sp)
+if (debugger_sp && debugger_sp->GetUseSourceCache())
   debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
   }
   return file_sp;
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -212,6 +212,11 @@
   // use-color changed. Ping the prompt so it can reset the ansi terminal
   // codes.
   SetPrompt(GetPrompt());
+} else if (property_path == g_debugger_properties[ePropertyUseSourceCache].name) {
+  // use-source-cache changed. Wipe out the cache contents if it was disabled.
+  if (!GetUseSourceCache()) {
+m_source_file_cache.Clear();
+  }
 } else if (is_load_script && target_sp &&
load_script_old_value == eLoadScriptFromSymFileWarn) {
   if (target_sp->TargetProperties::GetLoadScriptFromSymbolFile() ==
@@ -338,6 +343,20 @@
   return ret;
 }
 
+bool Debugger::GetUseSourceCache() const {
+  const uint32_t idx = ePropertyUseSourceCache;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
+bool Debugger::SetUseSourceCache(bool b) {
+  const uint32_t idx = ePropertyUseSourceCache;
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  if (!ret) {
+m_source_file_cache.Clear();
+  }
+  return ret;
+}
 bool Debugger::GetHighlightSource() const {
   const uint32_t idx = ePropertyHighlightSource;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -103,6 +103,10 @@
 Global,
 DefaultTrue,
 Desc<"Whether to use Ansi color codes or not.">;
+  def UseSourceCache: Property<"use-source-cache", "Boolean">,
+Global,
+DefaultTrue,
+Desc<"Whether to cache source files in memory or not.">;
   def AutoOneLineSummaries: Property<"auto-one-line-summaries", "Boolean">,
 Global,
 DefaultTrue,
Index: lldb/source/API/SBDebugger.cpp
===
--- lldb/source/API/SBDebugger.cpp
+++ lldb/source/API/SBDebugger.cpp
@@ -1374,6 +1374,18 @@
   return (m_opaque_sp ? m_opaque_sp->GetUseColor() : false);
 }
 
+bool SBDebugger::SetUseSourceCache(bool value) {
+  LLDB_RECORD_METHOD(bool, SBDebugger, SetUseSourceCache, (bool), value);
+
+  return (m_opaque_sp ? m_opaque_sp->SetUseSourceCache(value) : false);
+}
+
+bool SBDebugger::GetUseSourceCache() const {
+  LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBDebugger, GetUseSourceCache);
+
+  return (m_opaque_sp ? m_opaque_sp->GetUseSourceCache() : false);
+}
+
 bool SBDebugger::GetDescription(SBStream ) {
   LLDB_RECORD_METHOD(bool, SBDebugger, GetDescription, (lldb::SBStream &),
  description);
Index: lldb/include/lldb/Core/SourceManager.h
===
--- lldb/include/lldb/Core/SourceManager.h
+++ lldb/include/lldb/Core/SourceManager.h
@@ -101,6 +101,9 @@
 void AddSourceFile(const FileSP _sp);
 FileSP FindSourceFile(const FileSpec _spec) const;
 
+// Removes all elements from the cache.
+void Clear() { m_file_cache.clear(); }
+
   protected:
 typedef std::map FileCache;
 FileCache m_file_cache;
Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -273,6 +273,10 @@
 
   bool SetUseColor(bool use_color);
 
+  bool GetUseSourceCache() const;
+
+  bool SetUseSourceCache(bool use_source_cache);
+
   bool 

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay updated this revision to Diff 253013.
emrekultursay added a comment.

- 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804

Files:
  lldb/include/lldb/API/SBDebugger.h
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/SourceManager.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/SourceManager.cpp

Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -72,7 +72,7 @@
   FileSP file_sp;
   if (same_as_previous)
 file_sp = m_last_file_sp;
-  else if (debugger_sp)
+  else if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +95,7 @@
 else
   file_sp = std::make_shared(file_spec, debugger_sp);
 
-if (debugger_sp)
+if (debugger_sp && debugger_sp->GetUseSourceCache())
   debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
   }
   return file_sp;
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -212,6 +212,11 @@
   // use-color changed. Ping the prompt so it can reset the ansi terminal
   // codes.
   SetPrompt(GetPrompt());
+} else if (property_path == g_debugger_properties[ePropertyUseSourceCache].name) {
+  // use-source-cache changed. Wipe out the cache contents if it was disabled.
+  if (!GetUseSourceCache()) {
+m_source_file_cache.Clear();
+  }
 } else if (is_load_script && target_sp &&
load_script_old_value == eLoadScriptFromSymFileWarn) {
   if (target_sp->TargetProperties::GetLoadScriptFromSymbolFile() ==
@@ -338,6 +343,20 @@
   return ret;
 }
 
+bool Debugger::GetUseSourceCache() const {
+  const uint32_t idx = ePropertyUseSourceCache;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
+bool Debugger::SetUseSourceCache(bool b) {
+  const uint32_t idx = ePropertyUseSourceCache;
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  if (!ret) {
+m_source_file_cache.Clear();
+  }
+  return ret;
+}
 bool Debugger::GetHighlightSource() const {
   const uint32_t idx = ePropertyHighlightSource;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -103,6 +103,10 @@
 Global,
 DefaultTrue,
 Desc<"Whether to use Ansi color codes or not.">;
+  def UseSourceCache: Property<"use-source-cache", "Boolean">,
+Global,
+DefaultTrue,
+Desc<"Whether to cache source files in memory or not.">;
   def AutoOneLineSummaries: Property<"auto-one-line-summaries", "Boolean">,
 Global,
 DefaultTrue,
Index: lldb/source/API/SBDebugger.cpp
===
--- lldb/source/API/SBDebugger.cpp
+++ lldb/source/API/SBDebugger.cpp
@@ -1374,6 +1374,18 @@
   return (m_opaque_sp ? m_opaque_sp->GetUseColor() : false);
 }
 
+bool SBDebugger::SetUseSourceCache(bool value) {
+  LLDB_RECORD_METHOD(bool, SBDebugger, SetUseSourceCache, (bool), value);
+
+  return (m_opaque_sp ? m_opaque_sp->SetUseSourceCache(value) : false);
+}
+
+bool SBDebugger::GetUseSourceCache() const {
+  LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBDebugger, GetUseSourceCache);
+
+  return (m_opaque_sp ? m_opaque_sp->GetUseSourceCache() : false);
+}
+
 bool SBDebugger::GetDescription(SBStream ) {
   LLDB_RECORD_METHOD(bool, SBDebugger, GetDescription, (lldb::SBStream &),
  description);
Index: lldb/include/lldb/Core/SourceManager.h
===
--- lldb/include/lldb/Core/SourceManager.h
+++ lldb/include/lldb/Core/SourceManager.h
@@ -101,6 +101,9 @@
 void AddSourceFile(const FileSP _sp);
 FileSP FindSourceFile(const FileSpec _spec) const;
 
+// Removes all elements from the cache.
+void Clear() { m_file_cache.clear(); }
+
   protected:
 typedef std::map FileCache;
 FileCache m_file_cache;
Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -273,6 +273,10 @@
 
   bool SetUseColor(bool use_color);
 
+  bool GetUseSourceCache() const;
+
+  bool 

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath added a comment.

Yeah, the differences between windows 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.

In D76804#1942723 , @emrekultursay 
wrote:

> 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).




Comment at: lldb/source/Core/Debugger.cpp:350
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  SetPrompt(GetPrompt());
+  return ret;

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76804



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76804: Add new LLDB setting: use-source-cache

[Emre Kultursay via Phabricator via lldb-commits]

emrekultursay created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
emrekultursay added a child revision: D76805: Fix 
SourceManager::SourceFileCache insertion.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76804

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/SourceManager.cpp


Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -72,7 +72,7 @@
   FileSP file_sp;
   if (same_as_previous)
 file_sp = m_last_file_sp;
-  else if (debugger_sp)
+  else if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +95,7 @@
 else
   file_sp = std::make_shared(file_spec, debugger_sp);
 
-if (debugger_sp)
+if (debugger_sp && debugger_sp->GetUseSourceCache())
   debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
   }
   return file_sp;
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -338,6 +338,18 @@
   return ret;
 }
 
+bool Debugger::GetUseSourceCache() const {
+  const uint32_t idx = ePropertyUseSourceCache;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
+bool Debugger::SetUseSourceCache(bool b) {
+  const uint32_t idx = ePropertyUseSourceCache;
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  SetPrompt(GetPrompt());
+  return ret;
+}
 bool Debugger::GetHighlightSource() const {
   const uint32_t idx = ePropertyHighlightSource;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -103,6 +103,10 @@
 Global,
 DefaultTrue,
 Desc<"Whether to use Ansi color codes or not.">;
+  def UseSourceCache: Property<"use-source-cache", "Boolean">,
+Global,
+DefaultTrue,
+Desc<"Whether to cache source files in memory or not.">;
   def AutoOneLineSummaries: Property<"auto-one-line-summaries", "Boolean">,
 Global,
 DefaultTrue,
Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -273,6 +273,10 @@
 
   bool SetUseColor(bool use_color);
 
+  bool GetUseSourceCache() const;
+
+  bool SetUseSourceCache(bool use_source_cache);
+
   bool GetHighlightSource() const;
 
   lldb::StopShowColumn GetStopShowColumn() const;


Index: lldb/source/Core/SourceManager.cpp
===
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -72,7 +72,7 @@
   FileSP file_sp;
   if (same_as_previous)
 file_sp = m_last_file_sp;
-  else if (debugger_sp)
+  else if (debugger_sp && debugger_sp->GetUseSourceCache())
 file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -95,7 +95,7 @@
 else
   file_sp = std::make_shared(file_spec, debugger_sp);
 
-if (debugger_sp)
+if (debugger_sp && debugger_sp->GetUseSourceCache())
   debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
   }
   return file_sp;
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -338,6 +338,18 @@
   return ret;
 }
 
+bool Debugger::GetUseSourceCache() const {
+  const uint32_t idx = ePropertyUseSourceCache;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
+bool Debugger::SetUseSourceCache(bool b) {
+  const uint32_t idx = ePropertyUseSourceCache;
+  bool ret = m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+  SetPrompt(GetPrompt());
+  return ret;
+}
 bool Debugger::GetHighlightSource() const {
   const uint32_t idx = ePropertyHighlightSource;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Core/CoreProperties.td