[Lldb-commits] [PATCH] D113789: Add the ability to cache information for a module between debugger instances.

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

There are two things that could be added. One is already mentioned in the comments, which is logging error messages in a way that we can track bugs in an automated fashion, as right now the errors are being thrown away or transformed into simple true/false values that lose the information of the actual issue. Now that we have a rich telemetry in place, we should use llvm::Error/Expected as much as possible and log the errors in both a log channel and the target stats. The second item to discuss is that there should be a configurable maximum size of the entire cache folder (e.g. 70GB), so that, for example, when lldb initializes it cleans up the oldest items that make the cache blow up. I can also add telemetry from the IDE side to track the size of the total cache folder, but I imagine that if we don't put a limiter, we might cause some issues.



================
Comment at: lldb/include/lldb/Core/Mangled.h:24-28
+namespace llvm {
+namespace gsym {
+class FileWriter;
+} // namespace gsym
+} // namespace llvm
----------------
being a little bit strict, you should move the FileWriter class out of gsym. That will also make it more generic and compelling to use for other stuff


================
Comment at: lldb/include/lldb/Host/FileSystem.h:93
+  /// Set the access and modification time of the given file.
+  bool SetAccessAndModificationTime(const FileSpec &file_spec,
+                                    llvm::sys::TimePoint<> time);
----------------
this should return an llvm::Error and the caller should decide whether to consume the error or not.


================
Comment at: lldb/include/lldb/Host/FileSystem.h:167-168
+  /// \{
+  Status Remove(const FileSpec &file_spec);
+  Status Remove(const llvm::Twine &path);
+  /// \}
----------------
Rename this to RemoveFile to avoid any ambiguities


================
Comment at: lldb/source/Core/CoreProperties.td:23
+    DefaultStringValue<"">,
+    Desc<"The path to the LLDB module cache directory.">;
 }
----------------
mention here what the default directory ends up being used if this setting is left unchanged


================
Comment at: lldb/source/Core/Module.cpp:1669-1676
+  id_strm << m_file.GetPath() << m_arch.GetTriple().str();
+  if (m_object_name)
+    id_strm << m_object_name.GetStringRef();
+  if (m_object_offset > 0)
+    id_strm << m_object_offset;
+  const auto mtime = llvm::sys::toTimeT(m_object_mod_time);
+  if (mtime > 0)
----------------
i'd prefer if there's a separator character like `;` between each element just to avoid any future possible bugs


================
Comment at: lldb/source/Core/Module.cpp:1680
+
+llvm::Optional<FileSpec> Module::GetCacheDirectory() {
+  using namespace llvm;
----------------
this function is doing many things and reading is harder. Besides splitting it, we should log in the target stats if the cache was enabled or not, if the cache was used or not in a per module basis and any errors associated. This will help us fix issues without having to wait for users' reports.
you should also print the error in a log channel


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2723
+      if (m_symtab_up->LoadFromCache())
+        return m_symtab_up.get();
       symbol_id += ParseSymbolTable(m_symtab_up.get(), symbol_id, symtab);
----------------
log here in the target stats if the cache was effectively used or not


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:602
+      if (m_symtab_up->LoadFromCache())
+        return m_symtab_up.get();
       std::lock_guard<std::recursive_mutex> guard(m_symtab_up->GetMutex());
----------------
same here about telemetry


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113789