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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

This patch effectively introduces a file format to cache lldb internal state. While the tests and the code do give some information for what it looks like, some documentation about this format would be nice.

I like this idea a lot! My comments were kind of surface level but should help fix some of the rough edges of the patch.



================
Comment at: lldb/include/lldb/Core/Module.h:879
   StatsDuration &GetSymtabParseTime() { return m_symtab_parse_time; }
-  
   /// Accessor for the symbol table index time metric.
----------------
nit: unnecessary whitespace change


================
Comment at: lldb/include/lldb/Core/Module.h:951
+  ///
+  /// The hash should be enough to indentify the file on disk and the
+  /// architecture of the file. If the module represents an object inside of a
----------------
typo: `indentify` -> `identify`


================
Comment at: lldb/include/lldb/Core/Module.h:959
+  ///   from the same file
+  /// - a .o file inside of a BSD archive. Each .o file will have a object name
+  ///   and object offset that should produce a unique hash. The object offset
----------------
`have a object name` -> `have an object name`


================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:710-713
+  lldb::addr_t m_file_offset;
+  /// The length of this object file if it is known. This can be zero if length
+  /// is unknown or can't be determined.
+  lldb::addr_t m_length;
----------------
Curious to know, why `lldb::addr_t` instead of something like `lldb::offset_t`?


================
Comment at: lldb/include/lldb/Symbol/Symbol.h:21-22
+  class FileWriter;
+}
+}
+
----------------
nit: you have a namespace comment when you do this in an earlier file but not here. what does clang-format do?


================
Comment at: lldb/source/Core/Mangled.cpp:411-412
+  MangledOnly = 2u,
+  /// If we have a Mangled object with two different names that are not related
+  /// then we need to save both strings.
+  MangledAndDemangled = 3u
----------------
What does "two different names that are not related" mean here?


================
Comment at: lldb/source/Core/Module.cpp:65
 
+
 #include <cassert>
----------------
nit: unrelated whitespace change


================
Comment at: lldb/source/Core/Module.cpp:1706
+  // Append the object name to the path as a directory. The object name will be
+  // valie if we have an object file from a BSD archive like "foo.a(bar.o)".
+  if (m_object_name)
----------------
typo: `valie` -> `valid`


================
Comment at: lldb/source/Host/common/FileSystem.cpp:134
+    return false;
+  auto file_or_err = Open(file_spec, File::OpenOptions::eOpenOptionReadWrite, lldb::eFilePermissionsUserRead | lldb::eFilePermissionsUserWrite, /*should_close_fd=*/true);
+  if (file_or_err)
----------------
Could you break this line into multiple lines?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113789